mbox series

[v2,0/3] Allow passing pathspecs to git range-diff

Message ID pull.1335.v2.git.1661506770.gitgitgadget@gmail.com (mailing list archive)
Headers show
Series Allow passing pathspecs to git range-diff | expand

Message

Jean-Noël Avila via GitGitGadget Aug. 26, 2022, 9:39 a.m. UTC 
I just had the need to find out upstream commits corresponding to a handful
of backported commits, and most importantly, identify upstream commits
touching a given file that had not yet been backported.

This new mode helped me identify them.

Changes since v1:

 * The command-line parameter parsing now avoids duplicating code as much as
   possible.
 * This also fixes a bug where git range-diff <incorrect-symmetric-range> --
   <pathspec> was mistaken for using the three-revision stanza.
 * Consistent validation of the command-line arguments has been extracted
   into its own patch.
 * Sadly, these changes make the overall diff much larger. I hope that the
   readability is worth that price.

Johannes Schindelin (3):
  range-diff: reorder argument handling
  range-diff: consistently validate the arguments
  range-diff: optionally accept pathspecs

 Documentation/git-range-diff.txt |   4 ++
 builtin/range-diff.c             | 101 +++++++++++++++++++++++--------
 range-diff.c                     |   2 +-
 t/t3206-range-diff.sh            |  13 +++-
 4 files changed, 94 insertions(+), 26 deletions(-)


base-commit: 795ea8776befc95ea2becd8020c7a284677b4161
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1335%2Fdscho%2Frange-diff-with-pathspec-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1335/dscho/range-diff-with-pathspec-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1335

