threads / patch / 12411

patchMake builtin-reset.c use parse_options.

Subject: [PATCH] Make builtin-reset.c use parse_options.

## tl;dr

15 messages between Mar 1, 2008 and Mar 4, 2008. Diffs are folded; open one to read it.

replies: 14people: 5as markdown or json

Carlos Rica· Mar 1, 2008, 16:29 UTC · lore
Signed-off-by: Carlos Rica <jasampler@gmail.com>
---
 builtin-reset.c |   47 ++++++++++++++++++++---------------------------
 1 files changed, 20 insertions(+), 27 deletions(-)
Show changes to builtin-reset.c +19 −23
diff --git a/builtin-reset.c b/builtin-reset.c
index af0037e..71892d0 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -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;
-	}
Junio C Hamano· Mar 2, 2008, 02:53 UTC · re: Carlos Rica · lore

Re: [PATCH] Make builtin-reset.c use parse_options.

Carlos Rica <jasampler@gmail.com> writes:
> Signed-off-by: Carlos Rica <jasampler@gmail.com>
Hmmm.  "git reset -h" now defaults to --hard? 
It somehow feels a bit risky for new people.  I dunno.
Carlos Rica· Mar 2, 2008, 12:37 UTC · re: Junio C Hamano · lore

Re: [PATCH] Make builtin-reset.c use parse_options.

On Sun, Mar 2, 2008 at 3:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 8 quoted lines
> Carlos Rica <jasampler@gmail.com> writes:
>
>  > Signed-off-by: Carlos Rica <jasampler@gmail.com>
>
>  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· Mar 2, 2008, 16:18 UTC · re: Carlos Rica · lore

Re: [PATCH] Make builtin-reset.c use parse_options.

"Carlos Rica" <jasampler@gmail.com> writes:
Show 13 quoted lines
> On Sun, Mar 2, 2008 at 3:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Carlos Rica <jasampler@gmail.com> writes:
>>
>>  > Signed-off-by: Carlos Rica <jasampler@gmail.com>
>>
>>  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· Mar 3, 2008, 14:07 UTC · re: Junio C Hamano · lore

Re: [PATCH] Make builtin-reset.c use parse_options.

On Sun, Mar 2, 2008 at 5:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
>  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· Mar 3, 2008, 17:07 UTC · re: Carlos Rica · lore

Re: [PATCH] Make builtin-reset.c use parse_options.

On Mon, Mar 03, 2008 at 02:07:57PM +0000, Carlos Rica wrote:
Show 9 quoted lines
> On Sun, Mar 2, 2008 at 5:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >  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.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Junio C Hamano· Mar 3, 2008, 22:40 UTC · re: Pierre Habouzit · lore

Re: [PATCH] Make builtin-reset.c use parse_options.

Pierre Habouzit <madcoder@debian.org> writes:
Show 14 quoted lines
> On Mon, Mar 03, 2008 at 02:07:57PM +0000, Carlos Rica wrote:
>> On Sun, Mar 2, 2008 at 5:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> >  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.
Yeah, I do not know if this really matters in real life, though.
Pierre Habouzit· Mar 4, 2008, 09:45 UTC · re: Junio C Hamano · lore

Re: [PATCH] Make builtin-reset.c use parse_options.

On Mon, Mar 03, 2008 at 10:40:40PM +0000, Junio C Hamano wrote:
Show 18 quoted lines
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > On Mon, Mar 03, 2008 at 02:07:57PM +0000, Carlos Rica wrote:
> >> On Sun, Mar 2, 2008 at 5:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >> >  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.
> 
> 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.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Johannes Schindelin· Mar 2, 2008, 16:32 UTC · re: Carlos Rica · lore

Re: [PATCH] Make builtin-reset.c use parse_options.

Hi,
On Sun, 2 Mar 2008, Carlos Rica wrote:
Show 11 quoted lines
> On Sun, Mar 2, 2008 at 3:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
> > Carlos Rica <jasampler@gmail.com> writes:
> >
> >  > Signed-off-by: Carlos Rica <jasampler@gmail.com>
> >
> >  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

Alex Riesen· Mar 2, 2008, 09:40 UTC · re: Carlos Rica · lore

Re: [PATCH] Make builtin-reset.c use parse_options.

Carlos Rica, Sat, Mar 01, 2008 17:29:38 +0100:
Show 12 quoted lines
> @@ -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[] = {
"static const"?
Carlos Rica· Mar 2, 2008, 12:54 UTC · re: Alex Riesen · lore

Re: [PATCH] Make builtin-reset.c use parse_options.

On Sun, Mar 2, 2008 at 10:40 AM, Alex Riesen <raa.lkml@gmail.com> wrote:
Show 18 quoted lines
> Carlos Rica, Sat, Mar 01, 2008 17:29:38 +0100:
>
> > @@ -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[] = {
>
>  "static const"?
>
>

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

Alex Riesen· Mar 2, 2008, 15:55 UTC · re: Carlos Rica · lore

Re: [PATCH] Make builtin-reset.c use parse_options.

Carlos Rica, Sun, Mar 02, 2008 13:54:10 +0100:
Show 22 quoted lines
> On Sun, Mar 2, 2008 at 10:40 AM, Alex Riesen <raa.lkml@gmail.com> wrote:
> > Carlos Rica, Sat, Mar 01, 2008 17:29:38 +0100:
> >
> > > @@ -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[] = {
> >
> >  "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· Mar 2, 2008, 18:40 UTC · re: Alex Riesen · lore

Re: [PATCH] Make builtin-reset.c use parse_options.

On Sun, Mar 2, 2008 at 4:55 PM, Alex Riesen <raa.lkml@gmail.com> wrote:
>
>  "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· Mar 2, 2008, 21:38 UTC · re: Carlos Rica · lore

Re: [PATCH] Make builtin-reset.c use parse_options.

Carlos Rica, Sun, Mar 02, 2008 19:40:09 +0100:
Show 5 quoted lines
> On Sun, Mar 2, 2008 at 4:55 PM, Alex Riesen <raa.lkml@gmail.com> wrote:
> >
> >  "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).

> 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?
> I don't know benefits of making all of them "static".
It is initialized statically.
> Has this been discussed previously?
Yeah. Sometime around 1972.
Carlos Rica· Mar 3, 2008, 14:39 UTC · re: Alex Riesen · lore

Re: [PATCH] Make builtin-reset.c use parse_options.

On Sun, Mar 2, 2008 at 10:38 PM, Alex Riesen <raa.lkml@gmail.com> wrote:
Show 18 quoted lines
> Carlos Rica, Sun, Mar 02, 2008 19:40:09 +0100:
>
> > On Sun, Mar 2, 2008 at 4:55 PM, Alex Riesen <raa.lkml@gmail.com> wrote:
>  > >
>  > >  "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).
>
>
>  > 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.

← back to recent threads