Make builtin-reset.c use parse_options.

PATCH In review Carlos Rica 5 participants 15 messages Jun 15, 2016

lore

Share & tools

lore

Carlos Rica

author

2016-06-15 22:44 UTC #0 lore raw

PATCH 1/1 Make builtin-reset.c use parse_options.

Signed-off-by Carlos Rica <[email protected]>

---
 builtin-reset.c |   47 ++++++++++++++++++++---------------------------
 1 files changed, 20 insertions(+), 27 deletions(-)

builtin-reset.c +19 ~~-23~~

index af0037e..71892d0 100644
@@ -17,9 +17,13 @@
 #include "diffcore.h"
 #include "tree.h"
 #include "branch.h"
+#include "parse-options.h"
 
-static const char builtin_reset_usage[] =
-"git-reset [--mixed | --soft | --hard] [-q] [<commit-ish>] [ [--] <paths>...]";
+static const char * const git_reset_usage[] = {
+	"git-reset [--mixed | --soft | --hard] [-q] [<commit>]",
+	"git-reset [--mixed] <commit> [--] <paths>...",
+	NULL
+};
 
 static char *args_to_str(const char **argv)
 {

@@ -169,40 +173,31 @@ static const char *reset_type_names[] = { "mixed", "soft", "hard", NULL };
 
 int cmd_reset(int argc, const char **argv, const char *prefix)
 {
-	int i = 1, reset_type = NONE, update_ref_status = 0, quiet = 0;
+	int i = 0, reset_type = NONE, update_ref_status = 0, quiet = 0;
 	const char *rev = "HEAD";
 	unsigned char sha1[20], *orig = NULL, sha1_orig[20],
 				*old_orig = NULL, sha1_old_orig[20];
 	struct commit *commit;
 	char *reflog_action, msg[1024];
+	struct option options[] = {
+		OPT_SET_INT(0, "mixed", &reset_type,
+						"reset HEAD and index", MIXED),
+		OPT_SET_INT(0, "soft", &reset_type, "reset only HEAD", SOFT),
+		OPT_SET_INT(0, "hard", &reset_type,
+				"reset HEAD, index and working tree", HARD),
+		OPT_BOOLEAN('q', NULL, &quiet,
+				"disable showing new HEAD in hard reset"),
+		OPT_END()
+	};
 
 	git_config(git_default_config);
 
+	argc = parse_options(argc, argv, options, git_reset_usage,
+						PARSE_OPT_KEEP_DASHDASH);
 	reflog_action = args_to_str(argv);
 	setenv("GIT_REFLOG_ACTION", reflog_action, 0);
 
-	while (i < argc) {
-		if (!strcmp(argv[i], "--mixed")) {
-			reset_type = MIXED;
-			i++;
-		}
-		else if (!strcmp(argv[i], "--soft")) {
-			reset_type = SOFT;
-			i++;
-		}
-		else if (!strcmp(argv[i], "--hard")) {
-			reset_type = HARD;
-			i++;
-		}
-		else if (!strcmp(argv[i], "-q")) {
-			quiet = 1;
-			i++;
-		}
-		else
-			break;
-	}

Carlos Rica

author

2016-06-15 22:44 UTC #2 lore raw

Quoted 4 lines · Alex Riesen

Carlos Rica, Sat, Mar 01, 2008 17:29:38 +0100:
Quoted 2 lines
(quoted diff) +0 -0
@@ -169,40 +173,31 @@ static const char *reset_type_names[] = { "mixed", "soft", "hard", NULL };
 

Quoted 14 lines

builtin-reset.c static const char *reset_type_names[] = { "mixed", "soft", "hard", NULL }; · line 182

Quoted 10 lines

builtin-reset.c +2 -1

 int cmd_reset(int argc, const char **argv, const char *prefix)
 {
-     int i = 1, reset_type = NONE, update_ref_status = 0, quiet = 0;
+     int i = 0, reset_type = NONE, update_ref_status = 0, quiet = 0;
      const char *rev = "HEAD";
      unsigned char sha1[20], *orig = NULL, sha1_orig[20],
                              *old_orig = NULL, sha1_old_orig[20];
      struct commit *commit;
      char *reflog_action, msg[1024];
+     struct option options[] = {

"static const"?

"static const" what?
options? cmd_reset? reset_type_names?

Alex Riesen

2016-06-15 22:44 UTC #3 lore raw

Carlos Rica, Sun, Mar 02, 2008 13:54:10 +0100:

Quoted 16 lines · Alex Riesen

Quoted 15 lines

Carlos Rica, Sat, Mar 01, 2008 17:29:38 +0100:

Quoted 12 lines

builtin-reset.c +2 -1

@@ -169,40 +173,31 @@ static const char *reset_type_names[] = { "mixed", "soft", "hard", NULL };
 
 int cmd_reset(int argc, const char **argv, const char *prefix)
 {
-     int i = 1, reset_type = NONE, update_ref_status = 0, quiet = 0;
+     int i = 0, reset_type = NONE, update_ref_status = 0, quiet = 0;
      const char *rev = "HEAD";
      unsigned char sha1[20], *orig = NULL, sha1_orig[20],
                              *old_orig = NULL, sha1_old_orig[20];
      struct commit *commit;
      char *reflog_action, msg[1024];
+     struct option options[] = {

Quoted 6 lines

Quoted 3 lines
"static const"?
"static const" what?
options? cmd_reset? reset_type_names?

"static const struct option options[] = {"

the others are already either statics or const properly

Carlos Rica

author

2016-06-15 22:44 UTC #4 lore raw

Quoted 2 lines · Alex Riesen

"static const struct option options[] = {"

The other files using parse_options have only "static", or nothing.

To make "options" static, then reset_type and quiet should be static too, otherwise it cannot compile (in my system).

I don't know benefits of making all of them "static". Has this been discussed previously?

Alex Riesen

2016-06-15 22:44 UTC #5 lore raw

Carlos Rica, Sun, Mar 02, 2008 19:40:09 +0100:

Quoted 5 lines · Alex Riesen

Quoted 2 lines
"static const struct option options[] = {"
The other files using parse_options have only "static", or nothing.

Well, they all miss something. Besides all nice things about static syntax checking, the compiler (GCC) can optimize string constants to use the same data (not that it is interesting in this particular case).

Quoted 2 lines

To make "options" static, then reset_type and quiet should be static too, otherwise it cannot compile (in my system).

Of course. Is it a problem for user-interface level code?

Quoted 1 line

I don't know benefits of making all of them "static".

It is initialized statically.

Quoted 1 line

Has this been discussed previously?

Yeah. Sometime around 1972.

Carlos Rica

author

2016-06-15 22:44 UTC #6 lore raw

Quoted 14 lines · Alex Riesen

Carlos Rica, Sun, Mar 02, 2008 19:40:09 +0100:
Quoted 5 lines · Alex Riesen
Quoted 2 lines
"static const struct option options[] = {"
The other files using parse_options have only "static", or nothing.
Well, they all miss something. Besides all nice things about static syntax checking, the compiler (GCC) can optimize string constants to use the same data (not that it is interesting in this particular case).

Quoted 4 lines

Quoted 2 lines
To make "options" static, then reset_type and quiet should be static too, otherwise it cannot compile (in my system).
Of course. Is it a problem for user-interface level code?

The only problem could come from reusing this code to calling many times to a cmd_reset function in the future. Then, I would prefer not to worry about previous values in the variables by using only automatic variables in the function.

Anyway, "const" is nice, since the options struct doesn't change.

Junio C Hamano

2016-06-15 22:44 UTC #7 lore raw

In reply to Carlos Rica

Quoted 1 line · Carlos Rica

Signed-off-by Carlos Rica <[email protected]>

Hmmm. "git reset -h" now defaults to --hard?

It somehow feels a bit risky for new people. I dunno.

Carlos Rica

author

2016-06-15 22:44 UTC #8 lore raw

Quoted 8 lines · Junio C Hamano

Quoted 1 line · Carlos Rica
Signed-off-by Carlos Rica <[email protected]>
Hmmm. "git reset -h" now defaults to --hard?
It somehow feels a bit risky for new people. I dunno.

I don't understand what do you mean.

Option -h just prints the options and exits. Do you mean that the help message is wrong?

Also, there is a test to check that "git reset" defaults to --mixed ('--mixed reset to HEAD should unadd the files' only run a "git reset" without parameters),
and changing it to do a --hard reset makes the tests to fail.

If there's a change in the behaviour of the command, please, show me how to test it. I cannot see anything wrong now.

-- Carlos

Junio C Hamano

2016-06-15 22:44 UTC #9 lore raw

Quoted 5 lines · Carlos Rica

Quoted 4 lines · Junio C Hamano
Quoted 1 line · Carlos Rica
Signed-off-by Carlos Rica <[email protected]>

Quoted 8 lines

Quoted 3 lines
Hmmm. "git reset -h" now defaults to --hard?
It somehow feels a bit risky for new people. I dunno.
I don't understand what do you mean.
Option -h just prints the options and exits.
Do you mean that the help message is wrong?

I guess I mistested when I first ran it. "-h" is Ok.

	$ ./git-reset -h
        usage: git-reset ...

Although "--h" still favors "--hard" over "--help":

	$ ./git-reset --h
        HEAD is now at c149184...

Carlos Rica

author

2016-06-15 22:44 UTC #10 lore raw

Quoted 5 lines · Junio C Hamano

Although "--h" still favors "--hard" over "--help":
        $ ./git-reset --h
        HEAD is now at c149184...

Pierre, is there a way to give preference to --help over --hard when someone uses --h in command line?

Pierre Habouzit

2016-06-15 22:44 UTC #11 lore raw

Quoted 9 lines · Carlos Rica

Quoted 5 lines · Junio C Hamano
Although "--h" still favors "--hard" over "--help":
        $ ./git-reset --h
        HEAD is now at c149184...
Pierre, is there a way to give preference to --help over --hard when someone uses --h in command line?

The problem is that --help (and --help-all for the matter) are "magic"
arguments that parse-options is not aware of when it deals with abbreviations.

I assume the sole way is to always test against --help (--help-all
whom --help is a prefix anyways) for prefixes.

So basically we should replace the block in parse-options.c:

        if (ambiguous_option)
            ...
        if (abbrev_option)
            return get_value(p, abbrev_option, abbrev_flags);

with something that basically does:

   ambiguous:
       if (ambiguous_option)
            ...
       if (abbrev_option) {
           if (clashes with --help) {
               ambiguous_option = "help";
               ambiguous_flags = 0;
               goto ambiguous;
           }
           return get_value....
      }

the "if (clashes with --help)" obviously has to be expansed as real code, and should use an array of hardcoded values so that it's extensible if the need arises.

Signature · 3 lines

·O·  Pierre Habouzit
··O                                                [email protected]
OOO                                                http://www.madism.org

Junio C Hamano

2016-06-15 22:44 UTC #12 lore raw

Quoted 8 lines · Pierre Habouzit

Quoted 7 lines · Carlos Rica
Quoted 5 lines · Junio C Hamano
Although "--h" still favors "--hard" over "--help":
        $ ./git-reset --h
        HEAD is now at c149184...

Quoted 6 lines

Quoted 2 lines
Pierre, is there a way to give preference to --help over --hard when someone uses --h in command line?
The problem is that --help (and --help-all for the matter) are "magic"
arguments that parse-options is not aware of when it deals with abbreviations.

Yeah, I do not know if this really matters in real life, though.

Pierre Habouzit

2016-06-15 22:44 UTC #13 lore raw

Quoted 10 lines · Junio C Hamano

Quoted 8 lines · Pierre Habouzit
Quoted 7 lines · Carlos Rica
Quoted 5 lines · Junio C Hamano
Although "--h" still favors "--hard" over "--help":
        $ ./git-reset --h
        HEAD is now at c149184...

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
Pierre, is there a way to give preference to --help over --hard when someone uses --h in command line?
The problem is that --help (and --help-all for the matter) are "magic"
arguments that parse-options is not aware of when it deals with abbreviations.
Yeah, I do not know if this really matters in real life, though.

I'm not sure either, that's why I didn't really bothered to _write_
the patch, I just mentioned how to do that if someone cares enough.

Signature · 3 lines

·O·  Pierre Habouzit
··O                                                [email protected]
OOO                                                http://www.madism.org

Johannes Schindelin

2016-06-15 22:44 UTC #14 lore raw

In reply to Carlos Rica

Hi,

Quoted 5 lines · Carlos Rica

Quoted 4 lines · Junio C Hamano
Quoted 1 line · Carlos Rica
Signed-off-by Carlos Rica <[email protected]>

Quoted 8 lines

Quoted 4 lines
Hmmm. "git reset -h" now defaults to --hard?
It somehow feels a bit risky for new people. I dunno.
Option -h just prints the options and exits.
From the original patch:

+ OPT_SET_INT(0, "hard", &reset_type,
+ "reset HEAD, index and working tree", HARD),

It might be that I am misreading something, but I gather that Junio missed that there is no short option here.

Junio, am I right?

Ciao,
Dscho