threads / patch / 55928

patch, 4 partsshow-branch: add missing tests, trivial color output fix

Subject: [PATCH 0/4] show-branch: add missing tests, trivial color output fix

## tl;dr

17 messages between Jun 14, 2021 and Jun 17, 2021. Diffs are folded; open one to read it.

replies: 16people: 4as markdown or json

Ævar Arnfjörð Bjarmason· Jun 14, 2021, 17:18 UTC · lore

This is a series I've had lying around for a while, when looking at the color output for show-branch I noticed it reset and re-enabled color for every single space character.

This fixes that, but mostly fixes the mostly non-existing tests for that old command it. It still has big blind spots, but now we have fewer blind spots.

Ævar Arnfjörð Bjarmason (4):
  show-branch tests: rename the one "show-branch" test file
  show-branch tests: modernize test code
  show-branch: fix and test --color output
  show-branch tests: add missing tests
 builtin/show-branch.c          |   9 +-
 t/t3202-show-branch-octopus.sh |  70 ----------------
 t/t3202-show-branch.sh         | 149 +++++++++++++++++++++++++++++++++
 3 files changed, 155 insertions(+), 73 deletions(-)
 delete mode 100755 t/t3202-show-branch-octopus.sh
 create mode 100755 t/t3202-show-branch.sh
-- 
2.32.0.555.g0268d380f7b
Ævar Arnfjörð Bjarmason· Jun 14, 2021, 17:18 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 3/4] show-branch: fix and test --color output

Fix the "show-branch --color" output so it doesn't needlessly color and reset each time it emits a space character.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/show-branch.c  |  9 ++++++---
 t/t3202-show-branch.sh | 30 ++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 3 deletions(-)
Show changes to 2 files +36 −3

builtin/show-branch.c, t/t3202-show-branch.sh

diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index d6d2dabeca8..d77ce7aeb38 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -939,9 +939,12 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 					mark = '*';
 				else
 					mark = '+';
-				printf("%s%c%s",
-				       get_color_code(i),
-				       mark, get_color_reset_code());
+				if (mark == ' ')
+					putchar(mark);
+				else
+					printf("%s%c%s",
+					       get_color_code(i),
+					       mark, get_color_reset_code());
 			}
 			putchar(' ');
 		}
diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index 7b06048905a..54025f03379 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -55,4 +55,34 @@ test_expect_success 'show-branch with showbranch.default' '
 	test_cmp expect actual
 '
 
+test_expect_success 'show-branch --color output' '
+	sed "s/^> //" >expect <<-\EOF &&
+	> <RED>!<RESET> [branch1] branch1
+	>  <GREEN>!<RESET> [branch2] branch2
+	>   <YELLOW>!<RESET> [branch3] branch3
+	>    <BLUE>!<RESET> [branch4] branch4
+	>     <MAGENTA>!<RESET> [branch5] branch5
+	>      <CYAN>!<RESET> [branch6] branch6
+	>       <BOLD;RED>!<RESET> [branch7] branch7
+	>        <BOLD;GREEN>!<RESET> [branch8] branch8
+	>         <BOLD;YELLOW>!<RESET> [branch9] branch9
+	>          <BOLD;BLUE>*<RESET> [branch10] branch10
+	> ----------
+	>          <BOLD;BLUE>*<RESET> [branch10] branch10
+	>         <BOLD;YELLOW>+<RESET>  [branch9] branch9
+	>        <BOLD;GREEN>+<RESET>   [branch8] branch8
+	>       <BOLD;RED>+<RESET>    [branch7] branch7
+	>      <CYAN>+<RESET>     [branch6] branch6
+	>     <MAGENTA>+<RESET>      [branch5] branch5
+	>    <BLUE>+<RESET>       [branch4] branch4
+	>   <YELLOW>+<RESET>        [branch3] branch3
+	>  <GREEN>+<RESET>         [branch2] branch2
+	> <RED>+<RESET>          [branch1] branch1
+	> <RED>+<RESET><GREEN>+<RESET><YELLOW>+<RESET><BLUE>+<RESET><MAGENTA>+<RESET><CYAN>+<RESET><BOLD;RED>+<RESET><BOLD;GREEN>+<RESET><BOLD;YELLOW>+<RESET><BOLD;BLUE>*<RESET> [branch10^] initial
+	EOF
+	git show-branch --color=always $(cat branches.sorted) >actual.raw &&
+	test_decode_color <actual.raw >actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
2.32.0.555.g0268d380f7b
Ævar Arnfjörð Bjarmason· Jun 14, 2021, 17:18 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 4/4] show-branch tests: add missing tests

Add missing tests for --remotes, --list and --merge-base. These are not exhaustive, but better than the nothing we have now.

There were some tests for this command added in f76412ed6db ([PATCH] Add 'git show-branch'., 2005-08-21) has never been properly tested, namely for the --all option in t6432-merge-recursive-space-options.sh, and some of --merge-base and --independent in t6010-merge-base.sh.

This fixes a few more blind spots, but there's still a lot of behavior that's not tested for.

