Re: [PATCH v7 p2 1/2] fast-export: don't handle uninteresting refs

[Felipe Contreras <felipe.contreras@gmail.com>]

On Thu, Nov 29, 2012 at 2:16 AM, Max Horn <postbox@quendi.de> wrote:
>
> On 28.11.2012, at 23:23, Felipe Contreras wrote:
>
>> They have been marked as UNINTERESTING for a reason, lets respect that.
>>
>> Currently the first ref is handled properly, but not the rest:
>>
>>  % git fast-export master ^uninteresting ^foo ^bar
>
> All these refs are assumed to point to the same object, right? I think it would be better if the commit message stated that explicitly. To make up for the lost space, you could then get rid of one of the four refs, I think three are sufficient to drive the message home ;-).

Yeah, they point to the same object.

> <snip>
>
>> The reason this happens is that before traversing the commits,
>> fast-export checks if any of the refs point to the same object, and any
>> duplicated ref gets added to a list in order to issue 'reset' commands
>> after the traversing. Unfortunately, it's not even checking if the
>> commit is flagged as UNINTERESTING. The fix of course, is to do
>> precisely that.
>
> Hm... So this might be me being a stupid n00b (I am not yet that familiar with the internal rep of things in git and all...)... but I found the "precisely that" par very confusing, because right afterwards, you say:

Yeah, the next part was added afterwards.

>> However, in order to do it properly we need to get the UNINTERESTING flag
>> from the command line ref, not from the commit object.
>
> So this sounds like you are saying "we do *precisely* that, except we don't, because it is more complicated, so we actually don't do this *precisely*, just manner of speaking..."

Well, we do check fro the UNINTERESTING flag, but on the ref, not on the commit.

> Some details here are beyond my knowledge, I am afraid, so I have to resort to guess: In particular it is not clear to me why the "however" part pops up: Reading it makes it sound as if the commit object also carries an UNINTERESTING flag, but we can't use it because of some reason (perhaps it doesn't have the semantics we need?), so we have to look at revs.pending instead. Right? Wrong? Or is it because the commit objects actually do *not* carry the UNINTERESTING bits, hence we need to look at revs.pending. Or is it due to yet another reason?

It's actually revs.cmdline, I typed the wrong one.

If you have two refs pointing to the same object, and you do 'one
^two', the object (e.g. 8c7a786) will get the UNINTERESTING flag, but
that doesn't tell us anything about the ref being a positive or a
negative one, and revs.pending only has the object flags. On the other
hand revs.cmdline does have the flags for the refs.

Does that explain it?

> Anyway, other than these nitpicky questions, this whole thing looks very logical to me, description and code alike. I also played around with tons of "fast-export" invocations, with and without this patch, and it seems to do what the description says. Finally, I went to the various long threads discussion prior versions of this patch, in particular those starting at
>   http://thread.gmane.org/gmane.comp.version-control.git/208725
> and
>   http://thread.gmane.org/gmane.comp.version-control.git/209355/focus=209370
>
> These contained some concerns. Sadly, several of those discussions ultimately degenerated into not-so-pleasant exchanges :-(, and my impression is that as a result some people are not so inclined to comment on these patches anymore at all. Which is a pity :-(. But overall, it seems this patch makes nothing worse, but fixes some things; and it is simple enough that it shouldn't make future improvements harder.
>
> So *I* at least am quite happy with this, it helps me! My impression is that Felipe's latest patch addresses most concerns people raised by means of an improved description. I couldn't find any in those threads that I feel still applies -- but of course those people should speak for themselves, I am simply afraid they don't want to be part of this anymore :-(.

Indeed. For all the concerns given I made a response to how that
either is not true, or doesn't really matter, and in the case of the
latter, I asked for examples where it would matter, only to receive
nothing. For whatever reason involved people are not responding, not a
single valid concern has been raised and remained.

So I think it's good.

Cheers.

-- 
Felipe Contreras