Michael Haggerty <mhag...@alum.mit.edu> writes:

> 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 <mhag...@alum.mit.edu>
> ---

Clean and straight-forward implementation that is very well
explained.  I like this much better than the previous round ;-).

Thanks, will re-queue.

>  refs.c               | 37 +++++++++++++++++++++++++++++++++++--
>  t/t3210-pack-refs.sh |  2 +-
>  2 files changed, 36 insertions(+), 3 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index ed54ed4..2957f6d 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1901,8 +1901,41 @@ static int repack_without_ref_fn(struct ref_entry 
> *entry, void *cb_data)
>  
>       if (!strcmp(data->refname, entry->name))
>               return 0;
> -     if (!ref_resolves_to_object(entry))
> -             return 0; /* Skip broken refs */
> +     if (entry->flag & REF_ISBROKEN) {
> +             /* This shouldn't happen to packed refs. */
> +             error("%s is broken!", entry->name);
> +             return 0;
> +     }
> +     if (!has_sha1_file(entry->u.value.sha1)) {
> +             unsigned char sha1[20];
> +             int flags;
> +
> +             if (read_ref_full(entry->name, sha1, 0, &flags))
> +                     /* We should at least have found the packed ref. */
> +                     die("Internal error");
> +             if ((flags & REF_ISSYMREF) || !(flags & REF_ISPACKED))
> +                     /*
> +                      * This packed reference is overridden by a
> +                      * loose reference, so it is OK that its value
> +                      * is no longer valid; for example, it might
> +                      * refer to an object that has been garbage
> +                      * collected.  For this purpose we don't even
> +                      * care whether the loose reference itself is
> +                      * invalid, broken, symbolic, etc.  Silently
> +                      * omit the packed reference from the output.
> +                      */
> +                     return 0;
> +             /*
> +              * There is no overriding loose reference, so the fact
> +              * that this reference doesn't refer to a valid object
> +              * indicates some kind of repository corruption.
> +              * Report the problem, then omit the reference from
> +              * the output.
> +              */
> +             error("%s does not point to a valid object!", entry->name);
> +             return 0;
> +     }
> +
>       len = snprintf(line, sizeof(line), "%s %s\n",
>                      sha1_to_hex(entry->u.value.sha1), entry->name);
>       /* this should not happen but just being defensive */
> diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
> index c032d88..559f602 100755
> --- a/t/t3210-pack-refs.sh
> +++ b/t/t3210-pack-refs.sh
> @@ -142,7 +142,7 @@ test_expect_success 'delete ref with dangling packed 
> version' '
>       test_cmp /dev/null result
>  '
>  
> -test_expect_failure 'delete ref while another dangling packed ref' '
> +test_expect_success 'delete ref while another dangling packed ref' '
>       git branch lamb &&
>       git commit --allow-empty -m "future garbage" &&
>       git pack-refs --all &&
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to