These new tests show the add (and possibly unintentional) behavior of --merge-base with one argument, and how its output is the same as "git merge-base" with N bases in this particular case. See the test added in f621a8454d1 (git-merge-base/git-show-branch --merge-base: Documentation and test, 2009-08-05) for a case where the two aren't the same.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t3202-show-branch.sh | 61 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)
Show changes to t/t3202-show-branch.sh +61 −0
diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index 54025f03379..ad9902a06b9 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -85,4 +85,65 @@ test_expect_success 'show-branch --color output' '
 	test_cmp expect actual
 '
 
+test_expect_success 'show branch --remotes' '
+	cat >expect.err <<-\EOF &&
+	No revs to be shown.
+	EOF
+	git show-branch -r 2>actual.err >actual.out &&
+	test_cmp expect.err actual.err &&
+	test_must_be_empty actual.out
+'
+
+test_expect_success 'setup show branch --list' '
+	sed "s/^> //" >expect <<-\EOF
+	>   [branch1] branch1
+	>   [branch2] branch2
+	>   [branch3] branch3
+	>   [branch4] branch4
+	>   [branch5] branch5
+	>   [branch6] branch6
+	>   [branch7] branch7
+	>   [branch8] branch8
+	>   [branch9] branch9
+	> * [branch10] branch10
+	EOF
+'
+
+test_expect_success 'show branch --list' '
+	git show-branch --list $(cat branches.sorted) >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'show branch --list has no --color output' '
+	git show-branch --color=always --list $(cat branches.sorted) >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'show branch --merge-base with one argument' '
+	for branch in $(cat branches.sorted)
+	do
+		git rev-parse $branch >expect &&
+		git show-branch --merge-base $branch >actual &&
+		test_cmp expect actual
+	done
+'
+
+test_expect_success 'show branch --merge-base with two arguments' '
+	for branch in $(cat branches.sorted)
+	do
+		git rev-parse initial >expect &&
+		git show-branch --merge-base initial $branch >actual &&
+		test_cmp expect actual
+	done
+'
+
+test_expect_success 'show branch --merge-base with N arguments' '
+	git rev-parse initial >expect &&
+	git show-branch --merge-base $(cat branches.sorted) >actual &&
+	test_cmp expect actual &&
+
+	git merge-base $(cat branches.sorted) >actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
2.32.0.555.g0268d380f7b
Michael J Gruber· Jun 15, 2021, 09:24 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 4/4] show-branch tests: add missing tests

Ævar Arnfjörð Bjarmason venit, vidit, dixit 2021-06-14 19:18:10:
Show 12 quoted lines
> Add missing tests for --remotes, --list and --merge-base. These are
> not exhaustive, but better than the nothing we have now.
> 
> There were some tests for this command added in f76412ed6db ([PATCH]
> Add 'git show-branch'., 2005-08-21) has never been properly tested,
> namely for the --all option in t6432-merge-recursive-space-options.sh,
> and some of --merge-base and --independent in t6010-merge-base.sh.
> 
> This fixes a few more blind spots, but there's still a lot of behavior
> that's not tested for.
> 
> These new tests show the add (and possibly unintentional) behavior of
"odd"

Other than that, I don't think show-branch was broken, so I somehow contest the phrase "fix" in this series, it's more of a clean-up.

Just to be sure: Users have no way of assigning a color code with background colors to the columns, which is why omitting the lookup and reset is correct for a space.

Now, people scripting around show-branch might be bitten by that change because the number of (unprocessed) characters in the output changes, or because a control character which they used to "tr" is not there any more. They should not (script this command), I guess.

Show 84 quoted lines
> --merge-base with one argument, and how its output is the same as "git
> merge-base" with N bases in this particular case. See the test added
> in f621a8454d1 (git-merge-base/git-show-branch --merge-base:
> Documentation and test, 2009-08-05) for a case where the two aren't
> the same.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/t3202-show-branch.sh | 61 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
> index 54025f03379..ad9902a06b9 100755
> --- a/t/t3202-show-branch.sh
> +++ b/t/t3202-show-branch.sh
> @@ -85,4 +85,65 @@ test_expect_success 'show-branch --color output' '
>         test_cmp expect actual
>  '
>  
> +test_expect_success 'show branch --remotes' '
> +       cat >expect.err <<-\EOF &&
> +       No revs to be shown.
> +       EOF
> +       git show-branch -r 2>actual.err >actual.out &&
> +       test_cmp expect.err actual.err &&
> +       test_must_be_empty actual.out
> +'
> +
> +test_expect_success 'setup show branch --list' '
> +       sed "s/^> //" >expect <<-\EOF
> +       >   [branch1] branch1
> +       >   [branch2] branch2
> +       >   [branch3] branch3
> +       >   [branch4] branch4
> +       >   [branch5] branch5
> +       >   [branch6] branch6
> +       >   [branch7] branch7
> +       >   [branch8] branch8
> +       >   [branch9] branch9
> +       > * [branch10] branch10
> +       EOF
> +'
> +
> +test_expect_success 'show branch --list' '
> +       git show-branch --list $(cat branches.sorted) >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'show branch --list has no --color output' '
> +       git show-branch --color=always --list $(cat branches.sorted) >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'show branch --merge-base with one argument' '
> +       for branch in $(cat branches.sorted)
> +       do
> +               git rev-parse $branch >expect &&
> +               git show-branch --merge-base $branch >actual &&
> +               test_cmp expect actual
> +       done
> +'
> +
> +test_expect_success 'show branch --merge-base with two arguments' '
> +       for branch in $(cat branches.sorted)
> +       do
> +               git rev-parse initial >expect &&
> +               git show-branch --merge-base initial $branch >actual &&
> +               test_cmp expect actual
> +       done
> +'
> +
> +test_expect_success 'show branch --merge-base with N arguments' '
> +       git rev-parse initial >expect &&
> +       git show-branch --merge-base $(cat branches.sorted) >actual &&
> +       test_cmp expect actual &&
> +
> +       git merge-base $(cat branches.sorted) >actual &&
> +       test_cmp expect actual
> +'
> +
>  test_done
> -- 
> 2.32.0.555.g0268d380f7b
>
Ævar Arnfjörð Bjarmason· Jun 14, 2021, 17:18 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 1/4] show-branch tests: rename the one "show-branch" test file

