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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Oct 31, 2012, 01:23 UTC
Message-ID: <CAMP44s3pZsDa8w46JWmxFt=BdrxDxnB_r1p50p7eOiaVcjNs-w@mail.gmail.com>
In-Reply-To: <20121031005748.GW15167@elie.Belkin>

Hi,

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

Show 5 quoted lines

> Felipe Contreras wrote:
>
>> They have been marked as UNINTERESTING for a reason, lets respect that.
>
> So, the above description conveyed zero information, as you mentioned.

I meant, this, of course:

>> They have been marked as UNINTERESTING for a reason, lets respect that.
>
> This patch looks unsafe,

Which you know, because you received that message without the mistake.

> A clearer explanation would be the following:
>
>         fast-export: don't emit "reset" command for negative refs

What is a negative ref?

>         When "git fast-export" encounters two refs on the commandline

commandline?

Only two refs? How about four?

>         referring to the same commit, it exports the first during the usual
>         commit walk and the second using a "reset" command in a final pass
>         over extra_refs:

That is not exactly true: (next^{commit}).

Show 11 quoted lines

>                 $ git fast-export master next
>                 reset refs/heads/master
>                 commit refs/heads/master
>                 mark :1
>                 author Jonathan Nieder <jrnieder@gmail.com> 1351644412 -0700
>                 committer Jonathan Nieder <jrnieder@gmail.com> 1351644412 -0700
>                 data 17
>                 My first commit!
>
>                 reset refs/heads/next
>                 from :1

I don't think this example is good. Where does it say that 'next' points to master? Using 'points-to-master' or a 'git branch stable master' and using 'master stable'.

Even simpler would be to use 'git fast-export master master'; it would show the same behavior.

Show 15 quoted lines

>         Unfortunately the code to do this doesn't distinguish between positive
>         and negative refs, producing confusing results:
>
>                 $ git fast-export ^master next
>                 reset refs/heads/next
>                 from :0
>
>                 $ git fast-export master ^next
>                 reset refs/heads/next
>                 from :0
>
>         Use revs->cmdline instead of revs->pending to iterate over the rev-list
>         arguments, checking the UNINTERESTING flag bit to distinguish between
>         positive (master, --all, etc) and negative (next.., --not --all, etc)
>         revs and avoid enqueueing negative revs in extra_revs.

Use what? You mean, "To solve the problem, lets use".

But this is not correct, cmdline is not being used. Have you even looked at the patch?

>         This does not affect revs that were excluded from the revision walk
>         because pointed to by a mark, since those use the SHOWN bit on the
>         commit object itself and not UNINTERESTING on the rev_cmdline_entry.

revs? You mean commits?

"excluded because point to by a mark"? Doesn't sound like proper grammar. Maybe "excluded because they were pointed to by a mark".

And I don't see why this paragraph is needed at all. Why would the reader think marks have anything to do with this? There's no mention of marks before.

This might help you, or other people involved in the problem, but not anybody else. Anything related to marks is completely orthogonal to this patch, and there's no point in mentioning that.

-- 
Felipe Contreras