Range-diff vs v1:

 1:  7fe4f228ae0 ! 1:  150f29a1c48 range-diff: reorder argument handling
     @@ Commit message
          In preparation for allowing pathspecs in `git range-diff` invocations,
          where we no longer have the luxury of using the number of arguments to
          disambiguate between these three different ways to specify the commit
     -    ranges, we need to order these cases by argument count, in ascending
     +    ranges, we need to order these cases by argument count, in descending
          order.
      
          This patch is best viewed with `--color-moved`.
     @@ builtin/range-diff.c: int cmd_range_diff(int argc, const char **argv, const char
       		diffopt.use_color = 1;
       
      -	if (argc == 2) {
     --		if (!is_range_diff_range(argv[0]))
     --			die(_("not a commit range: '%s'"), argv[0]);
     --		strbuf_addstr(&range1, argv[0]);
     --
     --		if (!is_range_diff_range(argv[1]))
     --			die(_("not a commit range: '%s'"), argv[1]);
     --		strbuf_addstr(&range2, argv[1]);
     ++	if (argc == 3) {
     ++		strbuf_addf(&range1, "%s..%s", argv[0], argv[1]);
     ++		strbuf_addf(&range2, "%s..%s", argv[0], argv[2]);
     ++	} else if (argc == 2) {
     + 		if (!is_range_diff_range(argv[0]))
     + 			die(_("not a commit range: '%s'"), argv[0]);
     + 		strbuf_addstr(&range1, argv[0]);
     +@@ builtin/range-diff.c: int cmd_range_diff(int argc, const char **argv, const char *prefix)
     + 		if (!is_range_diff_range(argv[1]))
     + 			die(_("not a commit range: '%s'"), argv[1]);
     + 		strbuf_addstr(&range2, argv[1]);
      -	} else if (argc == 3) {
      -		strbuf_addf(&range1, "%s..%s", argv[0], argv[1]);
      -		strbuf_addf(&range2, "%s..%s", argv[0], argv[2]);
     --	} else if (argc == 1) {
     -+	if (argc == 1) {
     + 	} else if (argc == 1) {
       		const char *b = strstr(argv[0], "..."), *a = argv[0];
       		int a_len;
     - 
     -@@ builtin/range-diff.c: int cmd_range_diff(int argc, const char **argv, const char *prefix)
     - 			b = "HEAD";
     - 		strbuf_addf(&range1, "%s..%.*s", b, a_len, a);
     - 		strbuf_addf(&range2, "%.*s..%s", a_len, a, b);
     -+	} else if (argc == 2) {
     -+		if (!is_range_diff_range(argv[0]))
     -+			die(_("not a commit range: '%s'"), argv[0]);
     -+		strbuf_addstr(&range1, argv[0]);
     -+
     -+		if (!is_range_diff_range(argv[1]))
     -+			die(_("not a commit range: '%s'"), argv[1]);
     -+		strbuf_addstr(&range2, argv[1]);
     -+	} else if (argc == 3) {
     -+		strbuf_addf(&range1, "%s..%s", argv[0], argv[1]);
     -+		strbuf_addf(&range2, "%s..%s", argv[0], argv[2]);
     - 	} else {
     - 		error(_("need two commit ranges"));
     - 		usage_with_options(builtin_range_diff_usage, options);
 -:  ----------- > 2:  4cd7f09557c range-diff: consistently validate the arguments
 2:  064b147451b ! 3:  a52ad40e015 range-diff: optionally accept a pathspec
     @@ Metadata
      Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
      
       ## Commit message ##
     -    range-diff: optionally accept a pathspec
     +    range-diff: optionally accept pathspecs
      
          The `git range-diff` command can be quite expensive, which is not a
          surprise given that the underlying algorithm to match up pairs of
     @@ Documentation/git-range-diff.txt: DESCRIPTION
      
       ## builtin/range-diff.c ##
      @@ builtin/range-diff.c: int cmd_range_diff(int argc, const char **argv, const char *prefix)
     + 		OPT_END()
     + 	};
       	struct option *options;
     - 	int res = 0;
     +-	int res = 0;
     ++	int i, dash_dash = -1, res = 0;
       	struct strbuf range1 = STRBUF_INIT, range2 = STRBUF_INIT;
     -+	struct object_id oid;
     -+	const char *p;
     + 	struct object_id oid;
     ++	const char *three_dots = NULL;
       
       	git_config(git_diff_ui_config, NULL);
       
     @@ builtin/range-diff.c: int cmd_range_diff(int argc, const char **argv, const char
       	diff_setup_done(&diffopt);
       
      @@ builtin/range-diff.c: int cmd_range_diff(int argc, const char **argv, const char *prefix)
     - 			b = "HEAD";
     - 		strbuf_addf(&range1, "%s..%.*s", b, a_len, a);
     - 		strbuf_addf(&range2, "%.*s..%s", a_len, a, b);
     -+	} else if (argc > 1 && (p = strstr(argv[0], "..."))) {
     -+		const char *a = argv[0];
     -+		int a_len = (int)(p - a);
     -+
     -+		if (!a_len) {
     -+			a = "HEAD";
     -+			a_len = strlen(a);
     + 	if (!simple_color)
     + 		diffopt.use_color = 1;
     + 
     +-	if (argc == 3) {
     +-		if (get_oid_committish(argv[0], &oid))
     ++	for (i = 0; i < argc; i++)
     ++		if (!strcmp(argv[i], "--")) {
     ++			dash_dash = i;
     ++			break;
      +		}
     -+		p += 3;
     -+		if (!*p)
     -+			p = "HEAD";
     -+		strbuf_addf(&range1, "%s..%.*s", p, a_len, a);
     -+		strbuf_addf(&range2, "%.*s..%s", a_len, a, p);
     -+		strvec_pushv(&other_arg, argv + 1);
     - 	} else if (argc == 2) {
     - 		if (!is_range_diff_range(argv[0]))
     - 			die(_("not a commit range: '%s'"), argv[0]);
     ++
     ++	if (dash_dash == 3 ||
     ++	    (dash_dash < 0 && argc > 2 &&
     ++	     !get_oid_committish(argv[0], &oid) &&
     ++	     !get_oid_committish(argv[1], &oid) &&
     ++	     !get_oid_committish(argv[2], &oid))) {
     ++		if (dash_dash < 0)
     ++			; /* already validated arguments */
     ++		else if (get_oid_committish(argv[0], &oid))
     + 			usage_msg_optf(_("not a revision: '%s'"),
     + 				       builtin_range_diff_usage, options,
     + 				       argv[0]);
      @@ builtin/range-diff.c: int cmd_range_diff(int argc, const char **argv, const char *prefix)
     - 		if (!is_range_diff_range(argv[1]))
     - 			die(_("not a commit range: '%s'"), argv[1]);
     - 		strbuf_addstr(&range2, argv[1]);
     -+	} else if (argc > 2 &&
     -+	    is_range_diff_range(argv[0]) && is_range_diff_range(argv[1])) {
     -+		strbuf_addstr(&range1, argv[0]);
     -+		strbuf_addstr(&range2, argv[1]);
     -+		strvec_pushv(&other_arg, argv + 2);
     - 	} else if (argc == 3) {
     + 
       		strbuf_addf(&range1, "%s..%s", argv[0], argv[1]);
       		strbuf_addf(&range2, "%s..%s", argv[0], argv[2]);
     -+	} else if (argc > 3 &&
     -+		   get_oid_committish(argv[0], &oid) &&
     -+		   get_oid_committish(argv[1], &oid) &&
     -+		   get_oid_committish(argv[2], &oid)) {
     -+		strbuf_addf(&range1, "%s..%s", argv[0], argv[1]);
     -+		strbuf_addf(&range2, "%s..%s", argv[0], argv[2]);
     -+		strvec_pushv(&other_arg, argv + 3);
     - 	} else {
     - 		error(_("need two commit ranges"));
     - 		usage_with_options(builtin_range_diff_usage, options);
     +-	} else if (argc == 2) {
     +-		if (!is_range_diff_range(argv[0]))
     ++
     ++		strvec_pushv(&other_arg, argv +
     ++			     (dash_dash < 0 ? 3 : dash_dash));
     ++	} else if (dash_dash == 2 ||
     ++		   (dash_dash < 0 && argc > 1 &&
     ++		    is_range_diff_range(argv[0]) &&
     ++		    is_range_diff_range(argv[1]))) {
     ++		if (dash_dash < 0)
     ++			; /* already validated arguments */
     ++		else if (!is_range_diff_range(argv[0]))
     + 			usage_msg_optf(_("not a commit range: '%s'"),
     + 				       builtin_range_diff_usage, options,
     + 				       argv[0]);
     +@@ builtin/range-diff.c: int cmd_range_diff(int argc, const char **argv, const char *prefix)
     + 
     + 		strbuf_addstr(&range1, argv[0]);
     + 		strbuf_addstr(&range2, argv[1]);
     +-	} else if (argc == 1) {
     +-		const char *b = strstr(argv[0], "..."), *a = argv[0];
     ++
     ++		strvec_pushv(&other_arg, argv +
     ++			     (dash_dash < 0 ? 2 : dash_dash));
     ++	} else if (dash_dash == 1 ||
     ++		   (dash_dash < 0 && argc > 0 &&
     ++		    (three_dots = strstr(argv[0], "...")))) {
     ++		const char *a, *b;
     + 		int a_len;
     + 
     +-		if (!b)
     ++		if (dash_dash < 0)
     ++			; /* already validated arguments */
     ++		else if (!(three_dots = strstr(argv[0], "...")))
     + 			usage_msg_optf(_("not a symmetric range: '%s'"),
     +-				       builtin_range_diff_usage, options,
     +-				       argv[0]);
     ++					 builtin_range_diff_usage, options,
     ++					 argv[0]);
     + 
     +-		a_len = (int)(b - a);
     +-		if (!a_len) {
     ++		if (three_dots == argv[0]) {
     + 			a = "HEAD";
     + 			a_len = strlen(a);
     ++		} else {
     ++			a = argv[0];
     ++			a_len = (int)(three_dots - a);
     + 		}
     +-		b += 3;
     +-		if (!*b)
     ++
     ++		if (three_dots[3])
     ++			b = three_dots + 3;
     ++		else
     + 			b = "HEAD";
     ++
     + 		strbuf_addf(&range1, "%s..%.*s", b, a_len, a);
     + 		strbuf_addf(&range2, "%.*s..%s", a_len, a, b);
     ++
     ++		strvec_pushv(&other_arg, argv +
     ++			     (dash_dash < 0 ? 1 : dash_dash));
     + 	} else
     + 		usage_msg_opt(_("need two commit ranges"),
     + 			      builtin_range_diff_usage, options);
      
       ## range-diff.c ##
      @@ range-diff.c: static int read_patches(const char *range, struct string_list *list,
     @@ range-diff.c: static int read_patches(const char *range, struct string_list *lis
       	cp.git_cmd = 1;
      
       ## t/t3206-range-diff.sh ##
     +@@ t/t3206-range-diff.sh: test_expect_success 'A^! and A^-<n> (unmodified)' '
     + '
     + 
     + test_expect_success 'A^{/..} is not mistaken for a range' '
     +-	test_must_fail git range-diff topic^.. topic^{/..} 2>error &&
     ++	test_must_fail git range-diff topic^.. topic^{/..} -- 2>error &&
     + 	test_i18ngrep "not a commit range" error
     + '
     + 
      @@ t/t3206-range-diff.sh: test_expect_success '--left-only/--right-only' '
       	test_cmp expect actual
       '