v2branch: introduce --show-current display option

v2 reroll of a previously-discussed patch. Thanks to everyone for their comments. Based on feedback:

1. Command is now a verb: git branch --show-current.

2. Changed to gitster's suggested implementation: nothing is printed
 if HEAD does not point to a symbolic ref. A fatal
 error if HEAD is a symbolic ref but does not start with refs/heads/.

3. Added a test to show this works with worktrees

A process question to the list. The patch adds a new localizable string that gets output in case of repository corruption. I happen to speak a couple of the languages that have po files. Is it accepted practice to also include po edits in my patch in such a case, or should that be left to the regular l10n workflow?

Daniels Umanovskis (1):
  branch: introduce --show-current display option

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

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>
---
 Documentation/git-branch.txt |  6 +++++-
 builtin/branch.c             | 21 ++++++++++++++++--
 t/t3203-branch-output.sh     | 41 ++++++++++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+), 3 deletions(-)

On Wed, Oct 10, 2018 at 10:54:32PM +0200, Daniels Umanovskis wrote:

I also wondered what happens in an unborn-branch state (i.e., we are on refs/heads/master, but have not yet made any commits).

In that case, resolve_ref_unsafe() will return the branch name, but a null oid. And you'll print that. Which seems sensible.

We should not advertise this to be used for scripting. The git-branch command is porcelain, and we reserve the right to change its output between versions, or based on user config. Fortunately, there is already a blessed way to get this in a script, which is:

  git symbolic-ref [--short] HEAD

I'm not opposed to having "branch --show-current" as a more user-facing alternative for people who want to query the state. I do wonder if people would want it to show extra information, like:

  - if we're detached, from where (like the normal "branch --list"
    shows)

  - are we on an unborn branch

  - are we ahead/behind an upstream (like "branch -v")

I guess that's slowly reinventing the first line of "git status -b". Maybe that's a good thing; possibly this should just be a way to get that data without doing the rest of git-status, and it's perhaps more discoverable since it's part of git-branch. I dunno.

It just seems like in its current form it might be in an uncanny valley where it is not quite scriptable plumbing, but not as informative as other porcelain.

-Peff

On Wed, Oct 10, 2018 at 08:34:40PM -0400, Jeff King wrote:

I agree it feels a bit out of place, and still think that

    $ git branch --list HEAD

would be a good candidate to be taught how to print the current branch.

I suggested this in the previous iteration but either got lost in the noise or was uninteresting. If the latter, I would love to receive feedback on it. https://public-inbox.org/git/20181010142423.GA3390@rigel/

Something like the following (not meant as a real patch), would show the current branch when attached, (HEAD detached at hash) when detached, and nothing if unborn branch.

This will also keep the current formatting git branch uses (which is sliglty harder to parse). I view it as a plus. Otherwise people will eventually start parsing it instead of using the recommended plumbing.

-- Cheers Rafael Ascensão

-- >8 --

On 10/11/18 5:43 PM, Rafael Ascensão wrote:

I am not a fan because it would be yet another inconsistency in the Git command interface. An argument given after git branch --list means a pattern for the branches to list. Making HEAD print the current branch would be an exception to what an argument in that place means. Yes, HEAD itself is a very special string in git, but I'm not a fan of syntax where a specific argument value does something very different from any other value in that place.

On Thu, Oct 11, 2018 at 06:36:02PM +0200, Daniels Umanovskis wrote:

Yeah, I agree. If we were to go this route, it should probably be:

  git branch --list-head

Which sounds a lot like what you are proposing, but I think the name implies more strongly "show --list, but only for the HEAD". I.e., for the detached case, show the "HEAD detached at..." text.

-Peff

On Thu, Oct 11, 2018 at 06:36:02PM +0200, Daniels Umanovskis wrote:

The output of the proposed command is also a bit inconsistent with the usual output given by git branch, specifically the space alignment on the left, color and * marker.

In addition to not respecting --color, it also ignores --verbose and --format. At this stage it's closer to what I would expect from $git rev-parse --abbrev-ref HEAD; than something coming out of $git branch; Resolving HEAD makes it consistent with rest.

On Thu, Oct 11, 2018 at 01:51:36PM -0400, Jeff King wrote:

