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

[Junio C Hamano <gitster@pobox.com>]

Daniels Umanovskis <daniels@umanovskis.se> writes:

> +static void print_current_branch_name(void)

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

> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
> index ee6787614..8d2020aea 100755
> --- a/t/t3203-branch-output.sh
> +++ b/t/t3203-branch-output.sh
> @@ -100,6 +100,47 @@ 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
> +'

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

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

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

> +test_expect_success 'git branch `--show-current` works properly when tag exists' '
> +	cat >expect <<-\EOF &&
> +	branch-and-tag-name
> +	EOF
> +	git checkout -b branch-and-tag-name &&
> +	git tag branch-and-tag-name &&
> +	git branch --show-current >actual &&
> +	git checkout branch-one &&
> +	git branch -d branch-and-tag-name &&
> +	test_cmp expect actual
> +'

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

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

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.

> +
>  test_expect_success 'git branch shows detached HEAD properly' '
>  	cat >expect <<EOF &&
>  * (HEAD detached at $(git rev-parse --short HEAD^0))