Skip to content

Commit 8bfcb3a

Browse files
chris3torekgitster
chris3torek
authored and
gitster
committed
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>
1 parent bafa2d7 commit 8bfcb3a

File tree

2 files changed

+202
-13
lines changed

2 files changed

+202
-13
lines changed

‎builtin/diff.c‎

Lines changed: 111 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#define USE_THE_INDEX_COMPATIBILITY_MACROS
77
#include "cache.h"
88
#include "config.h"
9+
#include "ewah/ewok.h"
910
#include "lockfile.h"
1011
#include "color.h"
1112
#include "commit.h"
@@ -254,6 +255,108 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv
254255
return run_diff_files(revs, options);
255256
}
256257

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+
257360
int cmd_diff(int argc, const char **argv, const char *prefix)
258361
{
259362
int i;
@@ -263,6 +366,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
263366
struct object_array_entry *blob[2];
264367
int nongit = 0, no_index = 0;
265368
int result = 0;
369+
struct symdiff sdiff;
266370

267371
/*
268372
* 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)
382486
}
383487
}
384488

489+
symdiff_prepare(&rev, &sdiff);
385490
for (i = 0; i < rev.pending.nr; i++) {
386491
struct object_array_entry *entry = &rev.pending.objects[i];
387492
struct object *obj = entry->item;
@@ -396,6 +501,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
396501
obj = &get_commit_tree(((struct commit *)obj))->object;
397502

398503
if (obj->type == OBJ_TREE) {
504+
if (sdiff.skip && bitmap_get(sdiff.skip, i))
505+
continue;
399506
obj->flags |= flags;
400507
add_object_array(obj, name, &ent);
401508
} else if (obj->type == OBJ_BLOB) {
@@ -437,21 +544,12 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
437544
usage(builtin_diff_usage);
438545
else if (ent.nr == 1)
439546
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);
441551
result = builtin_diff_tree(&rev, argc, argv,
442552
&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]);
455553
} else
456554
result = builtin_diff_combined(&rev, argc, argv,
457555
ent.objects, ent.nr);

‎t/t4068-diff-symmetric.sh‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -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

0 commit comments

Comments
 (0)