Re: [PATCH 3/3] fast-export: output reset command for commandline revs
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Nov 7, 2011, 08:58 UTC
- Message-ID
- <20111107085846.GA30641@elie.hsd1.il.comcast.net>
- In-Reply-To
- <CAGdFq_gkSxvw9Di_mUqS5N0bgCWh-dygMe_DWcR+ENAo=A-3=A@mail.gmail.com>
Sverre Rabbelier wrote:
> On Sun, Nov 6, 2011 at 06:01, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 5 quoted lines
>> 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.
Show 5 quoted lines
>>> +#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.
Show 9 quoted lines
>>> + 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().
[...]
Show 5 quoted lines
>> 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?
[...]
Show 5 quoted lines
>> 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