Re: [PATCH] branch: let '--edit-description' default to rebased branch during rebase

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Jan 12, 2020, 12:14 UTC
Message-ID: <20200112121402.GH32750@szeder.dev>
In-Reply-To: <CAPig+cRCMXjjPHc2O8fLmaSm9m-ZO3qR2BoZwG3s5dLHNbiFFQ@mail.gmail.com>

On Sat, Jan 11, 2020 at 08:27:11PM -0500, Eric Sunshine wrote:

Show 12 quoted lines

> On Sat, Jan 11, 2020 at 9:55 AM Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
> > On Sat, Jan 11, 2020 at 5:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > > On Sat, Jan 11, 2020 at 7:36 AM <marcandre.lureau@redhat.com> wrote:
> > > > +                               if (wt_status_check_rebase(NULL, &state)) {
> > > > +                                       branch_name = state.branch;
> > > > +                               }
> 
> Taking a deeper look at the code, I'm wondering it would make more
> sense to call wt_status_get_state(), which handles 'rebase' and
> 'bisect'. Is there a reason that you limited this check to only
> 'rebase'?

While I do think that defaulting to edit the description of the rebased branch makes sense, I'm not sure how that would work with bisect.

What branch name does wt_status_get_state() return while bisecting? The branch where I started from? Because that's what 'git status' shows:

  ~/src/git (mybranch)$ git bisect start v2.21.0 v2.20.0
  Bisecting: 334 revisions left to test after this (roughly 8 steps)
  [b99a579f8e434a7757f90895945b5711b3f159d5] Merge branch 'sb/more-repo-in-api'
  ~/src/git ((b99a579f8e...)|BISECTING)$ git status 
  HEAD detached at b99a579f8e
  You are currently bisecting, started from branch 'mybranch'.
    (use "git bisect reset" to get back to the original branch)
  
  nothing to commit, working tree clean

But am I really on that branch? Does it really makes sense to edit the description of 'mybranch' by default while bisecting through an old revision range? I do not think so.

Show 12 quoted lines

> > > >                 if (edit_branch_description(branch_name))
> > > >                         return 1;
> > > > +
> > > > +               free(branch_name);
> > >
> > > That `return 1` just above this free() is leaking 'branch_name', isn't it?
> >
> > right, let's fix that too
> 
> Looking at the code itself (rather than consulting only the patch), I
> see that there are a couple more early returns leaking 'branch_name',
> so they need to be handled, as well.

'git branch --edit-description' is a one-shot operation: it allows to edit only one branch description per invocation, and then the process exits right away, whether the operation was successful or some error occurred. I'm not sure free()ing 'branch_name' is worth the effort (and even if it does, I think it should be a separate preparatory patch).