From: "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: [PATCH v2 0/3] Allow passing pathspecs to git range-diff
Date: Fri, 26 Aug 2022 09:39:27 +0000 [thread overview]
Message-ID: <pull.1335.v2.git.1661506770.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.1335.git.1661258122.gitgitgadget@gmail.com>
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
'
--
gitgitgadget
next prev parent reply other threads:[~2022-08-26 9:39 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-23 12:35 [PATCH 0/2] Allow passing pathspecs to git range-diff Johannes Schindelin via GitGitGadget
2022-08-23 12:35 ` [PATCH 1/2] range-diff: reorder argument handling Johannes Schindelin via GitGitGadget
2022-08-23 12:35 ` [PATCH 2/2] range-diff: optionally accept a pathspec Johannes Schindelin via GitGitGadget
2022-08-24 21:00 ` Junio C Hamano
2022-08-26 9:39 ` Johannes Schindelin via GitGitGadget [this message]
2022-08-26 9:39 ` [PATCH v2 1/3] range-diff: reorder argument handling Johannes Schindelin via GitGitGadget
2022-08-26 9:39 ` [PATCH v2 2/3] range-diff: consistently validate the arguments Johannes Schindelin via GitGitGadget
2022-08-26 16:54 ` Junio C Hamano
2022-08-26 9:39 ` [PATCH v2 3/3] range-diff: optionally accept pathspecs Johannes Schindelin via GitGitGadget
2022-08-26 17:02 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=pull.1335.v2.git.1661506770.gitgitgadget@gmail.com \
--to=gitgitgadget@gmail.com \
--cc=git@vger.kernel.org \
--cc=johannes.schindelin@gmx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.