[v4] branch: introduce --show-current display option
diff mbox series

Message ID 20181012133321.20580-1-daniels@umanovskis.se
State New
Headers show
Series
  • [v4] branch: introduce --show-current display option
Related show

Commit Message

Daniels Umanovskis Oct. 12, 2018, 1:33 p.m. UTC
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(-)

Comments

Eric Sunshine Oct. 12, 2018, 1:43 p.m. UTC | #1
On Fri, Oct 12, 2018 at 9:34 AM Daniels Umanovskis
<daniels@umanovskis.se> wrote:
> 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.
>
> Signed-off-by: Daniels Umanovskis <daniels@umanovskis.se>
> ---
> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
> @@ -100,6 +100,49 @@ test_expect_success 'git branch -v pattern does not show branch summaries' '
> +test_expect_success 'git branch `--show-current` works properly when tag exists' '
> +       cat >expect <<-\EOF &&
> +       branch-and-tag-name
> +       EOF
> +       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 &&
> +       git branch --show-current >actual &&
> +       git checkout branch-one &&

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

> +       test_cmp expect actual
> +'
Junio C Hamano Oct. 16, 2018, 5:59 a.m. UTC | #2
Daniels Umanovskis <daniels@umanovskis.se> writes:

> +test_expect_success 'git branch `--show-current` works properly with worktrees' '
> +	cat >expect <<-\EOF &&
> +	branch-one
> +	branch-two
> +	EOF
> +	git checkout branch-one &&
> +	git worktree add worktree branch-two &&
> +	(
> +		git branch --show-current &&
> +		cd worktree &&
> +		git branch --show-current

This is not wrong per-se, but

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

would be shorter.

> +	) >actual &&
> +	test_cmp expect actual
> +'
Junio C Hamano Oct. 16, 2018, 11:09 p.m. UTC | #3
Eric Sunshine <sunshine@sunshineco.com> writes:

>> +test_expect_success 'git branch `--show-current` works properly when tag exists' '
>> +       cat >expect <<-\EOF &&
>> +       branch-and-tag-name
>> +       EOF
>> +       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 &&
>> +       git branch --show-current >actual &&
>> +       git checkout branch-one &&
>
> This cleanup "checkout" needs to be encapsulated within a
> test_when_finished(), doesn't it? Preferably just after the "git
> checkout -b" invocation.

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

diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index 1bf708dffc..d1f4fec9de 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -119,12 +119,14 @@ test_expect_success 'git branch `--show-current` works properly when tag exists'
 	cat >expect <<-\EOF &&
 	branch-and-tag-name
 	EOF
-	test_when_finished "git branch -D branch-and-tag-name" &&
+	test_when_finished "
+		git checkout branch-one
+		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 &&
 	git branch --show-current >actual &&
-	git checkout branch-one &&
 	test_cmp expect actual
 '
 
@@ -137,8 +139,7 @@ test_expect_success 'git branch `--show-current` works properly with worktrees'
 	git worktree add worktree branch-two &&
 	(
 		git branch --show-current &&
-		cd worktree &&
-		git branch --show-current
+		git -C worktree branch --show-current
 	) >actual &&
 	test_cmp expect actual
 '
Eric Sunshine Oct. 16, 2018, 11:26 p.m. UTC | #4
On Tue, Oct 16, 2018 at 7:09 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > This cleanup "checkout" needs to be encapsulated within a
> > test_when_finished(), doesn't it? Preferably just after the "git
> > checkout -b" invocation.
>
> In the meantime, here is what I'll have in 'pu' on top.
>
> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
> @@ -119,12 +119,14 @@ test_expect_success 'git branch `--show-current` works properly when tag exists'
>         cat >expect <<-\EOF &&
>         branch-and-tag-name
>         EOF
> -       test_when_finished "git branch -D branch-and-tag-name" &&
> +       test_when_finished "
> +               git checkout branch-one
> +               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 &&
>         git branch --show-current >actual &&
> -       git checkout branch-one &&
>         test_cmp expect actual
>  '

This make sense to me.

> @@ -137,8 +139,7 @@ test_expect_success 'git branch `--show-current` works properly with worktrees'
>         git worktree add worktree branch-two &&
>         (
>                 git branch --show-current &&
> -               cd worktree &&
> -               git branch --show-current
> +               git -C worktree branch --show-current
>         ) >actual &&
>         test_cmp expect actual
>  '

The subshell '(...)' could become '{...}' now that the 'cd' is gone,
but that's a minor point.
Rafael Ascensão Oct. 17, 2018, 9:39 a.m. UTC | #5
On Fri, Oct 12, 2018 at 03:33:21PM +0200, Daniels Umanovskis wrote:
> Intended both for scripting and interactive/informative use.
> Unlike git branch --list, no filtering is needed to just get the
> branch name.

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

