Re: [PATCH] Avoid loading commits twice in log with diffs

[Junio C Hamano <gitster@pobox.com>]

Thomas Rast <trast@student.ethz.ch> writes:

> Test                      with patch        before
> --------------------------------------------------------------------
> 4000.2: log --raw -3000   0.50(0.43+0.06)   0.54(0.46+0.06) +7.0%***
> 4000.3: log -p -3000      2.34(2.20+0.13)   2.37(2.22+0.13) +1.2%
> --------------------------------------------------------------------
> Significance hints:  '.' 0.1  '*' 0.05  '**' 0.01  '***' 0.001

It may be a silly question but what is a significance hint?

> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> ---
>  log-tree.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/log-tree.c b/log-tree.c
> index eb1a1b4..277a38f 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -715,7 +715,7 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log
>  {
>  	int showed_log;
>  	struct commit_list *parents;
> -	unsigned const char *sha1 = commit->object.sha1;
> +	unsigned const char *sha1 = commit->tree->object.sha1;

Overall I think this goes in the right direction and I can see why
the changes in later two hunks are correct, but I am not sure if we
can safely assume that the caller has parsed the incoming commit and
we have a good commit->tree here already.

Right now, this static function has a single call-site in a public
function log_tree_commit(), whose existing callers may all pass an
already parsed commit, but I feel somewhat uneasy to do the above
without some mechanism in place (either parse it here or in the
caller if unparsed, or document that log_tree_commit() must be
called with a parsed commit and perhaps add an assert there) to
ensure that the invariant is not broken in the future.

Thanks.

>  	if (!opt->diff && !DIFF_OPT_TST(&opt->diffopt, EXIT_WITH_STATUS))
>  		return 0;
> @@ -742,7 +742,8 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log
>  			 * parent, showing summary diff of the others
>  			 * we merged _in_.
>  			 */
> -			diff_tree_sha1(parents->item->object.sha1, sha1, "", &opt->diffopt);
> +			parse_commit(parents->item);
> +			diff_tree_sha1(parents->item->tree->object.sha1, sha1, "", &opt->diffopt);
>  			log_tree_diff_flush(opt);
>  			return !opt->loginfo;
>  		}
> @@ -755,7 +756,8 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log
>  	for (;;) {
>  		struct commit *parent = parents->item;
>  
> -		diff_tree_sha1(parent->object.sha1, sha1, "", &opt->diffopt);
> +		parse_commit(parent);
> +		diff_tree_sha1(parent->tree->object.sha1, sha1, "", &opt->diffopt);
>  		log_tree_diff_flush(opt);
>  
>  		showed_log |= !opt->loginfo;