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

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Jan 28, 2022, 11:04 UTC
Message-ID: <87lez0p1db.fsf@coati.i-did-not-set--mail-host-address--so-tickle-me>
In-Reply-To: <xmqqmtjh0x5f.fsf@gitster.g>

Junio C Hamano writes:

Show 17 quoted lines

> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
>> When pruning refs fails, we currently print an error to stderr, but
>> still exit 0 from 'git fetch'.  Since this is a genuine error fetch
>> should be exiting with some non-zero exit code.  Make it so.
>>
>> The --prune option was introduced in f360d844de ("builtin-fetch: add
>> --prune option", 2009-11-10).  Unfortunately it's unclear from that
>> commit whether ignoring the exit code was an oversight or
>> intentional, but it feels like an oversight.
>
> It is a good idea to signal, with a non-zero status, that the user
> needs to inspect the situation and possibly take a corrective
> action.  I however do not know if it is sufficient to just give the
> same exit status as returned by prune_refs(), which may or may not
> happen to crash with other possible non-zero exit status to make it
> indistinguishable from other kinds of errors.

Not sure I understand this correctly. Are you saying that we should take the return value from prune refs, and always munge that to the same exit code for 'git fetch --prune' if it fails? E.g. so prune_refs() could return 1 or 2 or 3 or whatever, but we would always want the exit code for 'fetch' to be 1?

Happy to implement that.

Show 25 quoted lines

>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
>> index 20f7110ec1..df824cc3d0 100755
>> --- a/t/t5510-fetch.sh
>> +++ b/t/t5510-fetch.sh
>> @@ -164,6 +164,16 @@ test_expect_success 'fetch --prune --tags with refspec prunes based on refspec'
>>  	git rev-parse sometag
>>  '
>>
>> +test_expect_success REFFILES 'fetch --prune fails to delete branches' '
>> +	cd "$D" &&
>> +	git clone . prune-fail &&
>> +	cd prune-fail &&
>> +	git update-ref refs/remotes/origin/extrabranch main &&
>> +	>.git/packed-refs.new &&
>> +	test_must_fail git fetch --prune origin
>
> Is it because the lockfile ".new" on packed-refs prevents deletion
> of refs but does not block creation and updates to existing refs
> that it is an effective test for the "--prune" issue?  If we somehow
> "locked" the whole ref updates, then the fetch would fail even
> without "--prune", so it may be "effective", but smells like knowing
> too much implementation detail.  Yuck, but I do not offhand think of
> any better way (it is easier to think of worse ways), so without
> further input, I would say that this is the best (or "least bad") we
> can do.

Yes, that's correct. New refs will be created as loose refs, so they don't care about packed-refs. However deletions can potentially be happening in packed-refs, and that's why it fails when 'packed-refs.new' exists.

I don't love the test either, but I also can't think of a better way to do this.

Show 6 quoted lines

> Another tangent #leftoverbits is that many tests in this script are
> so old-style that chdir around without isolating themselves in
> subshells, while some do.  We may want to clean them up when the
> file is quiescent.
>
> Thanks.