Rename the only *show-branch* test file to indicate that more tests belong it in than just the one-off octopus test it now contains.

The test was initially added in ce567d1867a (Add test to show that show-branch misses out the 8th column, 2008-07-23) and 11ee57bc4c4 (sort_in_topological_order(): avoid setting a commit flag, 2008-07-23). Those two add almost the same content, one with a test_expect_success and the other a test_expect_failure (a bug being tested for was fixed on one of the branches).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/{t3202-show-branch-octopus.sh => t3202-show-branch.sh} | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
 rename t/{t3202-show-branch-octopus.sh => t3202-show-branch.sh} (95%)
Show changes to 2 files +1 −1

t/t3202-show-branch-octopus.sh, t/t3202-show-branch.sh

diff --git a/t/t3202-show-branch-octopus.sh b/t/t3202-show-branch.sh
similarity index 95%
rename from t/t3202-show-branch-octopus.sh
rename to t/t3202-show-branch.sh
index 5cb0126cfed..8cfbbf79c1b 100755
--- a/t/t3202-show-branch-octopus.sh
+++ b/t/t3202-show-branch.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-test_description='test show-branch with more than 8 heads'
+test_description='test show-branch'
 
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME

-- 
2.32.0.555.g0268d380f7b
Ævar Arnfjörð Bjarmason· Jun 14, 2021, 17:18 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 2/4] show-branch tests: modernize test code

Modernize test code added in ce567d1867a (Add test to show that show-branch misses out the 8th column, 2008-07-23) and 11ee57bc4c4 (sort_in_topological_order(): avoid setting a commit flag, 2008-07-23) to use test helpers.

I'm renaming "out" to "actual" for consistency with other tests, and introducing a "branches.sorted" file in the setup, to make it clear that it's important that the list be sorted in this particular way.

The "show-branch" output is indented with spaces, which would cause complaints under "git show --check" with an indented here-doc block. Let's prefix the lines with "> " to work around that, and to make it clear that the leading whitespace is important.

We can also get rid of the hardcoding of "main" added here in 334afbc76fb (tests: mark tests relying on the current default for `init.defaultBranch`, 2020-11-18). For this test we're setting up an "initial" commit anyway, and now that we've moved over to test_commit we can reference that instead.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t3202-show-branch.sh | 92 ++++++++++++++++++------------------------
 1 file changed, 40 insertions(+), 52 deletions(-)
Show changes to t/t3202-show-branch.sh +40 −51
diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index 8cfbbf79c1b..7b06048905a 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -2,69 +2,57 @@
 
 test_description='test show-branch'
 
-GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
-export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
-
 . ./test-lib.sh
 
-numbers="1 2 3 4 5 6 7 8 9 10"
-
 test_expect_success 'setup' '
-
-	> file &&
-	git add file &&
-	test_tick &&
-	git commit -m initial &&
-
-	for i in $numbers
+	test_commit initial &&
+	for i in $(test_seq 1 10)
 	do
-		git checkout -b branch$i main &&
-		> file$i &&
-		git add file$i &&
-		test_tick &&
-		git commit -m branch$i || return 1
-	done
-
+		git checkout -b branch$i initial &&
+		test_commit --no-tag branch$i
+	done &&
+	git for-each-ref \
+		--sort=version:refname \
+		--format="%(refname:strip=2)" \
+		"refs/heads/branch*" >branches.sorted &&
+	sed "s/^> //" >expect <<-\EOF
+	> ! [branch1] branch1
+	>  ! [branch2] branch2
+	>   ! [branch3] branch3
+	>    ! [branch4] branch4
+	>     ! [branch5] branch5
+	>      ! [branch6] branch6
+	>       ! [branch7] branch7
+	>        ! [branch8] branch8
+	>         ! [branch9] branch9
+	>          * [branch10] branch10
+	> ----------
+	>          * [branch10] branch10
+	>         +  [branch9] branch9
+	>        +   [branch8] branch8
+	>       +    [branch7] branch7
+	>      +     [branch6] branch6
+	>     +      [branch5] branch5
+	>    +       [branch4] branch4
+	>   +        [branch3] branch3
+	>  +         [branch2] branch2
+	> +          [branch1] branch1
+	> +++++++++* [branch10^] initial
+	EOF
 '
 
