git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: "Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>,
	Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>,
	Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
Subject: [PATCH v3 7/8] stash: eliminate crude option parsing
Date: Mon, 17 Feb 2020 17:25:21 +0000
Message-ID: <d34eaf4a27236591a9bc345d5a2cb465542a3243.1581960322.git.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.530.v3.git.1581960322.gitgitgadget@gmail.com>

From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

Eliminate crude option parsing and rely on real parsing instead, because
1) Crude parsing is crude, for example it's not capable of
   handling things like `git stash -m Message`
2) Adding options in two places is inconvenient and prone to bugs

As a side result, the case of `git stash -m Message` gets fixed.
Also give a good error message instead of just throwing usage at user.

----

Some review of what's been happening to this code:

Before [1], `git-stash.sh` only verified that all args begin with `-` :

	# The default command is "push" if nothing but options are given
	seen_non_option=
	for opt
	do
		case "$opt" in
		--) break ;;
		-*) ;;
		*) seen_non_option=t; break ;;
		esac
	done

Later, [1] introduced the duplicate code I'm now removing, also making
the previous test more strict by white-listing options.

----

[1] Commit 40af1468 ("stash: convert `stash--helper.c` into `stash.c`" 2019-02-26)

Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
 builtin/stash.c  | 59 +++++++++++++++++-------------------------------
 t/t3903-stash.sh |  5 ++++
 2 files changed, 26 insertions(+), 38 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 879fc5f3683..ed84ff2e168 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1451,8 +1451,10 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
 	return ret;
 }
 
-static int push_stash(int argc, const char **argv, const char *prefix)
+static int push_stash(int argc, const char **argv, const char *prefix,
+		      int push_assumed)
 {
+	int force_assume = 0;
 	int keep_index = -1;
 	int patch_mode = 0;
 	int include_untracked = 0;
@@ -1474,10 +1476,22 @@ static int push_stash(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	if (argc)
+	if (argc) {
+		force_assume = !strcmp(argv[0], "-p");
 		argc = parse_options(argc, argv, prefix, options,
 				     git_stash_push_usage,
-				     0);
+				     PARSE_OPT_KEEP_DASHDASH);
+	}
+
+	if (argc) {
+		if (!strcmp(argv[0], "--")) {
+			argc--;
+			argv++;
+		} else if (push_assumed && !force_assume) {
+			die("subcommand wasn't specified; 'push' can't be assumed due to unexpected token '%s'",
+			    argv[0]);
+		}
+	}
 
 	parse_pathspec(&ps, 0, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,
 		       prefix, argv);
@@ -1550,7 +1564,6 @@ static int use_builtin_stash(void)
 
 int cmd_stash(int argc, const char **argv, const char *prefix)
 {
-	int i = -1;
 	pid_t pid = getpid();
 	const char *index_file;
 	struct argv_array args = ARGV_ARRAY_INIT;
@@ -1583,7 +1596,7 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
 		    (uintmax_t)pid);
 
 	if (!argc)
-		return !!push_stash(0, NULL, prefix);
+		return !!push_stash(0, NULL, prefix, 0);
 	else if (!strcmp(argv[0], "apply"))
 		return !!apply_stash(argc, argv, prefix);
 	else if (!strcmp(argv[0], "clear"))
@@ -1603,45 +1616,15 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
 	else if (!strcmp(argv[0], "create"))
 		return !!create_stash(argc, argv, prefix);
 	else if (!strcmp(argv[0], "push"))
-		return !!push_stash(argc, argv, prefix);
+		return !!push_stash(argc, argv, prefix, 0);
 	else if (!strcmp(argv[0], "save"))
 		return !!save_stash(argc, argv, prefix);
 	else if (*argv[0] != '-')
 		usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
 			      git_stash_usage, options);
 
-	if (strcmp(argv[0], "-p")) {
-		while (++i < argc && strcmp(argv[i], "--")) {
-			/*
-			 * `akpqu` is a string which contains all short options,
-			 * except `-m` which is verified separately.
-			 */
-			if ((strlen(argv[i]) == 2) && *argv[i] == '-' &&
-			    strchr("akpqu", argv[i][1]))
-				continue;
-
-			if (!strcmp(argv[i], "--all") ||
-			    !strcmp(argv[i], "--keep-index") ||
-			    !strcmp(argv[i], "--no-keep-index") ||
-			    !strcmp(argv[i], "--patch") ||
-			    !strcmp(argv[i], "--quiet") ||
-			    !strcmp(argv[i], "--include-untracked"))
-				continue;
-
-			/*
-			 * `-m` and `--message=` are verified separately because
-			 * they need to be immediately followed by a string
-			 * (i.e.`-m"foobar"` or `--message="foobar"`).
-			 */
-			if (starts_with(argv[i], "-m") ||
-			    starts_with(argv[i], "--message="))
-				continue;
-
-			usage_with_options(git_stash_usage, options);
-		}
-	}
-
+	/* Assume 'stash push' */
 	argv_array_push(&args, "push");
 	argv_array_pushv(&args, argv);
-	return !!push_stash(args.argc, args.argv, prefix);
+	return !!push_stash(args.argc, args.argv, prefix, 1);
 }
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index ea56e85e70d..3ad23e2502b 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -285,6 +285,11 @@ test_expect_success 'stash --no-keep-index' '
 	test bar,bar2 = $(cat file),$(cat file2)
 '
 
