[RFC PATCH 0/2] Allow detached forms (--option arg) for git log options.

[RFC PATCH 0/2] Allow detached forms (--option arg) for git log options.

[Matthieu Moy]

Hi,

This has been bothering me for a while: many commands accept detached
form (like "git commit -m message" instead of "git commit -mmessage"),
but others don't, in particular, git log options like

git log -S<string>
git log --grep=<string>

do not accept spaces.

This small patch serie is a very early RFC: it implements the feature
for just two options. There are at least 4 ways towards a real
implementations:

1) nobody except me likes the feature, drop the RFC.

2) Implement the same for other options. That's very repetitive (for
   each option, there are two ifs: a prefixcmp and a strcmp), I don't
   like it much.

3) Write a function or macro that accepts both variants, and use it
   everywhere.

4) use parse-option for "git log" options and then get the feature for
   free.

Hence my question: is there any reason why "git log" hasn't been
migrated to parse-option? Or is it only that nobody did it yet?

What do you think?

Thanks,

Matthieu Moy (2):
  Allow "git log --grep foo" as synonym for "git log --grep=foo".
  Allow "git log -S string" as synonym for "git log -Sstring".

 diff.c     |    5 +++++
 revision.c |    4 ++++
 2 files changed, 9 insertions(+), 0 deletions(-)

-- 
1.7.2.23.g58c3b.dirty

[RFC PATCH 1/2] Allow "git log --grep foo" as synonym for "git log --grep=foo".