> +	} else if (show_current) {
> +		print_current_branch_name();
> +		return 0;

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
Johannes Schindelin Oct. 17, 2018, 10:18 a.m. UTC | #6
Hi Eric,

On Tue, 16 Oct 2018, Eric Sunshine wrote:

> On Tue, Oct 16, 2018 at 7:09 PM Junio C Hamano <gitster@pobox.com> wrote:
> > Eric Sunshine <sunshine@sunshineco.com> writes:
> > > This cleanup "checkout" needs to be encapsulated within a
> > > test_when_finished(), doesn't it? Preferably just after the "git
> > > checkout -b" invocation.
> >
> > In the meantime, here is what I'll have in 'pu' on top.
> >
> > diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
> > @@ -119,12 +119,14 @@ test_expect_success 'git branch `--show-current` works properly when tag exists'
> >         cat >expect <<-\EOF &&
> >         branch-and-tag-name
> >         EOF
> > -       test_when_finished "git branch -D branch-and-tag-name" &&
> > +       test_when_finished "
> > +               git checkout branch-one
> > +               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 &&
> >         git branch --show-current >actual &&
> > -       git checkout branch-one &&
> >         test_cmp expect actual
> >  '
> 
> This make sense to me.
> 
> > @@ -137,8 +139,7 @@ test_expect_success 'git branch `--show-current` works properly with worktrees'
> >         git worktree add worktree branch-two &&
> >         (
> >                 git branch --show-current &&
> > -               cd worktree &&
> > -               git branch --show-current
> > +               git -C worktree branch --show-current
> >         ) >actual &&
> >         test_cmp expect actual
> >  '
> 
> The subshell '(...)' could become '{...}' now that the 'cd' is gone,
> but that's a minor point.

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
Eric Sunshine Oct. 17, 2018, 10:39 a.m. UTC | #7
On Wed, Oct 17, 2018 at 6:18 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> 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
>         real    0m20.973s
>         $ time ./t0006-date.sh --quiet --no-chain-lint
>         real    0m13.607s
>
> My suspicion: it is essentially the `(exit 117)` that adds about 100ms to
> every of those 67 test cases.

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

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

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

--- 8< ---
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 3f95bfda60..48323e503c 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -675,8 +675,7 @@ test_run_ () {
 		trace=
 		# 117 is magic because it is unlikely to match the exit
 		# code of other programs
-		if $(printf '%s\n' "$1" | sed -f "$GIT_BUILD_DIR/t/chainlint.sed" | grep -q '?![A-Z][A-Z]*?!') ||
-			test "OK-117" != "$(test_eval_ "(exit 117) && $1${LF}${LF}echo OK-\$?" 3>&1)"
+		if test "OK-117" != "$(test_eval_ "(exit 117) && $1${LF}${LF}echo OK-\$?" 3>&1)"
 		then
 			error "bug in the test script: broken &&-chain or run-away HERE-DOC: $1"
 		fi
--- 8< ---
Daniels Umanovskis Oct. 17, 2018, 5:36 p.m. UTC | #8
On 10/17/18 11:39 AM, Rafael Ascensão wrote:
> On Fri, Oct 12, 2018 at 03:33:21PM +0200, Daniels Umanovskis wrote:
>> Intended both for scripting and interactive/informative use.
>> Unlike git branch --list, no filtering is needed to just get the
>> branch name.
> 
> Are we going forward with advertising this as a scriptable alternative?

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.
Johannes Schindelin Oct. 18, 2018, 9:51 a.m. UTC | #9
Hi Eric,

On Wed, 17 Oct 2018, Eric Sunshine wrote:

> On Wed, Oct 17, 2018 at 6:18 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > 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
> >         real    0m20.973s
> >         $ time ./t0006-date.sh --quiet --no-chain-lint
> >         real    0m13.607s
> >
> > My suspicion: it is essentially the `(exit 117)` that adds about 100ms to
> > every of those 67 test cases.
> 
> 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.)
> 
> > With that in mind, I would like to suggest that we should start to be very
> > careful about using subshells in our test suite.
> 
> You could disable the subshell chain-linter like this if you want test the (exit 117) goop in isolation:
> 
> --- 8< ---
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 3f95bfda60..48323e503c 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -675,8 +675,7 @@ test_run_ () {
>  		trace=
>  		# 117 is magic because it is unlikely to match the exit
>  		# code of other programs
> -		if $(printf '%s\n' "$1" | sed -f "$GIT_BUILD_DIR/t/chainlint.sed" | grep -q '?![A-Z][A-Z]*?!') ||
> -			test "OK-117" != "$(test_eval_ "(exit 117) && $1${LF}${LF}echo OK-\$?" 3>&1)"
> +		if test "OK-117" != "$(test_eval_ "(exit 117) && $1${LF}${LF}echo OK-\$?" 3>&1)"
>  		then
>  			error "bug in the test script: broken &&-chain or run-away HERE-DOC: $1"
>  		fi
> --- 8< ---

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
Eric Sunshine Oct. 18, 2018, 2:19 p.m. UTC | #10
On Thu, Oct 18, 2018 at 5:51 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Wed, 17 Oct 2018, Eric Sunshine wrote:
> > On Wed, Oct 17, 2018 at 6:18 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > > My suspicion: it is essentially the `(exit 117)` that adds about 100ms to
> > > every of those 67 test cases.
> >
> > 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:
>
> 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?

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.

Patch
diff mbox series

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index bf5316ffa..0babb9b1b 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -9,7 +9,7 @@  SYNOPSIS
 --------
 [verse]
 'git branch' [--color[=<when>] | --no-color] [-r | -a]
-	[--list] [-v [--abbrev=<length> | --no-abbrev]]
+	[--list] [--show-current] [-v [--abbrev=<length> | --no-abbrev]]
 	[--column[=<options>] | --no-column] [--sort=<key>]
 	[(--merged | --no-merged) [<commit>]]
 	[--contains [<commit]] [--no-contains [<commit>]]
@@ -160,6 +160,10 @@  This option is only applicable in non-verbose mode.
 	branch --list 'maint-*'`, list only the branches that match
 	the pattern(s).
 
+--show-current::
+	Print the name of the current branch. In detached HEAD state,
+	nothing is printed.
+
 -v::
 -vv::
 --verbose::
diff --git a/builtin/branch.c b/builtin/branch.c
index c396c4153..46f91dc06 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -443,6 +443,21 @@  static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
 	free(to_free);
 }
 
+static void print_current_branch_name(void)
+{
+	int flags;
+	const char *refname = resolve_ref_unsafe("HEAD", 0, NULL, &flags);
+	const char *shortname;
+	if (!refname)
+		die(_("could not resolve HEAD"));
+	else if (!(flags & REF_ISSYMREF))
+		return;
+	else if (skip_prefix(refname, "refs/heads/", &shortname))
+		puts(shortname);
+	else
+		die(_("HEAD (%s) points outside of refs/heads/"), refname);
+}
+
 static void reject_rebase_or_bisect_branch(const char *target)
 {
 	struct worktree **worktrees = get_worktrees(0);
@@ -581,6 +596,7 @@  static int edit_branch_description(const char *branch_name)
 int cmd_branch(int argc, const char **argv, const char *prefix)
 {
 	int delete = 0, rename = 0, copy = 0, force = 0, list = 0;
+	int show_current = 0;
 	int reflog = 0, edit_description = 0;
 	int quiet = 0, unset_upstream = 0;
 	const char *new_upstream = NULL;
@@ -620,6 +636,7 @@  int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_BIT('c', "copy", &copy, N_("copy a branch and its reflog"), 1),
 		OPT_BIT('C', NULL, &copy, N_("copy a branch, even if target exists"), 2),
 		OPT_BOOL('l', "list", &list, N_("list branch names")),
+		OPT_BOOL(0, "show-current", &show_current, N_("show current branch name")),
 		OPT_BOOL(0, "create-reflog", &reflog, N_("create the branch's reflog")),
 		OPT_BOOL(0, "edit-description", &edit_description,
 			 N_("edit the description for the branch")),
@@ -662,14 +679,15 @@  int cmd_branch(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options, builtin_branch_usage,
 			     0);
 
-	if (!delete && !rename && !copy && !edit_description && !new_upstream && !unset_upstream && argc == 0)
+	if (!delete && !rename && !copy && !edit_description && !new_upstream &&
+	    !show_current && !unset_upstream && argc == 0)
 		list = 1;
 
 	if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr ||
 	    filter.no_commit)
 		list = 1;
 
-	if (!!delete + !!rename + !!copy + !!new_upstream +
+	if (!!delete + !!rename + !!copy + !!new_upstream + !!show_current +
 	    list + unset_upstream > 1)
 		usage_with_options(builtin_branch_usage, options);
 
@@ -697,6 +715,9 @@  int cmd_branch(int argc, const char **argv, const char *prefix)
 		if (!argc)
 			die(_("branch name required"));
 		return delete_branches(argc, argv, delete > 1, filter.kind, quiet);
+	} else if (show_current) {
+		print_current_branch_name();
+		return 0;
 	} else if (list) {
 		/*  git branch --local also shows HEAD when it is detached */
 		if ((filter.kind & FILTER_REFS_BRANCHES) && filter.detached)
diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index ee6787614..1bf708dff 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -100,6 +100,49 @@  test_expect_success 'git branch -v pattern does not show branch summaries' '
 	test_must_fail git branch -v branch*
 '
 
+test_expect_success 'git branch `--show-current` shows current branch' '
+	cat >expect <<-\EOF &&
+	branch-two
+	EOF
+	git checkout branch-two &&
+	git branch --show-current >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'git branch `--show-current` is silent when detached HEAD' '
+	git checkout HEAD^0 &&
+	git branch --show-current >actual &&
+	test_must_be_empty actual
+'
+
+test_expect_success 'git branch `--show-current` works properly when tag exists' '
+	cat >expect <<-\EOF &&
+	branch-and-tag-name
+	EOF
+	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 &&
+	git branch --show-current >actual &&
+	git checkout branch-one &&
+	test_cmp expect actual
+'
+
+test_expect_success 'git branch `--show-current` works properly with worktrees' '
+	cat >expect <<-\EOF &&
+	branch-one
+	branch-two
+	EOF
+	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
+'
+
 test_expect_success 'git branch shows detached HEAD properly' '
 	cat >expect <<EOF &&
 * (HEAD detached at $(git rev-parse --short HEAD^0))