Re: [PATCH 17/33] repack_without_ref(): silence errors for dangling packed refs

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 15, 2013, 17:39 UTC
Message-ID: <7vhaj7vgk6.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1365944088-10588-18-git-send-email-mhagger@alum.mit.edu>

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

Show 8 quoted lines

> Stop emitting an error message for dangling packed references found
> when deleting another packed reference.  See the previous commit for a
> longer explanation of the issue.
>
> Change repack_without_ref_fn() to silently ignore dangling packed
> references.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

Somehow this feels as if it is sweeping the problem under the rug.

If you ignore a ref for which a loose ref exists when you update a packed refs file, whether the stale "packed" one points at an object that is still there or an object that has been garbage collected, you would not even have to check if the "ref" resolves to object or anything like that, no?

Am I missing something?

This one feels iffy in the otherwise pleasant-to-read series.

Show 65 quoted lines

> ---
>  refs.c               | 17 ++++++++++-------
>  t/t3210-pack-refs.sh |  2 +-
>  2 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 51f68d3..eadbc2a 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -531,15 +531,17 @@ static void sort_ref_dir(struct ref_dir *dir)
>  
>  /*
>   * Return true iff the reference described by entry can be resolved to
> - * an object in the database.  Emit a warning if the referred-to
> - * object does not exist.
> + * an object in the database.  If report_errors is true, emit a
> + * warning if the referred-to object does not exist.
>   */
> -static int ref_resolves_to_object(struct ref_entry *entry)
> +static int ref_resolves_to_object(struct ref_entry *entry, int report_errors)
>  {
>  	if (entry->flag & REF_ISBROKEN)
>  		return 0;
>  	if (!has_sha1_file(entry->u.value.sha1)) {
> -		error("%s does not point to a valid object!", entry->name);
> +		if (report_errors)
> +			error("%s does not point to a valid object!",
> +			      entry->name);
>  		return 0;
>  	}
>  	return 1;
> @@ -578,7 +580,7 @@ static int do_one_ref(struct ref_entry *entry, void *cb_data)
>  		return 0;
>  
>  	if (!((data->flags & DO_FOR_EACH_INCLUDE_BROKEN) ||
> -	      ref_resolves_to_object(entry)))
> +	      ref_resolves_to_object(entry, 1)))
>  		return 0;
>  
>  	current_ref = entry;
> @@ -1897,8 +1899,9 @@ 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 */
> +	/* Silently skip broken refs: */
> +	if (!ref_resolves_to_object(entry, 0))
> +		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 &&