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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Oct 31, 2012, 01:03 UTC
Message-ID: <CAMP44s1ftDijYpZW_Reu5qNi1T_L52_353ngNaRW3W1gz+k9jw@mail.gmail.com>
In-Reply-To: <20121030235506.GT15167@elie.Belkin>

On Wed, Oct 31, 2012 at 12:55 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 22 quoted lines

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

This is already the case.

I don't see what part of my patch description would give you the idea that this would change in any way how the objects are flagged, or how get_revision() decides how to traverse them.

I clearly stated that this doesn't affect *the first* ref, which is handled properly already; this patch affects *the rest* of the refs, of which you have none in that command above.

Show 7 quoted lines

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

Again, this is already the case RIGHT NOW.

And nothing in my description should give you an idea that anything would change for this case because the 2nd ref (*the first* doesn't get affected), is not marked as UNINTERESTING.

Not only you are not reading what is in the description, but I don't think you understand what the code actually does, and how it behaves.

Let me give you some examples:

% git fast-export ^next next reset refs/heads/next from :0

% git fast-export ^next next^{commit} # nothing % git fast-export ^next next~0 # nothing % git fast-export ^next next~1 # nothing % git fast-export ^next next~2 # nothing ... # you get the idea

The *only time* when this patch would have any effect is when you specify more than *one ref*, and they both point to *exactly the same object*.

Additionally, and this is something I just found out; when the are pure refs (e.g. 'next'), and not refs to objects (e.g. 'next^{commit}').

In any other case; *there would be no change*.

After my patch:

% git fast-export ^next next # nothing % git fast-export ^next next^{commit} # nothing % git fast-export ^next next~0 # nothing % git fast-export ^next next~1 # nothing % git fast-export ^next next~2 # nothing ... # you get the idea

> 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

It does not introduce regressions.

I don't think it's my job to explain to you how 'git fast-export' works. Above you made too many assumptions of what get broken, when in fact that's the current behavior already... maybe, just maybe, you are also making wrong assumptions about this patch as well.

-- 
Felipe Contreras