# [PATCH] Make builtin-reset.c use parse_options.

15 messages from 2008-03-01 to 2008-03-04. Participants: Carlos Rica, Junio C Hamano, Alex Riesen, Johannes Schindelin, Pierre Habouzit.
Thread: https://gitlist.dev/t/12411

## Carlos Rica, 2008-03-01 16:29

Subject: [PATCH] Make builtin-reset.c use parse_options.
Message-ID: <47C98472.8000002@gmail.com>
URL: https://gitlist.dev/e/47C98472.8000002%40gmail.com

```

Signed-off-by: Carlos Rica <jasampler@gmail.com>
---
 builtin-reset.c |   47 ++++++++++++++++++++---------------------------
 1 files changed, 20 insertions(+), 27 deletions(-)

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, 2008-03-02 02:53

Subject: Re: [PATCH] Make builtin-reset.c use parse_options.
Message-ID: <7vir05x2di.fsf@gitster.siamese.dyndns.org>
URL: https://gitlist.dev/e/7vir05x2di.fsf%40gitster.siamese.dyndns.org
In-Reply-To: <47C98472.8000002@gmail.com>

```
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.

```

## Alex Riesen, 2008-03-02 09:40

Subject: Re: [PATCH] Make builtin-reset.c use parse_options.
Message-ID: <20080302094031.GA2973@steel.home>
URL: https://gitlist.dev/e/20080302094031.GA2973%40steel.home
In-Reply-To: <47C98472.8000002@gmail.com>

```
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"?


```

## Carlos Rica, 2008-03-02 12:37

Subject: Re: [PATCH] Make builtin-reset.c use parse_options.
Message-ID: <1b46aba20803020437p2a5128a5wfee3e98c32c1a610@mail.gmail.com>
URL: https://gitlist.dev/e/1b46aba20803020437p2a5128a5wfee3e98c32c1a610%40mail.gmail.com
In-Reply-To: <7vir05x2di.fsf@gitster.siamese.dyndns.org>

```
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?

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

```

## Carlos Rica, 2008-03-02 12:54

Subject: Re: [PATCH] Make builtin-reset.c use parse_options.
Message-ID: <1b46aba20803020454v322f061bi4f34a737e9e2b7f3@mail.gmail.com>
URL: https://gitlist.dev/e/1b46aba20803020454v322f061bi4f34a737e9e2b7f3%40mail.gmail.com
In-Reply-To: <20080302094031.GA2973@steel.home>

```
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?

```

## Alex Riesen, 2008-03-02 15:55

Subject: Re: [PATCH] Make builtin-reset.c use parse_options.
Message-ID: <20080302155541.GH2973@steel.home>
URL: https://gitlist.dev/e/20080302155541.GH2973%40steel.home
In-Reply-To: <1b46aba20803020454v322f061bi4f34a737e9e2b7f3@mail.gmail.com>

```
Carlos Rica, Sun, Mar 02, 2008 13:54:10 +0100:
> 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

```

## Junio C Hamano, 2008-03-02 16:18

Subject: Re: [PATCH] Make builtin-reset.c use parse_options.
Message-ID: <7vbq5xqetq.fsf@gitster.siamese.dyndns.org>
URL: https://gitlist.dev/e/7vbq5xqetq.fsf%40gitster.siamese.dyndns.org
In-Reply-To: <1b46aba20803020437p2a5128a5wfee3e98c32c1a610@mail.gmail.com>

```
"Carlos Rica" <jasampler@gmail.com> writes:

> 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...


```

## Johannes Schindelin, 2008-03-02 16:32

Subject: Re: [PATCH] Make builtin-reset.c use parse_options.
Message-ID: <alpine.LSU.1.00.0803021631210.22527@racer.site>
URL: https://gitlist.dev/e/alpine.LSU.1.00.0803021631210.22527%40racer.site
In-Reply-To: <1b46aba20803020437p2a5128a5wfee3e98c32c1a610@mail.gmail.com>

```
Hi,

On Sun, 2 Mar 2008, Carlos Rica wrote:

> 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


```

## Carlos Rica, 2008-03-02 18:40

Subject: Re: [PATCH] Make builtin-reset.c use parse_options.
Message-ID: <1b46aba20803021040ofec56b9q120a654a192eec09@mail.gmail.com>
URL: https://gitlist.dev/e/1b46aba20803021040ofec56b9q120a654a192eec09%40mail.gmail.com
In-Reply-To: <20080302155541.GH2973@steel.home>

```
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, 2008-03-02 21:38

Subject: Re: [PATCH] Make builtin-reset.c use parse_options.
Message-ID: <20080302213843.GA13954@steel.home>
URL: https://gitlist.dev/e/20080302213843.GA13954%40steel.home
In-Reply-To: <1b46aba20803021040ofec56b9q120a654a192eec09@mail.gmail.com>

```
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?

> 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, 2008-03-03 14:07

Subject: Re: [PATCH] Make builtin-reset.c use parse_options.
Message-ID: <1b46aba20803030607v726a6ba1y49ca8524e09c6eae@mail.gmail.com>
URL: https://gitlist.dev/e/1b46aba20803030607v726a6ba1y49ca8524e09c6eae%40mail.gmail.com
In-Reply-To: <7vbq5xqetq.fsf@gitster.siamese.dyndns.org>

```
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?

```

## Carlos Rica, 2008-03-03 14:39

Subject: Re: [PATCH] Make builtin-reset.c use parse_options.
Message-ID: <1b46aba20803030639i73557fb3v44de6387b8fd57a7@mail.gmail.com>
URL: https://gitlist.dev/e/1b46aba20803030639i73557fb3v44de6387b8fd57a7%40mail.gmail.com
In-Reply-To: <20080302213843.GA13954@steel.home>

```
On Sun, Mar 2, 2008 at 10:38 PM, Alex Riesen <raa.lkml@gmail.com> wrote:
> 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.

```

## Pierre Habouzit, 2008-03-03 17:07

Subject: Re: [PATCH] Make builtin-reset.c use parse_options.
Message-ID: <20080303170706.GA23210@artemis.madism.org>
URL: https://gitlist.dev/e/20080303170706.GA23210%40artemis.madism.org
In-Reply-To: <1b46aba20803030607v726a6ba1y49ca8524e09c6eae@mail.gmail.com>

```
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.

  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, 2008-03-03 22:40

Subject: Re: [PATCH] Make builtin-reset.c use parse_options.
Message-ID: <7vwsojfn2v.fsf@gitster.siamese.dyndns.org>
URL: https://gitlist.dev/e/7vwsojfn2v.fsf%40gitster.siamese.dyndns.org
In-Reply-To: <20080303170706.GA23210@artemis.madism.org>

```
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.

```

## Pierre Habouzit, 2008-03-04 09:45

Subject: Re: [PATCH] Make builtin-reset.c use parse_options.
Message-ID: <20080304094527.GB32019@artemis.madism.org>
URL: https://gitlist.dev/e/20080304094527.GB32019%40artemis.madism.org
In-Reply-To: <7vwsojfn2v.fsf@gitster.siamese.dyndns.org>

```
On Mon, Mar 03, 2008 at 10:40:40PM +0000, Junio C Hamano wrote:
> 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

```