git diff -h help is succinct, but perhaps too much so.
The symmetric-diff syntax, git diff A...B, is defined by the documentation to compare the merge base of A and B to commit B. It does so just fine when there is a merge base. It compares A and B directly if there is no merge base, and it is overly forgiving of bad arguments after which it can produce nonsensical diffs. It also behaves badly with other odd/incorrect usages, such as git diff A..B C..D.
The first patch simply adjusts a test that will fail if the second patch is accepted. The second patch adds special handling for the symmetric diff syntax so that the option parsing works, plus a small test suite. The third patch updates the documentation, including adding a section for combined commits, and makes the help output more verbose (to match the SYNOPSIS and provide common diff options like git-diff-files, for instance).
Changes since v1:
* updated commit messages
* rewrote prepare function
* added tests to reject bad two-dot usage as well
* removed A..B syntax from usage (and fixed typo)
* fixed up three-dot synopsis text
Chris Torek (3):
t/t3430: avoid undefined git diff behavior
git diff: improve range handling
Documentation: usage for diff combined commits
Documentation/git-diff.txt | 20 ++++--
builtin/diff.c | 132 +++++++++++++++++++++++++++++++++----
t/t3430-rebase-merges.sh | 2 +-
t/t4068-diff-symmetric.sh | 91 +++++++++++++++++++++++++
4 files changed, 226 insertions(+), 19 deletions(-)
create mode 100755 t/t4068-diff-symmetric.sh
base-commit: 20514004ddf1a3528de8933bc32f284e175e1012
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-804%2Fchris3torek%2Fcleanup-diff-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-804/chris3torek/cleanup-diff-v3
Pull-Request: https://github.com/git/git/pull/804
Range-diff vs v2:
1: 2ccaad645ff = 1: 2ccaad645ff t/t3430: avoid undefined git diff behavior
2: 100fa403477 ! 2: 60aed3f9d65 git diff: improve A...B merge-base handling
@@ Metadata
Author: Chris Torek <chris.torek@gmail.com>
## Commit message ##
- git diff: improve A...B merge-base handling
+ 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).
@@ Commit message
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 A...B case and verify that
- there is at least one merge base, and 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.
+ 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>
@@ builtin/diff.c: static int builtin_diff_files(struct rev_info *revs, int argc, c
}
+struct symdiff {
-+ struct bitmap *skip; /* bitmap of commit indices to skip, or NULL */
-+ int warn; /* true if there were multiple merge bases */
-+ int base, left, right; /* index of chosen merge base and left&right */
++ struct bitmap *skip;
++ int warn;
++ const char *base, *left, *right;
+};
+
+/*
+ * Check for symmetric-difference arguments, and if present, arrange
-+ * everything we need to know to handle them correctly.
++ * everything we need to know to handle them correctly. As a bonus,
++ * weed out all bogus range-based revision specifications, e.g.,
++ * "git diff A..B C..D" or "git diff A..B C" get rejected.
+ *
+ * For an actual symmetric diff, *symdiff is set this way:
+ *
@@ builtin/diff.c: static int builtin_diff_files(struct rev_info *revs, int argc, c
+ * which there might be many, and A in A...B). Note that the
+ * chosen merge base and right side are NOT marked.
+ * - warn is set if there are multiple merge bases.
-+ * - base, left, and right hold the merge base and left and
-+ * right side indices, for warnings or errors.
++ * - base, left, and right point to the names to use in a
++ * warning about multiple merge bases.
+ *
+ * If there is no symmetric diff argument, sym->skip is NULL and
+ * sym->warn is cleared. The remaining fields are not set.
-+ *
-+ * If the user provides a symmetric diff with no merge base, or
-+ * more than one range, we do a usage-exit.
+ */
+static void symdiff_prepare(struct rev_info *rev, struct symdiff *sym)
+{
-+ int i, lcount = 0, rcount = 0, basecount = 0;
++ int i, is_symdiff = 0, basecount = 0, othercount = 0;
+ int lpos = -1, rpos = -1, basepos = -1;
+ struct bitmap *map = NULL;
+
@@ builtin/diff.c: static int builtin_diff_files(struct rev_info *revs, int argc, c
+ basecount++;
+ break; /* do mark all bases */
+ case REV_CMD_LEFT:
++ if (lpos > 0)
++ usage(builtin_diff_usage);
++ lpos = i;
+ if (obj->flags & SYMMETRIC_LEFT) {
-+ lpos = i;
-+ lcount++;
++ is_symdiff = 1;
+ break; /* do mark A */
+ }
+ continue;
+ case REV_CMD_RIGHT:
++ if (rpos > 0)
++ usage(builtin_diff_usage);
+ rpos = i;
-+ rcount++;
+ continue; /* don't mark B */
-+ default:
++ case REV_CMD_PARENTS_ONLY:
++ case REV_CMD_REF:
++ case REV_CMD_REV:
++ othercount++;
+ continue;
+ }
+ if (map == NULL)
@@ builtin/diff.c: static int builtin_diff_files(struct rev_info *revs, int argc, c
+ bitmap_set(map, i);
+ }
+
-+ if (lcount == 0) { /* not a symmetric difference */
++ /*
++ * Forbid any additional revs for both A...B and A..B.
++ */
++ if (lpos >= 0 && othercount > 0)
++ usage(builtin_diff_usage);
++
++ if (!is_symdiff) {
+ bitmap_free(map);
+ sym->warn = 0;
+ sym->skip = NULL;
+ return;
+ }
+
-+ if (lcount != 1)
-+ die(_("cannot use more than one symmetric difference"));
-+
-+ if (basecount == 0) {
-+ const char *lname = rev->pending.objects[lpos].name;
-+ const char *rname = rev->pending.objects[rpos].name;
-+ die(_("%s...%s: no merge base"), lname, rname);
-+ }
++ sym->left = rev->pending.objects[lpos].name;
++ sym->right = rev->pending.objects[rpos].name;
++ sym->base = rev->pending.objects[basepos].name;
++ if (basecount == 0)
++ die(_("%s...%s: no merge base"), sym->left, sym->right);
+ bitmap_unset(map, basepos); /* unmark the base we want */
-+ sym->base = basepos;
-+ sym->left = lpos;
-+ sym->right = rpos;
+ sym->warn = basecount > 1;
+ sym->skip = map;
+}
@@ builtin/diff.c: int cmd_diff(int argc, const char **argv, const char *prefix)
struct object_array_entry *entry = &rev.pending.objects[i];
struct object *obj = entry->item;
@@ builtin/diff.c: int cmd_diff(int argc, const char **argv, const char *prefix)
- die(_("invalid object '%s' given."), name);
- if (obj->type == OBJ_COMMIT)
obj = &get_commit_tree(((struct commit *)obj))->object;
--
+
if (obj->type == OBJ_TREE) {
+ if (sdiff.skip && bitmap_get(sdiff.skip, i))
+ continue;
@@ builtin/diff.c: int cmd_diff(int argc, const char **argv, const char *prefix)
result = builtin_diff_index(&rev, argc, argv);
- else if (ent.nr == 2)
+ else if (ent.nr == 2) {
-+ if (sdiff.warn) {
-+ const char *lname = rev.pending.objects[sdiff.left].name;
-+ const char *rname = rev.pending.objects[sdiff.right].name;
-+ const char *basename = rev.pending.objects[sdiff.base].name;
++ if (sdiff.warn)
+ warning(_("%s...%s: multiple merge bases, using %s"),
-+ lname, rname, basename);
-+ }
++ sdiff.left, sdiff.right, sdiff.base);
result = builtin_diff_tree(&rev, argc, argv,
&ent.objects[0], &ent.objects[1]);
- else if (ent.objects[0].item->flags & UNINTERESTING) {
@@ builtin/diff.c: int cmd_diff(int argc, const char **argv, const char *prefix)
- result = builtin_diff_tree(&rev, argc, argv,
- &ent.objects[0],
- &ent.objects[ent.nr-1]);
-- } else
-+ } else {
-+ if (sdiff.skip)
-+ usage(builtin_diff_usage);
+ } else
result = builtin_diff_combined(&rev, argc, argv,
ent.objects, ent.nr);
-+ }
- result = diff_result_code(&rev.diffopt, result);
- if (1 < rev.diffopt.skip_stat_unmatch)
- refresh_index_quietly();
## t/t4068-diff-symmetric.sh (new) ##
@@
@@ t/t4068-diff-symmetric.sh (new)
+
+test_expect_success 'diff with one merge base' '
+ git diff commit-D...br1 >tmp &&
-+ tail -1 tmp >actual &&
++ tail -n 1 tmp >actual &&
+ echo +f >expect &&
+ test_cmp expect actual
+'
@@ t/t4068-diff-symmetric.sh (new)
+
+test_expect_success 'diff with too many symmetric differences' '
+ test_must_fail git diff br1...master br2...br3 >tmp 2>err &&
-+ test_i18ngrep "fatal: cannot use more than one symmetric difference" err
++ test_i18ngrep "usage" err
+'
+
+test_expect_success 'diff with symmetric difference and extraneous arg' '
@@ t/t4068-diff-symmetric.sh (new)
+ test_i18ngrep "usage" err
+'
+
++test_expect_success 'diff with two ranges' '
++ test_must_fail git diff master br1..master br2..br3 >tmp 2>err &&
++ test_i18ngrep "usage" err
++'
++
++test_expect_success 'diff with ranges and extra arg' '
++ test_must_fail git diff master br1..master commit-D >tmp 2>err &&
++ test_i18ngrep "usage" err
++'
++
+test_done
3: b9b4c6f113d ! 3: a7da92cd635 Documentation: tweak git diff help slightly
@@ Metadata
Author: Chris Torek <chris.torek@gmail.com>
## Commit message ##
- Documentation: tweak git diff help slightly
+ Documentation: usage for diff combined commits
- Update the manual page synopsis to include the three-dot notation
- and the combined-diff option
+ Document the usage for producing combined commits with "git diff".
+ This includes updating the synopsis section.
+
+ While here, add the three-dot notation to the synopsis.
Make "git diff -h" print the same usage summary as the manual
page synopsis, minus the "A..B" form, which is now discouraged.
- Document the usage for producing combined commits.
-
Signed-off-by: Chris Torek <chris.torek@gmail.com>
## Documentation/git-diff.txt ##
@@ Documentation/git-diff.txt: two blob objects, or changes between two files on di
+ This form is to view the results of a merge commit. The first
+ listed <commit> must be the merge itself; the remaining two or
+ more commits should be its parents. A convenient way to produce
-+ the desired set of revisions is to use the {caret}@ suffix, i.e.,
-+ "git diff master master^@". This is equivalent to running "git
-+ show --format=" on the merge commit, e.g., "git show --format=
-+ master".
++ the desired set of revisions is to use the {caret}@ suffix.
++ For instance, if `master` names a merge commit, `git diff master
++ master^@` gives the same combined diff as `git show master`.
+
'git diff' [<options>] <commit>\...<commit> [--] [<path>...]::
Chris Torek via GitGitGadget
