From patchwork Thu Jun 1 17:38:14 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13264351 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id DE2A2C77B7E for ; Thu, 1 Jun 2023 17:38:18 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232904AbjFARiS (ORCPT ); Thu, 1 Jun 2023 13:38:18 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35834 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232900AbjFARiQ (ORCPT ); Thu, 1 Jun 2023 13:38:16 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id CD955138 for ; Thu, 1 Jun 2023 10:38:15 -0700 (PDT) Received: (qmail 5848 invoked by uid 109); 1 Jun 2023 17:38:15 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Thu, 01 Jun 2023 17:38:15 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 25279 invoked by uid 111); 1 Jun 2023 17:38:14 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Thu, 01 Jun 2023 13:38:14 -0400 Authentication-Results: peff.net; auth=none Date: Thu, 1 Jun 2023 13:38:14 -0400 From: Jeff King To: Jim Pryor Cc: Junio C Hamano , git@vger.kernel.org Subject: [PATCH 1/3] pathspec: factor out magic-to-name function Message-ID: <20230601173814.GA4165297@coredump.intra.peff.net> References: <20230601173724.GA4158369@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230601173724.GA4158369@coredump.intra.peff.net> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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 --- pathspec.c | 19 ++++++++++++------- pathspec.h | 8 ++++++++ 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/pathspec.c b/pathspec.c index 6966b265d3..5049dbb528 100644 --- a/pathspec.c +++ b/pathspec.c @@ -531,24 +531,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 a5b38e0907..fec4399bbc 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 patchwork Thu Jun 1 17:41:06 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13264355 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 166EFC7EE23 for ; Thu, 1 Jun 2023 17:41:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233001AbjFARlO (ORCPT ); Thu, 1 Jun 2023 13:41:14 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37492 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233007AbjFARlK (ORCPT ); Thu, 1 Jun 2023 13:41:10 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 61B99136 for ; Thu, 1 Jun 2023 10:41:08 -0700 (PDT) Received: (qmail 5885 invoked by uid 109); 1 Jun 2023 17:41:07 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Thu, 01 Jun 2023 17:41:07 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 25310 invoked by uid 111); 1 Jun 2023 17:41:07 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Thu, 01 Jun 2023 13:41:07 -0400 Authentication-Results: peff.net; auth=none Date: Thu, 1 Jun 2023 13:41:06 -0400 From: Jeff King To: Jim Pryor Cc: Junio C Hamano , git@vger.kernel.org Subject: [PATCH 2/3] diff: factor out --follow pathspec check Message-ID: <20230601174106.GB4165297@coredump.intra.peff.net> References: <20230601173724.GA4158369@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230601173724.GA4158369@coredump.intra.peff.net> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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 --- I don't love the die_on_error pattern here, but the obvious alternative of having the caller die when the boolean is false means it won't be able to give as descriptive a message. And I didn't think it was worth getting into passing out an err strbuf, given that there are only these two callers. 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 676de107d6..83d9b69dba 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -866,7 +866,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 3c88c37908..9f548f3471 100644 --- a/diff.c +++ b/diff.c @@ -4751,6 +4751,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 | @@ -4858,8 +4868,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 3a7a9e8b88..6c10ce289d 100644 --- a/diff.h +++ b/diff.h @@ -539,6 +539,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 patchwork Thu Jun 1 17:43:51 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13264356 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id F2A59C7EE23 for ; Thu, 1 Jun 2023 17:43:58 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233049AbjFARn6 (ORCPT ); Thu, 1 Jun 2023 13:43:58 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38798 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233066AbjFARnz (ORCPT ); Thu, 1 Jun 2023 13:43:55 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 33523197 for ; Thu, 1 Jun 2023 10:43:53 -0700 (PDT) Received: (qmail 5954 invoked by uid 109); 1 Jun 2023 17:43:52 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Thu, 01 Jun 2023 17:43:52 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 25318 invoked by uid 111); 1 Jun 2023 17:43:52 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Thu, 01 Jun 2023 13:43:52 -0400 Authentication-Results: peff.net; auth=none Date: Thu, 1 Jun 2023 13:43:51 -0400 From: Jeff King To: Jim Pryor Cc: Junio C Hamano , git@vger.kernel.org Subject: [PATCH 3/3] diff: detect pathspec magic not supported by --follow Message-ID: <20230601174351.GC4165297@coredump.intra.peff.net> References: <20230601173724.GA4158369@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230601173724.GA4158369@coredump.intra.peff.net> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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 --- diff.c | 15 +++++++++++++++ t/t4202-log.sh | 15 +++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/diff.c b/diff.c index 9f548f3471..8ca16fefe7 100644 --- a/diff.c +++ b/diff.c @@ -4753,11 +4753,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 ae73aef922..5b54d7928e 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 &&