Re: [PATCH v5] branch: introduce --show-current display option

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 26, 2018, 00:52 UTC
Message-ID: <xmqqefcdfs1j.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CAPig+cRVdogY8VLXcftbY=n9tQ9wDo4YrnrdU6+pZ3ch6uhZGA@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 6 quoted lines

>> +       test_when_finished "git tag -d branch-and-tag-name" &&
>> +       git tag branch-and-tag-name &&
>
> If git-tag crashes before actually creating the new tag, then "git tag
> -d", passed to test_when_finished(), will error out too, which is
> probably undesirable since "cleanup code" isn't expected to error out.

Ah, I somehow thought that clean-up actions set up via when_finished are allowed to fail without affecting the outcome, but apparently I was mistaken.

This however can be argued both ways---if you create a tag first and try to set up the clean-up action, during which you may get in trouble and end up leaving the tag behind. So rather than swapping the two lines, explicitly preparing for the case the clean-up action fails, i.e. the first alternative below, would be a good fix.

Also it is a good question if the tag need to be even cleaned up.

Show 9 quoted lines

> You could fix it this way:
>
>     test_when_finished "git tag -d branch-and-tag-name || :" &&
>     git tag branch-and-tag-name &&
>
> or, even better, just swap the two lines:
>
>     git tag branch-and-tag-name &&
>     test_when_finished "git tag -d branch-and-tag-name" &&

Show 9 quoted lines

> However, do you even need to clean up the tag? Are there tests
> following this one which expect a certain set of tags and fail if this
> new one is present? If not, a simpler approach might be just to leave
> the tag alone (and the branch too if that doesn't need to be cleaned
> up).
>
>> +       git branch --show-current >actual &&
>> +       test_cmp expect actual
>> +'

A bigger question we may want to ask ourselves is if we want to detect failures from these clean-up actions in the first place. There are many hits from "git grep 'when_finished .*|| :' t/", which may be a sign that the when_finished mechanism was misdesigned and we should simply ignore the exit status from the clean-up actions instead.

I haven't gone through the list of when_finished clean-up actions that do not end with "|| :"; I suspect some of them are simply being sloppy and would want to have "|| :", but what I want to find out out of such an audit is if there is a legitimate case where it helps to catch failures in the clean-up actions. If there is none, then ...