v5branch: introduce --show-current display option

When called with --show-current, git branch will print the current branch name and terminate. Only the actual name gets printed, without refs/heads. In detached HEAD state, nothing is output.

Intended both for scripting and interactive/informative use. Unlike git branch --list, no filtering is needed to just get the branch name.

Signed-off-by: Daniels Umanovskis <daniels@umanovskis.se>
---

Submitting v5 now that a week has passed since latest maintainer comments.

This is basically v4 but with small fixes to the test, as proposed by Junio on pu, and additionally replacing a subshell with { .. } since Dscho and Eric discovered the negative performance effects of subshell invocations.

 Documentation/git-branch.txt |  6 ++++-
 builtin/branch.c             | 25 ++++++++++++++++++--
 t/t3203-branch-output.sh     | 44 ++++++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+), 3 deletions(-)

On Thu, Oct 25, 2018 at 3:04 PM Daniels Umanovskis <daniels@umanovskis.se> wrote:

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

Eric Sunshine <sunshine@sunshineco.com> writes:

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.

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:

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

Eric Sunshine <sunshine@sunshineco.com> writes:

We've discussed about the exit status from clean-up code already, but another thing worth noticing is that it probably is easier to see what is going on if we use a single when-finished to clear both branch and the tag with the same name. Something like

	test_when_finished "
		git checkout branch-one
		git branch -D branch-and-tag-name
		git tag -d branch-and-tag-name
		:
	" &&

upfront before doing anything else. "checkout" may break if the test that follows when-finished accidentally removes branch-one and that would cascade to a failure to remove branch-and-tag-name branch (because we fail to move away from it), but because there is no && in between, we'd clean as much as we could in such a case, which may or may not be a good thing. And then we hide the exit code by having a ":" at the end.

print_current_branch_name() tries to resolve HEAD and die() when it doesn't resolve it successfully. But the conditions being tested are always unreachable because early in branch:cmd_branch() the same logic is performed.

Eliminate the duplicate and unreachable code, and update the current logic to the more reliable check for the detached head.

Signed-off-by: Rafael Ascensão <rafa.almas@gmail.com>
---

This patch is meant to be either applied or squashed on top of the current series.

I am basing the claims of it being more reliable of what Junio suggested on a previous iteration of this series: https://public-inbox.org/git/xmqq4ldtgehs.fsf@gitster-ct.c.googlers.com/

But the main goal of this patch is to just bring some attention to this, as I mentioned it in a previous thread but it got lost. After asking on #git-devel, the suggestion was to send it as an incremental patch. So here it is. :)

I still think the mention about scripting should be removed from the original commit message, leaving it open to being taught other tricks like --verbose that aren't necessarily script-friendly.

Cheers

 builtin/branch.c | 23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)

Rafael Ascensão <rafa.almas@gmail.com> writes:

Nice.

I'd prefer to see scriptors avoid using "git branch", too.

Unlike end-user facing documentation where we promise "we do X and will continue to do so because of Y" to the readers, the log message is primarily for recording the original motivation of the change, so that we can later learn "we did X back then because we thought Y". When we want to revise X, we revisit if the reason Y is still valid.

So in that sense, the door to "break" the scriptability is still open.

This idea of yours seems to lead to a better implementation, and indeed "got lost" is a good way to describe what happened---I do not recall seeing it, for example. Thanks for bringing it back.

You've eliminated the "now unnecessary" helper and do everything inside cmd_branch(), so perhaps this can be made function local, no?

Nice to see we can reuse the resolve_refdup() we already have.

Ah, I wondered why we do not have to skip-prefix, but it is already done for us when we validated that an attached HEAD points at a local branch. Good.

I did something that resulted in the mailing list not being cc'd. Apologies to Junio and Daniels for the double send. :(

On Thu, Nov 08, 2018 at 10:11:02AM +0900, Junio C Hamano wrote:

Over at #git, commit messages are sometimes consulted to disambiguate or clarify certain details. Often the documentation is correct but people dispute over interpretations.

If someone came asking if `git branch` is parsable, I would advise against and direct them to the plumbing or format alternative. But if someone came over with a link to this commit asking the same question, I suspect the answer would be: it's probably safe to parse the output of this specific option because the commit says so. Thanks for clarifying this is wrong.

I was not sure if these 3 lines were global intentionally or if it was just an artifact from the past. Since it looks like the latter, I'll make them local.

-- Rafael Ascensão