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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 13, 2020, 01:59 UTC
Message-ID: <CAPig+cRvYzm8Cb-AWqOeANRziWyjhWXT32QJ6TsA1==8Joa4zQ@mail.gmail.com>
In-Reply-To: <20200112121402.GH32750@szeder.dev>

On Sun, Jan 12, 2020 at 7:14 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:

Show 12 quoted lines

> On Sat, Jan 11, 2020 at 08:27:11PM -0500, Eric Sunshine wrote:
> > 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'?
>
> What branch name does wt_status_get_state() return while bisecting?
> The branch where I started from?  Because that's what 'git status'
> shows:
> 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.

It's not clear what downside you are pointing out; i.e. why would it be a bad thing to be able to set the branch description even while bisecting -- especially since `git status` affirms that it knows the branch?

Show 8 quoted lines

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

It is one-shot, but the existing `--edit-description` code already cleans up after itself by releasing resources it allocated (as do other one-shot parts of cmd_branch()), so it would be odd and inconsistent for this new code to not clean up after itself, as well (or, more accurately, to only clean up after itself in some branches but not others).

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

A separate preparatory patch doesn't make sense in this case since 'branch_name' becomes "freeable" with this patch itself (prior to that, it was `const char *`).

Anyhow, a different approach was later proposed[1] which eliminates some of the ugliness.

[1]: https://lore.kernel.org/git/20200112101735.GA19676@flurp.local/