Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 30, 2012, 23:55 UTC
Message-ID: <20121030235506.GT15167@elie.Belkin>
In-Reply-To: <CAMP44s3ArAQXH+-EbH4MHYaV6fTAWdwGzBdZwzn_qtCABHyonQ@mail.gmail.com>

Felipe Contreras wrote:

> On Tue, Oct 30, 2012 at 11:07 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 6 quoted lines

>> Nope.  I just don't want regressions, and found a patch description
>> that did nothing to explain to the reader how it avoids regressions
>> more than a little disturbing.
>
> I see, so you don't have any specific case where this could cause
> regressions, you are just saying it _might_ (like all patches).

Yes, exactly. The commit log needs a description of the current behavior, the intent behind the current code, the change the patch makes, and the motivation behind that change, like all patches. Despite the nice examples, it doesn't currently have that.

The patch description just raises more questions for the reader. From the description, one might imagine that this patch causes

	git fast-export <mark args> master

not to emit anything when another branch that has already been exported is ahead of "master". If I understand correctly (though I haven't tested), this patch does cause

	git fast-export ^next master

not to emit anything when next is ahead of "master". That doesn't seem like progress.

I haven't reviewed the later patches in the series; maybe they fix these things. But in the long term it is much easier to understand and maintain a patch series that does not introduce regressions in the first place, and the context one might use to convincingly explain that a patch is not introducing a regression turns out to be essential for many other purposes as well.

Jonathan