From 8e32caaa783a3a6b1d94834ee6e2d21ac36ff39f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 1 Jun 2023 13:38:14 -0400 Subject: [PATCH 1/3] pathspec: factor out magic-to-name function When we have unsupported magic in a pathspec (because a command or code path does not support particular items), we list the unsupported ones in an error message. Let's factor out the code here that converts the bits back into their human-readable names, so that it can be used from other callers, which may want to provide more flexible error messages. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- pathspec.c | 19 ++++++++++++------- pathspec.h | 8 ++++++++ 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/pathspec.c b/pathspec.c index ab70fcbe61..f6fbb643ff 100644 --- a/pathspec.c +++ b/pathspec.c @@ -525,24 +525,29 @@ static int pathspec_item_cmp(const void *a_, const void *b_) return strcmp(a->match, b->match); } -static void NORETURN unsupported_magic(const char *pattern, - unsigned magic) +void pathspec_magic_names(unsigned magic, struct strbuf *out) { - struct strbuf sb = STRBUF_INIT; int i; for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) { const struct pathspec_magic *m = pathspec_magic + i; if (!(magic & m->bit)) continue; - if (sb.len) - strbuf_addstr(&sb, ", "); + if (out->len) + strbuf_addstr(out, ", "); if (m->mnemonic) - strbuf_addf(&sb, _("'%s' (mnemonic: '%c')"), + strbuf_addf(out, _("'%s' (mnemonic: '%c')"), m->name, m->mnemonic); else - strbuf_addf(&sb, "'%s'", m->name); + strbuf_addf(out, "'%s'", m->name); } +} + +static void NORETURN unsupported_magic(const char *pattern, + unsigned magic) +{ + struct strbuf sb = STRBUF_INIT; + pathspec_magic_names(magic, &sb); /* * We may want to substitute "this command" with a command * name. E.g. when "git add -p" or "git add -i" dies when running diff --git a/pathspec.h b/pathspec.h index 41f6adfbb4..688aedc553 100644 --- a/pathspec.h +++ b/pathspec.h @@ -130,6 +130,14 @@ void parse_pathspec_file(struct pathspec *pathspec, void copy_pathspec(struct pathspec *dst, const struct pathspec *src); void clear_pathspec(struct pathspec *); +/* + * Add a human-readable string to "out" representing the PATHSPEC_* flags set + * in "magic". The result is suitable for error messages, but not for + * parsing as pathspec magic itself (you get 'icase' with quotes, not + * :(icase)). + */ +void pathspec_magic_names(unsigned magic, struct strbuf *out); + static inline int ps_strncmp(const struct pathspec_item *item, const char *s1, const char *s2, size_t n) { From 9eac5954e81037f282ace2efff7110af3440dc19 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 1 Jun 2023 13:41:06 -0400 Subject: [PATCH 2/3] diff: factor out --follow pathspec check In --follow mode, we require exactly one pathspec. We check this condition in two places: - in diff_setup_done(), we complain if --follow is used with an inapropriate pathspec - in git-log's revision "tweak" function, we enable log.follow only if the pathspec allows it The duplication isn't a big deal right now, since the logic is so simple. But in preparation for it becoming more complex, let's pull it into a shared function. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/log.c | 2 +- diff.c | 14 ++++++++++++-- diff.h | 7 +++++++ 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/builtin/log.c b/builtin/log.c index a70fba198f..06e3a1a946 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -856,7 +856,7 @@ static void log_setup_revisions_tweak(struct rev_info *rev, struct setup_revision_opt *opt) { if (rev->diffopt.flags.default_follow_renames && - rev->prune_data.nr == 1) + diff_check_follow_pathspec(&rev->prune_data, 0)) rev->diffopt.flags.follow_renames = 1; if (rev->first_parent_only) diff --git a/diff.c b/diff.c index 469e18aed2..dc9f21a9dc 100644 --- a/diff.c +++ b/diff.c @@ -4721,6 +4721,16 @@ unsigned diff_filter_bit(char status) return filter_bit[(int) status]; } +int diff_check_follow_pathspec(struct pathspec *ps, int die_on_error) +{ + if (ps->nr != 1) { + if (die_on_error) + die(_("--follow requires exactly one pathspec")); + return 0; + } + return 1; +} + void diff_setup_done(struct diff_options *options) { unsigned check_mask = DIFF_FORMAT_NAME | @@ -4828,8 +4838,8 @@ void diff_setup_done(struct diff_options *options) options->diff_path_counter = 0; - if (options->flags.follow_renames && options->pathspec.nr != 1) - die(_("--follow requires exactly one pathspec")); + if (options->flags.follow_renames) + diff_check_follow_pathspec(&options->pathspec, 1); if (!options->use_color || external_diff()) options->color_moved = 0; diff --git a/diff.h b/diff.h index 8d770b1d57..31e7db9ab0 100644 --- a/diff.h +++ b/diff.h @@ -542,6 +542,13 @@ void repo_diff_setup(struct repository *, struct diff_options *); struct option *add_diff_options(const struct option *, struct diff_options *); int diff_opt_parse(struct diff_options *, const char **, int, const char *); void diff_setup_done(struct diff_options *); + +/* + * Returns true if the pathspec can work with --follow mode. If die_on_error is + * set, die() with a specific error message rather than returning false. + */ +int diff_check_follow_pathspec(struct pathspec *ps, int die_on_error); + int git_config_rename(const char *var, const char *value); #define DIFF_DETECT_RENAME 1 From 8260bc59023136edeaed1f1006a03f44cc849883 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 1 Jun 2023 13:43:51 -0400 Subject: [PATCH 3/3] diff: detect pathspec magic not supported by --follow The --follow code doesn't handle most forms of pathspec magic. We check that no unexpected ones have made it to try_to_follow_renames() with a runtime GUARD_PATHSPEC() check, which gives behavior like this: $ git log --follow ':(icase)makefile' >/dev/null BUG: tree-diff.c:596: unsupported magic 10 Aborted The same is true of ":(glob)", ":(attr)", and so on. It's good that we notice the problem rather than continuing and producing a wrong answer. But there are two non-ideal things: 1. The idea of GUARD_PATHSPEC() is to catch programming errors where low-level code gets unexpected pathspecs. We'd usually try to catch unsupported pathspecs by passing a magic_mask to parse_pathspec(), which would give the user a much better message like: pathspec magic not supported by this command: 'icase' That doesn't happen here because git-log usually _does_ support all types of pathspec magic, and so it passes "0" for the mask (this call actually happens in setup_revisions()). It needs to distinguish the normal case from the "--follow" one but currently doesn't. 2. In addition to --follow, we have the log.follow config option. When that is set, we try to turn on --follow mode only when there is a single pathspec (since --follow doesn't handle anything else). But really, that ought to be expanded to "use --follow when the pathspec supports it". Otherwise, we'd complain any time you use an exotic pathspec: $ git config log.follow true $ git log ':(icase)makefile' >/dev/null BUG: tree-diff.c:596: unsupported magic 10 Aborted We should instead just avoid enabling follow mode if it's not supported by this particular invocation. This patch expands our diff_check_follow_pathspec() function to cover pathspec magic, solving both problems. A few final notes: - we could also solve (1) by passing the appropriate mask to parse_pathspec(). But that's not great for two reasons. One is that the error message is less precise. It says "magic not supported by this command", but really it is not the command, but rather the --follow option which is the problem. The second is that it always calls die(). But for our log.follow code, we want to speculatively ask "is this pathspec OK?" and just get a boolean result. - This is obviously the right thing to do for ':(icase)' and most other magic options. But ':(glob)' is a bit odd here. The --follow code doesn't support wildcards, but we allow them anyway. From try_to_follow_renames(): #if 0 /* * We should reject wildcards as well. Unfortunately we * haven't got a reliable way to detect that 'foo\*bar' in * fact has no wildcards. nowildcard_len is merely a hint for * optimization. Let it slip for now until wildmatch is taught * about dry-run mode and returns wildcard info. */ if (opt->pathspec.has_wildcard) BUG("wildcards are not supported"); #endif So something like "git log --follow 'Make*'" is already doing the wrong thing, since ":(glob)" behavior is already the default (it is used only to countermand an earlier --noglob-pathspecs). So we _could_ loosen the guard to allow :(glob), since it just behaves the same as pathspecs do by default. But it seems like a backwards step to do so. It already doesn't work (it hits the BUG() case currently), and given that the user took an explicit step to say "this pathspec should glob", it is reasonable for us to say "no, --follow does not support globbing" (or in the case of log.follow, avoid turning on follow mode). Which is what happens after this patch. - The set of allowed pathspec magic is obviously the same as in GUARD_PATHSPEC(). We could perhaps factor these out to avoid repetition. The point of having separate masks and GUARD calls is that we don't necessarily know which parsed pathspecs will be used where. But in this case, the two are heavily correlated. Still, there may be some value in keeping them separate; it would make anyone think twice about adding new magic to the list in diff_check_follow_pathspec(). They'd need to touch try_to_follow_renames() as well, which is the code that would actually need to be updated to handle more exotic pathspecs. - The documentation for log.follow says that it enables --follow "...when a single is given". We could possibly expand that to say "with no unsupported pathspec magic", but that raises the question of documenting which magic is supported. I think the existing wording of "single " sufficiently encompasses the idea (the forbidden magic is stuff that might match multiple entries), and the spirit remains the same. Reported-by: Jim Pryor Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- diff.c | 15 +++++++++++++++ t/t4202-log.sh | 15 +++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/diff.c b/diff.c index dc9f21a9dc..df89996896 100644 --- a/diff.c +++ b/diff.c @@ -4723,11 +4723,26 @@ unsigned diff_filter_bit(char status) int diff_check_follow_pathspec(struct pathspec *ps, int die_on_error) { + unsigned forbidden_magic; + if (ps->nr != 1) { if (die_on_error) die(_("--follow requires exactly one pathspec")); return 0; } + + forbidden_magic = ps->items[0].magic & ~(PATHSPEC_FROMTOP | + PATHSPEC_LITERAL); + if (forbidden_magic) { + if (die_on_error) { + struct strbuf sb = STRBUF_INIT; + pathspec_magic_names(forbidden_magic, &sb); + die(_("pathspec magic not supported by --follow: %s"), + sb.buf); + } + return 0; + } + return 1; } diff --git a/t/t4202-log.sh b/t/t4202-log.sh index 2ce2b41174..b1f49d2954 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -187,6 +187,21 @@ test_expect_success 'git config log.follow does not die with no paths' ' git log -- ' +test_expect_success 'git log --follow rejects unsupported pathspec magic' ' + test_must_fail git log --follow ":(top,glob,icase)ichi" 2>stderr && + # check full error message; we want to be sure we mention both + # of the rejected types (glob,icase), but not the allowed one (top) + echo "fatal: pathspec magic not supported by --follow: ${SQ}glob${SQ}, ${SQ}icase${SQ}" >expect && + test_cmp expect stderr +' + +test_expect_success 'log.follow disabled with unsupported pathspec magic' ' + test_config log.follow true && + git log --format=%s ":(glob,icase)ichi" >actual && + echo third >expect && + test_cmp expect actual +' + test_expect_success 'git config log.follow is overridden by --no-follow' ' test_config log.follow true && git log --no-follow --pretty="format:%s" ichi >actual &&