-cat > expect << EOF
-! [branch1] branch1
- ! [branch2] branch2
-  ! [branch3] branch3
-   ! [branch4] branch4
-    ! [branch5] branch5
-     ! [branch6] branch6
-      ! [branch7] branch7
-       ! [branch8] branch8
-        ! [branch9] branch9
-         * [branch10] branch10
-----------
-         * [branch10] branch10
-        +  [branch9] branch9
-       +   [branch8] branch8
-      +    [branch7] branch7
-     +     [branch6] branch6
-    +      [branch5] branch5
-   +       [branch4] branch4
-  +        [branch3] branch3
- +         [branch2] branch2
-+          [branch1] branch1
-+++++++++* [branch10^] initial
-EOF
-
 test_expect_success 'show-branch with more than 8 branches' '
-
-	git show-branch $(for i in $numbers; do echo branch$i; done) > out &&
-	test_cmp expect out
-
+	git show-branch $(cat branches.sorted) >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'show-branch with showbranch.default' '
-	for i in $numbers; do
-		git config --add showbranch.default branch$i
+	for branch in $(cat branches.sorted)
+	do
+		test_config showbranch.default $branch --add
 	done &&
-	git show-branch >out &&
-	test_cmp expect out
+	git show-branch >actual &&
+	test_cmp expect actual
 '
 
 test_done

-- 
2.32.0.555.g0268d380f7b
Junio C Hamano· Jun 15, 2021, 03:11 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 0/4] show-branch: add missing tests, trivial color output fix

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 7 quoted lines
> This is a series I've had lying around for a while, when looking at
> the color output for show-branch I noticed it reset and re-enabled
> color for every single space character.
>
> This fixes that, but mostly fixes the mostly non-existing tests for
> that old command it. It still has big blind spots, but now we have
> fewer blind spots.

I am surprised if there are still users of this command that I wrote only for my own use and that I no longer use more than once every two months ;-)

Thanks.  Will queue.
Show 13 quoted lines
>
> Ævar Arnfjörð Bjarmason (4):
>   show-branch tests: rename the one "show-branch" test file
>   show-branch tests: modernize test code
>   show-branch: fix and test --color output
>   show-branch tests: add missing tests
>
>  builtin/show-branch.c          |   9 +-
>  t/t3202-show-branch-octopus.sh |  70 ----------------
>  t/t3202-show-branch.sh         | 149 +++++++++++++++++++++++++++++++++
>  3 files changed, 155 insertions(+), 73 deletions(-)
>  delete mode 100755 t/t3202-show-branch-octopus.sh
>  create mode 100755 t/t3202-show-branch.sh
Ævar Arnfjörð Bjarmason· Jun 17, 2021, 10:53 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 0/4] show-branch: add missing tests, trivial color output change

This v2 doesn't change any of the code (see range-diff), but better explains the change per Michael J Gruber's feedback in https://lore.kernel.org/git/162374905722.40525.516266574605586007.git@grubix.eu/

There's also a trivial grammar fix, s/add/odd/g.
Ævar Arnfjörð Bjarmason (4):
  show-branch tests: rename the one "show-branch" test file
  show-branch tests: modernize test code
  show-branch: don't <COLOR></RESET> for space characters
  show-branch tests: add missing tests
 builtin/show-branch.c          |   9 +-
 t/t3202-show-branch-octopus.sh |  70 ----------------
 t/t3202-show-branch.sh         | 149 +++++++++++++++++++++++++++++++++
 3 files changed, 155 insertions(+), 73 deletions(-)
 delete mode 100755 t/t3202-show-branch-octopus.sh
 create mode 100755 t/t3202-show-branch.sh
Range-diff against v1:
1:  7b8ac43339 = 1:  7b8ac43339 show-branch tests: rename the one "show-branch" test file
2:  27f94abaed = 2:  27f94abaed show-branch tests: modernize test code
3:  8db7029086 ! 3:  937e728f7f show-branch: fix and test --color output
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    show-branch: fix and test --color output
    +    show-branch: don't <COLOR></RESET> for space characters
     
    -    Fix the "show-branch --color" output so it doesn't needlessly color
    -    and reset each time it emits a space character.
    +    Change the colored output introduced in ab07ba2a24 (show-branch: color
    +    the commit status signs, 2009-04-22) to not color and reset each
    +    individual space character we use for padding. The intent is to color
    +    just the "!", "+" etc. characters.
    +
    +    This makes the output easier to test, so let's do that now. The test
    +    would be much more verbose without a color/reset for each space
    +    character. Since the coloring cycles through colors we previously had
    +    a "rainbow of space characters".
    +
    +    In theory this breaks things for anyone who's relying on the exact
    +    colored output of show-branch, in practice I'd think anyone parsing it
    +    isn't actively turning on the colored output.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
