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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 4, 2013, 19:14 UTC
Message-ID: <7vip1t7koi.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1370369299-20744-2-git-send-email-me@ell.io>

elliottcable <me@ell.io> writes:

Show 15 quoted lines

> --date-order is an excellent alternative to --topo-order if you want a feel for
> the *actual history*, chronologically, of your project. I use it often, with
> --graph as well; it's a great way to get an overview of a project's recent
> development history.
>
> However, in a project that rebases various in-development topic-branches often,
> it gets hard to demonstrate a *chronological history* of changes to the
> codebase, as this always “resets” the COMMITTER_DATE (which --date-order uses)
> to the time the rebase happened; which often means ‘last time all of the
> topic-branches were rebased on the latest fixes in master.’
>
> 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.

Show 12 quoted lines

>  builtin/log.c                          |  2 +-
>  builtin/rev-list.c                     |  1 +
>  builtin/rev-parse.c                    |  1 +
>  builtin/show-branch.c                  | 12 ++++-
>  commit.c                               | 83 ++++++++++++++++++++++++++++++----
>  commit.h                               |  3 +-
>  contrib/completion/git-completion.bash |  4 +-
>  po/de.po                               |  4 +-
>  po/git.pot                             |  2 +-
>  po/sv.po                               |  4 +-
>  po/vi.po                               |  4 +-
>  po/zh_CN.po                            |  4 +-

Please drop all the changes to po/ area; it is managed by the i18n coordinator and generated by an automated tool that extracts these strings from the code.

People who code should not (and do not have to) touch these files.

Show 14 quoted lines

>  revision.c                             | 11 ++++-
>  revision.h                             |  1 +
>  14 files changed, 110 insertions(+), 26 deletions(-)
>
> 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.

Show 7 quoted lines

> @@ -694,6 +697,11 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
>  			   show_branch_usage, PARSE_OPT_STOP_AT_NON_OPTION);
>  	if (all_heads)
>  		all_remotes = 1;
> +	/* I'm having trouble figuring out exactly what `lifo` stores. Why do both 'date-order' and
> +	 * 'topo-order' set the same variable!? Aren't they mutually exclusive? Since *both* set it, for
> +	 * the moment, I'm going to set it for '--authorship-order'; but that seems counterintuitive. */

Lines that are too wide.

	/*
         * Also please format multi-line comments
         * like this, nothing other than slash-asterisk
         * on the first and the last lines.
         */

Show 17 quoted lines

> @@ -301,7 +328,8 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s
>  			pptr = &commit_list_insert(new_parent, pptr)->next;
>  		}
>  	}
> -	item->date = parse_commit_date(bufptr, tail);
> +	item->date = parse_commit_committer_date(bufptr, tail);
> +	item->author_date = parse_commit_author_date(bufptr, tail);
> ...
> 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.