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

On Fri, Oct 26, 2018 at 09:52:24AM +0900, Junio C Hamano wrote:

Eric Sunshine [off-list ref] writes:

quoted

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.

If a when_finished block fails, we consider that a test failure. But if
we failed to create the tag, the test is failing anyway. Do we actually
care at that point?

We would still want to make sure we run the rest of the cleanup, but
looking at the definition of test_when_finished(), I think we do.

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

I think in the success case it is legitimately helpful. If that "tag -d"
failed above (after the tag creation and the rest of the test
succeeded), it would certainly be unexpected and we would want to know
that it happened. So I think "|| :" in this case is not just
unnecessary, but actively bad.

-Peff