threads / patch / 48135

patchbranch -l: print useful info whilst rebasing a non-local branch

Subject: [PATCH] branch -l: print useful info whilst rebasing a non-local branch

## tl;dr

27 messages between Mar 24, 2018 and Apr 4, 2018. Diffs are folded; open one to read it.

replies: 26people: 5as markdown or json

Kaartic Sivaraam· Mar 24, 2018, 18:38 UTC · lore

When rebasing interacitvely (rebase -i), "git branch -l" prints a line indicating the current branch being rebased. This works well when the interactive rebase was intiated when a local branch is checked out.

This doesn't play well when the rebase was initiated on a remote branch or an arbitrary commit that is not pointed to by a local branch. In this case "git branch -l" tries to print the name of a branch using an unintialized variable and thus tries to print a "null pointer string". As a consequence, it does not provide useful information while also inducing undefined behaviour.

So, print the commit from which the rebase started when interactive rebasing a non-local branch.

Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
 ref-filter.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
Show changes to ref-filter.c +9 −1
diff --git a/ref-filter.c b/ref-filter.c
index f9e25aea7..a4c917c96 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1310,8 +1310,16 @@ char *get_head_description(void)
 	wt_status_get_state(&state, 1);
 	if (state.rebase_in_progress ||
 	    state.rebase_interactive_in_progress)
+	{
+		const char *rebasing = NULL;
+		if (state.branch != NULL)
+			rebasing = state.branch;
+		else
+			rebasing = state.detached_from;
+
 		strbuf_addf(&desc, _("(no branch, rebasing %s)"),
-			    state.branch);
+			    rebasing);
+	}
 	else if (state.bisect_in_progress)
 		strbuf_addf(&desc, _("(no branch, bisect started on %s)"),
 			    state.branch);
-- 
2.17.0.rc0.231.g781580f06
Eric Sunshine· Mar 25, 2018, 01:34 UTC · re: Kaartic Sivaraam · lore

Re: [PATCH] branch -l: print useful info whilst rebasing a non-local branch

On Sat, Mar 24, 2018 at 2:38 PM, Kaartic Sivaraam <kaartic.sivaraam@gmail.com> wrote:

> When rebasing interacitvely (rebase -i), "git branch -l" prints a line

The "git branch -l" threw me since "-l" is short for --create-reflog. I'm guessing you meant "git branch --list".

Show 6 quoted lines
> indicating the current branch being rebased. This works well when the
> interactive rebase was intiated when a local branch is checked out.
>
> This doesn't play well when the rebase was initiated on a remote
> branch or an arbitrary commit that is not pointed to by a local
> branch.

A shorter way of saying "arbitrary commit ... not pointed at by local branch" would be "detached HEAD".

Show 7 quoted lines
> In this case "git branch -l" tries to print the name of a
> branch using an unintialized variable and thus tries to print a "null
> pointer string". As a consequence, it does not provide useful
> information while also inducing undefined behaviour.
>
> So, print the commit from which the rebase started when interactive
> rebasing a non-local branch.

Makes sense. The commit message gives enough information for the reader to understand the problem easily.

Show 8 quoted lines
> Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -1310,8 +1310,16 @@ char *get_head_description(void)
>         wt_status_get_state(&state, 1);
>         if (state.rebase_in_progress ||
>             state.rebase_interactive_in_progress)
> +       {
Style: attach '{' to the line above it (don't make it standalone)
Show 9 quoted lines
> +               const char *rebasing = NULL;
> +               if (state.branch != NULL)
> +                       rebasing = state.branch;
> +               else
> +                       rebasing = state.detached_from;
> +
>                 strbuf_addf(&desc, _("(no branch, rebasing %s)"),
> -                           state.branch);
> +                           rebasing);
You could collapse the whole thing back down to:
    strbuf_addf(&desc, _("(no branch, rebasing %s)"),
        state.branch ? state.branch : state.detached_from);
which means you don't need the 'rebasing' variable or the braces.
> +       }
>         else if (state.bisect_in_progress)
Style: cuddle 'else' with '}': } else
>                 strbuf_addf(&desc, _("(no branch, bisect started on %s)"),
>                             state.branch);

Can we have a couple new tests: one checking "git branch --list" for the typical case (when rebasing off a named branch) and one checking when rebasing from a detached HEAD?

Kaartic Sivaraam· Mar 25, 2018, 03:41 UTC · re: Eric Sunshine · lore

Re: [PATCH] branch -l: print useful info whilst rebasing a non-local branch

On Sunday 25 March 2018 07:04 AM, Eric Sunshine wrote:
Show 7 quoted lines
> On Sat, Mar 24, 2018 at 2:38 PM, Kaartic Sivaraam
> <kaartic.sivaraam@gmail.com> wrote:
>> When rebasing interacitvely (rebase -i), "git branch -l" prints a line
> 
> The "git branch -l" threw me since "-l" is short for --create-reflog.
> I'm guessing you meant "git branch --list".
> 

That's surprising, I just tried "git branch -l" on a repository and I did get a list of branch names. Is this a consequence of some option parsing weirdness ?!

To be honest, I actually assumed "-l" to be a shorthand for "--list" and didn't check with it in the documentation; which I should have. Sorry, for that. I still wonder why "git branch -l" prints a list of branch names when it is not a shorthand for "--list" ? (BTW, I'm also surprised by the fact that "-l" is not act shorthand for "--list"!)

Regardless, I'll update the commit message to use "--list" in place of "-l".
Show 10 quoted lines
>> indicating the current branch being rebased. This works well when the
>> interactive rebase was intiated when a local branch is checked out.
>>
>> This doesn't play well when the rebase was initiated on a remote
>> branch or an arbitrary commit that is not pointed to by a local
>> branch.
> 
> A shorter way of saying "arbitrary commit ... not pointed at by local
> branch" would be "detached HEAD".
> 

Thanks. I was actually searching for this word. It didn't strike when I wrote the commit message, yesterday.

Show 7 quoted lines
> You could collapse the whole thing back down to:
> 
>     strbuf_addf(&desc, _("(no branch, rebasing %s)"),
>         state.branch ? state.branch : state.detached_from);
> 
> which means you don't need the 'rebasing' variable or the braces.
> 
Nice point.
> Can we have a couple new tests: one checking "git branch --list" for
> the typical case (when rebasing off a named branch) and one checking
> when rebasing from a detached HEAD?
> 

Sure, but I guess it would take some time for me to add the tests. I'll send a v2 with the suggested changes.

-- 
Kaartic
Jeff King· Mar 25, 2018, 04:10 UTC · re: Kaartic Sivaraam · lore

Re: [PATCH] branch -l: print useful info whilst rebasing a non-local branch

On Sun, Mar 25, 2018 at 09:11:34AM +0530, Kaartic Sivaraam wrote:
Show 8 quoted lines
> >> When rebasing interacitvely (rebase -i), "git branch -l" prints a line
> > 
> > The "git branch -l" threw me since "-l" is short for --create-reflog.
> > I'm guessing you meant "git branch --list".
> 
> That's surprising, I just tried "git branch -l" on a repository and I
> did get a list of branch names. Is this a consequence of some option
> parsing weirdness ?!

Sort of. The "-l" option causes us to set the "reflog" variable to 1. And then we have no other command-line options, so we default to "--list" mode. The listing code does not look at the "reflog" variable at all, so it's just silently ignored.

So:
  git branch -l

_looks_ like it works, but only because list mode is the default. If you did:

  git branch -l foo

