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

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

Signed-off-by: Matthieu Moy <redacted>
---
 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

Signed-off-by: Matthieu Moy <redacted>
---
 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

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

Heya,

On Mon, Jul 26, 2010 at 13:14, Matthieu Moy [off-list ref] wrote:

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

TYVM.

-- 
Cheers,

Sverre Rabbelier

Heya,

On Mon, Jul 26, 2010 at 13:14, Matthieu Moy [off-list ref] 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

On Mon, Jul 26, 2010 at 18:14, Matthieu Moy [off-list ref] 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.

On Mon, Jul 26, 2010 at 02:31:09PM -0500, Jonathan Nieder wrote:

Hi Matthieu,

Matthieu Moy wrote:

quoted

                   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

Pierre Habouzit [off-list ref] writes:

On Mon, Jul 26, 2010 at 02:31:09PM -0500, Jonathan Nieder wrote:

quoted

Hi Matthieu,

Matthieu Moy wrote:

quoted

                   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

On Tue, Jul 27, 2010 at 08:10:35AM -0700, Jakub Narebski wrote:

Pierre Habouzit [off-list ref] writes:

quoted

On Mon, Jul 26, 2010 at 02:31:09PM -0500, Jonathan Nieder wrote:

quoted

Hi Matthieu,

Matthieu Moy wrote:

quoted

                   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

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

On Thu, Jul 29, 2010 at 11:16:42AM +0200, Jakub Narebski wrote:

On Wed, 28 Jul 2010, Pierre Habouzit wrote:

quoted

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

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