On Sat, Jan 11, 2020 at 08:27:11PM -0500, Eric Sunshine wrote:
> On Sat, Jan 11, 2020 at 9:55 AM Marc-André Lureau
> <marcandre.lureau@xxxxxxxxx> wrote:
> > On Sat, Jan 11, 2020 at 5:28 PM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> > > On Sat, Jan 11, 2020 at 7:36 AM <marcandre.lureau@xxxxxxxxxx> 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.
> > > > 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).
