All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: "Randall S. Becker" <randall.becker@nexbridge.ca>,
	Jeff King <peff@peff.net>, Elijah Newren <newren@gmail.com>,
	Elijah Newren <newren@gmail.com>
Subject: [PATCH] sparse-checkout: be consistent with end of options markers
Date: Sun, 24 Dec 2023 01:00:11 +0000	[thread overview]
Message-ID: <pull.1625.git.git.1703379611749.gitgitgadget@gmail.com> (raw)

From: Elijah Newren <newren@gmail.com>

93851746 (parse-options: decouple "--end-of-options" and "--",
2023-12-06) updated the world order to make callers of parse-options
that set PARSE_OPT_KEEP_UNKNOWN_OPT responsible for deciding what to
do with "--end-of-options" they may see after parse_options() returns.

This made a previous bug in sparse-checkout more visible; namely,
that

  git sparse-checkout [add|set] --[no-]cone --end-of-options ...

would simply treat "--end-of-options" as one of the paths to include in
the sparse-checkout.  But this was already problematic before; namely,

  git sparse-checkout [add|set| --[no-]cone --sikp-checks ...

would not give an error on the mis-typed "--skip-checks" but instead
simply treat "--sikp-checks" as a path or pattern to include in the
sparse-checkout, which is highly unfriendly.

This behavior begain when the command was converted to parse-options in
7bffca95ea (sparse-checkout: add '--stdin' option to set subcommand,
2019-11-21).  Back then it was just called KEEP_UNKNOWN. Later it was
renamed to KEEP_UNKNOWN_OPT in 99d86d60e5 (parse-options:
PARSE_OPT_KEEP_UNKNOWN only applies to --options, 2022-08-19) to clarify
that it was only about dashed options; we always keep non-option
arguments.  Looking at that original patch, both Peff and I think that
the author was simply confused about the mis-named option, and really
just wanted to keep the non-option arguments.  We never should have used
the flag all along (and the other cases were cargo-culted within the
file).

Remove the erroneous PARSE_OPT_KEEP_UNKNOWN_OPT flag now to fix this
bug.  Note that this does mean that anyone who might have been using

  git sparse-checkout [add|set] [--[no-]cone] --foo --bar

to request paths or patterns '--foo' and '--bar' will now have to use

  git sparse-checkout [add|set] [--[no-]cone] -- --foo --bar

That makes sparse-checkout more consistent with other git commands,
provides users much friendlier error messages and behavior, and is
consistent with the all-capps warning in git-sparse-checkout.txt that
this command "is experimental...its behavior...will likely change".  :-)

Signed-off-by: Elijah Newren <newren@gmail.com>
---
    [RFC] sparse-checkout: be consistent with end of options markers
    
    Follow-up to thread over at
    https://lore.kernel.org/git/CABPp-BF9XZeESHdxdcZ91Vsn5tKqQ6_3tC11e7t9vTFp=uufbg@mail.gmail.com/

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1625%2Fgitgitgadget%2Fsparse-checkout-end-of-options-consistency-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1625/gitgitgadget/sparse-checkout-end-of-options-consistency-v1
Pull-Request: https://github.com/git/git/pull/1625

 builtin/sparse-checkout.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 5c8ffb1f759..0e68e9b0b0d 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -777,8 +777,7 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, prefix,
 			     builtin_sparse_checkout_add_options,
-			     builtin_sparse_checkout_add_usage,
-			     PARSE_OPT_KEEP_UNKNOWN_OPT);
+			     builtin_sparse_checkout_add_usage, 0);
 
 	sanitize_paths(argc, argv, prefix, add_opts.skip_checks);
 
@@ -824,8 +823,7 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, prefix,
 			     builtin_sparse_checkout_set_options,
-			     builtin_sparse_checkout_set_usage,
-			     PARSE_OPT_KEEP_UNKNOWN_OPT);
+			     builtin_sparse_checkout_set_usage, 0);
 
 	if (update_modes(&set_opts.cone_mode, &set_opts.sparse_index))
 		return 1;
@@ -996,8 +994,7 @@ static int sparse_checkout_check_rules(int argc, const char **argv, const char *
 
 	argc = parse_options(argc, argv, prefix,
 			     builtin_sparse_checkout_check_rules_options,
-			     builtin_sparse_checkout_check_rules_usage,
-			     PARSE_OPT_KEEP_UNKNOWN_OPT);
+			     builtin_sparse_checkout_check_rules_usage, 0);
 
 	if (check_rules_opts.rules_file && check_rules_opts.cone_mode < 0)
 		check_rules_opts.cone_mode = 1;

base-commit: 055bb6e9969085777b7fab83e3fee0017654f134
-- 
gitgitgadget

             reply	other threads:[~2023-12-24  1:00 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-12-24  1:00 Elijah Newren via GitGitGadget [this message]
2023-12-24  8:32 ` [PATCH] sparse-checkout: be consistent with end of options markers Jeff King
2023-12-26 17:18   ` Junio C Hamano
2023-12-28 11:43     ` Jeff King
2023-12-26 17:26 ` Junio C Hamano
2023-12-26 19:19   ` Elijah Newren
2023-12-26 19:39 ` [PATCH v2] " Elijah Newren via GitGitGadget
2023-12-26 20:12   ` 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.1625.git.git.1703379611749.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=newren@gmail.com \
    --cc=peff@peff.net \
    --cc=randall.becker@nexbridge.ca \
    /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.