From: Sverre Rabbelier <srabbelier@gmail.com>
Date: Sun, 06 Nov 2011 19:48:02 GMT
Subject: Re: [PATCH 3/3] fast-export: output reset command for commandline revs
Message-ID: <CAGdFq_gkSxvw9Di_mUqS5N0bgCWh-dygMe_DWcR+ENAo=A-3=A@mail.gmail.com>
In-Reply-To: <20111106050126.GO27272@elie.hsd1.il.comcast.net>

Heya,

On Sun, Nov 6, 2011 at 06:01, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Thanks.  I'd suggest squashing in the test from patch 1/3 for easy
> reference (since each patch makes the other easier to understand).

Yes, agreed. The initial series was 5 patches in total, but splitting
it out for such a small series (and small patch at that) makes less
sense.


> These details seem like good details for the commit message, so the
> next puzzled person looking at the code can see what behavior is
> deliberate and what are the incidental side-effects.

All of it? I wasn't sure what part should go in the commit message.

> The "git fast-export a..$(git rev-parse HEAD^{commit})" case sounds
> worth a test.

A test_must_fail?

>> +#define REF_HANDLED (ALL_REV_FLAGS + 1)
>
> Could TMP_MARK be used for this?

I don't know its usage, is it?

> -static void handle_tags_and_duplicates(struct string_list *extra_refs)
>> +static void handle_tags_and_duplicates(struct rev_info *revs, struct string_list *extra_refs)
>>  {
>>       int i;
>>
>> +     /* even if no commits were exported, we need to export the ref */
>> +     for (i = 0; i < revs->cmdline.nr; i++) {
>
> Might be clearer in a new function.

Yes, probably. handle_cmdline_refs?

>> +             struct rev_cmdline_entry *elem = &revs->cmdline.rev[i];
>> +
>> +             if (elem->flags & UNINTERESTING)
>> +                     continue;
>> +
>> +             if (elem->whence != REV_CMD_REV && elem->whence != REV_CMD_RIGHT)
>> +                     continue;
>
> Oh, neat.

Yes, I must admit that this bit was easier than I dreaded it would be
(I must admit that's been a large reason that I haven't taken the time
to work on this till now). With the fast-export and remote-helper
tests to guide me, I was able to code-by-accident the right conditions
here :).

>> +
>> +             char *full_name;
>
> declaration-after-statement

Ah, yes.

>> +             dwim_ref(elem->name, strlen(elem->name), elem->item->sha1, &full_name);
>> +
>> +             if (!prefixcmp(full_name, "refs/tags/") &&
>
> What happens if dwim_ref fails, perhaps because a ref was deleted in
> the meantime?

That would be bad. I assumed that we have a lock on the refs, should I
add back the die check that's done by the other dwim_ref caller?

>> +                     (tag_of_filtered_mode != REWRITE ||
>> +                     !get_object_mark(elem->item)))
>> +                     continue;
>
> Style nit: this would be easier to read if the "if" condition doesn't
> line up with the code below it:
>
>                if (!prefixcmp(full_name, "refs/tags/")) {
>                        if (tag_of_filtered_mode != REWRITE ||
>                            !get_object_mark(elem->item))
>                                continue;
>                }

Yeah, that does look better :).

> If tag_of_filtered_mode == ABORT, we are going to die() soon, right?

I don't know to be honest, perhaps we would have already died by now?
I don't know the details of how the tag_of_filtered_mode part is
implemented.

> So this seems to be about tag_of_filtered_mode == DROP --- makes
> sense.
>
> When does the !get_object_mark() case come up?

Eh, it has something to do with it being a replacement (rather than
the same), maybe? This is mostly just taken from Dscho's original
patch.

>> +             if (!(elem->flags & REF_HANDLED)) {
>> +                     handle_reset(full_name, elem->item);
>> +                     elem->flags |= REF_HANDLED;
>> +             }
>
> Just curious: is the REF_HANDLED handling actually needed?  What
> would happen if fast-export included the redundant resets?

That would just be sloppy :). I don't think anything particularly bad
would happen.

> Thanks for a pleasant read.

Thanks for the review.

-- 
Cheers,

Sverre Rabbelier