Not sure which parts you meant, so I'll assume you didn't agree with me.

I doesn't seem far fetched to ask for an overview of my current branch, feature1, feature2 and all hotfixes with something like:

  $ git branch --verbose --list HEAD feature1 feature2 hotfix-*;

The 'what's my current branch' could be just a particular case of this form.

My defense to treat HEAD specially comes in the form that from the user perspective, HEAD is already being resolved to a commit when HEAD is detached (Showing the detached at <hash> message.)

Is there a strong reason to *not* "resolve" HEAD when it is attached? Would it be that bad to have some DWIM behaviour here? After all, as HEAD is an invalid name for a branch, nothing would ever match it anyways.

Thanks for the input. :) -- Cheers Rafael Ascensão

On 10/11/18 10:35 PM, Rafael Ascensão wrote:

The proposed command therefore takes a new switch. It's definitely not perfect, but doesn't give the existing --list new and different behavior.

The proposal is largely to have similar output to that command, yes. I expect that "show current branch" is something that's available in the branch command, even completely disregarding questions of whether it's API stable, etc.

On Thu, Oct 11, 2018 at 09:35:28PM +0100, Rafael Ascensão wrote:

Correct. ;)

I like your general idea, but I agree with Daniel that it introduces an inconsistency in the interface.

Right, I like that part. It's just that putting "HEAD" there already has a meaning: it would find refs/heads/HEAD.

Now I'll grant that's a bad name for a branch (and the source of other confusions, and I think perhaps even something a few commands actively discourage these days).

I don't think this is about resolving HEAD, or showing it. It's about the fact that arguments to "branch" are currently always branch-names, not full refs.

-Peff

On Thu, Oct 11, 2018 at 04:53:23PM -0400, Jeff King wrote:

Makes sense. My whole premise was based on the fact that refs/heads/HEAD wouldn't be supported. Now it's obvious to me that isn't necessarily true. And now I understand the real issue. Thanks for bearing with me.

I also agree with your proposed '--list-head' suggestion.

So, ideally, instead of my broken suggestion of:
    $ git branch --verbose --list HEAD feature1 hotfix-*;

The equivalent would be:
    $ git branch --verbose --list-head --list feature1 hotfix-*;

and it would coalesce nicely as long as --list-head conforms with the default formatting for --list.

-- Cheers Rafael Ascensão

Daniels Umanovskis <daniels@umanovskis.se> writes:

Is it a normal situation to have refname==NULL, or is it something worth reporting as an error?

Without passing the &flag argument, I do not think there is a reliable way to ask resolve_ref_unsafe() if "HEAD" is a symbolic ref.

	int flag;
	const char *refname = resolve_ref_unsafe("HEAD", 0, NULL, &flag);
	const char *branchname;

	if (!refname)
		die(...);
	else if (!(flag & REF_ISSYMREF))
		return; /* detached HEAD */
	else if (skip_prefix(refname, "refs/heads/", &branchname))
		puts(branchname);
	else
		die("HEAD (%s) points outside refs/heads/?", refname);

or something like that?

On 10/11/18 8:54 AM, Junio C Hamano wrote:

Looks like that would be in the case of looping symrefs or file backend failure, so seems a good idea to die() in that case.

If I'm reading the code correctly, resolve_ref_unsafe() will return "HEAD" or NULL if there's no symbolic reference, so anything else would indicate a symref, but even in that case checking the flag explicitly is definitely better to clearly show intent.

Will soon reply with v3 cleaning up the suggested patch accordingly.

On Thu, Oct 11, 2018 at 07:29:58PM +0200, Daniels Umanovskis wrote:

Yes, that matches my understanding, too.

-Peff

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

Cleaned up per suggestions, explicitly passing flags to clearly denote intent. If you consider the patch good conceptually, this implementation should hopefully be good enough to include.

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

Daniels Umanovskis <daniels@umanovskis.se> writes:

Thanks for fixing this (I fixed this in the previous round in my tree but forgot to tell you about it).

OK, that's trivial. We checkout a branch and make sure show-current reports the name of that branch. Good.

OK, and at the same time we make sure the command exits with success. Good.

