[PATCH v3 0/3] improve git-diff documentation and A...B handling

["Chris Torek via GitGitGadget" <gitgitgadget@gmail.com>]

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>...]::
       

-- 
gitgitgadget