Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Oct 31, 2012, 02:13 UTC
Message-ID: <CAMP44s26chrdESPmQGRYBN4dy-C-fGjbgWbR7fBs71bViRYa-w@mail.gmail.com>
In-Reply-To: <20121031003721.GV15167@elie.Belkin>

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

Show 40 quoted lines

> Felipe Contreras wrote:
>
>> --- a/builtin/fast-export.c
>> +++ b/builtin/fast-export.c
>> @@ -523,11 +523,16 @@ static void get_tags_and_duplicates(struct object_array *pending,
>>                               typename(e->item->type));
>>                       continue;
>>               }
>> -             if (commit->util) {
>> -                     /* more than one name for the same object */
>> +
>> +             /*
>> +              * This ref will not be updated through a commit, lets make
>> +              * sure it gets properly upddated eventually.
>> +              */
>> +             if (commit->util || commit->object.flags & SHOWN) {
>>                       if (!(commit->object.flags & UNINTERESTING))
>>                               string_list_append(extra_refs, full_name)->util = commit;
>> -             } else
>> +             }
>> +             if (!commit->util)
>>                       commit->util = full_name;
>
> Here's an explanation of why the above makes sense to me.
>
> get_tags_and_duplicates() gets called after the marks import and
> before the revision walk.  It walks through the revs from the
> commandline and for each one:
>
>  - peels it to a refname, and then to a commit
>  - stores the refname so fast-export knows what arg to pass to
>    the "commit" command during the revision walk
>  - if it already had a refname stored, instead adds the
>    (refname, commit) pair to the extra_refs list, so fast-export
>    knows to add a "reset" command later.
>
> If the commit already has the SHOWN flag set because it was pointed to
> by a mark, it is not going to come up in the revision walk, so it will
> not be mentioned in the output stream unless it is added to
> extra_refs.  That's what this patch does.

That is correct.

> Incidentally, the change from "else" to "if (!commit->util)" is
> unnecessary because if a commit is already SHOWN then it will not be
> encountered in the revision walk so commit->util does not need to be
> set.

Maybe, but that's yet another change, and with more changes come more possibilities of regressions. I haven't verified this is the case.

If this makes sense, I would do it in another, separate patch.

> If the commit does not have the SHOWN or UNINTERESTING flag set but it
> is going to get the UNINTERESTING flag set during the walk because of
> a negative commit listed on the command line, this patch won't help.

I don't know what that means in practice.

Cheers.

-- 
Felipe Contreras