Sverre Rabbelier wrote:
> On Sun, Nov 6, 2011 at 06:01, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
>> 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.
Yeah. My rule when in doubt has been to just include everything that
would remain meaningful over time that I could be putting in a cover
letter for the patch. The hard part is to try to be concise in doing
so.
>> The "git fast-export a..$(git rev-parse HEAD^{commit})" case sounds
>> worth a test.
>
> A test_must_fail?
Yep.
>>> +#define REF_HANDLED (ALL_REV_FLAGS + 1)
>>
>> Could TMP_MARK be used for this?
>
> I don't know its usage, is it?
Since handle_tags_and_duplicates() happens after the main revision
traversal, it would be safe. But it's probably not good style. Any
later revwalk would be confused by or clobber that flag.
My actual worry was that if there are too many rev flags some day,
this REF_HANDLED could wrap around to 0. Now I see that custom
per-command flags are not so rare --- it is just this idiom for
allocating them by adding 1 to the all-ones bitmask that is unusual.
The most common idiom is to simply start with 1u<<16:
#define REF_HANDLED (1u<<16)
unpack-objects uses 1u<<20 instead. blame starts with 1u<<12. reflog
starts with 1u<<10. A part of me wishes the command-specific flags
were allocated in revision.h like the standard ones so one could write
#define REF_HANDLED REVFLAGSUSR1
by analogy with SIGUSR1, or that there were some other mechanism for
avoiding collisions.
>>> + 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?
Sure, there's a lock. It doesn't stop a non-git process-gone-mad like
/bin/rm from deleting a file under .git/refs. :)
die()-ing on error sounds sane.
[...]
>> 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?
It's the handle_tag() call, later in handle_tags_and_duplicates().
[...]
>> 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.
Ah, this is similar to the mysterious case from patch 2/3.
Probably this is the "git fast-export a..a" case, where 'a' was not
dumped because UNINTERESTING but we still want to reset refs/tags/a to
point to it. But won't handle_tag() write
tags refs/heads/a
from :0
[tagger, etc]
when we get to it?
Side question: should the
for (i = extra_refs->nr - 1; i >= 0; i--) {
loop should be earlier in the function and set REF_HANDLED where
appropriate, to avoid resets for these objects, too?
[...]
>> 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.
I suppose this is needed to avoid pointless changes in output which
could break git's or other projects' test suites without good reason.
Makes sense.
Thanks for the clarifications.
Jonathan
--
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