Re: [PATCH 1/2] revision: keep track of the end-user input from the command line

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 26, 2011, 02:21 UTC
Message-ID: <7vy5yggbfv.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CAGdFq_he-skXpEy903RSY2ravDXo1eVCN19RzJeHXQCT__HTmA@mail.gmail.com>

Sverre Rabbelier <srabbelier@gmail.com> writes:

Show 6 quoted lines

>> Prepare a separate array to keep track of what syntactic element was used
>> to cause each object to appear in the pending array from the command line,
>> and populate it as setup_revisions() parses the command line.
>
> Thank you! I was really dreading looking into this myself, so I'm very
> glad that you could find the time to look into it yourself.

I debated long and hard if I should instead fatten object array entry and shove this information there without adding a new structure, which would have resulted in something very similar to what you had, so you should take some credit for the code, and also credit for a large part of the motivation (the second paragraph in the log is entirely your use case).

We might end up unifying this command line information array and the pending object array after reviewing what other future callers would want from this new information, but at least by doing it this way I can rest assured that no existing code that is unaware of the mechanism would get any unintended side effects in the earlier rounds.

Show 10 quoted lines

>> @@ -835,6 +853,7 @@ static void handle_one_reflog_commit(unsigned char *sha1, void *cb_data)
>>                struct object *o = parse_object(sha1);
>>                if (o) {
>>                        o->flags |= cb->all_flags;
>> +                       /* ??? CMDLINEFLAGS ??? */
>>                        add_pending_object(cb->all_revs, o, "");
>>                }
>>                else if (!cb->warned_bad_reflog) {
>
> What is happening here?

We could have add_rev_cmdline() call there if we really wanted to, but I decided not to do so for two reasons:

(1) with "rev-list -g HEAD", the user is not explicitly mentioning all
    objects in the reflog of the HEAD---it might still make sense to mark
    HEAD as explicitly mentioned as positive, but the code to do so will
    not sit here anyway; and more importantly,

(2) I was appalled by how broken the design and the implementation of
    walking the reflog was (it shoves _all_ the objects in the entire
    history recorded in the reflog to the pending queue before letting the
    caller do an iota of work). It is against the general design of Git,
    and the design of the revision walk machinery in particular that tries
    very hard to be incremental. It goes against a good software design
    taste.

For the latter reason, I think in the longer term we should correct the implementation to walk the reflog to keep an iterator (a structure that holds an open file descriptor to a reflog file, and perhaps a little bit of buffer) in struct rev_info, and teach get_revisions() to lazily read from there, reading the file backward. Once that happens, this callback from for_each_reflog_ent() will go away, so I didn't bother.