Re: [PATCH v2 0/3] allow checkout and branch to create branches on a merge base

From: Denton Liu <liu.denton@gmail.com>
Date: Apr 26, 2019, 23:40 UTC
Message-ID: <20190426234024.GA28055@dev-l>
In-Reply-To: <xmqqlfzwtlft.fsf@gitster-ct.c.googlers.com>

On Sat, Apr 27, 2019 at 08:07:34AM +0900, Junio C Hamano wrote:

Show 20 quoted lines

> Denton Liu <liu.denton@gmail.com> writes:
> 
> > Thanks for your comments, Eric and Junio.
> >
> > Eric, I've combined the `test_when_finished` calls together so that the
> > statements within appear in a more "logical" order.
> >
> > Junio, I've taken your suggestion and moved the change into
> > `create_branch`. Initially, I didn't want to do this because I didn't
> > want to change the semantics of git-branch but introducing the merge
> > base syntax seems to be a positive change so let's do it.
> > ...
> > Denton Liu (3):
> >   t2018: cleanup in current test
> >   t2018: demonstrate checkout -b merge base bug
> >   branch: make create_branch accept a merge base rev
> 
> Because "checkout -b new" is supposed to be merely a short-hand for
> a "branch new" followed by "checkout new", the lack of "branch new
> A...B" is the same "bug" as the lack of "checkout -b new A...B".

I didn't consider it to be the same "bug" because git-checkout.txt mentions the "..." syntax, whereas it isn't mentioned in git-branch.txt. That being said, now that I look at git-checkout.txt again, when doing a "regular" checkout, the parameter is called <branch> whereas when we're doing a checkout -b, it's called <start_point>, which doesn't mention "...".

Show 8 quoted lines

> 
> The second patch that does not talk about the former but singles out
> only the latter is being inconsistent.
> 
> One person's lack of feature is a bug to another person, and indeed,
> when we did "checkout A...B" in 2009, we weren't interested in doing
> the same for "checkout -b new", and nobody thought about adding that
> until now, and/or considered the lack of feature as a bug.

I agree, based on the above, I now see that it's a lack of feature and not a bug.

Show 16 quoted lines

> 
> We do not "demonstrate" the lack of a new feature in a patch with
> expect-failure, followed by another patch that adds the feature that
> flips expect-failure to expect-success.  A patch that teaches
> "checkout -b" about A...B, that is adding a missing feature, should
> not have to do so.  As it is shades of gray between a change being a
> bugfix and adding a new feature, switching the style of testing
> based on the distinction between them does not make much sense.  Be
> consistent and stick to just one style.  And having the test and the
> code change (be it adding a missing feature or fixing a bug) in a
> single patch makes patch management a lot simpler by making it
> harder to lose only one half.
> 
> Having a preliminary clean-up as a separate step is a good idea, but
> for this topic, I think the latter two should be combined into a
> single patch that changes the code and adds tests at the same time.

I'm going send a reroll to update the documentation to mention "..." in <start_point> and, while I'm at it, I'll do the squash.

Thanks for your comments,

Denton

> 
> Thanks.