@matheusbsbs / matheus-bagliano / commits / 53d00b39ce

log: use true parents for diff even when rewriting

When using pathspec filtering in combination with diff-based log output, parent simplification happens before the diff is computed. The diff is therefore against the *simplified* parents. This works okay, arguably by accident, in the normal case: simplification reduces to one parent as long as the commit is TREESAME to it. So the simplified parent of any given commit must have the same tree contents on the filtered paths as its true (unfiltered) parent. However, --full-diff breaks this guarantee, and indeed gives pretty spectacular results when comparing the output of git log --graph --stat ... git log --graph --full-diff --stat ... (--graph internally kicks in parent simplification, much like --parents). To fix it, store a copy of the parent list before simplification (in a slab) whenever --full-diff is in effect. Then use the stored parents instead of the simplified ones in the commit display code paths. The latter do not actually check for --full-diff to avoid duplicated code; they just grab the original parents if save_parents() has not been called for this revision walk. For ordinary commits it should be obvious that this is the right thing to do. Merge commits are a bit subtle. Observe that with default simplification, merge simplification is an all-or-nothing decision: either the merge is TREESAME to one parent and disappears, or it is different from all parents and the parent list remains intact. Redundant parents are not pruned, so the existing code also shows them as a merge. So if we do show a merge commit, the parent list just consists of the rewrite result on each parent. Running, e.g., --cc on this in --full-diff mode is not very useful: if any commits were skipped, some hunks will disagree with all sides of the merge (with one side, because commits were skipped; with the others, because they didn't have those changes in the first place). This triggers --cc showing these hunks spuriously. Therefore I believe that even for merge commits it is better to show the diffs wrt. the original parents. Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Helped-by: Junio C Hamano <gitster@pobox.com> Helped-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk> Signed-off-by: Thomas Rast <trast@inf.ethz.ch> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Thomas Rast committed Jul 31, 2013 at 22:13 UTC 53d00b39ce00073ab6b450488acc3f532a223e8f

7 files changed +90 -3

combine-diff.c

+2 -1

@@ -10,6 +10,7 @@

10 #include "refs.h"

11 #include "userdiff.h"

12 #include "sha1-array.h"

13 +#include "revision.h"

15 static struct combine_diff_path *intersect_paths(struct combine_diff_path *curr, int n, int num_parent)

16 {

@@ -1383,7 +1384,7 @@ void diff_tree_combined(const unsigned char *sha1,

1384 void diff_tree_combined_merge(const struct commit *commit, int dense,

1385 struct rev_info *rev)

1386 {

1386 - struct commit_list *parent = commit->parents;

1387 + struct commit_list *parent = get_saved_parents(rev, commit);

1388 struct sha1_array parents = SHA1_ARRAY_INIT;

1389

1390 while (parent) {

commit.c

+16

@@ -377,6 +377,22 @@ unsigned commit_list_count(const struct commit_list *l)

377 return c;

378 }

379

380 +struct commit_list *copy_commit_list(struct commit_list *list)

381 +{

382 + struct commit_list *head = NULL;

383 + struct commit_list **pp = &head;

384 + while (list) {

385 + struct commit_list *new;

386 + new = xmalloc(sizeof(struct commit_list));

387 + new->item = list->item;

388 + new->next = NULL;

389 + *pp = new;

390 + pp = &new->next;

391 + list = list->next;

392 + }

393 + return head;

394 +}

395 +

396 void free_commit_list(struct commit_list *list)

397 {

398 while (list) {

commit.h

@@ -62,6 +62,9 @@ struct commit_list *commit_list_insert_by_date(struct commit *item,

62 struct commit_list **list);

63 void commit_list_sort_by_date(struct commit_list **list);

65 +/* Shallow copy of the input list */

66 +struct commit_list *copy_commit_list(struct commit_list *list);

67 +

68 void free_commit_list(struct commit_list *list);

70 /* Commit formats */

log-tree.c

+1 -1

@@ -738,7 +738,7 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log

738 sha1 = commit->tree->object.sha1;

739

740 /* Root commit? */

741 - parents = commit->parents;

741 + parents = get_saved_parents(opt, commit);

742 if (!parents) {

743 if (opt->show_root_diff) {

744 diff_root_tree_sha1(sha1, "", &opt->diffopt);

revision.c

+42 -1

@@ -15,6 +15,7 @@

15 #include "string-list.h"

16 #include "line-log.h"

17 #include "mailmap.h"

18 +#include "commit-slab.h"

20 volatile show_early_output_fn_t show_early_output;

@@ -2763,7 +2764,7 @@ static int commit_match(struct commit *commit, struct rev_info *opt)

2764 return retval;

2765 }

2766

2766 -static inline int want_ancestry(struct rev_info *revs)

2767 +static inline int want_ancestry(const struct rev_info *revs)

2768 {

2769 return (revs->rewrite_parents || revs->children.name);

2770 }

@@ -2820,6 +2821,14 @@ enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)

2821 if (action == commit_show &&

2822 !revs->show_all &&

2823 revs->prune && revs->dense && want_ancestry(revs)) {

2824 + /*

2825 + * --full-diff on simplified parents is no good: it

2826 + * will show spurious changes from the commits that

2827 + * were elided. So we save the parents on the side

2828 + * when --full-diff is in effect.

2829 + */

2830 + if (revs->full_diff)

2831 + save_parents(revs, commit);

2832 if (rewrite_parents(revs, commit, rewrite_one) < 0)

2833 return commit_error;

2834 }

@@ -3038,6 +3047,8 @@ struct commit *get_revision(struct rev_info *revs)

3047 c = get_revision_internal(revs);

3048 if (c && revs->graph)

3049 graph_update(revs->graph, c);

3050 + if (!c)

3051 + free_saved_parents(revs);

3052 return c;

3053 }

3054

@@ -3069,3 +3080,33 @@ void put_revision_mark(const struct rev_info *revs, const struct commit *commit)

3080 fputs(mark, stdout);

3081 putchar(' ');

3082 }

3083 +

3084 +define_commit_slab(saved_parents, struct commit_list *);

3085 +

3086 +void save_parents(struct rev_info *revs, struct commit *commit)

3087 +{

3088 + struct commit_list **pp;

3089 +

3090 + if (!revs->saved_parents_slab) {

3091 + revs->saved_parents_slab = xmalloc(sizeof(struct saved_parents));

3092 + init_saved_parents(revs->saved_parents_slab);

3093 + }

3094 +

3095 + pp = saved_parents_at(revs->saved_parents_slab, commit);

3096 + assert(*pp == NULL);

3097 + *pp = copy_commit_list(commit->parents);

3098 +}

3099 +

3100 +struct commit_list *get_saved_parents(struct rev_info *revs, const struct commit *commit)

3101 +{

3102 + if (!revs->saved_parents_slab)

3103 + return commit->parents;

3104 +

3105 + return *saved_parents_at(revs->saved_parents_slab, commit);

3106 +}

3107 +

3108 +void free_saved_parents(struct rev_info *revs)

3109 +{

3110 + if (revs->saved_parents_slab)

3111 + clear_saved_parents(revs->saved_parents_slab);

3112 +}

revision.h

+20

@@ -25,6 +25,7 @@

25 struct rev_info;

26 struct log_info;

27 struct string_list;

28 +struct saved_parents;

30 struct rev_cmdline_info {

31 unsigned int nr;

@@ -187,6 +188,9 @@ struct rev_info {

188

189 /* line level range that we are chasing */

190 struct decoration line_log_data;

191 +

192 + /* copies of the parent lists, for --full-diff display */

193 + struct saved_parents *saved_parents_slab;

194 };

195

196 #define REV_TREE_SAME 0

@@ -273,4 +277,20 @@ typedef enum rewrite_result (*rewrite_parent_fn_t)(struct rev_info *revs, struct

277

278 extern int rewrite_parents(struct rev_info *revs, struct commit *commit,

279 rewrite_parent_fn_t rewrite_parent);

280 +

281 +/*

282 + * Save a copy of the parent list, and return the saved copy. This is

283 + * used by the log machinery to retrieve the original parents when

284 + * commit->parents has been modified by history simpification.

285 + *

286 + * You may only call save_parents() once per commit (this is checked

287 + * for non-root commits).

288 + *

289 + * get_saved_parents() will transparently return commit->parents if

290 + * history simplification is off.

291 + */

292 +extern void save_parents(struct rev_info *revs, struct commit *commit);

293 +extern struct commit_list *get_saved_parents(struct rev_info *revs, const struct commit *commit);

294 +extern void free_saved_parents(struct rev_info *revs);

295 +

296 #endif

t/t6012-rev-list-simplify.sh

@@ -127,4 +127,10 @@ test_expect_success 'full history simplification without parent' '

127 }

128 '

129

130 +test_expect_success '--full-diff is not affected by --parents' '

131 + git log -p --pretty="%H" --full-diff -- file >expected &&

132 + git log -p --pretty="%H" --full-diff --parents -- file >actual &&

133 + test_cmp expected actual

134 +'

135 +

136 test_done