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

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

[Carlos Rica]

Signed-off-by: Carlos Rica <jasampler@gmail.com>
---

   Since builtin-reset.c changed in the recent last update,
   this patch is built on top of it to make easier applying it.

   The only change from previous version is the struct options, now const.

   Note that --h ambiguity favoring --hard over --help is still here,
   not a big problem because --help cannot be abbreviated in git.

   I copied the SYNOPSIS from the documentation to enhance the
   help message, splitting the old one-line message in two lines.
   Now this usage is showed when you run "git reset -h" and
   also when you includes an unknown option in command line.

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

diff --git a/builtin-reset.c b/builtin-reset.c
index bb3e192..79424bb 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)
 {
@@ -165,40 +169,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];
+	const 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;
-	}

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

[Carlos Rica]

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;
-	}

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

[Junio C Hamano]

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.

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

[Carlos Rica]

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

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

[Junio C Hamano]

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

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

[Carlos Rica]

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?

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

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1567 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Junio C Hamano]

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.

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

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1104 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Johannes Schindelin]

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

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

[Alex Riesen]

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

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

[Carlos Rica]

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?

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

[Alex Riesen]

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

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

[Carlos Rica]

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?

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

[Alex Riesen]

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.

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

[Carlos Rica]

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.