Re: [PATCH 01/11] builtin rebase: support --onto

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Junio C Hamano <gitster@pobox.com>
Cc: Pratik Karki <predatoramigo@gmail.com>,
	git@vger.kernel.org, christian.couder@gmail.com,
	sbeller@google.com, alban.gruin@gmail.com
Subject: Re: [PATCH 01/11] builtin rebase: support --onto
Date: Fri, 24 Aug 2018 18:21:12 +0200 (DST)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.1808241813420.73@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <xmqq600kheb7.fsf@gitster-ct.c.googlers.com>

Hi Junio,

On Wed, 8 Aug 2018, Junio C Hamano wrote:

> Pratik Karki <predatoramigo@gmail.com> writes:
> 
> > The `--onto` option is important, as it allows to rebase a range of
> > commits onto a different base commit (which gave the command its odd
> > name: "rebase").
> 
> Is there anything unimportant?  A rhetorical question, of course.

You might think it is a rhetorical question, but obviously it is not, as
your reaction testifies.

But certainly there are more important options and less important options!
The most important options are those that are frequently used.

> The quite casual and natural use of "to rebase" as a verb in the
> first sentence contradicts with what the parenthetical "its odd
> name" comment says.  Perhaps drop everything after "(which..."?
> 
> i.e.
> 
> 	The `--onto` option allows to rebase a range of commits onto
> 	a different base commit.  Port the support for the option to
> 	the C re-implementation.

I'd rather keep it.

Remember, a story is easier to read than a dull academic treatise. I want
to have a little bit of a personal touch when I stumble over these commit
messages again. And I know I will.

> > This commit introduces options parsing so that different options can
> > be added in future commits.
> 
> We usually do not say "This commit does X", or (worse) "I do X in
> this commit".

Oh, don't we now? ;-)

(This *was* a rhetorical question, as *I* use this tense all the time, and
unless you have quietly rewritten my commit messages without my knowledge
nor consent, the Git commit history is full of these instances.)

> Instead, order the codebase to be like so, e.g.  "Support command line
> options by adding a call to parse_options(); later commits will add more
> options by building on top." or something like that.

To be quite frank with you, I hoped for a review that would focus a teeny
tiny bit on the correctness of the code.

If you want to continue to nit-pick the commit messages, that's fine, of
course, but do understand that I am not really prepared to change a whole
lot there, unless you point out outright errors or false statements. Those
naturally need fixing.

Also, please note that I will now *definitely* focus on bug fixes, as I am
really eager to get those speed improvements into Git for Windows v2.19.0.

And I don't know whether I have said this publicly yet: I will send the
next iterations of Pratik's patch series. He is busy with exams (GSoC
really caters for US schedules, students who are in countries with very
different university schedules are a bit out of luck here), and I really
want these patches.

Ciao,
Dscho