Re: [PATCH] Avoid loading commits twice in log with diffs
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 3, 2013, 07:06 UTC
- Message-ID
- <7va9qlc690.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <3193c96cf5b036a91bc78b508b8b30ac87ca0f21.1362218700.git.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
Show 6 quoted lines
> 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?
Show 15 quoted lines
> 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.
Show 22 quoted lines
> 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;