you would find that it does list "foo" at all, but instead creates a new branch "foo" with reflog.

Show 5 quoted lines
> To be honest, I actually assumed "-l" to be a shorthand for "--list" and
> didn't check with it in the documentation; which I should have. Sorry,
> for that. I still wonder why "git branch -l" prints a list of branch
> names when it is not a shorthand for "--list" ? (BTW, I'm also surprised
> by the fact that "-l" is not act shorthand for "--list"!)

It's historical and quite unfortunate. Doubly so since probably nobody has ever actually wanted to use the short "-l" to create a reflog, since it's typically the default and has been for a decade.

We've been hesitant to change it due to backwards compatibility. While "branch" is generally considered porcelain, it probably is the main scripting interface for creating branches (the only other option would be using "update-ref" manually). So I dunno. Maybe it would be OK to transition.

Alternatively, we could at least detect the situation that confused you:
Show changes to builtin/branch.c +3 −1
diff --git a/builtin/branch.c b/builtin/branch.c
index 6d0cea9d4b..89e7fdc89c 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -676,6 +676,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		colopts = 0;
 	}
 
+	if (list && reflog)
+		die(_("--reflog in list mode does not make sense"));
+
 	if (force) {
 		delete *= 2;
 		rename *= 2;

That doesn't help somebody mistakenly doing "git branch -l foo", but
more likely they'd do "git branch -l jk/*" if they were trying to list
branches (and then "branch" would barf with "that's not a valid branch
name", though that may still leave them quite confused).

-Peff
Eric Sunshine· Mar 25, 2018, 04:13 UTC · re: Jeff King · lore

Re: [PATCH] branch -l: print useful info whilst rebasing a non-local branch

On Sun, Mar 25, 2018 at 12:10 AM, Jeff King <peff@peff.net> wrote:
Show 11 quoted lines
> So:
>
>   git branch -l
>
> _looks_ like it works, but only because list mode is the default. If you
> did:
>
>   git branch -l foo
>
> you would find that it does list "foo" at all, but instead creates a new
> branch "foo" with reflog.
s/does/doesn't/
Eric Sunshine· Mar 25, 2018, 04:28 UTC · re: Jeff King · lore

Re: [PATCH] branch -l: print useful info whilst rebasing a non-local branch

On Sun, Mar 25, 2018 at 12:10 AM, Jeff King <peff@peff.net> wrote:
Show 12 quoted lines
> Alternatively, we could at least detect the situation that confused you:
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> @@ -676,6 +676,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> +       if (list && reflog)
> +               die(_("--reflog in list mode does not make sense"));
> +
>
> That doesn't help somebody mistakenly doing "git branch -l foo", but
> more likely they'd do "git branch -l jk/*" if they were trying to list
> branches (and then "branch" would barf with "that's not a valid branch
> name", though that may still leave them quite confused).

Assuming that existing clients of "-l" (if there are any) only invoke "git branch -l <name>" to create a new branch, then it would be possible to interpret "-l" as --list when <name> is an existing branch. That is, the "-l" in "git branch -l" and "git branch -l <existing-branch>..." is recognized as --list, and (for backward compatibility only) the "-l" in "git branch -l <new-branch>" is still recognized as --create-reflog.

This idea falls flat, however, if there are clients out there which actually depend upon "git branch -l <existing-branch>" failing.

Jeff King· Mar 25, 2018, 04:33 UTC · re: Eric Sunshine · lore

Re: [PATCH] branch -l: print useful info whilst rebasing a non-local branch

On Sun, Mar 25, 2018 at 12:28:30AM -0400, Eric Sunshine wrote:
Show 24 quoted lines
> On Sun, Mar 25, 2018 at 12:10 AM, Jeff King <peff@peff.net> wrote:
> > Alternatively, we could at least detect the situation that confused you:
> >
> > diff --git a/builtin/branch.c b/builtin/branch.c
> > @@ -676,6 +676,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> > +       if (list && reflog)
> > +               die(_("--reflog in list mode does not make sense"));
> > +
> >
> > That doesn't help somebody mistakenly doing "git branch -l foo", but
> > more likely they'd do "git branch -l jk/*" if they were trying to list
> > branches (and then "branch" would barf with "that's not a valid branch
> > name", though that may still leave them quite confused).
> 
> Assuming that existing clients of "-l" (if there are any) only invoke
> "git branch -l <name>" to create a new branch, then it would be
> possible to interpret "-l" as --list when <name> is an existing
> branch. That is, the "-l" in "git branch -l" and "git branch -l
> <existing-branch>..." is recognized as --list, and (for backward
> compatibility only) the "-l" in "git branch -l <new-branch>" is still
> recognized as --create-reflog.
> 
> This idea falls flat, however, if there are clients out there which
> actually depend upon "git branch -l <existing-branch>" failing.

I agree that might work most of the time as a sort of "do what I mean", but I'd prefer to avoid those kinds of magic rules if we can. They're very hard to explain to the user, and can be quite baffling when they go wrong.

IMHO we should do one of:
  1. Nothing. ;)
  2. Complain about "-l" in list mode to help educate users about the
     current craziness.
  3. Drop "-l" (probably with a deprecation period); it seems unlikely
     to me that anybody uses it for branch creation, and this would at
     least reduce the confusion (then it would just be "so why don't we
     have -l" instead of "why is -l not what I expect").
  4. Repurpose "-l" as a shortcut for --list (also after a deprecation
     period). This is slightly more dangerous in that it may confuse
     people using multiple versions of Git that cross the deprecation
     line. But that's kind of what the deprecation period is for...
-Peff
Kaartic Sivaraam· Mar 25, 2018, 06:54 UTC · re: Jeff King · lore

Re: [PATCH] branch -l: print useful info whilst rebasing a non-local branch

On Sunday 25 March 2018 10:03 AM, Jeff King wrote:
Show 5 quoted lines
> ...
> but I'd prefer to avoid those kinds of magic rules if we can. They're
> very hard to explain to the user, and can be quite baffling when they go
> wrong.
>
I fell the same too.
Show 17 quoted lines
> IMHO we should do one of:
> 
>   1. Nothing. ;)
> 
>   2. Complain about "-l" in list mode to help educate users about the
>      current craziness.
> 
>   3. Drop "-l" (probably with a deprecation period); it seems unlikely
>      to me that anybody uses it for branch creation, and this would at
>      least reduce the confusion (then it would just be "so why don't we
>      have -l" instead of "why is -l not what I expect").
> 
>   4. Repurpose "-l" as a shortcut for --list (also after a deprecation
>      period). This is slightly more dangerous in that it may confuse
>      people using multiple versions of Git that cross the deprecation
>      line. But that's kind of what the deprecation period is for...
> 

I think we should do 2 as a short term fix for sure. For the long term, I would prefer 4 as I think most users would expect "-l" to be a shortcut for "--list" particularly given the current situation that "git branch -l" lists all the branch names.

That said, I would not mind considering 3 if 4 has more bad consequences than the good it does (but I heavily doubt it ;-) ).

I don't consider 1 to be an option ;-)
-- 
Kaartic
Jacob Keller· Mar 25, 2018, 07:15 UTC · re: Jeff King · lore

Re: [PATCH] branch -l: print useful info whilst rebasing a non-local branch

On Sat, Mar 24, 2018 at 9:33 PM, Jeff King <peff@peff.net> wrote:
Show 7 quoted lines
> IMHO we should do one of:
>
>   1. Nothing. ;)
>
>   2. Complain about "-l" in list mode to help educate users about the
>      current craziness.
>

I think we should do this at a minimum. It's easy, and it doesn't break any scripts who are doing something sane.

>   3. Drop "-l" (probably with a deprecation period); it seems unlikely
>      to me that anybody uses it for branch creation, and this would at
>      least reduce the confusion (then it would just be "so why don't we
>      have -l" instead of "why is -l not what I expect").

Personally, I'd prefer this, because it's minimal effort on scripts part to fix themselves to use the long option name for reflog, and doesn't cause that much heart burn.

Show 7 quoted lines
>
>   4. Repurpose "-l" as a shortcut for --list (also after a deprecation
>      period). This is slightly more dangerous in that it may confuse
>      people using multiple versions of Git that cross the deprecation
>      line. But that's kind of what the deprecation period is for...
>
> -Peff

I don't think this is particularly all that valuable, since we default to list mode so it only helps if you want to pass an argument to the list mode (since otherwise we'd create a branch). Maybe it could be useful, but if we did it, I'd do it as a sort of double deprecation period where we use one period to remove the -l functionality entirely, before adding anything back. I think the *gain* of having -l is not really worth it though.

Regards, Jake

Jeff King· Mar 26, 2018, 07:25 UTC · re: Jacob Keller · lore

Re: [PATCH] branch -l: print useful info whilst rebasing a non-local branch

On Sun, Mar 25, 2018 at 12:15:42AM -0700, Jacob Keller wrote:
Show 22 quoted lines
> >   3. Drop "-l" (probably with a deprecation period); it seems unlikely
> >      to me that anybody uses it for branch creation, and this would at
> >      least reduce the confusion (then it would just be "so why don't we
> >      have -l" instead of "why is -l not what I expect").
> 
> Personally, I'd prefer this, because it's minimal effort on scripts
> part to fix themselves to use the long option name for reflog, and
> doesn't cause that much heart burn.
> 
> >
> >   4. Repurpose "-l" as a shortcut for --list (also after a deprecation
> >      period). This is slightly more dangerous in that it may confuse
> >      people using multiple versions of Git that cross the deprecation
> >      line. But that's kind of what the deprecation period is for...
> 
> I don't think this is particularly all that valuable, since we default
> to list mode so it only helps if you want to pass an argument to the
> list mode (since otherwise we'd create a branch). Maybe it could be
> useful, but if we did it, I'd do it as a sort of double deprecation
> period where we use one period to remove the -l functionality
> entirely, before adding anything back. I think the *gain* of having -l
> is not really worth it though.

OK, so here's some patches. We could do the first three now, wait a while before the fourth, and then wait a while (or never) on the fifth.

  [1/5]: t3200: unset core.logallrefupdates when testing reflog creation
  [2/5]: t: switch "branch -l" to "branch --create-reflog"
  [3/5]: branch: deprecate "-l" option
  [4/5]: branch: drop deprecated "-l" option
  [5/5]: branch: make "-l" a synonym for "--list"
 Documentation/git-branch.txt |  3 ++-
 builtin/branch.c             |  4 ++--
 t/t1410-reflog.sh            |  4 ++--
 t/t3200-branch.sh            | 34 +++++++++++++++++-----------------
 4 files changed, 23 insertions(+), 22 deletions(-)
-Peff
Jeff King· Mar 26, 2018, 07:26 UTC · re: Jeff King · lore

[PATCH 1/5] t3200: unset core.logallrefupdates when testing reflog creation

This test checks that the "-l" option creates a reflog. But in fact we'd create one even without it, since the default in a non-bare repository is to do so. Let's unset the config so we can be sure our "-l" option is kicking in.

Note that we can't do this with test_config, since that would leave the variable unset after our test finishes, confusing downstream tests (the helper is not smart enough to restore the previous value, and just always runs test_unconfig).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t3200-branch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t3200-branch.sh +1 −1
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 6c0b7ea4ad..e0c316b71a 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -50,7 +50,7 @@ $_z40 $HEAD $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000	branch:
 EOF
 test_expect_success 'git branch -l d/e/f should create a branch and a log' '
 	GIT_COMMITTER_DATE="2005-05-26 23:30" \
-	git branch -l d/e/f &&
+	git -c core.logallrefupdates=false branch -l d/e/f &&
 	test_path_is_file .git/refs/heads/d/e/f &&
 	test_path_is_file .git/logs/refs/heads/d/e/f &&
 	test_cmp expect .git/logs/refs/heads/d/e/f
-- 
2.17.0.rc1.509.g060626845b
Jeff King· Mar 26, 2018, 07:26 UTC · re: Jeff King · lore

[PATCH 2/5] t: switch "branch -l" to "branch --create-reflog"

In preparation for deprecating "-l", let's make sure we're using the recommended option ourselves.

This patch just mechanically converts "branch -l" to "branch --create-reflog". Note that with the exception of the actual "--create-reflog" test, we could actually remove "-l" entirely from most of these callers. That's because these days core.logallrefupdates defaults to true in a non-bare repository.

I've left them in place, though, since they serve to document the expectation of the test, even if they are technically noops.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t1410-reflog.sh |  4 ++--
 t/t3200-branch.sh | 34 +++++++++++++++++-----------------
 2 files changed, 19 insertions(+), 19 deletions(-)
Show changes to 2 files +19 −19

t/t1410-reflog.sh, t/t3200-branch.sh

diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index 553e26d9ce..8293131001 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -339,8 +339,8 @@ test_expect_failure 'reflog with non-commit entries displays all entries' '
 '
 
 test_expect_success 'reflog expire operates on symref not referrent' '
-	git branch -l the_symref &&
-	git branch -l referrent &&
+	git branch --create-reflog the_symref &&
+	git branch --create-reflog referrent &&
 	git update-ref referrent HEAD &&
 	git symbolic-ref refs/heads/the_symref refs/heads/referrent &&
 	test_when_finished "rm -f .git/refs/heads/referrent.lock" &&
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index e0c316b71a..da97b8a62b 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -48,9 +48,9 @@ test_expect_success 'git branch HEAD should fail' '
 cat >expect <<EOF
 $_z40 $HEAD $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000	branch: Created from master
 EOF
-test_expect_success 'git branch -l d/e/f should create a branch and a log' '
+test_expect_success 'git branch --create-reflog d/e/f should create a branch and a log' '
 	GIT_COMMITTER_DATE="2005-05-26 23:30" \
-	git -c core.logallrefupdates=false branch -l d/e/f &&
+	git -c core.logallrefupdates=false branch --create-reflog d/e/f &&
 	test_path_is_file .git/refs/heads/d/e/f &&
 	test_path_is_file .git/logs/refs/heads/d/e/f &&
 	test_cmp expect .git/logs/refs/heads/d/e/f
@@ -81,7 +81,7 @@ test_expect_success 'git branch -m dumps usage' '
 
 test_expect_success 'git branch -m m broken_symref should work' '
 	test_when_finished "git branch -D broken_symref" &&
-	git branch -l m &&
+	git branch --create-reflog m &&
 	git symbolic-ref refs/heads/broken_symref refs/heads/i_am_broken &&
 	git branch -m m broken_symref &&
 	git reflog exists refs/heads/broken_symref &&
@@ -89,13 +89,13 @@ test_expect_success 'git branch -m m broken_symref should work' '
 '
 
 test_expect_success 'git branch -m m m/m should work' '
-	git branch -l m &&
+	git branch --create-reflog m &&
 	git branch -m m m/m &&
 	git reflog exists refs/heads/m/m
 '
 
 test_expect_success 'git branch -m n/n n should work' '
-	git branch -l n/n &&
+	git branch --create-reflog n/n &&
 	git branch -m n/n n &&
 	git reflog exists refs/heads/n
 '
@@ -377,9 +377,9 @@ mv .git/config-saved .git/config
 git config branch.s/s.dummy Hello
 
 test_expect_success 'git branch -m s/s s should work when s/t is deleted' '
-	git branch -l s/s &&
+	git branch --create-reflog s/s &&
 	git reflog exists refs/heads/s/s &&
-	git branch -l s/t &&
+	git branch --create-reflog s/t &&
 	git reflog exists refs/heads/s/t &&
 	git branch -d s/t &&
 	git branch -m s/s s &&
@@ -443,7 +443,7 @@ test_expect_success 'git branch --copy dumps usage' '
 '
 
 test_expect_success 'git branch -c d e should work' '
-	git branch -l d &&
+	git branch --create-reflog d &&
 	git reflog exists refs/heads/d &&
 	git config branch.d.dummy Hello &&
 	git branch -c d e &&
@@ -458,7 +458,7 @@ test_expect_success 'git branch -c d e should work' '
 '
 
 test_expect_success 'git branch --copy is a synonym for -c' '
-	git branch -l copy &&
+	git branch --create-reflog copy &&
 	git reflog exists refs/heads/copy &&
 	git config branch.copy.dummy Hello &&
 	git branch --copy copy copy-to &&
@@ -485,7 +485,7 @@ test_expect_success 'git branch -c ee ef should copy ee to create branch ef' '
 '
 
 test_expect_success 'git branch -c f/f g/g should work' '
-	git branch -l f/f &&
+	git branch --create-reflog f/f &&
 	git reflog exists refs/heads/f/f &&
 	git config branch.f/f.dummy Hello &&
 	git branch -c f/f g/g &&
@@ -496,7 +496,7 @@ test_expect_success 'git branch -c f/f g/g should work' '
 '
 
 test_expect_success 'git branch -c m2 m2 should work' '
-	git branch -l m2 &&
+	git branch --create-reflog m2 &&
 	git reflog exists refs/heads/m2 &&
 	git config branch.m2.dummy Hello &&
 	git branch -c m2 m2 &&
@@ -505,18 +505,18 @@ test_expect_success 'git branch -c m2 m2 should work' '
 '
 
 test_expect_success 'git branch -c zz zz/zz should fail' '
-	git branch -l zz &&
+	git branch --create-reflog zz &&
 	git reflog exists refs/heads/zz &&
 	test_must_fail git branch -c zz zz/zz
 '
 
 test_expect_success 'git branch -c b/b b should fail' '
-	git branch -l b/b &&
+	git branch --create-reflog b/b &&
 	test_must_fail git branch -c b/b b
 '
 
 test_expect_success 'git branch -C o/q o/p should work when o/p exists' '
-	git branch -l o/q &&
+	git branch --create-reflog o/q &&
 	git reflog exists refs/heads/o/q &&
 	git reflog exists refs/heads/o/p &&
 	git branch -C o/q o/p
@@ -569,10 +569,10 @@ test_expect_success 'git branch -C master5 master5 should work when master is ch
 '
 
 test_expect_success 'git branch -C ab cd should overwrite existing config for cd' '
-	git branch -l cd &&
+	git branch --create-reflog cd &&
 	git reflog exists refs/heads/cd &&
 	git config branch.cd.dummy CD &&
-	git branch -l ab &&
+	git branch --create-reflog ab &&
 	git reflog exists refs/heads/ab &&
 	git config branch.ab.dummy AB &&
 	git branch -C ab cd &&
@@ -684,7 +684,7 @@ test_expect_success 'renaming a symref is not allowed' '
 '
 
 test_expect_success SYMLINKS 'git branch -m u v should fail when the reflog for u is a symlink' '
-	git branch -l u &&
+	git branch --create-reflog u &&
 	mv .git/logs/refs/heads/u real-u &&
 	ln -s real-u .git/logs/refs/heads/u &&
 	test_must_fail git branch -m u v
-- 
2.17.0.rc1.509.g060626845b
Jeff King· Mar 26, 2018, 07:28 UTC · re: Jeff King · lore

[PATCH 3/5] branch: deprecate "-l" option

The "-l" option is short for "--create-reflog". This has caused much confusion over the years. Most people expect it to work as "--list", because that would match the other "mode" options like -d/--delete and -m/--move, as well as the similar -l/--list option of git-tag.

Adding to the confusion, using "-l" _appears_ to work as "--list" in some cases:

  $ git branch -l
  * master

because the branch command defaults to listing (so even trying to specify --list in the command above is redundant). But that may bite the user later when they add a pattern, like:

  $ git branch -l foo

which does not return an empty list, but in fact creates a new branch (with a reflog, naturally) called "foo".

It's also probably quite uncommon for people to actually use "-l" to create a reflog. Since 0bee591869 (Enable reflogs by default in any repository with a working directory., 2006-12-14), this is the default in non-bare repositories. So it's rather unfortunate that the feature squats on the short-and-sweet "-l" (which was only added in 3a4b3f269c (Create/delete branch ref logs., 2006-05-19), meaning there were only 7 months where it was actually useful).

Let's deprecate "-l" in hopes of eventually dropping it (it's a little too soon to repurpose it to "--list", but we may even do that eventually).

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-branch.txt |  3 ++-
 builtin/branch.c             | 17 ++++++++++++++++-
 2 files changed, 18 insertions(+), 2 deletions(-)
Show changes to 2 files +18 −2

Documentation/git-branch.txt, builtin/branch.c

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index b3084c99c1..b959df1cbf 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -91,7 +91,6 @@ OPTIONS
 -D::
 	Shortcut for `--delete --force`.
 
--l::
 --create-reflog::
 	Create the branch's reflog.  This activates recording of
 	all changes made to the branch ref, enabling use of date
@@ -101,6 +100,8 @@ OPTIONS
 	The negated form `--no-create-reflog` only overrides an earlier
 	`--create-reflog`, but currently does not negate the setting of
 	`core.logAllRefUpdates`.
++
+The `-l` option is a deprecated synonym for `--create-reflog`.
 
 -f::
 --force::
diff --git a/builtin/branch.c b/builtin/branch.c
index 6d0cea9d4b..e50a5a1680 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -570,6 +570,15 @@ static int edit_branch_description(const char *branch_name)
 	return 0;
 }
 
+static int deprecated_reflog_option_cb(const struct option *opt,
+				       const char *arg, int unset)
+{
+	warning("the '-l' alias for '--create-reflog' is deprecated;");
+	warning("it will be removed in a future version of Git");
+	*(int *)opt->value = !unset;
+	return 0;
+}
+
 int cmd_branch(int argc, const char **argv, const char *prefix)
 {
 	int delete = 0, rename = 0, copy = 0, force = 0, list = 0;
@@ -612,7 +621,13 @@ 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(0, "list", &list, N_("list branch names")),
-		OPT_BOOL('l', "create-reflog", &reflog, N_("create the branch's reflog")),
+		OPT_BOOL(0, "create-reflog", &reflog, N_("create the branch's reflog")),
+		{
+			OPTION_CALLBACK, 'l', NULL, &reflog, NULL,
+			N_("deprecated synonym for --create-reflog"),
+			PARSE_OPT_NOARG | PARSE_OPT_HIDDEN,
+			deprecated_reflog_option_cb
+		},
 		OPT_BOOL(0, "edit-description", &edit_description,
 			 N_("edit the description for the branch")),
 		OPT__FORCE(&force, N_("force creation, move/rename, deletion"), PARSE_OPT_NOCOMPLETE),
-- 
2.17.0.rc1.509.g060626845b
Jeff King· Mar 26, 2018, 07:29 UTC · re: Jeff King · lore

[PATCH 4/5] branch: drop deprecated "-l" option

We marked the "-l" option as deprecated back in <insert sha1 here>. Now that sufficient time has passed, let's follow through and get rid of it.

Signed-off-by: Jeff King <peff@peff.net>
---
I'll need some help from the maintainer on the commit message. :)
 builtin/branch.c | 15 ---------------
 1 file changed, 15 deletions(-)
Show changes to builtin/branch.c +0 −15
diff --git a/builtin/branch.c b/builtin/branch.c
index e50a5a1680..f7cd333587 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -570,15 +570,6 @@ static int edit_branch_description(const char *branch_name)
 	return 0;
 }
 
-static int deprecated_reflog_option_cb(const struct option *opt,
-				       const char *arg, int unset)
-{
-	warning("the '-l' alias for '--create-reflog' is deprecated;");
-	warning("it will be removed in a future version of Git");
-	*(int *)opt->value = !unset;
-	return 0;
-}
-
 int cmd_branch(int argc, const char **argv, const char *prefix)
 {
 	int delete = 0, rename = 0, copy = 0, force = 0, list = 0;
@@ -622,12 +613,6 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_BIT('C', NULL, &copy, N_("copy a branch, even if target exists"), 2),
 		OPT_BOOL(0, "list", &list, N_("list branch names")),
 		OPT_BOOL(0, "create-reflog", &reflog, N_("create the branch's reflog")),
-		{
-			OPTION_CALLBACK, 'l', NULL, &reflog, NULL,
-			N_("deprecated synonym for --create-reflog"),
-			PARSE_OPT_NOARG | PARSE_OPT_HIDDEN,
-			deprecated_reflog_option_cb
-		},
 		OPT_BOOL(0, "edit-description", &edit_description,
 			 N_("edit the description for the branch")),
 		OPT__FORCE(&force, N_("force creation, move/rename, deletion"), PARSE_OPT_NOCOMPLETE),
-- 
2.17.0.rc1.509.g060626845b
Jeff King· Mar 26, 2018, 07:29 UTC · re: Jeff King · lore

[PATCH 5/5] branch: make "-l" a synonym for "--list"

The other "mode" options of git-branch have short-option aliases that are easy to type (e.g., "-d" and "-m"). Let's give "--list" the same treatment.

This also makes it consistent with the similar "git tag -l" option.

We didn't do this originally because "--create-reflog" was squatting on the "-l" option. Now that sufficient time has passed with that alias removed, we can finally repurpose it.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/branch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to builtin/branch.c +1 −1
diff --git a/builtin/branch.c b/builtin/branch.c
index f7cd333587..fd55e9720e 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -611,7 +611,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_BIT('M', NULL, &rename, N_("move/rename a branch, even if target exists"), 2),
 		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(0, "list", &list, N_("list branch names")),
+		OPT_BOOL('l', "list", &list, N_("list branch names")),
 		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")),
-- 
2.17.0.rc1.509.g060626845b
Eric Sunshine· Mar 26, 2018, 07:44 UTC · re: Jeff King · lore

Re: [PATCH] branch -l: print useful info whilst rebasing a non-local branch

On Mon, Mar 26, 2018 at 3:25 AM, Jeff King <peff@peff.net> wrote:
Show 8 quoted lines
> OK, so here's some patches. We could do the first three now, wait a
> while before the fourth, and then wait a while (or never) on the fifth.
>
>   [1/5]: t3200: unset core.logallrefupdates when testing reflog creation
>   [2/5]: t: switch "branch -l" to "branch --create-reflog"
>   [3/5]: branch: deprecate "-l" option
>   [4/5]: branch: drop deprecated "-l" option
>   [5/5]: branch: make "-l" a synonym for "--list"
The entire series looks good to me. FWIW,
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
Jacob Keller· Mar 26, 2018, 18:38 UTC · re: Eric Sunshine · lore

Re: [PATCH] branch -l: print useful info whilst rebasing a non-local branch

On Mon, Mar 26, 2018 at 12:44 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 13 quoted lines
> On Mon, Mar 26, 2018 at 3:25 AM, Jeff King <peff@peff.net> wrote:
>> OK, so here's some patches. We could do the first three now, wait a
>> while before the fourth, and then wait a while (or never) on the fifth.
>>
>>   [1/5]: t3200: unset core.logallrefupdates when testing reflog creation
>>   [2/5]: t: switch "branch -l" to "branch --create-reflog"
>>   [3/5]: branch: deprecate "-l" option
>>   [4/5]: branch: drop deprecated "-l" option
>>   [5/5]: branch: make "-l" a synonym for "--list"
>
> The entire series looks good to me. FWIW,
>
> Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
Same to me.
Reviewed-by: Jacob Keller <jacob.keller@gmail.com>

Thanks, Jake

Junio C Hamano· Mar 25, 2018, 17:06 UTC · re: Jeff King · lore

Re: [PATCH] branch -l: print useful info whilst rebasing a non-local branch

Jeff King <peff@peff.net> writes:
Show 6 quoted lines
> IMHO we should do one of:
>
>   1. Nothing. ;)
>
>   2. Complain about "-l" in list mode to help educate users about the
>      current craziness.

Nah. We've seen this, perhaps not often but enough times over long period of time. The above two would not fly as a longer term solution.

Show 10 quoted lines
>
>   3. Drop "-l" (probably with a deprecation period); it seems unlikely
>      to me that anybody uses it for branch creation, and this would at
>      least reduce the confusion (then it would just be "so why don't we
>      have -l" instead of "why is -l not what I expect").
>
>   4. Repurpose "-l" as a shortcut for --list (also after a deprecation
>      period). This is slightly more dangerous in that it may confuse
>      people using multiple versions of Git that cross the deprecation
>      line. But that's kind of what the deprecation period is for...
3. is prerequisite for 4.  If we haven't gone through both in 5
years we should be ashamed of ourselves ;-)  But at least we should
start 3. and aim to finish 3. in 2 years if not sooner.
Eric Sunshine· Mar 25, 2018, 05:48 UTC · re: Kaartic Sivaraam · lore

Re: [PATCH] branch -l: print useful info whilst rebasing a non-local branch

On Sun, Mar 25, 2018 at 09:11:34AM +0530, Kaartic Sivaraam wrote:
Show 7 quoted lines
> On Sunday 25 March 2018 07:04 AM, Eric Sunshine wrote:
> > Can we have a couple new tests: one checking "git branch --list" for
> > the typical case (when rebasing off a named branch) and one checking
> > when rebasing from a detached HEAD?
> 
> Sure, but I guess it would take some time for me to add the tests. I'll
> send a v2 with the suggested changes.
A couple more comments:
* Please run the commit message through a spell checker; it contains
  several typographical errors.
* I wonder if it makes sense to give slightly different output in the
  detached HEAD case. Normal output is:
      (no branch, rebasing <branch>)
  and, with your change, detached HEAD output is:
      (no branch, rebasing d3adb33f)
  which is okay, but perhaps it could be better; for instance:
      (no branch, rebasing detached HEAD d3adb33f)

Anyhow, I wrote the tests for you. When you re-roll, you can make the following patch 2/2 and your fix 1/2. (If you go with the above idea of using a slightly different wording for the detached HEAD case, then you'll need to adjust the 'grep' slightly in the second test.)

--- >8 ---
From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sun, 25 Mar 2018 01:29:58 -0400
Subject: [PATCH] t3200: verify "branch --list" sanity when rebasing from
 detached HEAD
"git branch --list" shows an in-progress rebase as:
  * (no branch, rebasing <branch>)
    master
    ...

However, if the rebase is started from a detached HEAD, then there is no <branch>, and it would attempt to print a NULL pointer. The previous commit fixed this problem, so add a test to verify that the output is sane in this situation.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t3200-branch.sh | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)
Show changes to t/t3200-branch.sh +24 −0
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 6c0b7ea4ad..d1f80c80ab 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -6,6 +6,7 @@
 test_description='git branch assorted tests'
 
 . ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-rebase.sh
 
 test_expect_success 'prepare a trivial repository' '
 	echo Hello >A &&
@@ -1246,6 +1247,29 @@ test_expect_success '--merged is incompatible with --no-merged' '
 	test_must_fail git branch --merged HEAD --no-merged HEAD
 '
 
+test_expect_success '--list during rebase' '
+	test_when_finished "reset_rebase" &&
+	git checkout master &&
+	FAKE_LINES="1 edit 2" &&
+	export FAKE_LINES &&
+	set_fake_editor &&
+	git rebase -i HEAD~2 &&
+	git branch --list >actual &&
+	grep "rebasing master" actual
+'
+
+test_expect_success '--list during rebase from detached HEAD' '
+	test_when_finished "reset_rebase && git checkout master" &&
+	git checkout HEAD^0 &&
+	oid=$(git rev-parse --short HEAD) &&
+	FAKE_LINES="1 edit 2" &&
+	export FAKE_LINES &&
+	set_fake_editor &&
+	git rebase -i HEAD~2 &&
+	git branch --list >actual &&
+	grep "rebasing $oid" actual
+'
+
 test_expect_success 'tracking with unexpected .fetch refspec' '
 	rm -rf a b c d &&
 	git init a &&
-- 
2.17.0.rc1.321.gba9d0f2565
--- >8 ---
Kaartic Sivaraam· Mar 25, 2018, 07:36 UTC · re: Eric Sunshine · lore

Re: [PATCH] branch -l: print useful info whilst rebasing a non-local branch

On Sunday 25 March 2018 11:18 AM, Eric Sunshine wrote:
Show 14 quoted lines
> On Sun, Mar 25, 2018 at 09:11:34AM +0530, Kaartic Sivaraam wrote:
>> On Sunday 25 March 2018 07:04 AM, Eric Sunshine wrote:
>>> Can we have a couple new tests: one checking "git branch --list" for
>>> the typical case (when rebasing off a named branch) and one checking
>>> when rebasing from a detached HEAD?
>>
>> Sure, but I guess it would take some time for me to add the tests. I'll
>> send a v2 with the suggested changes.
> 
> A couple more comments:
> 
> * Please run the commit message through a spell checker; it contains
>   several typographical errors.
> 

Thanks for motivating me to search for a spell checker. I have now discovered the spell check feature (:set spell) in Vim!

Show 13 quoted lines
> * I wonder if it makes sense to give slightly different output in the
>   detached HEAD case. Normal output is:
> 
>       (no branch, rebasing <branch>)
> 
>   and, with your change, detached HEAD output is:
> 
>       (no branch, rebasing d3adb33f)
> 
>   which is okay, but perhaps it could be better; for instance:
> 
>       (no branch, rebasing detached HEAD d3adb33f)
> 

I just recently discovered that the variable used to print information related to detached HEAD (state.detached_from) might also contain remote branch names (origin/master, etc.) other than commit hashes. So, it might make sense to distinguish detached HEAD.

> Anyhow, I wrote the tests for you. When you re-roll, you can make the
> following patch 2/2 and your fix 1/2.
Thanks a lot!
Show 69 quoted lines
> (If you go with the above idea
> of using a slightly different wording for the detached HEAD case, then
> you'll need to adjust the 'grep' slightly in the second test.)
> 
> --- >8 ---
> From: Eric Sunshine <sunshine@sunshineco.com>
> Date: Sun, 25 Mar 2018 01:29:58 -0400
> Subject: [PATCH] t3200: verify "branch --list" sanity when rebasing from
>  detached HEAD
> 
> "git branch --list" shows an in-progress rebase as:
> 
>   * (no branch, rebasing <branch>)
>     master
>     ...
> 
> However, if the rebase is started from a detached HEAD, then there is no
> <branch>, and it would attempt to print a NULL pointer. The previous
> commit fixed this problem, so add a test to verify that the output is
> sane in this situation.
> 
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  t/t3200-branch.sh | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 6c0b7ea4ad..d1f80c80ab 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -6,6 +6,7 @@
>  test_description='git branch assorted tests'
>  
>  . ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-rebase.sh
>  
>  test_expect_success 'prepare a trivial repository' '
>  	echo Hello >A &&
> @@ -1246,6 +1247,29 @@ test_expect_success '--merged is incompatible with --no-merged' '
>  	test_must_fail git branch --merged HEAD --no-merged HEAD
>  '
>  
> +test_expect_success '--list during rebase' '
> +	test_when_finished "reset_rebase" &&
> +	git checkout master &&
> +	FAKE_LINES="1 edit 2" &&
> +	export FAKE_LINES &&
> +	set_fake_editor &&
> +	git rebase -i HEAD~2 &&
> +	git branch --list >actual &&
> +	grep "rebasing master" actual
> +'
> +
> +test_expect_success '--list during rebase from detached HEAD' '
> +	test_when_finished "reset_rebase && git checkout master" &&
> +	git checkout HEAD^0 &&
> +	oid=$(git rev-parse --short HEAD) &&
> +	FAKE_LINES="1 edit 2" &&
> +	export FAKE_LINES &&
> +	set_fake_editor &&
> +	git rebase -i HEAD~2 &&
> +	git branch --list >actual &&
> +	grep "rebasing $oid" actual
> +'
> +
>  test_expect_success 'tracking with unexpected .fetch refspec' '
>  	rm -rf a b c d &&
>  	git init a &&
> 
-- 
Kaartic
Kaartic Sivaraam· Apr 3, 2018, 04:31 UTC · re: Kaartic Sivaraam · lore

[PATCH v2 1/2] branch --list: print useful info whilst interactive rebasing a detached HEAD

When rebasing interactively (rebase -i), "git branch --list" prints a line indicating the current branch being rebased. This works well when the interactive rebase is initiated when a local branch is checked out.

This doesn't play well when the rebase is initiated on a detached HEAD. When "git branch --list" tries to print information related to the interactive rebase in this case it tries to print the name of a branch using an uninitialized variable and thus tries to print a "null pointer string". As a consequence, it does not provide useful information while also inducing undefined behaviour.

So, print the point from which the rebase was started when interactive rebasing a detached HEAD.

Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
 ref-filter.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
Show changes to ref-filter.c +8 −4
diff --git a/ref-filter.c b/ref-filter.c
index f9e25aea7..db2baedfe 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1309,10 +1309,14 @@ char *get_head_description(void)
 	memset(&state, 0, sizeof(state));
 	wt_status_get_state(&state, 1);
 	if (state.rebase_in_progress ||
-	    state.rebase_interactive_in_progress)
-		strbuf_addf(&desc, _("(no branch, rebasing %s)"),
-			    state.branch);
-	else if (state.bisect_in_progress)
+	    state.rebase_interactive_in_progress) {
+		if (state.branch)
+			strbuf_addf(&desc, _("(no branch, rebasing %s)"),
+				    state.branch);
+		else
+			strbuf_addf(&desc, _("(no branch, rebasing detached HEAD %s)"),
+				    state.detached_from);
+	} else if (state.bisect_in_progress)
 		strbuf_addf(&desc, _("(no branch, bisect started on %s)"),
 			    state.branch);
 	else if (state.detached_from) {
-- 
2.17.0.rc0.231.g781580f06
Kaartic Sivaraam· Apr 3, 2018, 04:31 UTC · re: Kaartic Sivaraam · lore

[PATCH v2 2/2] t3200: verify "branch --list" sanity when rebasing from detached HEAD

From: Eric Sunshine <sunshine@sunshineco.com>
"git branch --list" shows an in-progress rebase as:
  * (no branch, rebasing <branch>)
    master
    ...

However, if the rebase is started from a detached HEAD, then there is no <branch>, and it would attempt to print a NULL pointer. The previous commit fixed this problem, so add a test to verify that the output is sane in this situation.

Note that the "detached HEAD" test case might actually fail in some cases as the actual output of "git branch --list" might contain remote branch names which is not considered by the test case as it is rare to happen in the test environment.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
 t/t3200-branch.sh | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)
Show changes to t/t3200-branch.sh +24 −0
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 503a88d02..738b5eb22 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -6,6 +6,7 @@
 test_description='git branch assorted tests'
 
 . ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-rebase.sh
 
 test_expect_success 'prepare a trivial repository' '
 	echo Hello >A &&
@@ -1246,6 +1247,29 @@ test_expect_success '--merged is incompatible with --no-merged' '
 	test_must_fail git branch --merged HEAD --no-merged HEAD
 '
 
+test_expect_success '--list during rebase' '
+	test_when_finished "reset_rebase" &&
+	git checkout master &&
+	FAKE_LINES="1 edit 2" &&
+	export FAKE_LINES &&
+	set_fake_editor &&
+	git rebase -i HEAD~2 &&
+	git branch --list >actual &&
+	test_i18ngrep "rebasing master" actual
+'
+
+test_expect_success '--list during rebase from detached HEAD' '
+	test_when_finished "reset_rebase && git checkout master" &&
+	git checkout HEAD^0 &&
+	oid=$(git rev-parse --short HEAD) &&
+	FAKE_LINES="1 edit 2" &&
+	export FAKE_LINES &&
+	set_fake_editor &&
+	git rebase -i HEAD~2 &&
+	git branch --list >actual &&
+	test_i18ngrep "rebasing detached HEAD $oid" actual
+'
+
 test_expect_success 'tracking with unexpected .fetch refspec' '
 	rm -rf a b c d &&
 	git init a &&
-- 
2.17.0.rc0.231.g781580f06
Eric Sunshine· Apr 3, 2018, 08:00 UTC · re: Kaartic Sivaraam · lore

Re: [PATCH v2 2/2] t3200: verify "branch --list" sanity when rebasing from detached HEAD

On Tue, Apr 3, 2018 at 12:31 AM, Kaartic Sivaraam <kaartic.sivaraam@gmail.com> wrote:

Show 17 quoted lines
> From: Eric Sunshine <sunshine@sunshineco.com>
>
> "git branch --list" shows an in-progress rebase as:
>
>   * (no branch, rebasing <branch>)
>     master
>     ...
>
> However, if the rebase is started from a detached HEAD, then there is no
> <branch>, and it would attempt to print a NULL pointer. The previous
> commit fixed this problem, so add a test to verify that the output is
> sane in this situation.
>
> Note that the "detached HEAD" test case might actually fail in some cases
> as the actual output of "git branch --list" might contain remote branch
> names which is not considered by the test case as it is rare to happen
> in the test environment.

This paragraph was not in the original patch[1]. I _think_ what you are saying (which took a while to decipher) is that if a command such as "git checkout origin/next" ever gets inserted into the script before the test, the test will be fooled since "git branch --list" will show "detached HEAD origin/next" rather than "detached HEAD d3adb33f", the latter of which is what the test is expecting.

Unfortunately, this paragraph makes it sound as if the test can fail randomly (which, I believe, is not the case), and nobody would want a test added which is unreliable, thus this paragraph is not helping to sell this patch (in fact, it's actively hurting it). Ideally, the test should be entirely deterministic so that it can't be fooled like this. Rather than including this (harmful) paragraph in the commit message, let's ensure that the test is deterministic (see below).

[1]: https://public-inbox.org/git/20180325054824.GA56795@flurp.local/
Show 8 quoted lines
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> ---
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> @@ -1246,6 +1247,29 @@ test_expect_success '--merged is incompatible with --no-merged' '
> +test_expect_success '--list during rebase from detached HEAD' '
> +       test_when_finished "reset_rebase && git checkout master" &&
> +       git checkout HEAD^0 &&

This is the line which I think is causing concern for you. If someone inserted a new test before this one which invoked "git checkout origin/next" (or something), then even after "git checkout HEAD^0", "git branch --list" would still report the unexpected "detached HEAD origin/next". Let's fix this, and make the test deterministic, by doing this instead:

    git checkout master^0 &&
Thanks.
Show 8 quoted lines
> +       oid=$(git rev-parse --short HEAD) &&
> +       FAKE_LINES="1 edit 2" &&
> +       export FAKE_LINES &&
> +       set_fake_editor &&
> +       git rebase -i HEAD~2 &&
> +       git branch --list >actual &&
> +       test_i18ngrep "rebasing detached HEAD $oid" actual
> +'
Kaartic Sivaraam· Apr 3, 2018, 12:58 UTC · re: Eric Sunshine · lore

Re: [PATCH v2 2/2] t3200: verify "branch --list" sanity when rebasing from detached HEAD

On Tuesday 03 April 2018 01:30 PM, Eric Sunshine wrote:
Show 12 quoted lines
>> Note that the "detached HEAD" test case might actually fail in some cases
>> as the actual output of "git branch --list" might contain remote branch
>> names which is not considered by the test case as it is rare to happen
>> in the test environment.
> 
> This paragraph was not in the original patch[1]. I _think_ what you
> are saying (which took a while to decipher) is that if a command such
> as "git checkout origin/next" ever gets inserted into the script
> before the test, the test will be fooled since "git branch --list"
> will show "detached HEAD origin/next" rather than "detached HEAD
> d3adb33f", the latter of which is what the test is expecting.
> 
Yeah, you're right. To know the reason for the unclear paragraph, see below.
Show 8 quoted lines
> Unfortunately, this paragraph makes it sound as if the test can fail
> randomly (which, I believe, is not the case), and nobody would want a
> test added which is unreliable, thus this paragraph is not helping to
> sell this patch (in fact, it's actively hurting it). Ideally, the test
> should be entirely deterministic so that it can't be fooled like this.
> Rather than including this (harmful) paragraph in the commit message,
> let's ensure that the test is deterministic (see below).
> 

Sorry for the harmful and not so clear paragraph! I actually kept that paragraph there to **remind me** that I have to fix the issue which it describes before sending out the patch but I somehow forgot about it after I added it initially :-(

Show 15 quoted lines
>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
>> @@ -1246,6 +1247,29 @@ test_expect_success '--merged is incompatible with --no-merged' '
>> +test_expect_success '--list during rebase from detached HEAD' '
>> +       test_when_finished "reset_rebase && git checkout master" &&
>> +       git checkout HEAD^0 &&
> 
> This is the line which I think is causing concern for you. If someone
> inserted a new test before this one which invoked "git checkout
> origin/next" (or something), then even after "git checkout HEAD^0",
> "git branch --list" would still report the unexpected "detached HEAD
> origin/next". Let's fix this, and make the test deterministic, by
> doing this instead:
> 
>     git checkout master^0 &&
>

Nice idea, will re-send a v3 with this fix and the harmful paragraph removed.

Thanks, Kaartic

Kaartic Sivaraam· Apr 3, 2018, 14:47 UTC · re: Eric Sunshine · lore

[PATCH v3 2/2] t3200: verify "branch --list" sanity when rebasing from detached HEAD

From: Eric Sunshine <sunshine@sunshineco.com>
"git branch --list" shows an in-progress rebase as:
  * (no branch, rebasing <branch>)
    master
    ...

However, if the rebase is started from a detached HEAD, then there is no <branch>, and it would attempt to print a NULL pointer. The previous commit fixed this problem, so add a test to verify that the output is sane in this situation.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
 t/t3200-branch.sh | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)
Show changes to t/t3200-branch.sh +24 −0
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 503a88d02..89fff3fa9 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -6,6 +6,7 @@
 test_description='git branch assorted tests'
 
 . ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-rebase.sh
 
 test_expect_success 'prepare a trivial repository' '
 	echo Hello >A &&
@@ -1246,6 +1247,29 @@ test_expect_success '--merged is incompatible with --no-merged' '
 	test_must_fail git branch --merged HEAD --no-merged HEAD
 '
 
+test_expect_success '--list during rebase' '
+	test_when_finished "reset_rebase" &&
+	git checkout master &&
+	FAKE_LINES="1 edit 2" &&
+	export FAKE_LINES &&
+	set_fake_editor &&
+	git rebase -i HEAD~2 &&
+	git branch --list >actual &&
+	test_i18ngrep "rebasing master" actual
+'
+
+test_expect_success '--list during rebase from detached HEAD' '
+	test_when_finished "reset_rebase && git checkout master" &&
+	git checkout master^0 &&
+	oid=$(git rev-parse --short HEAD) &&
+	FAKE_LINES="1 edit 2" &&
+	export FAKE_LINES &&
+	set_fake_editor &&
+	git rebase -i HEAD~2 &&
+	git branch --list >actual &&
+	test_i18ngrep "rebasing detached HEAD $oid" actual
+'
+
 test_expect_success 'tracking with unexpected .fetch refspec' '
 	rm -rf a b c d &&
 	git init a &&
-- 
2.17.0.484.g0c8726318
Eric Sunshine· Apr 4, 2018, 08:09 UTC · re: Kaartic Sivaraam · lore

Re: [PATCH v3 2/2] t3200: verify "branch --list" sanity when rebasing from detached HEAD

On Tue, Apr 3, 2018 at 10:47 AM, Kaartic Sivaraam <kaartic.sivaraam@gmail.com> wrote:

Show 15 quoted lines
> From: Eric Sunshine <sunshine@sunshineco.com>
>
> "git branch --list" shows an in-progress rebase as:
>
>   * (no branch, rebasing <branch>)
>     master
>     ...
>
> However, if the rebase is started from a detached HEAD, then there is no
> <branch>, and it would attempt to print a NULL pointer. The previous
> commit fixed this problem, so add a test to verify that the output is
> sane in this situation.
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Thanks. This re-roll looks fine.
Show 48 quoted lines
> ---
>  t/t3200-branch.sh | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 503a88d02..89fff3fa9 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -6,6 +6,7 @@
>  test_description='git branch assorted tests'
>
>  . ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-rebase.sh
>
>  test_expect_success 'prepare a trivial repository' '
>         echo Hello >A &&
> @@ -1246,6 +1247,29 @@ test_expect_success '--merged is incompatible with --no-merged' '
>         test_must_fail git branch --merged HEAD --no-merged HEAD
>  '
>
> +test_expect_success '--list during rebase' '
> +       test_when_finished "reset_rebase" &&
> +       git checkout master &&
> +       FAKE_LINES="1 edit 2" &&
> +       export FAKE_LINES &&
> +       set_fake_editor &&
> +       git rebase -i HEAD~2 &&
> +       git branch --list >actual &&
> +       test_i18ngrep "rebasing master" actual
> +'
> +
> +test_expect_success '--list during rebase from detached HEAD' '
> +       test_when_finished "reset_rebase && git checkout master" &&
> +       git checkout master^0 &&
> +       oid=$(git rev-parse --short HEAD) &&
> +       FAKE_LINES="1 edit 2" &&
> +       export FAKE_LINES &&
> +       set_fake_editor &&
> +       git rebase -i HEAD~2 &&
> +       git branch --list >actual &&
> +       test_i18ngrep "rebasing detached HEAD $oid" actual
> +'
> +
>  test_expect_success 'tracking with unexpected .fetch refspec' '
>         rm -rf a b c d &&
>         git init a &&
> --
> 2.17.0.484.g0c8726318
Kaartic Sivaraam· Apr 3, 2018, 05:02 UTC · re: Kaartic Sivaraam · lore

[PATCH v2 0/2] branch --list: print useful info whilst interactive rebasing a detached HEAD

Ok, I seem to have forgotten the cover letter. So, here's one.
The changes from v1 are as follows:
* Changes to the commit message of 1/2 to fix some errors
* Code changes to 1/2 to address the comments from v1
* Patch 2/2 is new. It's adds tests for the issue that 1/2 tries to fix.
  It's written by Eric with a little fix by me to make it work with
  GETTEXT_POISON.
An interdiff for 1/2:
Show changes to ref-filter.c +7 −11
diff --git a/ref-filter.c b/ref-filter.c
index a4c917c96..db2baedfe 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1309,18 +1309,14 @@ char *get_head_description(void)
        memset(&state, 0, sizeof(state));
        wt_status_get_state(&state, 1);
        if (state.rebase_in_progress ||
-           state.rebase_interactive_in_progress)
-       {
-               const char *rebasing = NULL;
-               if (state.branch != NULL)
-                       rebasing = state.branch;
+           state.rebase_interactive_in_progress) {
+               if (state.branch)
+                       strbuf_addf(&desc, _("(no branch, rebasing %s)"),
+                                   state.branch);
                else
-                       rebasing = state.detached_from;
-
-               strbuf_addf(&desc, _("(no branch, rebasing %s)"),
-                           rebasing);
-       }
-       else if (state.bisect_in_progress)
+                       strbuf_addf(&desc, _("(no branch, rebasing
detached HEAD %s)"),
+                                   state.detached_from);
+       } else if (state.bisect_in_progress)
                strbuf_addf(&desc, _("(no branch, bisect started on %s)"),
                            state.branch);
        else if (state.detached_from) {



Eric Sunshine (1):
  t3200: verify "branch --list" sanity when rebasing from detached HEAD

Kaartic Sivaraam (1):
  branch --list: print useful info whilst interactive rebasing a
    detached HEAD

 ref-filter.c      | 12 ++++++++----
 t/t3200-branch.sh | 24 ++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 4 deletions(-)

← back to recent threads