Heya,
On Sun, Nov 6, 2011 at 06:01, Jonathan Nieder <jrnieder@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html