+test_expect_success 'dont assume push with non-option args' '
+	test_must_fail git stash -q drop 2>err &&
+	test_i18ngrep -e "subcommand wasn'\''t specified; '\''push'\'' can'\''t be assumed due to unexpected token '\''drop'\''" err
+'
+
 test_expect_success 'stash --invalid-option' '
 	echo bar5 >file &&
 	echo bar6 >file2 &&
-- 
gitgitgadget


  parent reply	other threads:[~2020-02-17 17:25 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-16 16:09 [PATCH 0/8] Support --pathspec-from-file in rm, stash Alexandr Miloslavskiy via GitGitGadget
2020-01-16 16:09 ` [PATCH 1/8] doc: rm: synchronize <pathspec> description Alexandr Miloslavskiy via GitGitGadget
2020-01-21 19:14   ` Junio C Hamano
2020-01-16 16:09 ` [PATCH 2/8] rm: support the --pathspec-from-file option Alexandr Miloslavskiy via GitGitGadget
2020-01-21 19:36   ` Junio C Hamano
2020-02-10 14:46     ` Alexandr Miloslavskiy
2020-02-10 18:48       ` Junio C Hamano
2020-02-17 17:25         ` Alexandr Miloslavskiy
2020-01-16 16:09 ` [PATCH 3/8] doc: stash: split options from description (1) Alexandr Miloslavskiy via GitGitGadget
2020-01-16 16:09 ` [PATCH 4/8] doc: stash: split options from description (2) Alexandr Miloslavskiy via GitGitGadget
2020-01-21 20:21   ` Junio C Hamano
2020-02-10 14:47     ` Alexandr Miloslavskiy
2020-01-16 16:09 ` [PATCH 5/8] doc: stash: document more options Alexandr Miloslavskiy via GitGitGadget
2020-01-21 20:29   ` Junio C Hamano
2020-01-16 16:09 ` [PATCH 6/8] doc: stash: synchronize <pathspec> description Alexandr Miloslavskiy via GitGitGadget
2020-01-21 20:29   ` Junio C Hamano
2020-01-16 16:09 ` [PATCH 7/8] stash: eliminate crude option parsing Alexandr Miloslavskiy via GitGitGadget
2020-01-16 16:09 ` [PATCH 8/8] stash push: support the --pathspec-from-file option Alexandr Miloslavskiy via GitGitGadget
2020-02-10 14:45 ` [PATCH v2 0/8] Support --pathspec-from-file in rm, stash Alexandr Miloslavskiy via GitGitGadget
2020-02-10 14:45   ` [PATCH v2 1/8] doc: rm: synchronize <pathspec> description Alexandr Miloslavskiy via GitGitGadget
2020-02-10 14:45   ` [PATCH v2 2/8] rm: support the --pathspec-from-file option Alexandr Miloslavskiy via GitGitGadget
2020-02-10 20:39     ` Junio C Hamano
2020-02-17 17:27       ` Alexandr Miloslavskiy
2020-02-17 17:59         ` Junio C Hamano
2020-02-17 21:03           ` Junio C Hamano
2020-02-10 14:45   ` [PATCH v2 3/8] doc: stash: split options from description (1) Alexandr Miloslavskiy via GitGitGadget
2020-02-10 14:45   ` [PATCH v2 4/8] doc: stash: split options from description (2) Alexandr Miloslavskiy via GitGitGadget
2020-02-10 14:45   ` [PATCH v2 5/8] doc: stash: document more options Alexandr Miloslavskiy via GitGitGadget
2020-02-10 14:45   ` [PATCH v2 6/8] doc: stash: synchronize <pathspec> description Alexandr Miloslavskiy via GitGitGadget
2020-02-10 14:45   ` [PATCH v2 7/8] stash: eliminate crude option parsing Alexandr Miloslavskiy via GitGitGadget
2020-02-10 14:45   ` [PATCH v2 8/8] stash push: support the --pathspec-from-file option Alexandr Miloslavskiy via GitGitGadget
2020-02-17 17:25   ` [PATCH v3 0/8] Support --pathspec-from-file in rm, stash Alexandr Miloslavskiy via GitGitGadget
2020-02-17 17:25     ` [PATCH v3 1/8] doc: rm: synchronize <pathspec> description Alexandr Miloslavskiy via GitGitGadget
2020-02-17 17:25     ` [PATCH v3 2/8] rm: support the --pathspec-from-file option Alexandr Miloslavskiy via GitGitGadget
2020-02-17 18:06       ` Alexandr Miloslavskiy
2020-02-17 17:25     ` [PATCH v3 3/8] doc: stash: split options from description (1) Alexandr Miloslavskiy via GitGitGadget
2020-02-17 17:25     ` [PATCH v3 4/8] doc: stash: split options from description (2) Alexandr Miloslavskiy via GitGitGadget
2020-02-17 17:25     ` [PATCH v3 5/8] doc: stash: document more options Alexandr Miloslavskiy via GitGitGadget
2020-02-17 17:25     ` [PATCH v3 6/8] doc: stash: synchronize <pathspec> description Alexandr Miloslavskiy via GitGitGadget
2020-02-17 17:25     ` Alexandr Miloslavskiy via GitGitGadget [this message]
2020-02-17 17:25     ` [PATCH v3 8/8] stash push: support the --pathspec-from-file option Alexandr Miloslavskiy via GitGitGadget

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

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d34eaf4a27236591a9bc345d5a2cb465542a3243.1581960322.git.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=alexandr.miloslavskiy@syntevo.com \
    --cc=git@vger.kernel.org \
    --cc=ungureanupaulsebastian@gmail.com \
    /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

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ http://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git