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

Eric Sunshine [off-list ref] writes:

quoted

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

quoted

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