4:  c54c6a7b20 ! 4:  dde0177235 show-branch tests: add missing tests
    @@ Commit message
         This fixes a few more blind spots, but there's still a lot of behavior
         that's not tested for.
     
    -    These new tests show the add (and possibly unintentional) behavior of
    +    These new tests show the odd (and possibly unintentional) behavior of
         --merge-base with one argument, and how its output is the same as "git
         merge-base" with N bases in this particular case. See the test added
         in f621a8454d1 (git-merge-base/git-show-branch --merge-base:
-- 
2.32.0.571.gdba276db2c
Ævar Arnfjörð Bjarmason· Jun 17, 2021, 10:53 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 1/4] show-branch tests: rename the one "show-branch" test file

Rename the only *show-branch* test file to indicate that more tests belong it in than just the one-off octopus test it now contains.

The test was initially added in ce567d1867a (Add test to show that show-branch misses out the 8th column, 2008-07-23) and 11ee57bc4c4 (sort_in_topological_order(): avoid setting a commit flag, 2008-07-23). Those two add almost the same content, one with a test_expect_success and the other a test_expect_failure (a bug being tested for was fixed on one of the branches).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/{t3202-show-branch-octopus.sh => t3202-show-branch.sh} | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
 rename t/{t3202-show-branch-octopus.sh => t3202-show-branch.sh} (95%)
Show changes to 2 files +1 −1

t/t3202-show-branch-octopus.sh, t/t3202-show-branch.sh

diff --git a/t/t3202-show-branch-octopus.sh b/t/t3202-show-branch.sh
similarity index 95%
rename from t/t3202-show-branch-octopus.sh
rename to t/t3202-show-branch.sh
index 5cb0126cfe..8cfbbf79c1 100755
--- a/t/t3202-show-branch-octopus.sh
+++ b/t/t3202-show-branch.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-test_description='test show-branch with more than 8 heads'
+test_description='test show-branch'
 
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME

-- 
2.32.0.571.gdba276db2c
Felipe Contreras· Jun 17, 2021, 21:16 UTC · re: Ævar Arnfjörð Bjarmason · lore

RE: [PATCH v2 1/4] show-branch tests: rename the one "show-branch" test file

Ævar Arnfjörð Bjarmason wrote:
Show 7 quoted lines
> diff --git a/t/t3202-show-branch-octopus.sh b/t/t3202-show-branch.sh
> similarity index 95%
> rename from t/t3202-show-branch-octopus.sh
> rename to t/t3202-show-branch.sh
> index 5cb0126cfe..8cfbbf79c1 100755
> --- a/t/t3202-show-branch-octopus.sh
> +++ b/t/t3202-show-branch.sh
Makes sense.
-- 
Felipe Contreras
Ævar Arnfjörð Bjarmason· Jun 17, 2021, 10:53 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 2/4] show-branch tests: modernize test code

Modernize test code added in ce567d1867a (Add test to show that show-branch misses out the 8th column, 2008-07-23) and 11ee57bc4c4 (sort_in_topological_order(): avoid setting a commit flag, 2008-07-23) to use test helpers.

I'm renaming "out" to "actual" for consistency with other tests, and introducing a "branches.sorted" file in the setup, to make it clear that it's important that the list be sorted in this particular way.

The "show-branch" output is indented with spaces, which would cause complaints under "git show --check" with an indented here-doc block. Let's prefix the lines with "> " to work around that, and to make it clear that the leading whitespace is important.

We can also get rid of the hardcoding of "main" added here in 334afbc76fb (tests: mark tests relying on the current default for `init.defaultBranch`, 2020-11-18). For this test we're setting up an "initial" commit anyway, and now that we've moved over to test_commit we can reference that instead.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t3202-show-branch.sh | 92 ++++++++++++++++++------------------------
 1 file changed, 40 insertions(+), 52 deletions(-)
Show changes to t/t3202-show-branch.sh +40 −51
diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index 8cfbbf79c1..7b06048905 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -2,69 +2,57 @@
 
 test_description='test show-branch'
 
-GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
-export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
-
 . ./test-lib.sh
 
-numbers="1 2 3 4 5 6 7 8 9 10"
-
 test_expect_success 'setup' '
-
-	> file &&
-	git add file &&
-	test_tick &&
-	git commit -m initial &&
-
-	for i in $numbers
+	test_commit initial &&
+	for i in $(test_seq 1 10)
 	do
-		git checkout -b branch$i main &&
-		> file$i &&
-		git add file$i &&
-		test_tick &&
-		git commit -m branch$i || return 1
-	done
-
+		git checkout -b branch$i initial &&
+		test_commit --no-tag branch$i
+	done &&
+	git for-each-ref \
+		--sort=version:refname \
+		--format="%(refname:strip=2)" \
+		"refs/heads/branch*" >branches.sorted &&
+	sed "s/^> //" >expect <<-\EOF
+	> ! [branch1] branch1
+	>  ! [branch2] branch2
+	>   ! [branch3] branch3
+	>    ! [branch4] branch4
+	>     ! [branch5] branch5
+	>      ! [branch6] branch6
+	>       ! [branch7] branch7
+	>        ! [branch8] branch8
+	>         ! [branch9] branch9
+	>          * [branch10] branch10
+	> ----------
+	>          * [branch10] branch10
+	>         +  [branch9] branch9
+	>        +   [branch8] branch8
+	>       +    [branch7] branch7
+	>      +     [branch6] branch6
+	>     +      [branch5] branch5
+	>    +       [branch4] branch4
+	>   +        [branch3] branch3
+	>  +         [branch2] branch2
+	> +          [branch1] branch1
+	> +++++++++* [branch10^] initial
+	EOF
 '
 
