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  wrote:
> On Mon, Mar 26, 2018 at 3:25 AM, Jeff King  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 

Same to me.

Reviewed-by: Jacob Keller 

Thanks,
Jake

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

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

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

[Junio C Hamano]

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

[Kaartic Sivaraam]

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 )
> 
>   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 
> 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 )
> master
> ...
> 
> However, if the rebase is started from a detached HEAD, then there is no
> , 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 
> ---
>  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



signature.asc
Description: OpenPGP digital signature

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

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



signature.asc
Description: OpenPGP digital signature

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 )

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

However, if the rebase is started from a detached HEAD, then there is no
, 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 
---
 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

[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  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 " to create a new branch, then it would be
> possible to interpret "-l" as --list when  is an existing
> branch. That is, the "-l" in "git branch -l" and "git branch -l
> ..." is recognized as --list, and (for backward
> compatibility only) the "-l" in "git branch -l " is still
> recognized as --create-reflog.
> 
> This idea falls flat, however, if there are clients out there which
> actually depend upon "git branch -l " 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 12:10 AM, Jeff King  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 " to create a new branch, then it would be
possible to interpret "-l" as --list when  is an existing
branch. That is, the "-l" in "git branch -l" and "git branch -l
..." is recognized as --list, and (for backward
compatibility only) the "-l" in "git branch -l " is still
recognized as --create-reflog.

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

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

[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

[Kaartic Sivaraam]

On Sunday 25 March 2018 07:04 AM, Eric Sunshine wrote:
> On Sat, Mar 24, 2018 at 2:38 PM, 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 ?!

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(, _("(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



signature.asc
Description: OpenPGP digital signature

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
 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 
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -1310,8 +1310,16 @@ char *get_head_description(void)
> wt_status_get_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(, _("(no branch, rebasing %s)"),
> -   state.branch);
> +   rebasing);

You could collapse the whole thing back down to:

strbuf_addf(, _("(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(, _("(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?

[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 
---
 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(, 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(, _("(no branch, rebasing %s)"),
-   state.branch);
+   rebasing);
+   }
else if (state.bisect_in_progress)
strbuf_addf(, _("(no branch, bisect started on %s)"),
state.branch);
-- 
2.17.0.rc0.231.g781580f06