Re: [PATCH] fetch --prune: exit with error if pruning fails

["Thomas Gummerer" <t.gummerer@gmail.com>]

Johannes Schindelin writes:

> Hi Junio,
>
> On Thu, 27 Jan 2022, Junio C Hamano wrote:
>
>> Thomas Gummerer <t.gummerer@gmail.com> writes:
>>
>> > +		if (retcode) {
>> > +			free_refs(ref_map);
>> > +			goto cleanup;
>> >  		}
>>
>> This part is iffy.  We tried to prune refs, we may have removed some
>> of the refs missing from the other side but we may still have some
>> other refs that are missing from the other side due to the failure
>> we noticed.
>>
>> Is it sensible to abort the fetching?  I am undecided, but without
>> further input, my gut reaction is that it is safe and may even be
>> better to treat this as a soft error and try to go closer to where
>> the user wanted to go as much as possible by continuing to fetch
>> from the other side, given that we have already paid for the cost of
>> discovering the refs from the other side.

> I am not so sure. When pruning failed, there may very well be directories
> or files in the way of fetching the refs as desired. And it might be even
> worse if pruning failed _without_ the fetch failing afterwards: the user
> specifically asked for stale refs to be cleaned up, the command succeeded,
> but did not do what the user asked for.

I was thinking along similar lines here.  I was going back and forth
between letting the fetch continue, and then exiting with a non-zero
exit code, and just erroring out directly.  I ended up with the latter
because it felt like the right thing to do for the user.  The command
failed, so I'd think it's more confusing that it does more work after
that (even if we already did part of that work).

But I don't care too much one way or another, as long as we end up with
a non-zero exit code when the fetch fails.

> Maybe Thomas has an even stronger argument in favor of erroring out. In
> any case, I don't think that `--prune` should be a "best effort, otherwise
> just shrug" option. If we wanted that, we could introduce
> `--prune-best-effort` or some such...

I don't have a strong argument on whether we should error out
immediately after failing to prune the refs, or just erroring out after
doing the rest of the work, other than exiting early feels right to me.

From the comments upthread it seems to me that the argument is more
whether we should continue after failing to prune, and exit with a
non-zero exit code, or if we should just fail early.

Though I'm happy to introduce a '--prune-best-effort' option or
something like that if we still want to preserve that option for users.
I don't really see people wanting to use that though, and it should be
very rare that '--prune' fails, but the rest of the fetch succeeds.