Re: [PATCH v3 3/3] fetch, remote: retain old OIDs when pruning refs

[Junio C Hamano <gitster@pobox.com>]

Maciej Ciemborowicz <maciej.ciemborowicz@gmail.com> writes:

>  	if (!dry_run) {
>  		if (transaction) {
>  			for (ref = stale_refs; ref; ref = ref->next) {
> -				result = ref_transaction_delete(transaction, ref->name, NULL,
> -								NULL, 0, "fetch: prune", &err);
> +				result = ref_transaction_delete(transaction, ref->name,
> +							&ref->new_oid, NULL, 0,
> +							"fetch: prune", &err);
>  				if (result)
>  					goto cleanup;
>  			}
>  		} else {
> +			for (ref = stale_refs; ref; ref = ref->next) {
> +				string_list_append(&refnames, ref->name);
> +				oid_array_append(&old_oids, &ref->new_oid);
> +			}
>  			result = refs_delete_refs(get_main_ref_store(the_repository),
>  						  "fetch: prune", &refnames,
> -						  NULL, 0);
> +						  &old_oids, 0);
>  		}
> +		if (result)
> +			goto cleanup;
>  	}

Hmph, I may not be reading the code correctly, but the last "goto
cleanup" in the above block can happen when refs_delete_refs() call
that internally uses the best effort transaction sees an error.  If
we were about to prune 30 refs but failed to prune one of them, and
if we are running with non-negative verbosity, don't we still want
to make the "[deleted]" report for the 29 of them and possibly
report "[failed to delete]" for the one that failed?

>  
>  	if (verbosity >= 0) {
>  		int summary_width = transport_summary_width(stale_refs);
>  
> +		if (!refnames.nr)
> +			for (ref = stale_refs; ref; ref = ref->next)
> +				string_list_append(&refnames, ref->name);
>  		for (ref = stale_refs; ref; ref = ref->next) {
>  			display_ref_update(display_state, '-', _("[deleted]"), NULL,
>  					   _("(none)"), ref->name,

> @@ -1639,17 +1650,24 @@ static int prune_remote(const char *remote, int dry_run)
>  	printf_ln(_("Pruning %s"), remote);
>  	printf_ln(_("URL: %s"), states.remote->url.v[0]);
>  
> -	for_each_string_list_item(item, &states.stale)
> -		string_list_append(&refs_to_prune, item->util);
> -	string_list_sort(&refs_to_prune);
> +	for_each_string_list_item(item, &states.stale) {
> +		struct stale_ref *stale_ref = item->util;
> +
> +		string_list_append(&refs_to_prune, stale_ref->name);
> +		oid_array_append(&old_oids, &stale_ref->oid);
> +	}
>  
> -	if (!dry_run)
> +	if (!dry_run) {
>  		result |= refs_delete_refs(get_main_ref_store(the_repository),
>  					   "remote: prune", &refs_to_prune,
> -					   NULL, 0);
> +					   &old_oids, 0);
> +		if (result)
> +			goto cleanup;
> +	}

Ditto.  Beyond the post context of this hunk ... 

>  	for_each_string_list_item(item, &states.stale) {
> -		const char *refname = item->util;
> +		struct stale_ref *stale_ref = item->util;
> +		const char *refname = stale_ref->name;
>  
>  		if (dry_run)
>  			printf_ln(_(" * [would prune] %s"),

... around here is a code that reports "* [pruned]" for the ones
that we successfully removed, which is now ignored when even one of
the bulk removal fails.