git diff: improve range handling
When git diff is given a symmetric difference A...B, it chooses some merge base from the two specified commits (as documented). This fails, however, if there is *no* merge base: instead, you see the differences between A and B, which is certainly not what is expected. Moreover, if additional revisions are specified on the command line ("git diff A...B C"), the results get a bit weird: * If there is a symmetric difference merge base, this is used as the left side of the diff. The last final ref is used as the right side. * If there is no merge base, the symmetric status is completely lost. We will produce a combined diff instead. Similar weirdness occurs if you use, e.g., "git diff C A...B D". Likewise, using multiple two-dot ranges, or tossing extra revision specifiers into the command line with two-dot ranges, or mixing two and three dot ranges, all produce nonsense. To avoid all this, add a routine to catch the range cases and verify that that the arguments make sense. As a side effect, produce a warning showing *which* merge base is being used when there are multiple choices; die if there is no merge base. Signed-off-by: Chris Torek <chris.torek@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Chris Torek committed
Jun 12, 2020 at 16:19 UTC
8bfcb3a690126e6222f0d4f7012b0f68bb748018
2 files changed
+202
-13
builtin/diff.c
+111
-13
@@ -6,6 +6,7 @@
6
#define USE_THE_INDEX_COMPATIBILITY_MACROS
7
#include "cache.h"
8
#include "config.h"
9
+#include "ewah/ewok.h"
10
#include "lockfile.h"
11
#include "color.h"
12
#include "commit.h"
@@ -254,6 +255,108 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv
255
return run_diff_files(revs, options);
256
}
257
258
+struct symdiff {
259
+ struct bitmap *skip;
260
+ int warn;
261
+ const char *base, *left, *right;
262
+};
263
+
264
+/*
265
+ * Check for symmetric-difference arguments, and if present, arrange
266
+ * everything we need to know to handle them correctly. As a bonus,
267
+ * weed out all bogus range-based revision specifications, e.g.,
268
+ * "git diff A..B C..D" or "git diff A..B C" get rejected.
269
+ *
270
+ * For an actual symmetric diff, *symdiff is set this way:
271
+ *
272
+ * - its skip is non-NULL and marks *all* rev->pending.objects[i]
273
+ * indices that the caller should ignore (extra merge bases, of
274
+ * which there might be many, and A in A...B). Note that the
275
+ * chosen merge base and right side are NOT marked.
276
+ * - warn is set if there are multiple merge bases.
277
+ * - base, left, and right point to the names to use in a
278
+ * warning about multiple merge bases.
279
+ *
280
+ * If there is no symmetric diff argument, sym->skip is NULL and
281
+ * sym->warn is cleared. The remaining fields are not set.
282
+ */
283
+static void symdiff_prepare(struct rev_info *rev, struct symdiff *sym)
284
+{
285
+ int i, is_symdiff = 0, basecount = 0, othercount = 0;
286
+ int lpos = -1, rpos = -1, basepos = -1;
287
+ struct bitmap *map = NULL;
288
+
289
+ /*
290
+ * Use the whence fields to find merge bases and left and
291
+ * right parts of symmetric difference, so that we do not
292
+ * depend on the order that revisions are parsed. If there
293
+ * are any revs that aren't from these sources, we have a
294
+ * "git diff C A...B" or "git diff A...B C" case. Or we
295
+ * could even get "git diff A...B C...E", for instance.
296
+ *
297
+ * If we don't have just one merge base, we pick one
298
+ * at random.
299
+ *
300
+ * NB: REV_CMD_LEFT, REV_CMD_RIGHT are also used for A..B,
301
+ * so we must check for SYMMETRIC_LEFT too. The two arrays
302
+ * rev->pending.objects and rev->cmdline.rev are parallel.
303
+ */
304
+ for (i = 0; i < rev->cmdline.nr; i++) {
305
+ struct object *obj = rev->pending.objects[i].item;
306
+ switch (rev->cmdline.rev[i].whence) {
307
+ case REV_CMD_MERGE_BASE:
308
+ if (basepos < 0)
309
+ basepos = i;
310
+ basecount++;
311
+ break; /* do mark all bases */
312
+ case REV_CMD_LEFT:
313
+ if (lpos >= 0)
314
+ usage(builtin_diff_usage);
315
+ lpos = i;
316
+ if (obj->flags & SYMMETRIC_LEFT) {
317
+ is_symdiff = 1;
318
+ break; /* do mark A */
319
+ }
320
+ continue;
321
+ case REV_CMD_RIGHT:
322
+ if (rpos >= 0)
323
+ usage(builtin_diff_usage);
324
+ rpos = i;
325
+ continue; /* don't mark B */
326
+ case REV_CMD_PARENTS_ONLY:
327
+ case REV_CMD_REF:
328
+ case REV_CMD_REV:
329
+ othercount++;
330
+ continue;
331
+ }
332
+ if (map == NULL)
333
+ map = bitmap_new();
334
+ bitmap_set(map, i);
335
+ }
336
+
337
+ /*
338
+ * Forbid any additional revs for both A...B and A..B.
339
+ */
340
+ if (lpos >= 0 && othercount > 0)
341
+ usage(builtin_diff_usage);
342
+
343
+ if (!is_symdiff) {
344
+ bitmap_free(map);
345
+ sym->warn = 0;
346
+ sym->skip = NULL;
347
+ return;
348
+ }
349
+
350
+ sym->left = rev->pending.objects[lpos].name;
351
+ sym->right = rev->pending.objects[rpos].name;
352
+ sym->base = rev->pending.objects[basepos].name;
353
+ if (basecount == 0)
354
+ die(_("%s...%s: no merge base"), sym->left, sym->right);
355
+ bitmap_unset(map, basepos); /* unmark the base we want */
356
+ sym->warn = basecount > 1;
357
+ sym->skip = map;
358
+}
359
+
360
int cmd_diff(int argc, const char **argv, const char *prefix)
361
{
362
int i;
@@ -263,6 +366,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
366
struct object_array_entry *blob[2];
367
int nongit = 0, no_index = 0;
368
int result = 0;
369
+ struct symdiff sdiff;
370
371
/*
372
* We could get N tree-ish in the rev.pending_objects list.
@@ -382,6 +486,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
486
}
487
}
488
489
+ symdiff_prepare(&rev, &sdiff);
490
for (i = 0; i < rev.pending.nr; i++) {
491
struct object_array_entry *entry = &rev.pending.objects[i];
492
struct object *obj = entry->item;
@@ -396,6 +501,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
501
obj = &get_commit_tree(((struct commit *)obj))->object;
502
503
if (obj->type == OBJ_TREE) {
504
+ if (sdiff.skip && bitmap_get(sdiff.skip, i))
505
+ continue;
506
obj->flags |= flags;
507
add_object_array(obj, name, &ent);
508
} else if (obj->type == OBJ_BLOB) {
@@ -437,21 +544,12 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
544
usage(builtin_diff_usage);
545
else if (ent.nr == 1)
546
result = builtin_diff_index(&rev, argc, argv);
440
- else if (ent.nr == 2)
547
+ else if (ent.nr == 2) {
548
+ if (sdiff.warn)
549
+ warning(_("%s...%s: multiple merge bases, using %s"),
550
+ sdiff.left, sdiff.right, sdiff.base);
551
result = builtin_diff_tree(&rev, argc, argv,
552
&ent.objects[0], &ent.objects[1]);
443
- else if (ent.objects[0].item->flags & UNINTERESTING) {
444
- /*
445
- * diff A...B where there is at least one merge base
446
- * between A and B. We have ent.objects[0] ==
447
- * merge-base, ent.objects[ents-2] == A, and
448
- * ent.objects[ents-1] == B. Show diff between the
449
- * base and B. Note that we pick one merge base at
450
- * random if there are more than one.
451
- */
452
- result = builtin_diff_tree(&rev, argc, argv,
453
- &ent.objects[0],
454
- &ent.objects[ent.nr-1]);
553
} else
554
result = builtin_diff_combined(&rev, argc, argv,
555
ent.objects, ent.nr);
t/t4068-diff-symmetric.sh
new
+91
@@ -0,0 +1,91 @@
1
+#!/bin/sh
2
+
3
+test_description='behavior of diff with symmetric-diff setups'
4
+
5
+. ./test-lib.sh
6
+
7
+# build these situations:
8
+# - normal merge with one merge base (br1...b2r);
9
+# - criss-cross merge ie 2 merge bases (br1...master);
10
+# - disjoint subgraph (orphan branch, br3...master).
11
+#
12
+# B---E <-- master
13
+# / \ /
14
+# A X
15
+# \ / \
16
+# C---D--G <-- br1
17
+# \ /
18
+# ---F <-- br2
19
+#
20
+# H <-- br3
21
+#
22
+# We put files into a few commits so that we can verify the
23
+# output as well.
24
+
25
+test_expect_success setup '
26
+ git commit --allow-empty -m A &&
27
+ echo b >b &&
28
+ git add b &&
29
+ git commit -m B &&
30
+ git checkout -b br1 HEAD^ &&
31
+ echo c >c &&
32
+ git add c &&
33
+ git commit -m C &&
34
+ git tag commit-C &&
35
+ git merge -m D master &&
36
+ git tag commit-D &&
37
+ git checkout master &&
38
+ git merge -m E commit-C &&
39
+ git checkout -b br2 commit-C &&
40
+ echo f >f &&
41
+ git add f &&
42
+ git commit -m F &&
43
+ git checkout br1 &&
44
+ git merge -m G br2 &&
45
+ git checkout --orphan br3 &&
46
+ git commit -m H
47
+'
48
+
49
+test_expect_success 'diff with one merge base' '
50
+ git diff commit-D...br1 >tmp &&
51
+ tail -n 1 tmp >actual &&
52
+ echo +f >expect &&
53
+ test_cmp expect actual
54
+'
55
+
56
+# The output (in tmp) can have +b or +c depending
57
+# on which merge base (commit B or C) is picked.
58
+# It should have one of those two, which comes out
59
+# to seven lines.
60
+test_expect_success 'diff with two merge bases' '
61
+ git diff br1...master >tmp 2>err &&
62
+ test_line_count = 7 tmp &&
63
+ test_line_count = 1 err
64
+'
65
+
66
+test_expect_success 'diff with no merge bases' '
67
+ test_must_fail git diff br2...br3 >tmp 2>err &&
68
+ test_i18ngrep "fatal: br2...br3: no merge base" err
69
+'
70
+
71
+test_expect_success 'diff with too many symmetric differences' '
72
+ test_must_fail git diff br1...master br2...br3 >tmp 2>err &&
73
+ test_i18ngrep "usage" err
74
+'
75
+
76
+test_expect_success 'diff with symmetric difference and extraneous arg' '
77
+ test_must_fail git diff master br1...master >tmp 2>err &&
78
+ test_i18ngrep "usage" err
79
+'
80
+
81
+test_expect_success 'diff with two ranges' '
82
+ test_must_fail git diff master br1..master br2..br3 >tmp 2>err &&
83
+ test_i18ngrep "usage" err
84
+'
85
+
86
+test_expect_success 'diff with ranges and extra arg' '
87
+ test_must_fail git diff master br1..master commit-D >tmp 2>err &&
88
+ test_i18ngrep "usage" err
89
+'
90
+
91
+test_done