@matheusbsbs / matheus-bagliano / commits / ab292bc4f3

repack_without_ref(): silence errors for dangling packed refs

Stop emitting an error message when deleting a packed reference if we find another dangling packed reference that is overridden by a loose reference. See the previous commit for a longer explanation of the issue. We have to be careful to make sure that the invalid packed reference really *is* overridden by a loose reference; otherwise what we have found is repository corruption, which we *should* report. Please note that this approach is vulnerable to a race condition similar to the race conditions already known to affect packed references [1]: * Process 1 tries to peel packed reference X as part of deleting another packed reference. It discovers that X does not refer to a valid object (because the object that it referred to has been garbage collected). * Process 2 tries to delete reference X. It starts by deleting the loose reference X. * Process 1 checks whether there is a loose reference X. There is not (it has just been deleted by process 2), so process 1 reports a spurious error "X does not point to a valid object!" The worst case seems relatively harmless, and the fix is identical to the fix that will be needed for the other race conditions (namely holding a lock on the packed-refs file during *all* reference deletions), so we leave the cleaning up of all of them as a future project. [1] http://thread.gmane.org/gmane.comp.version-control.git/211956 Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Michael Haggerty committed Apr 22, 2013 at 21:52 UTC ab292bc4f30dd29d3d111b040b9e982f20b9ceb7

2 files changed +36 -3

refs.c

+35 -2

@@ -1902,8 +1902,41 @@ static int repack_without_ref_fn(struct ref_entry *entry, void *cb_data)

1902

1903 if (!strcmp(data->refname, entry->name))

1904 return 0;

1905 - if (!ref_resolves_to_object(entry))

1906 - return 0; /* Skip broken refs */

1905 + if (entry->flag & REF_ISBROKEN) {

1906 + /* This shouldn't happen to packed refs. */

1907 + error("%s is broken!", entry->name);

1908 + return 0;

1909 + }

1910 + if (!has_sha1_file(entry->u.value.sha1)) {

1911 + unsigned char sha1[20];

1912 + int flags;

1913 +

1914 + if (read_ref_full(entry->name, sha1, 0, &flags))

1915 + /* We should at least have found the packed ref. */

1916 + die("Internal error");

1917 + if ((flags & REF_ISSYMREF) || !(flags & REF_ISPACKED))

1918 + /*

1919 + * This packed reference is overridden by a

1920 + * loose reference, so it is OK that its value

1921 + * is no longer valid; for example, it might

1922 + * refer to an object that has been garbage

1923 + * collected. For this purpose we don't even

1924 + * care whether the loose reference itself is

1925 + * invalid, broken, symbolic, etc. Silently

1926 + * omit the packed reference from the output.

1927 + */

1928 + return 0;

1929 + /*

1930 + * There is no overriding loose reference, so the fact

1931 + * that this reference doesn't refer to a valid object

1932 + * indicates some kind of repository corruption.

1933 + * Report the problem, then omit the reference from

1934 + * the output.

1935 + */

1936 + error("%s does not point to a valid object!", entry->name);

1937 + return 0;

1938 + }

1939 +

1940 len = snprintf(line, sizeof(line), "%s %s\n",

1941 sha1_to_hex(entry->u.value.sha1), entry->name);

1942 /* this should not happen but just being defensive */

t/t3210-pack-refs.sh

+1 -1

@@ -140,7 +140,7 @@ test_expect_success 'delete ref with dangling packed version' '

140 test_cmp /dev/null result

141 '

142

143 -test_expect_failure 'delete ref while another dangling packed ref' '

143 +test_expect_success 'delete ref while another dangling packed ref' '

144 git branch lamb &&

145 git commit --allow-empty -m "future garbage" &&

146 git pack-refs --all &&