It is a bit curious why you remove the branch but not the tag after this test. If we are cleaning after ourselves, removing both would be equally good, if not cleaner. If having both absolutely harms later tests but having just one is OK, then any failure in this test between the time branch-and-tag-name tag gets created and the time branch-and-tag-name branch gets removed will leave the repository with both the tag and the branch, which will be the state in which later tests start, so having "branch -d" at this spot in the sequence is not a good idea anyway.

So two equally valid choices are to remove "branch -d" and then either:

 (1) leave both branch and tag after this test in the test
     repository

 (2) use test_when_finished, i.e.

	echo branch-and-tag-name >expect &&
	test_when_finished "git branch -D branch-and-tag-name" &&
	git checkout -b branch-and-tag-name &&
	test_when_finished "git tag -d branch-and-tag-name" &&
	git tag branch-and-tag-name &&
	...

     to arrange them to be cleaned once this test is done.

(1) is only valid if they do not harm later tests. I guess you remove the branch because you did not want to touch later tests that checks output from "git branch --list", in which case you'd want (2).

Please do *not* cd around without being in a subshell. If the second --show-current failed for some reason, "cd .." will not be executed, and the next and subsequent test will start inside ./worktree subdirectory, which is likely to break the expectations of them. Perhaps something like

	git checkout branch-one &&
	git worktree add worktree branch-two &&
	(
		git branch --show-current &&
		cd worktree && git branch --show-current
	) >actual &&
	test_cmp expect actual

or its modern equivalent

	git checkout branch-one &&
	git worktree add worktree branch-two &&
	(
		git branch --show-current &&
		git -C worktree branch --show-current
	) >actual &&
	test_cmp expect actual

Note that the latter _could_ be written without subshell, i.e.

	git branch --show-current >actual &&
	git -C worktree branch --show-current >>actual &&

but I personally tend to prefer with a single redirection into ">actual", as that is easier to later add _more_ commands to redirect into 'actual' to be inspected without having to worry about details like repeating ">>actual" or only the first one must be ">actual" (iow, the preference comes mostly from maintainability concerns).

Thanks.

On 10/12/18 1:15 AM, Junio C Hamano wrote:

Thanks for this explanation! You're right, I removed the branch because it otherwise breaks subsequent tests, while the tag doesn't matter. I'll go take a look at how test_when_finished can be used.

Understood, thanks for explaining this as well.

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

Compared to v3, fixed up test cases according to Junio's input

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

On Fri, Oct 12, 2018 at 9:34 AM Daniels Umanovskis <daniels@umanovskis.se> wrote:

This cleanup "checkout" needs to be encapsulated within a test_when_finished(), doesn't it? Preferably just after the "git checkout -b" invocation.

Eric Sunshine <sunshine@sunshineco.com> writes:

In the meantime, here is what I'll have in 'pu' on top.

 t/t3203-branch-output.sh | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

On Tue, Oct 16, 2018 at 7:09 PM Junio C Hamano <gitster@pobox.com> wrote:

This make sense to me.

The subshell '(...)' could become '{...}' now that the 'cd' is gone, but that's a minor point.

Hi Eric,

On Tue, 16 Oct 2018, Eric Sunshine wrote:

Maybe not so minor.

I realized yesterday that the &&-chain linting we use for every single test case takes a noticeable chunk of time:

	$ time ./t0006-date.sh --quiet
	# passed all 67 test(s)
	1..67

	real    0m20.973s
	user    0m2.662s
	sys     0m14.789s

	$ time ./t0006-date.sh --quiet --no-chain-lint
	# passed all 67 test(s)
	1..67

	real    0m13.607s
	user    0m1.330s
	sys     0m8.070s

My suspicion: it is essentially the `(exit 117)` that adds about 100ms to every of those 67 test cases.

(Remember: a subshell requires a fork, and the `fork()` emulation on Windows requires all kinds of things to be copied to a new process, including memory and open file descriptors, before the `exec()` will undo at least part of that.)

With that in mind, I would like to suggest that we should start to be very careful about using subshells in our test suite.

Ciao, Dscho

On Wed, Oct 17, 2018 at 6:18 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

The subshell chain-linter adds a 'sed' and 'grep' invocation to each test which doesn't help. (v1 of the subshell chain-linter only added a 'sed', but that changed with v2.)

