Junio C Hamano writes:
> Thomas Gummerer <t.gummerer@xxxxxxxxx> 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.
>> 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.
> 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.
