Web lists-archives.com

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




Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes:

>> +       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.

> 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" &&

> 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
...