-cat > expect << EOF
-! [branch1] branch1
- ! [branch2] branch2
-  ! [branch3] branch3
-   ! [branch4] branch4
-    ! [branch5] branch5
-     ! [branch6] branch6
-      ! [branch7] branch7
-       ! [branch8] branch8
-        ! [branch9] branch9
-         * [branch10] branch10
-----------
-         * [branch10] branch10
-        +  [branch9] branch9
-       +   [branch8] branch8
-      +    [branch7] branch7
-     +     [branch6] branch6
-    +      [branch5] branch5
-   +       [branch4] branch4
-  +        [branch3] branch3
- +         [branch2] branch2
-+          [branch1] branch1
-+++++++++* [branch10^] initial
-EOF
-
 test_expect_success 'show-branch with more than 8 branches' '
-
-	git show-branch $(for i in $numbers; do echo branch$i; done) > out &&
-	test_cmp expect out
-
+	git show-branch $(cat branches.sorted) >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'show-branch with showbranch.default' '
-	for i in $numbers; do
-		git config --add showbranch.default branch$i
+	for branch in $(cat branches.sorted)
+	do
+		test_config showbranch.default $branch --add
 	done &&
-	git show-branch >out &&
-	test_cmp expect out
+	git show-branch >actual &&
+	test_cmp expect actual
 '
 
 test_done

-- 
2.32.0.571.gdba276db2c
Felipe Contreras· Jun 17, 2021, 21:29 UTC · re: Ævar Arnfjörð Bjarmason · lore

RE: [PATCH v2 2/4] show-branch tests: modernize test code

Ævar Arnfjörð Bjarmason wrote:
Show 8 quoted lines
> Modernize test code added in ce567d1867a (Add test to show that
> show-branch misses out the 8th column, 2008-07-23) and
> 11ee57bc4c4 (sort_in_topological_order(): avoid setting a commit flag,
> 2008-07-23) to use test helpers.
> 
> I'm renaming "out" to "actual" for consistency with other tests, and
> introducing a "branches.sorted" file in the setup, to make it clear
> that it's important that the list be sorted in this particular way.
That is better.
> The "show-branch" output is indented with spaces, which would cause
> complaints under "git show --check" with an indented here-doc
> block. Let's prefix the lines with "> " to work around that, and to
> make it clear that the leading whitespace is important.

I'm not sure this is an improvement. To me the original code is just fine. Also, I don't think writing an 'expect' file belong in a setup step.

Additionally I would do this change in a separate patch.
Show 5 quoted lines
> We can also get rid of the hardcoding of "main" added here in
> 334afbc76fb (tests: mark tests relying on the current default for
> `init.defaultBranch`, 2020-11-18). For this test we're setting up an
> "initial" commit anyway, and now that we've moved over to test_commit
> we can reference that instead.
That's also good.

All the changes in this patch look good to me, however, they are smashed together in a way that makes the review harder, I see:

 1. Use test_commit
 2. Rename out to actual
 3. Use >actual instead of > actual
 4. Use test_seq instead of $numbers
 5. Use branches.sorted instead of branch$i
 6. Use test_config instead of git config
 7. Use internal sed 's/^ //' instead of outside cat

I'm on-board with 6 out of 7, but if these were done it at least 2 patches, they would be clearer. I in fact would prefer one patch per change (although maybe squash 3 with 2).

Cheers.
-- 
Felipe Contreras
Ævar Arnfjörð Bjarmason· Jun 17, 2021, 10:53 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 4/4] show-branch tests: add missing tests

Add missing tests for --remotes, --list and --merge-base. These are not exhaustive, but better than the nothing we have now.

There were some tests for this command added in f76412ed6db ([PATCH] Add 'git show-branch'., 2005-08-21) has never been properly tested, namely for the --all option in t6432-merge-recursive-space-options.sh, and some of --merge-base and --independent in t6010-merge-base.sh.

This fixes a few more blind spots, but there's still a lot of behavior that's not tested for.

These new tests show the odd (and possibly unintentional) behavior of --merge-base with one argument, and how its output is the same as "git merge-base" with N bases in this particular case. See the test added in f621a8454d1 (git-merge-base/git-show-branch --merge-base: Documentation and test, 2009-08-05) for a case where the two aren't the same.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t3202-show-branch.sh | 61 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)
Show changes to t/t3202-show-branch.sh +61 −0
diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index 54025f0337..ad9902a06b 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -85,4 +85,65 @@ test_expect_success 'show-branch --color output' '
 	test_cmp expect actual
 '
 
