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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 25, 2018, 01:34 UTC
Message-ID: <CAPig+cQ8xw23SGhpx5qtDEyzJGR1v4L2Lm9tEWe56Rh3c8Q3cg@mail.gmail.com>
In-Reply-To: <20180324183844.4565-1-kaartic.sivaraam@gmail.com>

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?