Re: [PATCH 5/6] checkout-index: disallow "--no-stage" option

On Sun, Jan 31, 2016 at 06:18:39PM -0800, Junio C Hamano wrote:

quoted

 builtin/checkout-index.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index f8179a7..7a9b561 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c

@@ -133,6 +133,8 @@ static struct lock_file lock_file;
 static int option_parse_stage(const struct option *opt,
 			      const char *arg, int unset)
 {
+	if (unset)
+		return error(_("--stage cannot be negated"));

Hmm, it is surprising that there is no parse-options flag that says
"this cannot be negated".

There is, but you have to stop using the nice OPT_CALLBACK macro and do
a full-on "{}" struct literal in the options. Since we have a callback,
I figured doing this is less ugly to look at. But it does mean making up
our own message.

I didn't actually try it yesterday; having just done so, it's a lot less
bad than I expected. And I also noticed yet another problem with it. :-/

How about this as a replacement patch?

-- >8 --
Subject: [PATCH] checkout-index: disallow "--no-stage" option

We do not really expect people to use "--no-stage", but if
they do, git currently segfaults. We could instead have it
undo the effects of a previous "--stage", but this gets
tricky around the "to_tempfile" flag. We cannot simply reset
it to 0, because we don't know if it was set by a previous
"--stage=all" or an explicit "--temp" option.

We could solve this by setting a flag and resolving
to_tempfile later, but it's not worth the effort. Nobody
actually wants to use "--no-stage"; we are just trying to
fix a potential segfault here.

While we're in the area, let's improve the user-facing
messages for this option. The error string should be
translatable, and we should give some hint in the "-h"
output about what can go in the argument field.

Signed-off-by: Jeff King <redacted>
---
I also notice that you cannot use "--stage=0" to reset a previous
"--stage=1". It probably would not hurt to allow that, but I left it out
of this patch.

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

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index f8179a7..92c6967 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c

@@ -141,7 +141,7 @@ static int option_parse_stage(const struct option *opt,
 		if ('1' <= ch && ch <= '3')
 			checkout_stage = arg[0] - '0';
 		else
-			die("stage should be between 1 and 3 or all");
+			die(_("stage should be between 1 and 3 or all"));
 	}
 	return 0;
 }

@@ -173,9 +173,9 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 			N_("write the content to temporary files")),
 		OPT_STRING(0, "prefix", &state.base_dir, N_("string"),
 			N_("when creating files, prepend <string>")),
-		OPT_CALLBACK(0, "stage", NULL, NULL,
+		{ OPTION_CALLBACK, 0, "stage", NULL, "1-3|all",
 			N_("copy out the files from named stage"),
-			option_parse_stage),
+			PARSE_OPT_NONEG, option_parse_stage },
 		OPT_END()
 	};
 

-- 
2.7.0.489.g6faad84