RFC patch, 2 partsAllow 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(-)

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

Heya,

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

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

Sverre Rabbelier <srabbelier@gmail.com> writes:

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

Miles Bader <miles@gnu.org> writes:

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

Sverre Rabbelier <srabbelier@gmail.com> writes:

Try this:

  git commit --message foo

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

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

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.

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

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.

On Tue, Jul 27, 2010 at 11:56, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

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.

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

Except if you read carefully ;-)

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

On Tue, Jul 27, 2010 at 13:26, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Ah, missed that. Sorry for the noise.

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

Heya,

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

TYVM.

Hi Matthieu,

Matthieu Moy wrote:

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

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

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.

Pierre Habouzit <madcoder@debian.org> writes:

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?

On Tue, Jul 27, 2010 at 08:10:35AM -0700, Jakub Narebski 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.

On Wed, 28 Jul 2010, Pierre Habouzit wrote:

e.g. what?

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

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

Pierre Habouzit wrote:

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