Re: [PATCH/RFC] rev-list: add --authorship-order alternative ordering

From: Elliott Cable <me@ell.io>
Date: Jun 6, 2013, 19:03 UTC
Message-ID: <CAPZ477OFM6D4n_Wz-OozN=aYn5-LmNA2ggL+9GNrbGrRQh9pRQ@mail.gmail.com>
In-Reply-To: <7vobbl60aj.fsf@alter.siamese.dyndns.org>

On Tue, Jun 4, 2013 at 3:14 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 7 quoted lines

> elliottcable <me@ell.io> writes:
>> Thus, I've added an --authorship-order version of --date-order, which relies
>> upon the AUTHOR_DATE instead of the COMMITTER_DATE; this means that old commits
>> will continue to show up chronologically in-order despite rebasing.
>> ---
>
> Missing sign-off.  Please see Documentation/SubmittingPatches.

Will-do.

I read that part, and was rather confused. At no point did I get the idea that I should sign-off *my own initial commit*. Perhaps that part of the documentation needs to be slightly re-written? Would that be a welcome change?

On Tue, Jun 4, 2013 at 3:14 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 25 quoted lines

> elliottcable <me@ell.io> writes:
>> diff --git a/builtin/log.c b/builtin/log.c
>> index 9e21232..54d4d7f 100644
>> --- a/builtin/log.c
>> +++ b/builtin/log.c
>> @@ -237,7 +237,7 @@ static void log_show_early(struct rev_info *revs, struct commit_list *list)
>>       int i = revs->early_output;
>>       int show_header = 1;
>>
>> -     sort_in_topological_order(&list, revs->lifo);
>> +     sort_in_topological_order(&list, revs->lifo, revs->use_author);
>
> The name "use-author" is a clear sign that the person who added this
> code were too narrowly focused to think "author" automatically would
> mean "author date" ;-).
>
> It probably makes sense to revamp sort_in_topological_order(), so
> that its second parameter is not a boolean 'lifo' that tells too
> much about its implementation without telling what it actually
> means.  Instead, we can make it an enum sort_order, that tells it to
> emit the commits in committer-date order, author-date order, or
> graph-traversal order.
>
> And update revs->lifo to use that same enum, without adding
> use_author_date bit to rev_info.

I'll look into replacing lifo with an enum as soon as I can sit back down to update this patch. For the moment, nothing more than committer_date_sort and author_date_sort, I suppose?

Overview being, I suppose, that `lifo` will no longer exist (since it effectively determines, when truthy, that we operate in a *non*-date-ordered topological method); then have commiter_date_order and author_date_order bits in an enum, with zero being lifo/straightforward-topological-order. Sound about right?

I'll try and make this a separate patch. First commit, to replace lifo with an enum; second commit, to *actually implement* the code obeying that enum when it is set to author_date_order.

On Tue, Jun 4, 2013 at 5:22 PM, Jeff King <peff@peff.net> wrote:

Show 36 quoted lines

> On Tue, Jun 04, 2013 at 12:14:21PM -0700, Junio C Hamano wrote:
>
>> > diff --git a/commit.h b/commit.h
>> > index 67bd509..de07525 100644
>> > --- a/commit.h
>> > +++ b/commit.h
>> > @@ -17,6 +17,7 @@ struct commit {
>> >     void *util;
>> >     unsigned int indegree;
>> >     unsigned long date;
>> > +   unsigned long author_date;
>>
>> While walking we keep many of them in-core, and 8-byte each for each
>> commit objects add up.  We do not want to make "struct commit" any
>> larger than it already is.
>>
>> Having said that, I do not see a reasonable alternative
>> implementation than adding an author-date field to struct commit
>> without major restructuring if we were to add this feature.
>>
>> So please do not take this part of the response as a "patch rejected
>> because we do not want to add anything to this structure".  We'll
>> think of something down the road, but as an independent topic after
>> this gets in (or doesn't).
>
> Yeah, I had the same thought. Maybe this is a good candidate to build on
> top of the jk/commit-info slab experiment. The topo-sort could allocate
> an extra slab for author-date (or even expand the indegree slab to hold
> both indegree and author date), use it during the sort, and then free it
> afterwards.
>
> Elliott: you can see the relevant changes to the topo-sort in commit
> 96c4f4a (commit: allow associating auxiliary info on-demand,
> 2013-04-09).
>
> -Peff

Again, might be a little over my head. If you really think it's best that I look into that branch, I will try. :)

Meantime, is there any other, more-immediate approach you can think of? I thought, for a moment, of only storing *either* the committer *or* the author date in the commit-struct at a given time, and flagging with a single bit ... but I'm not sure how widely-spread the nead for committer-date currently is. Maybe I can go back and parse-out the author date *when I need it*, instead, though that sounds slow …

Epilogue: I'll make the *obvious* changes mentioned above sometime
within the next week (I'm more than a little swamped; in the middle of
a big breakup, and a big move, simultaneously! :( ), especially the
enum instead of the use_author bit … and submit another patch RFC. It
won't be finalized until we can decide what to do about the extra
8bytes in the commit struct, though. More input welcome.