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

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

[Kaartic Sivaraam]

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

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

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

[Eric Sunshine]

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

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

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

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

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

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

[Kaartic Sivaraam]

[-- Attachment #1.1: Type: text/plain, Size: 2078 bytes --]

On Sunday 25 March 2018 07:04 AM, Eric Sunshine wrote:
> 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".


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


> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 870 bytes --]

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

[Jeff King]

On Sun, Mar 25, 2018 at 09:11:34AM +0530, Kaartic Sivaraam 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 ?!

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.

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

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

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

[Eric Sunshine]

On Sun, Mar 25, 2018 at 12:10 AM, Jeff King <peff@peff.net> wrote:
> 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/

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

[Eric Sunshine]

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.

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

[Jeff King]

On Sun, Mar 25, 2018 at 12:28:30AM -0400, Eric Sunshine wrote:

> 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

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

[Eric Sunshine]

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.

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

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

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

[Kaartic Sivaraam]

[-- Attachment #1.1: Type: text/plain, Size: 1436 bytes --]

On Sunday 25 March 2018 10:03 AM, Jeff King wrote:
> ...
> 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.

> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 870 bytes --]

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

[Jacob Keller]

On Sat, Mar 24, 2018 at 9:33 PM, Jeff King <peff@peff.net> wrote:
> 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.

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

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

[Kaartic Sivaraam]

[-- Attachment #1.1: Type: text/plain, Size: 3842 bytes --]

On Sunday 25 March 2018 11:18 AM, Eric Sunshine wrote:
> 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!


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


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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 870 bytes --]

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

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

[Jeff King]

On Sun, Mar 25, 2018 at 12:15:42AM -0700, Jacob Keller wrote:

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

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

[Jeff King]

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

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

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

[Jeff King]

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

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

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

[Jeff King]

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

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

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

[Jeff King]

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

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

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

[Jeff King]

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

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

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

[Eric Sunshine]

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>

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

[Jacob Keller]

On Mon, Mar 26, 2018 at 12:44 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> 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

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

[Kaartic Sivaraam]

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

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

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

[Kaartic Sivaraam]

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

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

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

[Kaartic Sivaraam]

[-- Attachment #1.1: Type: text/plain, Size: 2081 bytes --]

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:

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Eric Sunshine]

On Tue, Apr 3, 2018 at 12:31 AM, Kaartic Sivaraam
<kaartic.sivaraam@gmail.com> wrote:
> 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/

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

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

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

[Kaartic Sivaraam]

[-- Attachment #1.1: Type: text/plain, Size: 2425 bytes --]

On Tuesday 03 April 2018 01:30 PM, Eric Sunshine wrote:
>> 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.


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


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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Kaartic Sivaraam]

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

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

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

[Eric Sunshine]

On Tue, Apr 3, 2018 at 10:47 AM, Kaartic Sivaraam
<kaartic.sivaraam@gmail.com> wrote:
> 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.

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