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

[Jonathan Nieder <jrnieder@gmail.com>]

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

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