+test_expect_success 'show branch --remotes' '
+	cat >expect.err <<-\EOF &&
+	No revs to be shown.
+	EOF
+	git show-branch -r 2>actual.err >actual.out &&
+	test_cmp expect.err actual.err &&
+	test_must_be_empty actual.out
+'
+
+test_expect_success 'setup show branch --list' '
+	sed "s/^> //" >expect <<-\EOF
+	>   [branch1] branch1
+	>   [branch2] branch2
+	>   [branch3] branch3
+	>   [branch4] branch4
+	>   [branch5] branch5
+	>   [branch6] branch6
+	>   [branch7] branch7
+	>   [branch8] branch8
+	>   [branch9] branch9
+	> * [branch10] branch10
+	EOF
+'
+
+test_expect_success 'show branch --list' '
+	git show-branch --list $(cat branches.sorted) >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'show branch --list has no --color output' '
+	git show-branch --color=always --list $(cat branches.sorted) >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'show branch --merge-base with one argument' '
+	for branch in $(cat branches.sorted)
+	do
+		git rev-parse $branch >expect &&
+		git show-branch --merge-base $branch >actual &&
+		test_cmp expect actual
+	done
+'
+
+test_expect_success 'show branch --merge-base with two arguments' '
+	for branch in $(cat branches.sorted)
+	do
+		git rev-parse initial >expect &&
+		git show-branch --merge-base initial $branch >actual &&
+		test_cmp expect actual
+	done
+'
+
+test_expect_success 'show branch --merge-base with N arguments' '
+	git rev-parse initial >expect &&
+	git show-branch --merge-base $(cat branches.sorted) >actual &&
+	test_cmp expect actual &&
+
+	git merge-base $(cat branches.sorted) >actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
2.32.0.571.gdba276db2c
Felipe Contreras· Jun 17, 2021, 21:44 UTC · re: Ævar Arnfjörð Bjarmason · lore

RE: [PATCH v2 4/4] show-branch tests: add missing tests

Ævar Arnfjörð Bjarmason wrote:
> Add missing tests for --remotes, --list and --merge-base. These are
> not exhaustive, but better than the nothing we have now.
Indeed, this is better.
Show 68 quoted lines
> --- a/t/t3202-show-branch.sh
> +++ b/t/t3202-show-branch.sh
> @@ -85,4 +85,65 @@ test_expect_success 'show-branch --color output' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'show branch --remotes' '
> +	cat >expect.err <<-\EOF &&
> +	No revs to be shown.
> +	EOF
> +	git show-branch -r 2>actual.err >actual.out &&
> +	test_cmp expect.err actual.err &&
> +	test_must_be_empty actual.out
> +'
> +
> +test_expect_success 'setup show branch --list' '
> +	sed "s/^> //" >expect <<-\EOF
> +	>   [branch1] branch1
> +	>   [branch2] branch2
> +	>   [branch3] branch3
> +	>   [branch4] branch4
> +	>   [branch5] branch5
> +	>   [branch6] branch6
> +	>   [branch7] branch7
> +	>   [branch8] branch8
> +	>   [branch9] branch9
> +	> * [branch10] branch10
> +	EOF
> +'
> +
> +test_expect_success 'show branch --list' '
> +	git show-branch --list $(cat branches.sorted) >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'show branch --list has no --color output' '
> +	git show-branch --color=always --list $(cat branches.sorted) >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'show branch --merge-base with one argument' '
> +	for branch in $(cat branches.sorted)
> +	do
> +		git rev-parse $branch >expect &&
> +		git show-branch --merge-base $branch >actual &&
> +		test_cmp expect actual
> +	done
> +'
> +
> +test_expect_success 'show branch --merge-base with two arguments' '
> +	for branch in $(cat branches.sorted)
> +	do
> +		git rev-parse initial >expect &&
> +		git show-branch --merge-base initial $branch >actual &&
> +		test_cmp expect actual
> +	done
> +'
> +
> +test_expect_success 'show branch --merge-base with N arguments' '
> +	git rev-parse initial >expect &&
> +	git show-branch --merge-base $(cat branches.sorted) >actual &&
> +	test_cmp expect actual &&
> +
> +	git merge-base $(cat branches.sorted) >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_done
All these look good to me.
Cheers.
-- 
Felipe Contreras
Ævar Arnfjörð Bjarmason· Jun 17, 2021, 10:53 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 3/4] show-branch: don't <COLOR></RESET> for space characters

Change the colored output introduced in ab07ba2a24 (show-branch: color the commit status signs, 2009-04-22) to not color and reset each individual space character we use for padding. The intent is to color just the "!", "+" etc. characters.

This makes the output easier to test, so let's do that now. The test would be much more verbose without a color/reset for each space character. Since the coloring cycles through colors we previously had a "rainbow of space characters".

In theory this breaks things for anyone who's relying on the exact colored output of show-branch, in practice I'd think anyone parsing it isn't actively turning on the colored output.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/show-branch.c  |  9 ++++++---
 t/t3202-show-branch.sh | 30 ++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 3 deletions(-)
Show changes to 2 files +36 −3

builtin/show-branch.c, t/t3202-show-branch.sh

diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index d6d2dabeca..d77ce7aeb3 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -939,9 +939,12 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 					mark = '*';
 				else
 					mark = '+';
-				printf("%s%c%s",
-				       get_color_code(i),
-				       mark, get_color_reset_code());
+				if (mark == ' ')
+					putchar(mark);
+				else
+					printf("%s%c%s",
+					       get_color_code(i),
+					       mark, get_color_reset_code());
 			}
 			putchar(' ');
 		}
diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index 7b06048905..54025f0337 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -55,4 +55,34 @@ test_expect_success 'show-branch with showbranch.default' '
 	test_cmp expect actual
 '
 
+test_expect_success 'show-branch --color output' '
+	sed "s/^> //" >expect <<-\EOF &&
+	> <RED>!<RESET> [branch1] branch1
+	>  <GREEN>!<RESET> [branch2] branch2
+	>   <YELLOW>!<RESET> [branch3] branch3
+	>    <BLUE>!<RESET> [branch4] branch4
+	>     <MAGENTA>!<RESET> [branch5] branch5
+	>      <CYAN>!<RESET> [branch6] branch6
+	>       <BOLD;RED>!<RESET> [branch7] branch7
+	>        <BOLD;GREEN>!<RESET> [branch8] branch8
+	>         <BOLD;YELLOW>!<RESET> [branch9] branch9
+	>          <BOLD;BLUE>*<RESET> [branch10] branch10
+	> ----------
+	>          <BOLD;BLUE>*<RESET> [branch10] branch10
+	>         <BOLD;YELLOW>+<RESET>  [branch9] branch9
+	>        <BOLD;GREEN>+<RESET>   [branch8] branch8
+	>       <BOLD;RED>+<RESET>    [branch7] branch7
+	>      <CYAN>+<RESET>     [branch6] branch6
+	>     <MAGENTA>+<RESET>      [branch5] branch5
+	>    <BLUE>+<RESET>       [branch4] branch4
+	>   <YELLOW>+<RESET>        [branch3] branch3
+	>  <GREEN>+<RESET>         [branch2] branch2
+	> <RED>+<RESET>          [branch1] branch1
+	> <RED>+<RESET><GREEN>+<RESET><YELLOW>+<RESET><BLUE>+<RESET><MAGENTA>+<RESET><CYAN>+<RESET><BOLD;RED>+<RESET><BOLD;GREEN>+<RESET><BOLD;YELLOW>+<RESET><BOLD;BLUE>*<RESET> [branch10^] initial
+	EOF
+	git show-branch --color=always $(cat branches.sorted) >actual.raw &&
+	test_decode_color <actual.raw >actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
2.32.0.571.gdba276db2c
Felipe Contreras· Jun 17, 2021, 21:41 UTC · re: Ævar Arnfjörð Bjarmason · lore

RE: [PATCH v2 3/4] show-branch: don't <COLOR></RESET> for space characters

Ævar Arnfjörð Bjarmason wrote:
> Change the colored output introduced in ab07ba2a24 (show-branch: color
> the commit status signs, 2009-04-22) to not color and reset each
> individual space character we use for padding. The intent is to color
> just the "!", "+" etc. characters.
Obviously a fix, but perhaps show the current behavior:
<RED>+<RESET><GREEN> <RESET><YELLOW> <RESET><BLUE> <RESET><MAGENTA> <RESET><CYAN> <RESET><BOLD;RED> <RESET><BOLD;GREEN> <RESET><BOLD;YELLOW> <RESET><BOLD;BLUE> <RESET> [branch1] branch1
Versus:
<RED>+<RESET>          [branch1] branch1
> In theory this breaks things for anyone who's relying on the exact
> colored output of show-branch, in practice I'd think anyone parsing it
> isn't actively turning on the colored output.

Please let's limit our worries to real users. Few people use `git show-branch`, even less would rely on parsing its bogus output.

Show 15 quoted lines
> --- a/builtin/show-branch.c
> +++ b/builtin/show-branch.c
> @@ -939,9 +939,12 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
>  					mark = '*';
>  				else
>  					mark = '+';
> -				printf("%s%c%s",
> -				       get_color_code(i),
> -				       mark, get_color_reset_code());
> +				if (mark == ' ')
> +					putchar(mark);
> +				else
> +					printf("%s%c%s",
> +					       get_color_code(i),
> +					       mark, get_color_reset_code());
Very simple and obvious improvement.
Cheers.
-- 
Felipe Contreras
Felipe Contreras· Jun 17, 2021, 21:46 UTC · re: Ævar Arnfjörð Bjarmason · lore

RE: [PATCH v2 0/4] show-branch: add missing tests, trivial color output change

Ævar Arnfjörð Bjarmason wrote:
Show 11 quoted lines
> This v2 doesn't change any of the code (see range-diff), but better
> explains the change per Michael J Gruber's feedback in
> https://lore.kernel.org/git/162374905722.40525.516266574605586007.git@grubix.eu/
> 
> There's also a trivial grammar fix, s/add/odd/g.
> 
> Ævar Arnfjörð Bjarmason (4):
>   show-branch tests: rename the one "show-branch" test file
>   show-branch tests: modernize test code
>   show-branch: don't <COLOR></RESET> for space characters
>   show-branch tests: add missing tests

All these look good to me (I would prefer patch #2 to be split into multiple patches, but that's not a deal-breaker).

Reviewed-by: Felipe Contreras <felipe.contreras@gmail.com>
-- 
Felipe Contreras

← back to recent threads