[Matthieu Moy]

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 revision.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/revision.c b/revision.c
index 7e82efd..e93bbd9 100644
--- a/revision.c
+++ b/revision.c
@@ -1148,6 +1148,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 			       int *unkc, const char **unkv)
 {
 	const char *arg = argv[0];
+	const char *optarg = argv[1];
 
 	/* pseudo revision arguments */
 	if (!strcmp(arg, "--all") || !strcmp(arg, "--branches") ||
@@ -1374,6 +1375,9 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		add_header_grep(revs, GREP_HEADER_COMMITTER, arg+12);
 	} else if (!prefixcmp(arg, "--grep=")) {
 		add_message_grep(revs, arg+7);
+	} else if (!strcmp(arg, "--grep")) {
+		add_message_grep(revs, optarg);
+		return 2;
 	} else if (!strcmp(arg, "--extended-regexp") || !strcmp(arg, "-E")) {
 		revs->grep_filter.regflags |= REG_EXTENDED;
 	} else if (!strcmp(arg, "--regexp-ignore-case") || !strcmp(arg, "-i")) {
-- 
1.7.2.23.g58c3b.dirty

Re: [RFC PATCH 1/2] Allow "git log --grep foo" as synonym for "git log --grep=foo".

[Sverre Rabbelier]

Heya,

On Mon, Jul 26, 2010 at 13:14, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
> +       } else if (!strcmp(arg, "--grep")) {
> +               add_message_grep(revs, optarg);
> +               return 2;

This one makes a little less sense since to me '--flag' are always
booleans, whereas '-m' can take an argument (such as '-m' from 'git
commit'.

-- 
Cheers,

Sverre Rabbelier

Re: [RFC PATCH 1/2] Allow "git log --grep foo" as synonym for "git log --grep=foo".

[Miles Bader]

Sverre Rabbelier <srabbelier@gmail.com> writes:
>> +       } else if (!strcmp(arg, "--grep")) {
>> +               add_message_grep(revs, optarg);
>> +               return 2;
>
> This one makes a little less sense since to me '--flag' are always
> booleans, whereas '-m' can take an argument (such as '-m' from 'git
> commit'.

The fact that --grep requires the "=" is amazingly confusing if you're
used to standard GNU long-argument parsing (which many standard
utilities use, and which git's argument syntax is clearly modelled
after), where both forms are equivalent, and documentation typically
only refers to the "=" form, but implicitly allows the separate-args
form.

I'm continually getting tripped up by git's idiosynchratic argument
parsing, and it's nice to see it getting cleaned up a bit...

-Miles

-- 
The trouble with most people is that they think with their hopes or
fears or wishes rather than with their minds.  -- Will Durant

Re: [RFC PATCH 1/2] Allow "git log --grep foo" as synonym for "git log --grep=foo".

[Jakub Narebski]

Miles Bader <miles@gnu.org> writes:
> Sverre Rabbelier <srabbelier@gmail.com> writes:

>>> +       } else if (!strcmp(arg, "--grep")) {
>>> +               add_message_grep(revs, optarg);
>>> +               return 2;
>>
>> This one makes a little less sense since to me '--flag' are always
>> booleans, whereas '-m' can take an argument (such as '-m' from 'git
>> commit'.
> 
> The fact that --grep requires the "=" is amazingly confusing if you're
> used to standard GNU long-argument parsing (which many standard
> utilities use, and which git's argument syntax is clearly modelled
> after), where both forms are equivalent, and documentation typically
> only refers to the "=" form, but implicitly allows the separate-args
> form.

I think that parseopt allows both sticky (-mfoo, --message=foo) and
non-sticky (-m foo, --message foo) forms, if I remember it correctly
with exception of arguments with *optional* parameters which require
sticky form.
 
> I'm continually getting tripped up by git's idiosynchratic argument
> parsing, and it's nice to see it getting cleaned up a bit...

I guess that this solution is simpler than moving to parseopt... is
that because log options and diff options crop everywhere?  Do
parseopt have no support for sub-parsers, like e.g. argp from libc:

  (libc.info.gz)Argp
  (libc.info.gz)Argp Children    
  http://www.gnu.org/s/libc/manual/html_node/Argp-Children.html

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [RFC PATCH 1/2] Allow "git log --grep foo" as synonym for "git log --grep=foo".

[Matthieu Moy]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> Heya,
>
> On Mon, Jul 26, 2010 at 13:14, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
>> +       } else if (!strcmp(arg, "--grep")) {
>> +               add_message_grep(revs, optarg);
>> +               return 2;
>
> This one makes a little less sense since to me '--flag' are always
> booleans, whereas '-m' can take an argument (such as '-m' from 'git
> commit'.

Try this:

  git commit --message foo

... it works. Parse-options allows it, but handwritten option parsing
for git log doesn't.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC PATCH 1/2] Allow "git log --grep foo" as synonym for "git log --grep=foo".

[Ævar Arnfjörð Bjarmason]

On Mon, Jul 26, 2010 at 18:14, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:

> +       } else if (!strcmp(arg, "--grep")) {
> +               add_message_grep(revs, optarg);
> +               return 2;

This looks good. I've been bitten by git-log's non-standard option
parsing. But there's still a lot of options that need the =, no?:

    21 matches for "="" in buffer: revision.c

                     1163:    if (!prefixcmp(arg, "--max-count=")) {
       1166:    } else if (!prefixcmp(arg, "--skip=")) {
       1181:    } else if (!prefixcmp(arg, "--max-age=")) {
       1183:    } else if (!prefixcmp(arg, "--since=")) {
       1185:    } else if (!prefixcmp(arg, "--after=")) {
       1187:    } else if (!prefixcmp(arg, "--min-age=")) {
       1189:    } else if (!prefixcmp(arg, "--before=")) {
       1191:    } else if (!prefixcmp(arg, "--until=")) {
       1272:    } else if (!prefixcmp(arg, "--unpacked=")) {
       1297:    } else if (!prefixcmp(arg, "--pretty=") ||
!prefixcmp(arg, "--format=")) {
       1304:    } else if (!prefixcmp(arg, "--show-notes=")) {
       1346:    } else if (!prefixcmp(arg, "--abbrev=")) {
       1362:    } else if (!strncmp(arg, "--date=", 7)) {
       1371:    else if (!prefixcmp(arg, "--author=")) {
       1373:    } else if (!prefixcmp(arg, "--committer=")) {
       1375:    } else if (!prefixcmp(arg, "--grep=")) {
       1385:    } else if (!prefixcmp(arg, "--encoding=")) {
       1515:            if (!prefixcmp(arg, "--glob=")) {
       1521:            if (!prefixcmp(arg, "--branches=")) {
       1527:            if (!prefixcmp(arg, "--tags=")) {
       1533:            if (!prefixcmp(arg, "--remotes=")) {

I think changing the option parsing so that it handles all the long
options consistently would be very nice (along with some tests). But
just making --grep a special case is more confusing than requiring =
everywhere.

Re: [RFC PATCH 1/2] Allow "git log --grep foo" as synonym for "git log --grep=foo".

[Matthieu Moy]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Mon, Jul 26, 2010 at 18:14, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
>
>> +       } else if (!strcmp(arg, "--grep")) {
>> +               add_message_grep(revs, optarg);
>> +               return 2;
>
> This looks good. I've been bitten by git-log's non-standard option
> parsing. But there's still a lot of options that need the =, no?:

Sure, that's why the patch is just an RFC. I wanted to start the
discussion before diving into the repetitive task or migration to
parse-option for others, and I picked --grep and -S because they're
the ones which annoys me the most.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC PATCH 1/2] Allow "git log --grep foo" as synonym for "git log --grep=foo".

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 27, 2010 at 11:56, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Mon, Jul 26, 2010 at 18:14, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
>>
>>> +       } else if (!strcmp(arg, "--grep")) {
>>> +               add_message_grep(revs, optarg);
>>> +               return 2;
>>
>> This looks good. I've been bitten by git-log's non-standard option
>> parsing. But there's still a lot of options that need the =, no?:
>
> Sure, that's why the patch is just an RFC. I wanted to start the
> discussion before diving into the repetitive task or migration to
> parse-option for others, and I picked --grep and -S because they're
> the ones which annoys me the most.

Ah, there was nothing to indicate that, so I thought I'd mention the
rest. I look forward to seeing what you'll come up with to consolidate
the option parsing later on.

Jakub's suggestion seems particularly interesting, maybe we can use
the optparse lib here with some modifications to allow it to parse a
subset of the total number of options.

Re: [RFC PATCH 1/2] Allow "git log --grep foo" as synonym for "git log --grep=foo".

[Matthieu Moy]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Tue, Jul 27, 2010 at 11:56, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>
>> Sure, that's why the patch is just an RFC. I wanted to start the
>> discussion before diving into the repetitive task or migration to
>> parse-option for others, and I picked --grep and -S because they're
>> the ones which annoys me the most.
>
> Ah, there was nothing to indicate that,

Except if you read carefully ;-)

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> This small patch serie is a very early RFC: it implements the feature
> for just two options.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC PATCH 1/2] Allow "git log --grep foo" as synonym for "git log --grep=foo".

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 27, 2010 at 13:26, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Ęvar Arnfjörš Bjarmason <avarab@gmail.com> writes:
>
>> On Tue, Jul 27, 2010 at 11:56, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>>
>>> Sure, that's why the patch is just an RFC. I wanted to start the
>>> discussion before diving into the repetitive task or migration to
>>> parse-option for others, and I picked --grep and -S because they're
>>> the ones which annoys me the most.
>>
>> Ah, there was nothing to indicate that,
>
> Except if you read carefully ;-)
>
> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> This small patch serie is a very early RFC: it implements the feature
>> for just two options.

Ah, missed that. Sorry for the noise.

[RFC PATCH 2/2] Allow "git log -S string" as synonym for "git log -Sstring".

[Matthieu Moy]

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 diff.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/diff.c b/diff.c
index 17873f3..4e3be89 100644
--- a/diff.c
+++ b/diff.c
@@ -2993,6 +2993,7 @@ static int diff_scoreopt_parse(const char *opt);
 int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 {
 	const char *arg = av[0];
+	const char *optarg = av[1];
 
 	/* Output format options */
 	if (!strcmp(arg, "-p") || !strcmp(arg, "-u") || !strcmp(arg, "--patch"))
@@ -3182,6 +3183,10 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		options->line_termination = 0;
 	else if (!prefixcmp(arg, "-l"))
 		options->rename_limit = strtoul(arg+2, NULL, 10);
+	else if (!strcmp(arg, "-S")) {
+		options->pickaxe = optarg;
+		return 2;
+	}
 	else if (!prefixcmp(arg, "-S"))
 		options->pickaxe = arg + 2;
 	else if (!strcmp(arg, "--pickaxe-all"))
-- 
1.7.2.23.g58c3b.dirty

Re: [RFC PATCH 2/2] Allow "git log -S string" as synonym for "git log -Sstring".

[Sverre Rabbelier]

Heya,

On Mon, Jul 26, 2010 at 13:14, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
>

> +       else if (!strcmp(arg, "-S")) {
> +               options->pickaxe = optarg;
> +               return 2;
> +       }

TYVM.

-- 
Cheers,

Sverre Rabbelier

Re: [RFC PATCH 0/2] Allow detached forms (--option arg) for git log options.

[Jonathan Nieder]

Hi Matthieu,

Matthieu Moy wrote:

>                    is there any reason why "git log" hasn't been
> migrated to parse-option? Or is it only that nobody did it yet?

Please go ahead. :)

The difficult piece is that the diff and revision handling options are
shared by a large number of commands.

I think my favorite idea is to provide macros to include the
appropriate entries in option tables[1].  Plus side: very easy for
callers to use.  Downside: bloats the option tables, though I think
that can be worked around.

Junio seemed to suggest that adapting the current multi-pass procedure
might be easier[2].

That said, in the meantime, something like this series (just for -S
it would be a big improvement already) would make sense to me.

Thanks.
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/85354/focus=85391
[2] http://thread.gmane.org/gmane.comp.version-control.git/85354/focus=85362

Re: [RFC PATCH 0/2] Allow detached forms (--option arg) for git log options.

[Pierre Habouzit]

On Mon, Jul 26, 2010 at 02:31:09PM -0500, Jonathan Nieder wrote:
> Hi Matthieu,
> 
> Matthieu Moy wrote:
> 
> >                    is there any reason why "git log" hasn't been
> > migrated to parse-option? Or is it only that nobody did it yet?
> 
> Please go ahead. :)

I started it in the past, but never went around to actually do it.

I started to get rid of most of the bitfields to use explicit or-ed
fields, but stopped at that, I don't even remember if those patches got
merged or not.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

Re: [RFC PATCH 0/2] Allow detached forms (--option arg) for git log options.

[Jakub Narebski]

Pierre Habouzit <madcoder@debian.org> writes:

> On Mon, Jul 26, 2010 at 02:31:09PM -0500, Jonathan Nieder wrote:
> > Hi Matthieu,
> > 
> > Matthieu Moy wrote:
> > 
> > >                    is there any reason why "git log" hasn't been
> > > migrated to parse-option? Or is it only that nobody did it yet?
> > 
> > Please go ahead. :)
> 
> I started it in the past, but never went around to actually do it.
> 
> I started to get rid of most of the bitfields to use explicit or-ed
> fields, but stopped at that, I don't even remember if those patches got
> merged or not.

Why did you feel this change was needed / necessary?  Was it
limitation of parseopt?  Or perhaps it was for portability reasons?
Or was it just the matter of code elegance?

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [RFC PATCH 0/2] Allow detached forms (--option arg) for git log options.

[Pierre Habouzit]

On Tue, Jul 27, 2010 at 08:10:35AM -0700, Jakub Narebski wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > On Mon, Jul 26, 2010 at 02:31:09PM -0500, Jonathan Nieder wrote:
> > > Hi Matthieu,
> > > 
> > > Matthieu Moy wrote:
> > > 
> > > >                    is there any reason why "git log" hasn't been
> > > > migrated to parse-option? Or is it only that nobody did it yet?
> > > 
> > > Please go ahead. :)
> > 
> > I started it in the past, but never went around to actually do it.
> > 
> > I started to get rid of most of the bitfields to use explicit or-ed
> > fields, but stopped at that, I don't even remember if those patches got
> > merged or not.
> 
> Why did you feel this change was needed / necessary?  Was it
> limitation of parseopt?  Or perhaps it was for portability reasons?
> Or was it just the matter of code elegance?

you cannot take the address of a bit portably in C, so you can't let
parseopt set/clear bits through bitfields (as in unsigned field : 1 in a
struct in C I mean).

So to use parseopt OPTION_BIT feature, you have to convert them to C
flags as in "unsigned flags" and explicit masks defines/enums.

IOW:

    struct foo {
       unsigned bar : 1,
		...
		baz : 1;
    };

Must be converted into:

    struct foo {
    #define FOO_FLAG_BAR (1U <<  1)
    ...
    #define FOO_FLAG_BAZ (1U << 18)
      unsigned flags;
    }

so that you can use parseopt.  that's what I meant.


This was done for the rev-list parsing stuff e.g.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

Re: [RFC PATCH 0/2] Allow detached forms (--option arg) for git log options.

[Jakub Narebski]

On Wed, 28 Jul 2010, Pierre Habouzit wrote:

> you cannot take the address of a bit portably in C, so you can't let
> parseopt set/clear bits through bitfields (as in unsigned field : 1 in a
> struct in C I mean).
> 
> So to use parseopt OPTION_BIT feature, you have to convert them to C
> flags as in "unsigned flags" and explicit masks defines/enums.
> 
> IOW:
> 
>     struct foo {
>        unsigned bar : 1,
> 		...
> 		  baz : 1;
>     };
> 
> Must be converted into:
> 
>     struct foo {
>     #define FOO_FLAG_BAR (1U <<  1)
>     ...
>     #define FOO_FLAG_BAZ (1U << 18)
>       unsigned flags;
>     }
> 
> so that you can use parseopt.  that's what I meant.
> 
> 
> This was done for the rev-list parsing stuff e.g.

e.g. what?

-- 
Jakub Narebski
Poland

Re: [RFC PATCH 0/2] Allow detached forms (--option arg) for git log options.

[Pierre Habouzit]

On Thu, Jul 29, 2010 at 11:16:42AM +0200, Jakub Narebski wrote:
> On Wed, 28 Jul 2010, Pierre Habouzit wrote:
> 
> > you cannot take the address of a bit portably in C, so you can't let
> > parseopt set/clear bits through bitfields (as in unsigned field : 1 in a
> > struct in C I mean).
> > 
> > So to use parseopt OPTION_BIT feature, you have to convert them to C
> > flags as in "unsigned flags" and explicit masks defines/enums.
> > 
> > IOW:
> > 
> >     struct foo {
> >        unsigned bar : 1,
> > 		...
> > 		  baz : 1;
> >     };
> > 
> > Must be converted into:
> > 
> >     struct foo {
> >     #define FOO_FLAG_BAR (1U <<  1)
> >     ...
> >     #define FOO_FLAG_BAZ (1U << 18)
> >       unsigned flags;
> >     }
> > 
> > so that you can use parseopt.  that's what I meant.
> > 
> > 
> > This was done for the rev-list parsing stuff e.g.
> 
> e.g. what?

err no, not rev-list, diff options: struct diff_options::flags and the
DIFF_OPT_* defines

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

Re: [RFC PATCH 0/2] Allow detached forms (--option arg) for git log options.

[Jonathan Nieder]

Pierre Habouzit wrote:

> you cannot take the address of a bit portably in C, so you can't let
> parseopt set/clear bits through bitfields (as in unsigned field : 1 in a
> struct in C I mean).

For the curious: I think this means doing something like
v1.5.4-rc0~186^2~1 (Make the diff_options bitfields be an unsigned
with explicit masks, 2007-11-10), which means instead of writing

	revs->topo_order = 1;

one would write something like

	REV_TRAV_SET(revs, TOPO_ORDER);

See [1] and [2].  Looks simple and reasonable.

While we are exploring ancient history, I find[3]:

	  I came up with the relocation thing because I feared
	that the msys port (and maybe other ?) that are about to
	use (or already do) threads would step on each other toes
	while recursing into a sub-array of options.

	  Johannes thinks that this never happens in our
	codebase, hence that my patches are an overkill.

	  The likely users of this feature are currently diff
	options (diff.c diff_opt_parse) and revisions
	(builtin-log.c setup_revisions).

	  Using Johannes patch, we will have to export a global
	struct diff_option (resp. struct rev_info) from diff.c
	(resp. revisions.c) and no function (or almost) would
	take struct diff_option (resp struct rev_info) as an
	argument because everyone would work on the global
	variable[0].

	  With my patches, we can work like we do now, with a
	more functional approach.

Is the relocation thing worth thinking about?  (Mind you, I was not
there, so I do not know what it is nor whether it was a dead end.)  If
so, is it documented anywhere?

The table-inclusion method[4] still appeals to me very much.  Well,
whatever seems to work best.

[1] http://thread.gmane.org/gmane.comp.version-control.git/63797/focus=63937
[2] http://thread.gmane.org/gmane.comp.version-control.git/83083/focus=83114
[3] http://thread.gmane.org/gmane.comp.version-control.git/63502/focus=63506
[4] http://thread.gmane.org/gmane.comp.version-control.git/63505/focus=63517