You could disable the subshell chain-linter like this if you want test the (exit 117) goop in isolation:

--- 8< ---

Hi Eric,

On Wed, 17 Oct 2018, Eric Sunshine wrote:

You're right! This is actually responsible for about five of those seven seconds. The subshell still hurts a little, as it means that every single of the almost 20,000 test cases we have gets slowed down by ~0.03s, which amounts to almost 10 minutes.

This is "only" for the Windows phase of our Continuous Testing, of course. Yet I think we can do better than this.

How difficult/involved, do you think, would it be to add a t/helper/ command for chain linting?

Ciao, Dscho

On Thu, Oct 18, 2018 at 5:51 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Probably more effort than it's worth, and it would only save one process invocation.

Since the subshell portion of the chain-linting is done by pure textual inspection, an alternative I had considered was to just perform it as a preprocess over the entire test suite, much like the other t/Makefile "test-lint" targets. In other words, the entire test suite might be tested in one go with something like this:

    sed -f chainlint.sed t*.sh | grep -q '?![A-Z][A-Z]*?!' &&
        echo "BROKEN &&-chain"

That won't work today since chainlint.sed isn't written to understand everything which we might see outside of a test_expect_*, but doing it that way is within the realm of possibility. There were two reasons why I didn't pursue that approach.

First, although I was expecting Windows folks to complain (or at least speak up) about the extra 'sed' and 'grep', nobody did, so my impression was that those two extra commands were likely lost in the noise of the rest of the boilerplate commands invoked by test_expect_success(), test_run_(), test_eval_(), etc., and by whatever expensive commands are invoked by each test itself. Second, the top-level &&-chain "(exit 117)" linting kicks in even when you run a single test script manually, say after editing a test, which is exactly when you want to discover that you botched a &&-chain, so it seemed a good idea for the subshell &&-chain linter to follow suit. The t/Makefile "test-lint" targets, on the other hand, don't kick in when running test scripts in isolation.

However, a pragmatic way to gain back those 10 minutes might be simply to disable the chain-linter for continuous integration testing on Windows, but leave it enabled on other platforms. This way, we'd still catch broken &&-chains, with the exception of tests which are specific to Windows, of which I think there are very few.

Daniels Umanovskis <daniels@umanovskis.se> writes:

This is not wrong per-se, but

		git branch --show-current &&
		git -C worktree branch --show-current

would be shorter.

On Fri, Oct 12, 2018 at 03:33:21PM +0200, Daniels Umanovskis wrote:

Are we going forward with advertising this as a scriptable alternative?

Do we need the slightly different check done in print_current_branch_name() ? A very similar check is already done early in cmd_branch.

builtin/branch.c:671
	head = resolve_refdup("HEAD", 0, &head_oid, NULL);
	if (!head)
		die(_("Failed to resolve HEAD as a valid ref."));
	if (!strcmp(head, "HEAD"))
		filter.detached = 1;
	else if (!skip_prefix(head, "refs/heads/", &head))
		die(_("HEAD not found below refs/heads!"));

What's being proposed can be achieved with

+ } else if (show_current) { + if (!filter.detached) + puts(head); + return 0;

without failing tests.

-- Cheers, Rafael Ascensão

On 10/17/18 11:39 AM, Rafael Ascensão wrote:

That's probably up to the maintainers, but I would not explicitly point it out as a script command, so my patch doesn't mention scripting use in the documentation for it. In reality it's useful for "soft scripting" like setting the shell $PS1, which doesn't require API stability guarantees the way proper scripts do.

On Wed, Oct 10, 2018 at 10:54:32PM +0200, Daniels Umanovskis wrote:

[...]

Up to this point everything talked about '--show-current' ...

... but here and in all the following tests you run

  git branch --current

which then fails with "error: unknown option `current'"

On Fri, Oct 12, 2018 at 12:53:26AM +0200, SZEDER Gábor wrote:

Ah, OK, just noticed v3 which has already fixed this.

On 10/12/18 12:56 AM, SZEDER Gábor wrote:

Yeah - squashed the wrong commits locally for v2. Thanks for pointing this out anyway!