Git development
 help / color / mirror / Atom feed
* Re: [PATCH/RFC] gitopt - command-line parsing enhancements
From: Eric Wong @ 2006-05-09 19:48 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vzmhr7fys.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano <junkio@cox.net> wrote:
> Eric Wong <normalperson@yhbt.net> writes:
> 
> > Here's my take at a new command-line option parser to reduce wear on my
> > fingers.  It handles both long and short options, permuting, automatic
> > abbreviations, required arguments, optional arguments, and bundling.
> 
> Taken a superficial look at it.
> 
> Sounds nice, might be a tad too ambitious though.  Looks
> intrusive at places.

I wasn't overly happy with the addition of global variables to existing
files and the way they're set (setup_revisions).  But at least they're
static.  Of course, I'm not yet certain that I haven't introduced new
bugs.  All the tests pass, at least...

> And scary, especially the "eat" macros are very scary.

They look weird at first, but I think they help readability and
maintainability once you get used to them.  They let you focus on the
important part of the function while hiding the boring parts from you.
Quite elegant, imho.

-- 
Eric Wong

^ permalink raw reply

* Re: [PATCH 1/6] gitopt: a new command-line option parser for git
From: Eric Wong @ 2006-05-09 19:39 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Timo Hirvonen, git
In-Reply-To: <7v8xpb73sq.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano <junkio@cox.net> wrote:
> Timo Hirvonen <tihirvon@gmail.com> writes:
> 
> > Eric Wong <normalperson@yhbt.net> wrote:
> >
> >>  * unbundling of short options: -uC20n20z => -u -C20 -n20 -z
> >
> > Does anyone ever use this?  I think this makes sense only for flags that
> > don't have parameters but that would create an ugly special case. Is it
> > too hard to type "-u -C=20 -n=20 -z"?
> 
> I can already hear in my head that people would start talking
> about "git understands insane abbeviations of options".  It
> might be unambiguous, but that does not change it is a bit on
> the insane side.  People would probably expect -nuz can be split
> into -n -u -z, and the current handcrafted mess (although it is
> more obvious and easy to work with when reading and maintaining
> the existing code) is not abbreviation friendly, which we would
> want to do something about.  But I think squashing options with
> parameters together is going a bit too far.

I think numeric parameters are unambiguous when bundled.
I'm used to things like `diff -ru10p` working, *shrug*

Non-numeric parameters can only be used if the option is at the end of the
bundled string:

git commit -sam'this is my commit message'  would work

git commit -m'say hello' would also work

but git commit -mas'this is my commit message' would not work as intended
(where user wanted -a -s, too)

> > --with-r => --patch-with-raw works great
> 
> I personally think this also is too much.

There are (currently) two types of abbreviations, one is the prefix one used
commonly in shell scripts:  -e|--e|--ed|--edi|--edit.  I think this should
always be supported as most of our shell scripts already do.

The other one tokenizes on '-' first and looks for a prefix match after
each '-'.  I'd like to make that at least optional:

diff --git a/gitopt.c b/gitopt.c
index 056e163..9ca6025 100644
--- a/gitopt.c
+++ b/gitopt.c
gitopt.c
@@ -427,7 +427,7 @@ static void fallback_long(const struct o
        }
 
        /* ok, try harder, based on tokenization on '-' */
-       if (found < 0) {
+       if (found < 0 && getenv("GIT_ABBREV_HARDER")) {
                for (i = 0; ost[i].l || ost[i].s; i++) {
                        s = &(ost[i]);
                        if (s->l && opt_token_match(s,cur)) {

-- 
Eric Wong

^ permalink raw reply

* Re: Implementing branch attributes in git config
From: Linus Torvalds @ 2006-05-09 19:24 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin, git
In-Reply-To: <7vzmhr3wje.fsf@assigned-by-dhcp.cox.net>



On Tue, 9 May 2006, Junio C Hamano wrote:
> 
> If we are shooting for "let's not do this again", I do not think
> (4) without some quoting convention is good enough.  Today, we
> are talking about branch names so we could give them artificial
> limits, which could be weaker than what we already have on the
> branch names, but we would later regret that, when we start
> wanting to have other names in the configuration (e.g. people's
> names).

Here's my suggestion as a patch.

NOTE! This patch could be applied right now, and to all the branches (to
make 1.x, 1.2.x and 1.3.x all support the _syntax_). Even if nothing 
actually uses it.

It just makes the syntax be

	[section<space>+"<randomstring>"]

where the only rule for "randomstring" is that it can't contain a newline, 
and if you really want to insert a double-quote, you do it with \".

It turns that into the section name "secion.randomstring".

So you could use this for things like

	[email "torvalds@osdl.org"]
		name = Linus Torvalds

if you wanted to do the "email->name" conversion as part of the config 
file format (I'm not claiming that is sensible, I'm just giving it as an 
insane example). That would show up as the association

	email.torvalds@osdl.org.name -> Linus Torvalds

which is easy to parse (the "." in the email _looks_ ambiguous, but it 
isn't: you know that there will always be a single key-name, so you find 
the key name with "strrchr(name, '.')" and things are entirely 
unambiguous).

Now, it doesn't do any repo-config stuff, since the whole point of this is 
to make this a legal syntax, not actually start _using_ it yet. If people 
think this is an acceptable syntax, then pls apply to everything, and 
worry about usage later.

		Linus

---
diff --git a/config.c b/config.c
index 0f518c9..f3b74e0 100644
--- a/config.c
+++ b/config.c
@@ -134,6 +134,41 @@ static int get_value(config_fn_t fn, cha
 	return fn(name, value);
 }
 
+static int get_extended_base_var(char *name, int baselen, int c)
+{
+	do {
+		if (c == '\n')
+			return -1;
+		c = get_next_char();
+	} while (isspace(c));
+
+	/* We require the format to be '[base "extension"]' */
+	if (c != '"')
+		return -1;
+	name[baselen++] = '.';
+
+	for (;;) {
+		int c = get_next_char();
+		if (c == '\n')
+			return -1;
+		if (c == '"')
+			break;
+		if (c == '\\') {
+			c = get_next_char();
+			if (c == '\n')
+				return -1;
+		}
+		name[baselen++] = c;
+		if (baselen > MAXNAME / 2)
+			return -1;
+	}
+
+	/* Final ']' */
+	if (get_next_char() != ']')
+		return -1;
+	return baselen;
+}
+
 static int get_base_var(char *name)
 {
 	int baselen = 0;
@@ -144,6 +179,8 @@ static int get_base_var(char *name)
 			return -1;
 		if (c == ']')
 			return baselen;
+		if (isspace(c))
+			return get_extended_base_var(name, baselen, c);
 		if (!isalnum(c) && c != '.')
 			return -1;
 		if (baselen > MAXNAME / 2)

^ permalink raw reply related

* Re: [PATCH 1/6] gitopt: a new command-line option parser for git
From: Eric Wong @ 2006-05-09 19:18 UTC (permalink / raw)
  To: Timo Hirvonen; +Cc: git
In-Reply-To: <20060509120809.4d9494b9.tihirvon@gmail.com>

Timo Hirvonen <tihirvon@gmail.com> wrote:
> Eric Wong <normalperson@yhbt.net> wrote:
> 
> >  * unbundling of short options: -uC20n20z => -u -C20 -n20 -z
> 
> Does anyone ever use this?  I think this makes sense only for flags that
> don't have parameters but that would create an ugly special case. Is it
> too hard to type "-u -C=20 -n=20 -z"?

It is more for me.  Many programs that I use already accept bundled
switches, and '=' and '-' are relatively far away and requires me
to stretch my hand uncomfortably (I have very small hands, and have
limited mobility in several fingers).

> >  * optional argument handling (-C<num>, -M<num>)
> >    -C <num> (with a space between them) has not changed,
> >    however, <num> can be a sha1, or a path
> 
> IMO optional arguments are usually bad idea.
> 
>     -C 2 (is "2" argument?)
>     -C2  (-C=2 or -C -2?)
> 
> Better to make it obvious there's an argument
> 
>     -C=2
> 
> or not support optional arguments at all and "-C 2" becomes unambiguous.

git has always supported optional argument handling like this.

I'm striving for backwards compatibility with existing usage.  That
means as a diff option, -C alone works, as does -C20.  I've made -C 20
_not_ work because it breaks existing usage (where 20 could be a
filename, or a tree-ish).  -C=20 would mean something
else, since I wanted to make pickaxe work exactly as it did before:
-S=var would search for '=var', not 'var'.

-- 
Eric Wong

^ permalink raw reply

* Re: Implementing branch attributes in git config
From: Junio C Hamano @ 2006-05-09 18:03 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git
In-Reply-To: <Pine.LNX.4.63.0605091321350.7652@wbgn013.biozentrum.uni-wuerzburg.de>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Okay, to summarize what people proposed (and that I remember):
>
> 1) [branch."AnY+String"]
>
> 2) multiple [branch]
>
> 3) magic <space>+<quoted>
>
> 4) [branch.just/allow-slashes/and-dashes]
>
> 5) the " for " notation

Thanks for a nice summary.

> Now, for the ease of use:
>
> (1), (3) and (4) are in the same league of easiness (except maybe that you 
> have to keep in mind to extra-quote in shell scripts with (1) and (3)), 
> (2) is especially good for people with a database mindset, and (5) is 
> annoying as hell.

One thing you might want to consider is variable types and
default values (eh, that makes two).

When Linus first introduced the config mechanism, he made it so
that a loosely coupled set of programs can take the "Why should
I care about other programs configuration" attitude, and
actively encouraged to do so by allowing custom config parsers
inherit from the default parser, like the way git_diff_config()
falls back on git_default_config() when it does not recognize
the variable.

It is quite a good design for most uses, except that it made it
inconvenient to implement things like "git-repo-config -l" and
"git-var -l".  The point of this design _is_ that they cannot
know what the set of possible variables, their types and the
default values when missing are, so by design the script that
used "git-var -l | grep" to read the configuration needed to
know that a boolean can be denoted by existence, value set to
zero or non-zero integer, or string "true"/"false" (i.e.
"filemode", "filemode=1", and "filemode = true" mean the same
thing; BTW I think we would probably want to add "yes"/"no"
here).

Later it was made easier to use by Pasky with "repo-config --type"
option.  The caller supplies the name of the variable and the
type and repo-config gets the value -- the caller knows what it
wants to use, so having it to know what type of things it is
interested in is not so bad, so the type problem was practically
solved.  But it still feels somewhat hacky.

With (2) and (5), we have a bound set of "se.ct.ion.variable";
we could enumerate the variables we care about, define what they
mean and what their types and default values are (we need to do
that for Documentation/config.txt anyway).  With many parts of
the standalone git plumbing programs migrating into builtins, I
think it is not a bad idea to have a central table of all the
configuration variables that the core knows about.  Porcelains
and scripts could define customized tables that describe the
sections/variables they also want to see and act on in the
configuration file, and call git-repo-config with that table as
an optional parameter.

> Now, for the ease of implementation:
>
> (1) and (3) are in the same league, they have to change the way the config 
> is parsed as well as make downcasing conditional in repo-config. (2) is 
> obviously hardest of all. (4) is very easy (one line in config.c), and (5) 
> easiest (nothing to do).
>
> Now, for the versatility, i.e. what you can express with the syntax:
>...
> Obviously, I deem (4) the best solution ATM, because it has all the 
> expressability needed, while being the simplest.

If we are shooting for "let's not do this again", I do not think
(4) without some quoting convention is good enough.  Today, we
are talking about branch names so we could give them artificial
limits, which could be weaker than what we already have on the
branch names, but we would later regret that, when we start
wanting to have other names in the configuration (e.g. people's
names).

^ permalink raw reply

* Re: Implementing branch attributes in git config
From: Linus Torvalds @ 2006-05-09 15:29 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Jakub Narebski, git
In-Reply-To: <Pine.LNX.4.63.0605091321350.7652@wbgn013.biozentrum.uni-wuerzburg.de>



On Tue, 9 May 2006, Johannes Schindelin wrote:
> 
> Okay, to summarize what people proposed (and that I remember):
> 
> 1) [branch."AnY+String"]

If we really change the syntax, I would oppose the ".". I realize I may 
have used it myself, and I think it would be good _internally_, but I 
think the syntax would be

	[branch "Any+String"]

which looks a lot more readable. Ie just a space (or, perhaps, "any 
combination of spaces and tabs").

> 4) [branch.just/allow-slashes/and-dashes]

Same here. If we break old parsers anyway, the "." has no redeeming 
value. You'd use it when _scripting_ stuff, but not anywhere else.

So in both cases, the above would turn into the _variable_ called 
"branch.Any+String/and-dashes.<key>" internally, and things that used "git 
repo-config" would say it that way, but the config file format should be 
human-readable.

We already do that human-readability thing. We say

	[core]
		name = Linus  Torvalds
		email = random

but we turn it internally into

	core.name  ->	"Linus Torvalds"
	core.email ->	"random"

and that's also the format you use for "git repo-config". Similarly, 
having

	[branch "master"]
		remote = git://....

in the config file should - for exactly the same reasons - be turned 
_internally_ into the association

	branch.master.remote    ->    git://...

and for exactly the same reason you'd just use

	git repo-config set branch.master.remote "git://..."

on the command line.

IOW, we _already_ do not match the internal and command line with the 
actal human-readable syntax.

			Linus

^ permalink raw reply

* Re: (patch) calloc->xaclloc in read-cache.c
From: Junio C Hamano @ 2006-05-09 13:26 UTC (permalink / raw)
  To: iler.ml; +Cc: git
In-Reply-To: <0IZ000KI11YCKL10@mxout4.netvision.net.il>

iler.ml@gmail.com writes:

> How about this.

Looks good, thanks.  If you needed some MUA trick that other
people might benefit from please feel free to send a patch to
Documentation/SubmittingPatches "MUA specific hints" section.

Except "Subject: [PATCH] blah", commit log message (in this case
what you would have on the Subject line is to the point and you
would not need any extra log message), "Signed-off-by: whom",
and perhaps "---\n".  Material that you do not want to have in
the final commit message, like "How about this" and diffstat if
you have one, would come after the "---\n" line.

Your message formatted in the preferred way becomes like this:

	Subject: [PATCH] read-cache.c: use xcalloc() not calloc()

	Elsewhere we use xcalloc(); we should consistently do so.

        Signed-off-by: Yakov Lerner <iler.ml@gmail.com>
	---

        * How about this?

         read-cache.c |    2 +-
         1 files changed, 1 insertions(+), 1 deletions(-)

        --- read-cache.c.000	2006-05-09 15:27:56.000000000 +0000
        +++ read-cache.c	2006-05-09 15:28:10.000000000 +0000
        @@ -552,7 +552,7 @@

                active_nr = ntohl(hdr->hdr_entries);
	...

^ permalink raw reply

* Re: (patch) calloc->xaclloc in read-cache.c
From: iler.ml @ 2006-05-09 16:14 UTC (permalink / raw)
  To: git

How about this.

--- read-cache.c.000	2006-05-09 15:27:56.000000000 +0000
+++ read-cache.c	2006-05-09 15:28:10.000000000 +0000
@@ -552,7 +552,7 @@
 
 	active_nr = ntohl(hdr->hdr_entries);
 	active_alloc = alloc_nr(active_nr);
-	active_cache = calloc(active_alloc, sizeof(struct cache_entry *));
+	active_cache = xcalloc(active_alloc, sizeof(struct cache_entry *));
 
 	offset = sizeof(*hdr);
 	for (i = 0; i < active_nr; i++) {

^ permalink raw reply

* Re: Unresolved issues #2
From: Nicolas Pitre @ 2006-05-09 13:09 UTC (permalink / raw)
  To: David Woodhouse; +Cc: Junio C Hamano, git
In-Reply-To: <1147174809.2794.12.camel@pmac.infradead.org>

On Tue, 9 May 2006, David Woodhouse wrote:

> I'm not sure about that. The payload is patches, isn't it? That's just
> text, too -- we aren't going to deal with diffs of binary content very
> well _anyway_, are we?

Yes we do.  GIT now has its own email friendly binary patch format.


Nicolas

^ permalink raw reply

* Re: [PATCH 1/6] gitopt: a new command-line option parser for git
From: Junio C Hamano @ 2006-05-09 12:58 UTC (permalink / raw)
  To: Timo Hirvonen, Eric Wong; +Cc: git
In-Reply-To: <20060509120809.4d9494b9.tihirvon@gmail.com>

Timo Hirvonen <tihirvon@gmail.com> writes:

> Eric Wong <normalperson@yhbt.net> wrote:
>
>>  * unbundling of short options: -uC20n20z => -u -C20 -n20 -z
>
> Does anyone ever use this?  I think this makes sense only for flags that
> don't have parameters but that would create an ugly special case. Is it
> too hard to type "-u -C=20 -n=20 -z"?

I can already hear in my head that people would start talking
about "git understands insane abbeviations of options".  It
might be unambiguous, but that does not change it is a bit on
the insane side.  People would probably expect -nuz can be split
into -n -u -z, and the current handcrafted mess (although it is
more obvious and easy to work with when reading and maintaining
the existing code) is not abbreviation friendly, which we would
want to do something about.  But I think squashing options with
parameters together is going a bit too far.

> --with-r => --patch-with-raw works great

I personally think this also is too much.

^ permalink raw reply

* Re: Unresolved issues #2
From: Bertrand Jacquin @ 2006-05-09 11:53 UTC (permalink / raw)
  To: David Woodhouse; +Cc: Junio C Hamano, git
In-Reply-To: <1147174809.2794.12.camel@pmac.infradead.org>

On 5/9/06, David Woodhouse <dwmw2@infradead.org> wrote:
> On Thu, 2006-05-04 at 01:15 -0700, Junio C Hamano wrote:
> >
> > * Message-ID:
> > <4fb292fa0604290630r19edd7ejf88642e33b350d1d@mail.gmail.com>
>
> >   David Woodhouse did a patch to allow specifying charset on the
> >   command line (and default to UTF-8) which is a move in the
> >   right direction, but Bertrand's system seems to have trouble
> >   with it.
>
> I thought Bertrand then confirmed that he was having trouble _before_
> applying my patch, too? His response when I asked it it appears without
> my patch was "[it] appear without in 1.3.1 and I can't seed mail with
> too. Also, 1.2.4 work fine here (without patch)."

Ok, to make short :
git-send-email 1.2.4 :
No EOF error on my smtp server.
git-send-email 1.3.1 :
EOF error on my smtp server.

I upgraded to 1.3.1 when I received patch from you, don't test 1.3.1
and then applied your patch, you test. And so test failed.

--
Beber
#e.fr@freenode

^ permalink raw reply

* Re: Unresolved issues #2
From: David Woodhouse @ 2006-05-09 11:40 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7v4q065hq0.fsf@assigned-by-dhcp.cox.net>

On Thu, 2006-05-04 at 01:15 -0700, Junio C Hamano wrote:
> 
> * Message-ID:
> <4fb292fa0604290630r19edd7ejf88642e33b350d1d@mail.gmail.com>
>   Content-type charset for send-email (Bertrand Jacquin)
> 
>   The output from format-patch by default is unmarked, which
>   means the commit message part is UTF-8 (by strong convention),
>   and the contents of the diff is whatever the contents of the
>   file is encoded in.

Email without a Content-Type: header is supposed to be ASCII. If it
contains 8-bit characters, it's invalid. It'll be interpreted by
different systems in different ways -- not necessarily as UTF-8. Some
may even just reject it, on grounds of RFC non-compliance.

>   David Woodhouse did a patch to allow specifying charset on the
>   command line (and default to UTF-8) which is a move in the
>   right direction, but Bertrand's system seems to have trouble
>   with it.

I thought Bertrand then confirmed that he was having trouble _before_
applying my patch, too? His response when I asked it it appears without
my patch was "[it] appear without in 1.3.1 and I can't seed mail with
too. Also, 1.2.4 work fine here (without patch)."

>   I think if we were to do this we probably need to teach
>   format-patch to optionally do multi-part.  We may not
>   necessarily want to mark the payload to be in the same
>   encoding as the commit message (not that git-apply cares -- to
>   it, the payload is just 8-bit unencoded text, but we would
>   want to protect it from getting mangled by e-mail transport). 

I'm not sure about that. The payload is patches, isn't it? That's just
text, too -- we aren't going to deal with diffs of binary content very
well _anyway_, are we?

Obviously, there's nothing to stop people from storing binary blobs in
GIT, but unless you want to start sending actual _blobs_ as attachments
instead of sending patches, I think there's no need to play with MIME
multipart stuff.

I've no particular objection to it, but it's a separate issue to
Bertanrd's. That's a bug-fix, while multipart is an RFE without much
point, IMO.

-- 
dwmw2

^ permalink raw reply

* Re: Implementing branch attributes in git config
From: Johannes Schindelin @ 2006-05-09 11:34 UTC (permalink / raw)
  To: Martin Waitz; +Cc: Linus Torvalds, sean, junkio, git
In-Reply-To: <20060509112641.GB3228@admingilde.org>

Hi,

On Tue, 9 May 2006, Martin Waitz wrote:

> So why is everybody trying to munch all branch related data into
> one .ini style config file?
> 
> why not simply use the mechanisms we use elsewhere and build something
> like our remotes or the new HEAD file?

Because it is good to have one consistent tool to query/update what makes 
up the configuration. Of course, we really could go about it like M$ who 
invent a hundred an twenty three ways to do the same thing, all with their 
own set of bugs.

I admit that repo-config has not been as stable as it could have been. 
That was my fault, certainly. But with the help of the list, it has become 
more stable.

Now, if we decide upon a totally different config format, okay, that's 
what it takes. But please let's not have several different formats 
*again*.

Ciao,
Dscho

^ permalink raw reply

* Re: Implementing branch attributes in git config
From: Martin Waitz @ 2006-05-09 11:26 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: sean, junkio, git
In-Reply-To: <Pine.LNX.4.64.0605082007100.3718@g5.osdl.org>

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

hoi :)

On Mon, May 08, 2006 at 08:08:41PM -0700, Linus Torvalds wrote:
> > What's the advantage of section quotation marks over just allowing these
> > characters in regular section names?  To be specific, what is wrong with:
> > 
> >    [jc/show-branch-dense]
> 
> This would _suck_
> 
> What if you have a branch called "core"? Not all all unlikely. 
> 
> Think about what a section like
> 
> 	[core]
> 
> really means.
> 
> Plus I really want to not be case sensitive by default. Case sensitivity 
> really is _not_ normal for this kind of config file syntax.

So why is everybody trying to munch all branch related data into
one .ini style config file?

why not simply use the mechanisms we use elsewhere and build something
like our remotes or the new HEAD file?

-- 
Martin Waitz

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

^ permalink raw reply

* Re: Implementing branch attributes in git config
From: Johannes Schindelin @ 2006-05-09 11:21 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: git
In-Reply-To: <e3p5om$djs$1@sea.gmane.org>

Hi,

On Tue, 9 May 2006, Jakub Narebski wrote:

> Linus Torvalds wrote:
> 
> > I would suggest a much more readable format:
> > 
> > [core]
> > ...
> > 
> > [branch "core"]
> > ...
> > 
> > [remote "core"]
> > ...
> > 
> > and yes, enforce the <space>+<quoted name> format. We'd turn it internally
> > into some random internal string format (probably replacing the space with
> > a dot, and removing the quotes, so it would become "remote.core.<key>").
> 
> I'm all for it. Nice compromise of [branch."miXedCaps"] and ["miXedCaps"],
> human readable end editable, and easy parsable.

Okay, to summarize what people proposed (and that I remember):

1) [branch."AnY+String"]

2) multiple [branch]

3) magic <space>+<quoted>

4) [branch.just/allow-slashes/and-dashes]

5) the " for " notation

Of all these, only (5) is backwards compatible, but it is also the only 
one where you have to type the branch name over and over again.

However, the old gits do not really know what to do with the [branch] 
section anyway, so you could consider (2) (and (4), if you do not have 
branch names with slashes and/or dashes) backwards-compatible, because git 
will continue to work -- ignoring the funny entries.

(1) and (3) definitely would make an old git choke.

Now, for the ease of use:

(1), (3) and (4) are in the same league of easiness (except maybe that you 
have to keep in mind to extra-quote in shell scripts with (1) and (3)), 
(2) is especially good for people with a database mindset, and (5) is 
annoying as hell.

Now, for the ease of implementation:

(1) and (3) are in the same league, they have to change the way the config 
is parsed as well as make downcasing conditional in repo-config. (2) is 
obviously hardest of all. (4) is very easy (one line in config.c), and (5) 
easiest (nothing to do).

Now, for the versatility, i.e. what you can express with the syntax:

The same for all (except for (4) which has very weak restrictions on the 
branch name).

Oh, I completely forgot about another proposal: (6) subkeys (something 
like "url[branchname] = blablabla"). It has about the same effects as (1) 
or (3).

Another thing: I completely ignored the case sensitivity. Because it is 
irrelevant. Why? Because you do not have two branches which are only 
different by case-ness. It is confusing, and that's why. And you don't 
need to handle the case specially, because the comparison is done by 
downcasing anyway.

Obviously, I deem (4) the best solution ATM, because it has all the 
expressability needed, while being the simplest.

Ciao,
Dscho

^ permalink raw reply

* Re: [PATCH 1/6] gitopt: a new command-line option parser for git
From: Timo Hirvonen @ 2006-05-09  9:08 UTC (permalink / raw)
  To: normalperson; +Cc: git
In-Reply-To: <11471512103526-git-send-email-normalperson@yhbt.net>

Eric Wong <normalperson@yhbt.net> wrote:

>  * unbundling of short options: -uC20n20z => -u -C20 -n20 -z

Does anyone ever use this?  I think this makes sense only for flags that
don't have parameters but that would create an ugly special case. Is it
too hard to type "-u -C=20 -n=20 -z"?

>  * optional argument handling (-C<num>, -M<num>)
>    -C <num> (with a space between them) has not changed,
>    however, <num> can be a sha1, or a path

IMO optional arguments are usually bad idea.

    -C 2 (is "2" argument?)
    -C2  (-C=2 or -C -2?)

Better to make it obvious there's an argument

    -C=2

or not support optional arguments at all and "-C 2" becomes unambiguous.

-- 
http://onion.dynserv.net/~timo/

^ permalink raw reply

* Re: [PATCH/RFC] gitopt - command-line parsing enhancements
From: Junio C Hamano @ 2006-05-09  8:35 UTC (permalink / raw)
  To: Eric Wong; +Cc: git
In-Reply-To: <1147151209168-git-send-email-normalperson@yhbt.net>

Eric Wong <normalperson@yhbt.net> writes:

> Here's my take at a new command-line option parser to reduce wear on my
> fingers.  It handles both long and short options, permuting, automatic
> abbreviations, required arguments, optional arguments, and bundling.

Taken a superficial look at it.

Sounds nice, might be a tad too ambitious though.  Looks
intrusive at places.

And scary, especially the "eat" macros are very scary.

I have to think about it a bit.

^ permalink raw reply

* [PATCH] apply: fix infinite loop with multiple patches with --index
From: Eric Wong @ 2006-05-09  8:08 UTC (permalink / raw)
  To: junkio, git; +Cc: Eric Wong

When multiple patches are passed to git-apply, it will attempt
to open multiple file descriptors to an index, which means
multiple entries will be in the circular cache_file_list.

This change makes git-apply only open the index once and
write the index at exit.

Signed-off-by: Eric Wong <normalperson@yhbt.net>

---

 apply.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

1d0b15a178abaf7ba61085f0acc70521bd71a961
diff --git a/apply.c b/apply.c
index 269210a..ca36391 100644
--- a/apply.c
+++ b/apply.c
@@ -19,6 +19,7 @@ #include "blob.h"
 //
 static const char *prefix;
 static int prefix_length = -1;
+static int newfd = -1;
 
 static int p_value = 1;
 static int allow_binary_replacement = 0;
@@ -1873,7 +1874,6 @@ static int use_patch(struct patch *p)
 
 static int apply_patch(int fd, const char *filename)
 {
-	int newfd;
 	unsigned long offset, size;
 	char *buffer = read_patch_file(fd, &size);
 	struct patch *list = NULL, **listp = &list;
@@ -1904,12 +1904,11 @@ static int apply_patch(int fd, const cha
 		size -= nr;
 	}
 
-	newfd = -1;
 	if (whitespace_error && (new_whitespace == error_on_whitespace))
 		apply = 0;
 
 	write_index = check_index && apply;
-	if (write_index)
+	if (write_index && newfd < 0)
 		newfd = hold_index_file_for_update(&cache_file, get_index_file());
 	if (check_index) {
 		if (read_cache() < 0)
@@ -1922,12 +1921,6 @@ static int apply_patch(int fd, const cha
 	if (apply)
 		write_out_results(list, skipped_patch);
 
-	if (write_index) {
-		if (write_cache(newfd, active_cache, active_nr) ||
-		    commit_index_file(&cache_file))
-			die("Unable to write new cachefile");
-	}
-
 	if (show_index_info)
 		show_index_list(list);
 
@@ -2085,5 +2078,12 @@ int main(int argc, char **argv)
 				whitespace_error == 1 ? "" : "s",
 				whitespace_error == 1 ? "s" : "");
 	}
+
+	if (write_index) {
+		if (write_cache(newfd, active_cache, active_nr) ||
+		    commit_index_file(&cache_file))
+			die("Unable to write new cachefile");
+	}
+
 	return 0;
 }
-- 
1.3.2.g45f7-dirty

^ permalink raw reply related

* Re: git-feed-mail-list.sh
From: Junio C Hamano @ 2006-05-09  7:32 UTC (permalink / raw)
  To: Bertrand Jacquin; +Cc: git, Linus Torvalds
In-Reply-To: <Pine.LNX.4.64.0605081951390.3718@g5.osdl.org>

Linus Torvalds <torvalds@osdl.org> writes:

> But if you want to get it for any random merges, you can always just do
>
> 	git log -11 --pretty=oneline ^$commit^ $commit^@ |
> 		sed 's/[0-9a-f]* // ; 11 s/.*/\.\.\./' 
>
> which will show up to the ten first commits that were merged (and turn the 
> eleventh one, if it exists, into "..." - that's a pretty disgusting trick 
> to make it show when you left things out).
>
> That "^$commit^ $commit^@" part is important. It may look like some 
> deranged git smiley, but it does exactly what you want it to do: take all 
> the parents of the commit, but ignore any commit reachable from the first 
> one (the "mainline" of the person who did the commit).
>
> The ^@ syntax is obviously pretty new, so it requires a modern git.

It is indeed very quite new.  Merged into "master" branch at the
beginning of this month.

I often wish we had a straightforward way to tell when a given
feature went into the mainline, not just appeared on a topic
branch.  In this case, I said:

	$ git whatchanged -p -S'"^@"' master -- revision.c

to find ea4a19 commit (Apr 30 00:54:29 2006 -0700).  But that
was when the feature was first made on one of my topic branches,
which is not what I was looking for.

By looking at gitk, I can then tell 83262e (May 1 01:54:27)
merged it to "next", and 746437 (May 1 22:55:40) merged it to
"master".

In general this is an unsolvable question, because I can have a
topic branch forked off of the tip of "master", cook it for a
few days without advancing "master" at all, and merge it to
"master" after that.  But such a merge will be a fast-forward.

^ permalink raw reply

* Re: [PATCH 5/6] gitopt: convert setup_revisions(), and diff_opt_parse()
From: Eric Wong @ 2006-05-09  7:16 UTC (permalink / raw)
  To: git
In-Reply-To: <11471512123005-git-send-email-normalperson@yhbt.net>

Eric Wong <normalperson@yhbt.net> wrote:
> I've added --raw to diff_opt_parse() for consistency's sake:
> --patch-with-raw would otherwise be inconsistently abbreviated
> to --raw if it wasn't added.

I was looking for this patch, but then I realized it slipped in here.

I've added --unified=<num>/-u<num> context here so I don't have to go
through the awkwardness of setting GIT_DIFF_OPTS in the environment.

-- 
Eric Wong

^ permalink raw reply

* Re: git-feed-mail-list.sh
From: Junio C Hamano @ 2006-05-09  7:15 UTC (permalink / raw)
  To: Bertrand Jacquin; +Cc: git, David Woodhouse, Linus Torvalds
In-Reply-To: <Pine.LNX.4.64.0605081805290.3718@g5.osdl.org>

Linus Torvalds <torvalds@osdl.org> writes:

> On Tue, 9 May 2006, Bertrand Jacquin wrote:
>
>> On 5/9/06, Linus Torvalds <torvalds@osdl.org> wrote:
>> > 
>> > Ie you could probably more easily parse the data from something like
>> > 
>> >         git show -B --patch-with-stat --pretty=fuller $commit
>> 
>> Is there a way to track merge like that ? Documentation is not very
>> clear and near from empty.
>
> Sure.
>
> If you want to track merges and get their patches, add the "--cc" flag, 
> which tells git to use the "conflict combination patch" that shows any 
> visible conflicts.

Actually, show defaults to --cc so what's shown is good as is.

> That said, the diffstat for merges is usually just a lot of noise. It's 
> sometimes nice (you've merged from a topic branch), but if you have merged 
> from the mainline _into_ a topic branch, it's just annoying.

True.  We made --cc --patch-with-stat to do the combined patch
text with diffstat for first-parent-diff to make it most natural
for merging into upstream, not merging from upstream.

^ permalink raw reply

* [PATCH 6/6] commit: allow --pretty= args to be abbreviated
From: Eric Wong @ 2006-05-09  5:06 UTC (permalink / raw)
  To: git; +Cc: Eric Wong
In-Reply-To: <11471512123005-git-send-email-normalperson@yhbt.net>

Signed-off-by: Eric Wong <normalperson@yhbt.net>

---

 commit.c |   42 +++++++++++++++++++++++++++++-------------
 1 files changed, 29 insertions(+), 13 deletions(-)

0a3aed7c25eca808b29f2318d5e4c087e03b9bfb
diff --git a/commit.c b/commit.c
index a056b25..055064a 100644
--- a/commit.c
+++ b/commit.c
@@ -22,23 +22,39 @@ struct sort_node
 
 const char *commit_type = "commit";
 
+struct cmt_fmt_map {
+	const char *n;
+	enum cmit_fmt v;
+} cmt_fmts[] = {
+	{ "raw",	CMIT_FMT_RAW },
+	{ "medium",	CMIT_FMT_MEDIUM },
+	{ "short",	CMIT_FMT_SHORT },
+	{ "full",	CMIT_FMT_FULL },
+	{ "fuller",	CMIT_FMT_FULLER },
+	{ "oneline",	CMIT_FMT_ONELINE },
+};
+
 enum cmit_fmt get_commit_format(const char *arg)
 {
+	int i, found = -1;
 	if (!*arg)
 		return CMIT_FMT_DEFAULT;
-	if (!strcmp(arg, "raw"))
-		return CMIT_FMT_RAW;
-	if (!strcmp(arg, "medium"))
-		return CMIT_FMT_MEDIUM;
-	if (!strcmp(arg, "short"))
-		return CMIT_FMT_SHORT;
-	if (!strcmp(arg, "full"))
-		return CMIT_FMT_FULL;
-	if (!strcmp(arg, "fuller"))
-		return CMIT_FMT_FULLER;
-	if (!strcmp(arg, "oneline"))
-		return CMIT_FMT_ONELINE;
-	die("invalid --pretty format");
+	for (i = 0; i < ARRAY_SIZE(cmt_fmts); i++) {
+		if (!strcmp(arg, cmt_fmts[i].n))
+			return cmt_fmts[i].v;
+	}
+
+	/* look for abbreviations */
+	for (i = 0; i < ARRAY_SIZE(cmt_fmts); i++) {
+		if (strstr(cmt_fmts[i].n, arg)) {
+			if (found >= 0)
+				die("invalid --pretty format: %s", arg);
+			found = i;
+		}
+	}
+	if (found >= 0)
+		return cmt_fmts[found].v;
+	die("invalid --pretty format: %s", arg);
 }
 
 static struct commit *check_commit(struct object *obj,
-- 
1.3.2.g0a3ae

^ permalink raw reply related

* [PATCH 5/6] gitopt: convert setup_revisions(), and diff_opt_parse()
From: Eric Wong @ 2006-05-09  5:06 UTC (permalink / raw)
  To: git; +Cc: Eric Wong
In-Reply-To: <11471512121152-git-send-email-normalperson@yhbt.net>

I've added --raw to diff_opt_parse() for consistency's sake:
--patch-with-raw would otherwise be inconsistently abbreviated
to --raw if it wasn't added.

--with-r => --patch-with-raw works great

Signed-off-by: Eric Wong <normalperson@yhbt.net>

---

 builtin-diff.c |  109 ++++---------
 builtin-log.c  |    7 -
 commit.c       |   12 +
 diff-files.c   |   22 +--
 diff-index.c   |   19 +-
 diff-tree.c    |   21 +-
 diff.c         |  182 ++++++++++++---------
 diff.h         |    1 
 gitopt/diff.h  |   26 +++
 http-push.c    |    2 
 rev-list.c     |   34 ++--
 revision.c     |  484 +++++++++++++++++++++++++-------------------------------
 revision.h     |    6 +
 13 files changed, 437 insertions(+), 488 deletions(-)
 create mode 100644 gitopt/diff.h

28cbff1d1b543e8234acc6cada0fc889d4767a59
diff --git a/builtin-diff.c b/builtin-diff.c
index d3ac581..ff4a0ec 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -12,6 +12,7 @@ #include "diffcore.h"
 #include "revision.h"
 #include "log-tree.h"
 #include "builtin.h"
+#include "gitopt/diff.h"
 
 /* NEEDSWORK: struct object has place for name but we _do_
  * know mode when we extracted the blob out of a tree, which
@@ -29,22 +30,12 @@ static int builtin_diff_files(struct rev
 			      int argc, const char **argv)
 {
 	int silent = 0;
-	while (1 < argc) {
-		const char *arg = argv[1];
-		if (!strcmp(arg, "--base"))
-			revs->max_count = 1;
-		else if (!strcmp(arg, "--ours"))
-			revs->max_count = 2;
-		else if (!strcmp(arg, "--theirs"))
-			revs->max_count = 3;
-		else if (!strcmp(arg, "-q"))
-			silent = 1;
-		else if (!strcmp(arg, "--raw"))
-			revs->diffopt.output_format = DIFF_FORMAT_RAW;
-		else
-			usage(builtin_diff_usage);
-		argv++; argc--;
-	}
+	struct exec_args *a;
+
+	g_rev = revs;
+	a = new_exec_args(argc);
+	if (gitopt_parse_ost_split(a, a, diff_files_ost, argc, argv))
+		usage(builtin_diff_usage);
 	/*
 	 * Make sure there are NO revision (i.e. pending object) parameter,
 	 * specified rev.max_count is reasonable (0 <= n <= 3), and
@@ -107,14 +98,8 @@ static int builtin_diff_b_f(struct rev_i
 	/* Blob vs file in the working tree*/
 	struct stat st;
 
-	while (1 < argc) {
-		const char *arg = argv[1];
-		if (!strcmp(arg, "--raw"))
-			revs->diffopt.output_format = DIFF_FORMAT_RAW;
-		else
-			usage(builtin_diff_usage);
-		argv++; argc--;
-	}
+	if (argc)
+		usage(builtin_diff_usage);
 	if (lstat(path, &st))
 		die("'%s': %s", path, strerror(errno));
 	if (!(S_ISREG(st.st_mode) || S_ISLNK(st.st_mode)))
@@ -135,14 +120,9 @@ static int builtin_diff_blobs(struct rev
 	/* Blobs */
 	unsigned mode = canon_mode(S_IFREG | 0644);
 
-	while (1 < argc) {
-		const char *arg = argv[1];
-		if (!strcmp(arg, "--raw"))
-			revs->diffopt.output_format = DIFF_FORMAT_RAW;
-		else
-			usage(builtin_diff_usage);
-		argv++; argc--;
-	}
+	if (argc)
+		usage(builtin_diff_usage);
+
 	stuff_change(&revs->diffopt,
 		     mode, mode,
 		     blob[0].sha1, blob[1].sha1,
@@ -155,17 +135,13 @@ static int builtin_diff_blobs(struct rev
 static int builtin_diff_index(struct rev_info *revs,
 			      int argc, const char **argv)
 {
-	int cached = 0;
-	while (1 < argc) {
-		const char *arg = argv[1];
-		if (!strcmp(arg, "--cached"))
-			cached = 1;
-		else if (!strcmp(arg, "--raw"))
-			revs->diffopt.output_format = DIFF_FORMAT_RAW;
-		else
-			usage(builtin_diff_usage);
-		argv++; argc--;
-	}
+	struct exec_args *a;
+
+	g_rev = revs;
+	a = new_exec_args(argc);
+	if (gitopt_parse_ost_split(a, a, diff_index_ost, argc, argv))
+		usage(builtin_diff_usage);
+
 	/*
 	 * Make sure there is one revision (i.e. pending object),
 	 * and there is no revision filtering parameters.
@@ -183,14 +159,6 @@ static int builtin_diff_tree(struct rev_
 {
 	const unsigned char *(sha1[2]);
 	int swap = 1;
-	while (1 < argc) {
-		const char *arg = argv[1];
-		if (!strcmp(arg, "--raw"))
-			revs->diffopt.output_format = DIFF_FORMAT_RAW;
-		else
-			usage(builtin_diff_usage);
-		argv++; argc--;
-	}
 
 	/* We saw two trees, ent[0] and ent[1].
 	 * unless ent[0] is unintesting, they are swapped
@@ -212,14 +180,6 @@ static int builtin_diff_combined(struct 
 	const unsigned char (*parent)[20];
 	int i;
 
-	while (1 < argc) {
-		const char *arg = argv[1];
-		if (!strcmp(arg, "--raw"))
-			revs->diffopt.output_format = DIFF_FORMAT_RAW;
-		else
-			usage(builtin_diff_usage);
-		argv++; argc--;
-	}
 	if (!revs->dense_combined_merges && !revs->combine_merges)
 		revs->dense_combined_merges = revs->combine_merges = 1;
 	parent = xmalloc(ents * sizeof(*parent));
@@ -250,6 +210,7 @@ int cmd_diff(int argc, const char **argv
 	int ents = 0, blobs = 0, paths = 0;
 	const char *path = NULL;
 	struct blobinfo blob[2];
+	struct exec_args *b;
 
 	/*
 	 * We could get N tree-ish in the rev.pending_objects list.
@@ -274,23 +235,14 @@ int cmd_diff(int argc, const char **argv
 	git_config(git_diff_config);
 	init_revisions(&rev);
 	rev.diffopt.output_format = DIFF_FORMAT_PATCH;
+	cached = 0;
 
-	argc = setup_revisions(argc, argv, &rev, NULL);
+	b = setup_revisions(argc, argv, &rev, NULL, diff_index_ost);
 	/* Do we have --cached and not have a pending object, then
 	 * default to HEAD by hand.  Eek.
 	 */
-	if (!rev.pending_objects) {
-		int i;
-		for (i = 1; i < argc; i++) {
-			const char *arg = argv[i];
-			if (!strcmp(arg, "--"))
-				break;
-			else if (!strcmp(arg, "--cached")) {
-				add_head(&rev);
-				break;
-			}
-		}
-	}
+	if (!rev.pending_objects && cached)
+		add_head(&rev);
 
 	for (list = rev.pending_objects; list; list = list->next) {
 		struct object *obj = list->item;
@@ -340,17 +292,18 @@ int cmd_diff(int argc, const char **argv
 	if (!ents) {
 		switch (blobs) {
 		case 0:
-			return builtin_diff_files(&rev, argc, argv);
+			return builtin_diff_files(&rev, b->argc, b->argv);
 			break;
 		case 1:
 			if (paths != 1)
 				usage(builtin_diff_usage);
-			return builtin_diff_b_f(&rev, argc, argv, blob, path);
+			return builtin_diff_b_f(&rev, b->argc, b->argv,
+						blob, path);
 			break;
 		case 2:
 			if (paths)
 				usage(builtin_diff_usage);
-			return builtin_diff_blobs(&rev, argc, argv, blob);
+			return builtin_diff_blobs(&rev, b->argc, b->argv, blob);
 			break;
 		default:
 			usage(builtin_diff_usage);
@@ -359,10 +312,10 @@ int cmd_diff(int argc, const char **argv
 	else if (blobs)
 		usage(builtin_diff_usage);
 	else if (ents == 1)
-		return builtin_diff_index(&rev, argc, argv);
+		return builtin_diff_index(&rev, b->argc, b->argv);
 	else if (ents == 2)
-		return builtin_diff_tree(&rev, argc, argv, ent);
+		return builtin_diff_tree(&rev, b->argc, b->argv, ent);
 	else
-		return builtin_diff_combined(&rev, argc, argv, ent, ents);
+		return builtin_diff_combined(&rev, b->argc, b->argv, ent, ents);
 	usage(builtin_diff_usage);
 }
diff --git a/builtin-log.c b/builtin-log.c
index 69f2911..b004bc5 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -14,14 +14,15 @@ static int cmd_log_wc(int argc, const ch
 		      struct rev_info *rev)
 {
 	struct commit *commit;
+	struct exec_args *b;
 
 	rev->abbrev = DEFAULT_ABBREV;
 	rev->commit_format = CMIT_FMT_DEFAULT;
 	rev->verbose_header = 1;
-	argc = setup_revisions(argc, argv, rev, "HEAD");
+	b = setup_revisions(argc, argv, rev, "HEAD", NULL);
 
-	if (argc > 1)
-		die("unrecognized argument: %s", argv[1]);
+	if (b->argc)
+		die("unrecognized argument: %s", b->argv[0]);
 
 	prepare_revision_walk(rev);
 	setup_pager();
diff --git a/commit.c b/commit.c
index 2717dd8..a056b25 100644
--- a/commit.c
+++ b/commit.c
@@ -26,17 +26,17 @@ enum cmit_fmt get_commit_format(const ch
 {
 	if (!*arg)
 		return CMIT_FMT_DEFAULT;
-	if (!strcmp(arg, "=raw"))
+	if (!strcmp(arg, "raw"))
 		return CMIT_FMT_RAW;
-	if (!strcmp(arg, "=medium"))
+	if (!strcmp(arg, "medium"))
 		return CMIT_FMT_MEDIUM;
-	if (!strcmp(arg, "=short"))
+	if (!strcmp(arg, "short"))
 		return CMIT_FMT_SHORT;
-	if (!strcmp(arg, "=full"))
+	if (!strcmp(arg, "full"))
 		return CMIT_FMT_FULL;
-	if (!strcmp(arg, "=fuller"))
+	if (!strcmp(arg, "fuller"))
 		return CMIT_FMT_FULLER;
-	if (!strcmp(arg, "=oneline"))
+	if (!strcmp(arg, "oneline"))
 		return CMIT_FMT_ONELINE;
 	die("invalid --pretty format");
 }
diff --git a/diff-files.c b/diff-files.c
index b9d193d..e5b69f4 100644
--- a/diff-files.c
+++ b/diff-files.c
@@ -7,6 +7,7 @@ #include "cache.h"
 #include "diff.h"
 #include "commit.h"
 #include "revision.h"
+#include "gitopt/diff.h"
 
 static const char diff_files_usage[] =
 "git-diff-files [-q] [-0/-1/2/3 |-c|--cc] [<common diff options>] [<path>...]"
@@ -15,26 +16,17 @@ COMMON_DIFF_OPTIONS_HELP;
 int main(int argc, const char **argv)
 {
 	struct rev_info rev;
-	int silent = 0;
+	struct exec_args *b;
 
+	silent = 0;
 	git_config(git_diff_config);
 	init_revisions(&rev);
 	rev.abbrev = 0;
+	g_rev = &rev;
 
-	argc = setup_revisions(argc, argv, &rev, NULL);
-	while (1 < argc && argv[1][0] == '-') {
-		if (!strcmp(argv[1], "--base"))
-			rev.max_count = 1;
-		else if (!strcmp(argv[1], "--ours"))
-			rev.max_count = 2;
-		else if (!strcmp(argv[1], "--theirs"))
-			rev.max_count = 3;
-		else if (!strcmp(argv[1], "-q"))
-			silent = 1;
-		else
-			usage(diff_files_usage);
-		argv++; argc--;
-	}
+	b = setup_revisions(argc, argv, &rev, NULL, diff_files_ost);
+	if (b->argc)
+		usage(diff_files_usage);
 	/*
 	 * Make sure there are NO revision (i.e. pending object) parameter,
 	 * rev.max_count is reasonable (0 <= n <= 3),
diff --git a/diff-index.c b/diff-index.c
index 8c9f601..92052dc 100644
--- a/diff-index.c
+++ b/diff-index.c
@@ -2,6 +2,8 @@ #include "cache.h"
 #include "diff.h"
 #include "commit.h"
 #include "revision.h"
+#include "gitopt.h"
+#include "gitopt/diff.h"
 
 static const char diff_cache_usage[] =
 "git-diff-index [-m] [--cached] "
@@ -11,22 +13,17 @@ COMMON_DIFF_OPTIONS_HELP;
 int main(int argc, const char **argv)
 {
 	struct rev_info rev;
-	int cached = 0;
-	int i;
+	struct exec_args *b;
+	cached = 0;
 
 	git_config(git_diff_config);
 	init_revisions(&rev);
 	rev.abbrev = 0;
 
-	argc = setup_revisions(argc, argv, &rev, NULL);
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
-			
-		if (!strcmp(arg, "--cached"))
-			cached = 1;
-		else
-			usage(diff_cache_usage);
-	}
+	b = setup_revisions(argc, argv, &rev, NULL, diff_index_ost);
+	if (b->argc)
+		usage(diff_cache_usage);
+
 	/*
 	 * Make sure there is one revision (i.e. pending object),
 	 * and there is no revision filtering parameters.
diff --git a/diff-tree.c b/diff-tree.c
index 7207867..240242c 100644
--- a/diff-tree.c
+++ b/diff-tree.c
@@ -2,6 +2,7 @@ #include "cache.h"
 #include "diff.h"
 #include "commit.h"
 #include "log-tree.h"
+#include "gitopt.h"
 
 static struct rev_info log_tree_opt;
 
@@ -58,6 +59,13 @@ static const char diff_tree_usage[] =
 "  --root        include the initial commit as diff against /dev/null\n"
 COMMON_DIFF_OPTIONS_HELP;
 
+static int read_stdin = 0;
+gitopt_eat(opt_stdin, read_stdin = 1;)
+static const struct opt_spec diff_tree_ost[] = {
+	{ "stdin",	0,	0,	0,	opt_stdin },
+	{ 0, 0 }
+};
+
 int main(int argc, const char **argv)
 {
 	int nr_sha1;
@@ -65,24 +73,17 @@ int main(int argc, const char **argv)
 	struct object *tree1, *tree2;
 	static struct rev_info *opt = &log_tree_opt;
 	struct object_list *list;
-	int read_stdin = 0;
+	struct exec_args *b;
 
 	git_config(git_diff_config);
 	nr_sha1 = 0;
 	init_revisions(opt);
 	opt->abbrev = 0;
 	opt->diff = 1;
-	argc = setup_revisions(argc, argv, opt, NULL);
-
-	while (--argc > 0) {
-		const char *arg = *++argv;
 
-		if (!strcmp(arg, "--stdin")) {
-			read_stdin = 1;
-			continue;
-		}
+	b = setup_revisions(argc, argv, opt, NULL, diff_tree_ost);
+	if (b->argc)
 		usage(diff_tree_usage);
-	}
 
 	/*
 	 * NOTE! "setup_revisions()" will have inserted the revisions
diff --git a/diff.c b/diff.c
index 5315270..3b56efc 100644
--- a/diff.c
+++ b/diff.c
@@ -9,6 +9,8 @@ #include "quote.h"
 #include "diff.h"
 #include "diffcore.h"
 #include "xdiff-interface.h"
+#include "gitopt.h"
+#include "gitopt/abbrev.h"
 
 static int use_size_cache;
 
@@ -407,7 +409,8 @@ static void builtin_diff(const char *nam
 			 struct diff_filespec *one,
 			 struct diff_filespec *two,
 			 const char *xfrm_msg,
-			 int complete_rewrite)
+			 int complete_rewrite,
+			 struct diff_options *o)
 {
 	mmfile_t mf1, mf2;
 	const char *lbl[2];
@@ -463,10 +466,9 @@ static void builtin_diff(const char *nam
 
 		ecbdata.label_path = lbl;
 		xpp.flags = XDF_NEED_MINIMAL;
-		xecfg.ctxlen = 3;
 		xecfg.flags = XDL_EMIT_FUNCNAMES;
 		if (!diffopts)
-			;
+			xecfg.ctxlen = o->ctxlen;
 		else if (!strncmp(diffopts, "--unified=", 10))
 			xecfg.ctxlen = strtoul(diffopts + 10, NULL, 10);
 		else if (!strncmp(diffopts, "-u", 2))
@@ -928,7 +930,8 @@ static void run_diff_cmd(const char *pgm
 			 struct diff_filespec *one,
 			 struct diff_filespec *two,
 			 const char *xfrm_msg,
-			 int complete_rewrite)
+			 int complete_rewrite,
+			 struct diff_options *o)
 {
 	if (pgm) {
 		run_external_diff(pgm, name, other, one, two, xfrm_msg,
@@ -937,7 +940,7 @@ static void run_diff_cmd(const char *pgm
 	}
 	if (one && two)
 		builtin_diff(name, other ? other : name,
-			     one, two, xfrm_msg, complete_rewrite);
+			     one, two, xfrm_msg, complete_rewrite, o);
 	else
 		printf("* Unmerged path %s\n", name);
 }
@@ -971,7 +974,7 @@ static void run_diff(struct diff_filepai
 
 	if (DIFF_PAIR_UNMERGED(p)) {
 		/* unmerged */
-		run_diff_cmd(pgm, p->one->path, NULL, NULL, NULL, NULL, 0);
+		run_diff_cmd(pgm, p->one->path, NULL, NULL, NULL, NULL, 0, o);
 		return;
 	}
 
@@ -1041,15 +1044,15 @@ static void run_diff(struct diff_filepai
 		 * needs to be split into deletion and creation.
 		 */
 		struct diff_filespec *null = alloc_filespec(two->path);
-		run_diff_cmd(NULL, name, other, one, null, xfrm_msg, 0);
+		run_diff_cmd(NULL, name, other, one, null, xfrm_msg, 0, o);
 		free(null);
 		null = alloc_filespec(one->path);
-		run_diff_cmd(NULL, name, other, null, two, xfrm_msg, 0);
+		run_diff_cmd(NULL, name, other, null, two, xfrm_msg, 0, o);
 		free(null);
 	}
 	else
 		run_diff_cmd(pgm, name, other, one, two, xfrm_msg,
-			     complete_rewrite);
+			     complete_rewrite, o);
 
 	free(name_munged);
 	free(other_munged);
@@ -1086,6 +1089,7 @@ void diff_setup(struct diff_options *opt
 	options->line_termination = '\n';
 	options->break_opt = -1;
 	options->rename_limit = -1;
+	options->ctxlen = 3;
 
 	options->change = diff_change;
 	options->add_remove = diff_addremove;
@@ -1126,76 +1130,100 @@ int diff_setup_done(struct diff_options 
 	return 0;
 }
 
+static struct diff_options *g_opt = NULL; /* gitopt needs globals :x */
+static int rv_diff_opt_parse = 0;
+
+gitopt_eat_opt_int(opt_unified,
+		g_opt->output_format = DIFF_FORMAT_PATCH;
+		if (ea->argc) g_opt->ctxlen = strtol(ea->argv[0], NULL, 10);)
+gitopt_eat(opt_p, g_opt->output_format = DIFF_FORMAT_PATCH;)
+gitopt_eat(opt_patch_with_raw,
+		g_opt->output_format = DIFF_FORMAT_PATCH;
+		g_opt->with_raw = 1;)
+gitopt_eat(opt_raw, g_opt->output_format = DIFF_FORMAT_RAW;)
+gitopt_eat(opt_stat,
+		g_opt->output_format = DIFF_FORMAT_DIFFSTAT;)
+gitopt_eat(opt_patch_with_stat,
+		g_opt->output_format = DIFF_FORMAT_PATCH;
+		g_opt->with_stat = 1;)
+gitopt_eat(opt_z, g_opt->line_termination = 0;)
+gitopt_eat_one_arg(opt_l, g_opt->rename_limit = strtoul(ea->argv[0],NULL,10);)
+gitopt_eat(opt_full_index, g_opt->full_index = 1;)
+gitopt_eat(opt_name_only, g_opt->output_format = DIFF_FORMAT_NAME;)
+gitopt_eat(opt_name_status,
+			g_opt->output_format = DIFF_FORMAT_NAME_STATUS;)
+gitopt_eat(opt_R, g_opt->reverse_diff = 1;)
+gitopt_eat_one_arg(opt_S, g_opt->pickaxe = ea->argv[0];)
+gitopt_eat(opt_s, g_opt->output_format = DIFF_FORMAT_NO_OUTPUT;)
+gitopt_eat_one_arg(opt_O, g_opt->orderfile = ea->argv[0];)
+gitopt_eat_one_arg(opt_diff_filter, g_opt->filter = ea->argv[0];)
+gitopt_eat(opt_pickaxe_all, g_opt->pickaxe_opts = DIFF_PICKAXE_ALL;)
+gitopt_eat(opt_pickaxe_regex, g_opt->pickaxe_opts = DIFF_PICKAXE_REGEX;)
+gitopt_eat_opt_int(opt_B,
+	if ((g_opt->break_opt = diff_scoreopt_parse(*argv)) == -1)
+		rv_diff_opt_parse = -1;)
+gitopt_eat_opt_int(opt_M,
+	if ((g_opt->rename_score = diff_scoreopt_parse(*argv)) == -1)
+		rv_diff_opt_parse = -1;
+	g_opt->detect_rename = DIFF_DETECT_RENAME;)
+gitopt_eat_opt_int(opt_C,
+	if ((g_opt->rename_score = diff_scoreopt_parse(*argv)) == -1)
+		rv_diff_opt_parse = -1;
+	g_opt->detect_rename = DIFF_DETECT_COPY;)
+gitopt_eat(opt_find_copies_harder, g_opt->find_copies_harder = 1;)
+gitopt_opt_abbrev(g_opt->abbrev)
+
+static const struct opt_spec diff_ost[] = {
+	{ 0,			'p',	0,	0,	opt_p },
+	{ "unified",		'u',	"%s",	ARG_OPTINT,	opt_unified },
+	{ "raw",		0,	0,	0,	opt_raw },
+	{ "patch-with-raw",	0,	0,	0,	opt_patch_with_raw },
+	{ "stat",		0,	0,	0,	opt_stat },
+	{ "patch-with-stat",	0,	0,	0,	opt_patch_with_stat },
+	{ 0,			'z',	0,	0,	opt_z },
+	{ 0,			'l',	"%s",	ARG_INT,opt_l },
+	{ "full-index",		0,	0,	0,	opt_full_index },
+	{ "name-only",		0,	0,	0,	opt_name_only },
+	{ "name-status",	0,	0,	0,	opt_name_status },
+	{ 0,			'R',	0,	0,	opt_R },
+	{ 0,			'S',	"%s",	ARG_ONE,opt_S },
+	{ 0,			's',	0,	0,	opt_s },
+	{ 0,			'O',	"%s",	ARG_ONE, opt_O },
+	{ "diff-filter",	0,	"%s",	ARG_ONE, opt_diff_filter },
+	{ "pickaxe-all",	0,	0,	0,	opt_pickaxe_all },
+	{ "pickaxe-regex",	0,	0,	0,	opt_pickaxe_regex },
+	{ 0,			'B',	"%s",	ARG_OPTINT,opt_B },
+	{ 0,			'M',	"%s",	ARG_OPTINT,opt_M },
+	{ 0,			'C',	"%s",	ARG_OPTINT,opt_C },
+	{ "find-copies-harder",	0,	0,	0,	opt_find_copies_harder},
+	abbrev_ost_row,
+	{ 0, 0 }
+};
+
+static void diff_non_option_cb(struct exec_args *b, const int argc,
+					const char **argv, int *argc_pos)
+{
+	rv_diff_opt_parse = 0;
+}
+
 int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 {
-	const char *arg = av[0];
-	if (!strcmp(arg, "-p") || !strcmp(arg, "-u"))
-		options->output_format = DIFF_FORMAT_PATCH;
-	else if (!strcmp(arg, "--patch-with-raw")) {
-		options->output_format = DIFF_FORMAT_PATCH;
-		options->with_raw = 1;
-	}
-	else if (!strcmp(arg, "--stat"))
-		options->output_format = DIFF_FORMAT_DIFFSTAT;
-	else if (!strcmp(arg, "--patch-with-stat")) {
-		options->output_format = DIFF_FORMAT_PATCH;
-		options->with_stat = 1;
-	}
-	else if (!strcmp(arg, "-z"))
-		options->line_termination = 0;
-	else if (!strncmp(arg, "-l", 2))
-		options->rename_limit = strtoul(arg+2, NULL, 10);
-	else if (!strcmp(arg, "--full-index"))
-		options->full_index = 1;
-	else if (!strcmp(arg, "--name-only"))
-		options->output_format = DIFF_FORMAT_NAME;
-	else if (!strcmp(arg, "--name-status"))
-		options->output_format = DIFF_FORMAT_NAME_STATUS;
-	else if (!strcmp(arg, "-R"))
-		options->reverse_diff = 1;
-	else if (!strncmp(arg, "-S", 2))
-		options->pickaxe = arg + 2;
-	else if (!strcmp(arg, "-s"))
-		options->output_format = DIFF_FORMAT_NO_OUTPUT;
-	else if (!strncmp(arg, "-O", 2))
-		options->orderfile = arg + 2;
-	else if (!strncmp(arg, "--diff-filter=", 14))
-		options->filter = arg + 14;
-	else if (!strcmp(arg, "--pickaxe-all"))
-		options->pickaxe_opts = DIFF_PICKAXE_ALL;
-	else if (!strcmp(arg, "--pickaxe-regex"))
-		options->pickaxe_opts = DIFF_PICKAXE_REGEX;
-	else if (!strncmp(arg, "-B", 2)) {
-		if ((options->break_opt =
-		     diff_scoreopt_parse(arg)) == -1)
-			return -1;
-	}
-	else if (!strncmp(arg, "-M", 2)) {
-		if ((options->rename_score =
-		     diff_scoreopt_parse(arg)) == -1)
-			return -1;
-		options->detect_rename = DIFF_DETECT_RENAME;
-	}
-	else if (!strncmp(arg, "-C", 2)) {
-		if ((options->rename_score =
-		     diff_scoreopt_parse(arg)) == -1)
-			return -1;
-		options->detect_rename = DIFF_DETECT_COPY;
-	}
-	else if (!strcmp(arg, "--find-copies-harder"))
-		options->find_copies_harder = 1;
-	else if (!strcmp(arg, "--abbrev"))
-		options->abbrev = DEFAULT_ABBREV;
-	else if (!strncmp(arg, "--abbrev=", 9)) {
-		options->abbrev = strtoul(arg + 9, NULL, 10);
-		if (options->abbrev < MINIMUM_ABBREV)
-			options->abbrev = MINIMUM_ABBREV;
-		else if (40 < options->abbrev)
-			options->abbrev = 40;
-	}
-	else
-		return 0;
-	return 1;
+	struct exec_args *a = new_exec_args(1);
+	int i = 0;
+	void (*old_non_option_cb)(struct exec_args *b, const int argc,
+				const char **argv, int *argc_pos);
+	g_opt = options;
+	rv_diff_opt_parse = 1;
+	gitopt_pass_through = 1;
+	old_non_option_cb = gitopt_non_option_cb;
+	gitopt_non_option_cb = diff_non_option_cb;
+
+	gitopt_parse_one_opt(a, a, diff_ost, ac, av, &i);
+
+	free_exec_args(a);
+	gitopt_non_option_cb = old_non_option_cb;
+
+	return rv_diff_opt_parse;
 }
 
 static int parse_num(const char **cp_p)
diff --git a/diff.h b/diff.h
index b3b2c4d..b5f016e 100644
--- a/diff.h
+++ b/diff.h
@@ -41,6 +41,7 @@ struct diff_options {
 	int rename_limit;
 	int setup;
 	int abbrev;
+	int ctxlen;
 
 	int nr_paths;
 	const char **paths;
diff --git a/gitopt/diff.h b/gitopt/diff.h
new file mode 100644
index 0000000..6530474
--- /dev/null
+++ b/gitopt/diff.h
@@ -0,0 +1,26 @@
+#ifndef GITOPT_DIFF_H
+#define GITOPT_DIFF_H
+
+static struct rev_info *g_rev;
+static int silent;
+static int cached;
+
+gitopt_eat(opt_base,		g_rev->max_count = 1;)
+gitopt_eat(opt_ours,		g_rev->max_count = 2;)
+gitopt_eat(opt_theres,		g_rev->max_count = 3;)
+gitopt_eat(opt_q,		silent = 1;)
+static const struct opt_spec diff_files_ost[] = {
+	{ "base",	0,	0,	0,	opt_base },
+	{ "ours",	0,	0,	0,	opt_ours },
+	{ "theres",	0,	0,	0,	opt_theres },
+	{ 0,		'q',	0,	0,	opt_q },
+	{ 0, 0 }
+};
+
+gitopt_eat(opt_cached,		cached = 1;)
+static const struct opt_spec diff_index_ost[] = {
+	{ "cached",	0,	0,	0,	opt_cached },
+	{ 0, 0 }
+};
+
+#endif /* GITOPT_DIFF_H */
diff --git a/http-push.c b/http-push.c
index b4327d9..9c16f3b 100644
--- a/http-push.c
+++ b/http-push.c
@@ -2499,7 +2499,7 @@ int main(int argc, char **argv)
 			commit_argc++;
 		}
 		init_revisions(&revs);
-		setup_revisions(commit_argc, commit_argv, &revs, NULL);
+		setup_revisions(commit_argc, commit_argv, &revs, NULL, NULL);
 		free(new_sha1_hex);
 		if (old_sha1_hex) {
 			free(old_sha1_hex);
diff --git a/rev-list.c b/rev-list.c
index 8b0ec38..3c72c59 100644
--- a/rev-list.c
+++ b/rev-list.c
@@ -7,6 +7,7 @@ #include "blob.h"
 #include "tree-walk.h"
 #include "diff.h"
 #include "revision.h"
+#include "gitopt.h"
 
 /* bits #0-15 in revision.h */
 
@@ -291,34 +292,31 @@ static void mark_edges_uninteresting(str
 	}
 }
 
+static struct rev_info *g_rev;
+gitopt_eat(opt_header,			g_rev->verbose_header = 1;)
+gitopt_eat(opt_timestamp,		show_timestamp = 1;)
+gitopt_eat(opt_bisect,			bisect_list = 1;)
+static const struct opt_spec rev_list_ost[] = {
+	{ "header",	0,	0,	0,	opt_header },
+	{ "timestamp",	0,	0,	0,	opt_timestamp },
+	{ "bisect",	0,	0,	0,	opt_bisect },
+	{ 0, 0 }
+};
+
 int main(int argc, const char **argv)
 {
 	struct commit_list *list;
-	int i;
+	struct exec_args *b;
 
 	init_revisions(&revs);
 	revs.abbrev = 0;
 	revs.commit_format = CMIT_FMT_UNSPECIFIED;
-	argc = setup_revisions(argc, argv, &revs, NULL);
-
-	for (i = 1 ; i < argc; i++) {
-		const char *arg = argv[i];
+	g_rev = &revs;
 
-		if (!strcmp(arg, "--header")) {
-			revs.verbose_header = 1;
-			continue;
-		}
-		if (!strcmp(arg, "--timestamp")) {
-			show_timestamp = 1;
-			continue;
-		}
-		if (!strcmp(arg, "--bisect")) {
-			bisect_list = 1;
-			continue;
-		}
+	b = setup_revisions(argc, argv, &revs, NULL, rev_list_ost);
+	if (b->argc)
 		usage(rev_list_usage);
 
-	}
 	if (revs.commit_format != CMIT_FMT_UNSPECIFIED) {
 		/* The command line has a --pretty  */
 		hdr_termination = '\n';
diff --git a/revision.c b/revision.c
index 2294b16..6769a48 100644
--- a/revision.c
+++ b/revision.c
@@ -6,6 +6,7 @@ #include "commit.h"
 #include "diff.h"
 #include "refs.h"
 #include "revision.h"
+#include "gitopt/abbrev.h"
 
 static char *path_name(struct name_path *path, const char *name)
 {
@@ -534,6 +535,198 @@ void init_revisions(struct rev_info *rev
 	diff_setup(&revs->diffopt);
 }
 
+static const char *g_def = NULL;
+
+/* I think I should just be able to use the all_* versions of these.. -ew */
+static struct rev_info *g_revs = NULL;
+static int g_flags = 0;
+static const struct opt_spec *g_extra_ost;
+
+gitopt_eat_int(opt_max_count,	g_revs->max_count = atoi(ea->argv[0]);)
+gitopt_eat_int(opt_max_age,	g_revs->max_age = atoi(ea->argv[0]);)
+gitopt_eat_int(opt_since,	g_revs->max_age = approxidate(ea->argv[0]);)
+gitopt_eat_int(opt_min_age,	g_revs->min_age = atoi(ea->argv[0]);)
+gitopt_eat_int(opt_until,	g_revs->min_age = approxidate(ea->argv[0]);)
+gitopt_eat(opt_all,		handle_all(g_revs, g_flags);)
+gitopt_eat(opt_not,		g_flags ^= UNINTERESTING;)
+gitopt_eat_arg(opt_default,	g_def = ea->argv[0];)
+gitopt_eat(opt_topo_order,	g_revs->topo_order = 1;)
+gitopt_eat(opt_date_order,	g_revs->topo_order = 1; g_revs->lifo = 0;)
+gitopt_eat(opt_parents,		g_revs->parents = 1;)
+gitopt_eat(opt_dense,		g_revs->dense = 1;)
+gitopt_eat(opt_sparse,		g_revs->dense = 0;)
+gitopt_eat(opt_remove_empty,	g_revs->remove_empty_trees = 1;)
+gitopt_eat(opt_no_merges,	g_revs->no_merges = 1;)
+gitopt_eat(opt_boundary,	g_revs->boundary = 1;)
+gitopt_eat(opt_objects,		g_revs->tag_objects = g_revs->tree_objects =
+					g_revs->blob_objects = 1;)
+gitopt_eat(opt_objects_edge,	g_revs->tag_objects = g_revs->tree_objects =
+				g_revs->blob_objects = g_revs->edge_hint = 1;)
+gitopt_eat(opt_unpacked,	g_revs->unpacked = 1;)
+gitopt_eat(opt_r,		g_revs->diff = g_revs->diffopt.recursive = 1;)
+gitopt_eat(opt_t,		g_revs->diff = g_revs->diffopt.recursive =
+					g_revs->diffopt.tree_in_recursive = 1;)
+gitopt_eat(opt_m,		g_revs->ignore_merges = 0;)
+gitopt_eat(opt_c,		g_revs->diff = g_revs->combine_merges = 1;
+				g_revs->dense_combined_merges = 0;)
+gitopt_eat(opt_cc,		g_revs->diff = g_revs->combine_merges =
+					g_revs->dense_combined_merges = 1;)
+gitopt_eat(opt_v,		g_revs->verbose_header = 1;)
+gitopt_eat_arg(opt_pretty,		g_revs->verbose_header = 1;
+				g_revs->commit_format = get_commit_format(
+								ea->argv[0]);)
+gitopt_eat(opt_root,		g_revs->show_root_diff = 1;)
+gitopt_eat(opt_no_commit_id,	g_revs->no_commit_id = 1;)
+gitopt_eat(opt_always,		g_revs->always_show_header = 1;)
+gitopt_opt_abbrev(g_revs->abbrev)
+gitopt_eat(opt_no_abbrev,	g_revs->abbrev = 0;)
+gitopt_eat(opt_abbrev_commit,	g_revs->abbrev_commit = 1;)
+gitopt_eat(opt_full_diff,	g_revs->full_diff = g_revs->diff = 1;)
+
+static const struct opt_spec setup_revisions_ost[] = {
+	{ "max-count",		'n',	"%s",	ARG_INT,	opt_max_count },
+	{ 0,			' ',	"%s",	ARG_INT,	opt_max_count },
+	{ "max-age",		0,	"%s",	ARG_INT,	opt_max_age },
+	{ "min-age",		0,	"%s",	ARG_INT,	opt_min_age },
+	{ "since",		0,	"%s",	ARG_ONE,	opt_since },
+	{ "after",		0,	"%s",	ARG_ONE,	opt_since },
+	{ "before",		0,	"%s",	ARG_ONE,	opt_until },
+	{ "until",		0,	"%s",	ARG_ONE,	opt_until },
+	{ "all",		0,	0,	0,		opt_all },
+	{ "not",		0,	0,	0,		opt_not },
+	{ "default",		0,	"%s",	ARG_ONE,	opt_default },
+	{ "topo-order",		0,	0,	0,	opt_topo_order },
+	{ "date-order",		0,	0,	0,	opt_date_order },
+	{ "parents",		0,	0,	0,	opt_parents },
+	{ "dense",		0,	0,	0,	opt_dense },
+	{ "sparse",		0,	0,	0,	opt_sparse },
+	{ "remove-empty",	0,	0,	0,	opt_remove_empty },
+	{ "no-merges",		0,	0,	0,	opt_no_merges },
+	{ "boundary",		0,	0,	0,	opt_boundary },
+	{ "objects",		0,	0,	0,	opt_objects },
+	{ "objects-edge",	0,	0,	0,	opt_objects_edge },
+	{ "unpacked",		0,	0,	0,	opt_unpacked },
+	{ 0,			'r',	0,	0,	opt_r },
+	{ 0,			't',	0,	0,	opt_t },
+	{ 0,			'm',	0,	0,	opt_m },
+	{ 0,			'c',	0,	0,	opt_c },
+	{ "cc",			0,	0,	0,	opt_cc },
+	{ 0,			'v',	0,	0,	opt_v },
+	{ "pretty",		0,	"%s",	ARG_ONE,	opt_pretty },
+	{ "root",		0,	0,	0,	opt_root },
+	{ "no-commit-id",	0,	0,	0,	opt_no_commit_id },
+	{ "always",		0,	0,	0,	opt_always },
+	{ "no-abbrev",		0,	0,	0,	opt_no_abbrev },
+	abbrev_ost_row,
+	{ "abbrev-commit",	0,	0,	0,	opt_abbrev_commit },
+	{ "full-diff",		0,	0,	0,	opt_full_diff },
+	{ 0, 0 }
+};
+
+/* call this for every non-option (and everything after "--") we have */
+static void setup_revisions_non_option_cb(struct exec_args *b,
+			const int argc, const char **argv, int *argc_pos)
+{
+	int i = *argc_pos;
+	const char *arg = argv[i];
+	unsigned char sha1[20];
+	struct object *object;
+	char *dotdot;
+	int local_flags;
+
+	if (arg[0] == '-') { /* handle diff options: */
+		int opts = diff_opt_parse(&(g_revs->diffopt),
+					argv + i, argc - 1);
+		if (opts > 0) {
+			g_revs->diff = 1;
+			*argc_pos += opts - 1;
+			return;
+		}
+		if (g_extra_ost) {
+			void (*old_non_option_cb)(struct exec_args *b,
+					const int argc, const char **argv,
+					int *argc_pos);
+			int j = 0;
+			old_non_option_cb = gitopt_non_option_cb;
+			gitopt_non_option_cb = gitopt_default_non_option_cb;
+			gitopt_parse_one_opt(b, b, g_extra_ost,
+						argc - i, argv + i, &j);
+			gitopt_non_option_cb = old_non_option_cb;
+			*argc_pos += j;
+		}
+		return;
+	}
+
+	/* otherwise it's a revision */
+	dotdot = strstr(arg, "..");
+	if (dotdot) {
+		unsigned char from_sha1[20];
+		const char *next = dotdot + 2;
+		const char *this = arg;
+		*dotdot = 0;
+		if (!*next)
+			next = "HEAD";
+		if (dotdot == arg)
+			this = "HEAD";
+		if (!get_sha1(this, from_sha1) &&
+		    !get_sha1(next, sha1)) {
+			struct object *exclude;
+			struct object *include;
+
+			exclude = get_reference(g_revs, this, from_sha1,
+						g_flags ^ UNINTERESTING);
+			include = get_reference(g_revs, next, sha1, g_flags);
+			if (!exclude || !include)
+				die("Invalid revision range %s..%s", arg, next);
+
+			if (!gitopt_dd_seen) {
+				*dotdot = '.';
+				verify_non_filename(g_revs->prefix, arg);
+			}
+			add_pending_object(g_revs, exclude, this);
+			add_pending_object(g_revs, include, next);
+			return;
+		}
+		*dotdot = '.';
+	}
+
+	dotdot = strstr(arg, "^@");
+	if (dotdot && !dotdot[2]) {
+		*dotdot = 0;
+		if (add_parents_only(g_revs, arg, g_flags))
+			return;
+		*dotdot = '^';
+	}
+	local_flags = 0;
+	if (*arg == '^') {
+		local_flags = UNINTERESTING;
+		arg++;
+	}
+	if (get_sha1(arg, sha1) < 0) {
+		int j;
+
+		if (gitopt_dd_seen || local_flags)
+			die("bad revision '%s'", arg);
+
+		/* If we didn't have a "--":
+		 * (1) all filenames must exist;
+		 * (2) all rev-args must not be interpretable
+		 *     as a valid filename.
+		 * but the latter we have checked in the main loop.
+		 */
+		for (j = *argc_pos; j < argc; j++)
+			verify_filename(g_revs->prefix, argv[j]);
+
+		g_revs->prune_data = get_pathspec(g_revs->prefix,
+						argv + *argc_pos);
+		return;
+	}
+	if (!gitopt_dd_seen)
+		verify_non_filename(g_revs->prefix, arg);
+	object = get_reference(g_revs, arg, sha1, g_flags ^ local_flags);
+	add_pending_object(g_revs, object, arg);
+}
+
 /*
  * Parse revision information, filling in the "rev_info" structure,
  * and removing the used arguments from the argument list.
@@ -541,14 +734,19 @@ void init_revisions(struct rev_info *rev
  * Returns the number of arguments left that weren't recognized
  * (which are also moved to the head of the argument list)
  */
-int setup_revisions(int argc, const char **argv, struct rev_info *revs, const char *def)
+struct exec_args *setup_revisions(int argc, const char **argv,
+				struct rev_info *revs, const char *def,
+				const struct opt_spec *extra_ost)
 {
-	int i, flags, seen_dashdash;
-	const char **unrecognized = argv + 1;
-	int left = 1;
+	int i;
+	struct exec_args *a, *b;
+
+	g_extra_ost = extra_ost;
+	g_def = def;
+	g_revs = revs;
+	gitopt_dd_seen = 0;
 
 	/* First, search for "--" */
-	seen_dashdash = 0;
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 		if (strcmp(arg, "--"))
@@ -556,274 +754,24 @@ int setup_revisions(int argc, const char
 		argv[i] = NULL;
 		argc = i;
 		revs->prune_data = get_pathspec(revs->prefix, argv + i + 1);
-		seen_dashdash = 1;
+		gitopt_dd_seen = 1;
 		break;
 	}
 
-	flags = 0;
-	for (i = 1; i < argc; i++) {
-		struct object *object;
-		const char *arg = argv[i];
-		unsigned char sha1[20];
-		char *dotdot;
-		int local_flags;
-
-		if (*arg == '-') {
-			int opts;
-			if (!strncmp(arg, "--max-count=", 12)) {
-				revs->max_count = atoi(arg + 12);
-				continue;
-			}
-			/* accept -<digit>, like traditional "head" */
-			if ((*arg == '-') && isdigit(arg[1])) {
-				revs->max_count = atoi(arg + 1);
-				continue;
-			}
-			if (!strcmp(arg, "-n")) {
-				if (argc <= i + 1)
-					die("-n requires an argument");
-				revs->max_count = atoi(argv[++i]);
-				continue;
-			}
-			if (!strncmp(arg,"-n",2)) {
-				revs->max_count = atoi(arg + 2);
-				continue;
-			}
-			if (!strncmp(arg, "--max-age=", 10)) {
-				revs->max_age = atoi(arg + 10);
-				continue;
-			}
-			if (!strncmp(arg, "--since=", 8)) {
-				revs->max_age = approxidate(arg + 8);
-				continue;
-			}
-			if (!strncmp(arg, "--after=", 8)) {
-				revs->max_age = approxidate(arg + 8);
-				continue;
-			}
-			if (!strncmp(arg, "--min-age=", 10)) {
-				revs->min_age = atoi(arg + 10);
-				continue;
-			}
-			if (!strncmp(arg, "--before=", 9)) {
-				revs->min_age = approxidate(arg + 9);
-				continue;
-			}
-			if (!strncmp(arg, "--until=", 8)) {
-				revs->min_age = approxidate(arg + 8);
-				continue;
-			}
-			if (!strcmp(arg, "--all")) {
-				handle_all(revs, flags);
-				continue;
-			}
-			if (!strcmp(arg, "--not")) {
-				flags ^= UNINTERESTING;
-				continue;
-			}
-			if (!strcmp(arg, "--default")) {
-				if (++i >= argc)
-					die("bad --default argument");
-				def = argv[i];
-				continue;
-			}
-			if (!strcmp(arg, "--topo-order")) {
-				revs->topo_order = 1;
-				continue;
-			}
-			if (!strcmp(arg, "--date-order")) {
-				revs->lifo = 0;
-				revs->topo_order = 1;
-				continue;
-			}
-			if (!strcmp(arg, "--parents")) {
-				revs->parents = 1;
-				continue;
-			}
-			if (!strcmp(arg, "--dense")) {
-				revs->dense = 1;
-				continue;
-			}
-			if (!strcmp(arg, "--sparse")) {
-				revs->dense = 0;
-				continue;
-			}
-			if (!strcmp(arg, "--remove-empty")) {
-				revs->remove_empty_trees = 1;
-				continue;
-			}
-			if (!strcmp(arg, "--no-merges")) {
-				revs->no_merges = 1;
-				continue;
-			}
-			if (!strcmp(arg, "--boundary")) {
-				revs->boundary = 1;
-				continue;
-			}
-			if (!strcmp(arg, "--objects")) {
-				revs->tag_objects = 1;
-				revs->tree_objects = 1;
-				revs->blob_objects = 1;
-				continue;
-			}
-			if (!strcmp(arg, "--objects-edge")) {
-				revs->tag_objects = 1;
-				revs->tree_objects = 1;
-				revs->blob_objects = 1;
-				revs->edge_hint = 1;
-				continue;
-			}
-			if (!strcmp(arg, "--unpacked")) {
-				revs->unpacked = 1;
-				continue;
-			}
-			if (!strcmp(arg, "-r")) {
-				revs->diff = 1;
-				revs->diffopt.recursive = 1;
-				continue;
-			}
-			if (!strcmp(arg, "-t")) {
-				revs->diff = 1;
-				revs->diffopt.recursive = 1;
-				revs->diffopt.tree_in_recursive = 1;
-				continue;
-			}
-			if (!strcmp(arg, "-m")) {
-				revs->ignore_merges = 0;
-				continue;
-			}
-			if (!strcmp(arg, "-c")) {
-				revs->diff = 1;
-				revs->dense_combined_merges = 0;
-				revs->combine_merges = 1;
-				continue;
-			}
-			if (!strcmp(arg, "--cc")) {
-				revs->diff = 1;
-				revs->dense_combined_merges = 1;
-				revs->combine_merges = 1;
-				continue;
-			}
-			if (!strcmp(arg, "-v")) {
-				revs->verbose_header = 1;
-				continue;
-			}
-			if (!strncmp(arg, "--pretty", 8)) {
-				revs->verbose_header = 1;
-				revs->commit_format = get_commit_format(arg+8);
-				continue;
-			}
-			if (!strcmp(arg, "--root")) {
-				revs->show_root_diff = 1;
-				continue;
-			}
-			if (!strcmp(arg, "--no-commit-id")) {
-				revs->no_commit_id = 1;
-				continue;
-			}
-			if (!strcmp(arg, "--always")) {
-				revs->always_show_header = 1;
-				continue;
-			}
-			if (!strcmp(arg, "--no-abbrev")) {
-				revs->abbrev = 0;
-				continue;
-			}
-			if (!strcmp(arg, "--abbrev")) {
-				revs->abbrev = DEFAULT_ABBREV;
-				continue;
-			}
-			if (!strcmp(arg, "--abbrev-commit")) {
-				revs->abbrev_commit = 1;
-				continue;
-			}
-			if (!strcmp(arg, "--full-diff")) {
-				revs->diff = 1;
-				revs->full_diff = 1;
-				continue;
-			}
-			opts = diff_opt_parse(&revs->diffopt, argv+i, argc-i);
-			if (opts > 0) {
-				revs->diff = 1;
-				i += opts - 1;
-				continue;
-			}
-			*unrecognized++ = arg;
-			left++;
-			continue;
-		}
-		dotdot = strstr(arg, "..");
-		if (dotdot) {
-			unsigned char from_sha1[20];
-			const char *next = dotdot + 2;
-			const char *this = arg;
-			*dotdot = 0;
-			if (!*next)
-				next = "HEAD";
-			if (dotdot == arg)
-				this = "HEAD";
-			if (!get_sha1(this, from_sha1) &&
-			    !get_sha1(next, sha1)) {
-				struct object *exclude;
-				struct object *include;
-
-				exclude = get_reference(revs, this, from_sha1, flags ^ UNINTERESTING);
-				include = get_reference(revs, next, sha1, flags);
-				if (!exclude || !include)
-					die("Invalid revision range %s..%s", arg, next);
-
-				if (!seen_dashdash) {
-					*dotdot = '.';
-					verify_non_filename(revs->prefix, arg);
-				}
-				add_pending_object(revs, exclude, this);
-				add_pending_object(revs, include, next);
-				continue;
-			}
-			*dotdot = '.';
-		}
-		dotdot = strstr(arg, "^@");
-		if (dotdot && !dotdot[2]) {
-			*dotdot = 0;
-			if (add_parents_only(revs, arg, flags))
-				continue;
-			*dotdot = '^';
-		}
-		local_flags = 0;
-		if (*arg == '^') {
-			local_flags = UNINTERESTING;
-			arg++;
-		}
-		if (get_sha1(arg, sha1)) {
-			int j;
-
-			if (seen_dashdash || local_flags)
-				die("bad revision '%s'", arg);
-
-			/* If we didn't have a "--":
-			 * (1) all filenames must exist;
-			 * (2) all rev-args must not be interpretable
-			 *     as a valid filename.
-			 * but the latter we have checked in the main loop.
-			 */
-			for (j = i; j < argc; j++)
-				verify_filename(revs->prefix, argv[j]);
+	a = new_exec_args(argc);
+	b = new_exec_args(argc);
+	gitopt_pass_through = 1;
+	gitopt_non_option_cb = setup_revisions_non_option_cb;
+	if (gitopt_parse_ost_split(a, b, setup_revisions_ost, argc, argv))
+		return b;
 
-			revs->prune_data = get_pathspec(revs->prefix, argv + i);
-			break;
-		}
-		if (!seen_dashdash)
-			verify_non_filename(revs->prefix, arg);
-		object = get_reference(revs, arg, sha1, flags ^ local_flags);
-		add_pending_object(revs, object, arg);
-	}
-	if (def && !revs->pending_objects) {
+	if (g_def && !revs->pending_objects) {
 		unsigned char sha1[20];
 		struct object *object;
-		if (get_sha1(def, sha1))
-			die("bad default revision '%s'", def);
-		object = get_reference(revs, def, sha1, 0);
-		add_pending_object(revs, object, def);
+		if (get_sha1(g_def, sha1))
+			die("bad default revision '%s'", g_def);
+		object = get_reference(revs, g_def, sha1, 0);
+		add_pending_object(revs, object, g_def);
 	}
 
 	if (revs->topo_order || revs->unpacked)
@@ -844,7 +792,7 @@ int setup_revisions(int argc, const char
 	revs->diffopt.abbrev = revs->abbrev;
 	diff_setup_done(&revs->diffopt);
 
-	return left;
+	return b;
 }
 
 void prepare_revision_walk(struct rev_info *revs)
diff --git a/revision.h b/revision.h
index 48d7b4c..11a5820 100644
--- a/revision.h
+++ b/revision.h
@@ -1,6 +1,8 @@
 #ifndef REVISION_H
 #define REVISION_H
 
+#include "gitopt.h"
+
 #define SEEN		(1u<<0)
 #define UNINTERESTING   (1u<<1)
 #define TREECHANGE	(1u<<2)
@@ -81,7 +83,9 @@ extern int rev_same_tree_as_empty(struct
 extern int rev_compare_tree(struct rev_info *, struct tree *t1, struct tree *t2);
 
 extern void init_revisions(struct rev_info *revs);
-extern int setup_revisions(int argc, const char **argv, struct rev_info *revs, const char *def);
+extern struct exec_args *setup_revisions(int argc, const char **argv,
+					struct rev_info *revs, const char *def,
+					const struct opt_spec *extra_ost);
 extern void prepare_revision_walk(struct rev_info *revs);
 extern struct commit *get_revision(struct rev_info *revs);
 
-- 
1.3.2.g0a3ae

^ permalink raw reply related

* [PATCH 4/6] ls-files: convert to using gitopt
From: Eric Wong @ 2006-05-09  5:06 UTC (permalink / raw)
  To: git; +Cc: Eric Wong
In-Reply-To: <1147151211399-git-send-email-normalperson@yhbt.net>

Another simple and straight forward conversion, imho.

Signed-off-by: Eric Wong <normalperson@yhbt.net>

---

 ls-files.c |  187 ++++++++++++++++++++++--------------------------------------
 1 files changed, 69 insertions(+), 118 deletions(-)

6a65d6f1e59638185c846920d8c6f0dbd82f1bf7
diff --git a/ls-files.c b/ls-files.c
index 4a4af1c..62f9e10 100644
--- a/ls-files.c
+++ b/ls-files.c
@@ -10,6 +10,8 @@ #include <fnmatch.h>
 
 #include "cache.h"
 #include "quote.h"
+#include "gitopt.h"
+#include "gitopt/abbrev.h"
 
 static int abbrev = 0;
 static int show_deleted = 0;
@@ -648,133 +650,82 @@ static const char ls_files_usage[] =
 	"[ --exclude-per-directory=<filename> ] [--full-name] [--abbrev] "
 	"[--] [<file>]*";
 
+static void tag_pfx()
+{
+	tag_cached = "H ";
+	tag_unmerged = "M ";
+	tag_removed = "R ";
+	tag_modified = "C ";
+	tag_other = "? ";
+	tag_killed = "K ";
+}
+
+static int exc_given = 0;
+
+gitopt_eat(opt_z, line_terminator = 0;)
+gitopt_eat(opt_t, tag_pfx();)
+gitopt_eat(opt_v, tag_pfx(); show_valid_bit = 1;)
+gitopt_eat(opt_c, show_cached = 1;)
+gitopt_eat(opt_d, show_deleted = 1;)
+gitopt_eat(opt_m, show_modified = 1;)
+gitopt_eat(opt_o, show_others = 1;)
+gitopt_eat(opt_i, show_ignored = 1;)
+gitopt_eat(opt_s, show_stage = 1;)
+gitopt_eat(opt_k, show_killed = 1;)
+gitopt_eat(opt_directory, show_other_directories = 1;)
+gitopt_eat(opt_no_empty_directory, hide_empty_directories = 1;)
+gitopt_eat(opt_u, show_stage = 1; show_unmerged = 1;)
+gitopt_eat_one_arg(opt_x,
+		exc_given = 1;
+		add_exclude(ea->argv[0], "", 0, &exclude_list[EXC_CMDL]);)
+gitopt_eat_one_arg(opt_X,
+		exc_given = 1;
+		add_excludes_from_file(ea->argv[0]);)
+gitopt_eat_one_arg(opt_exclude_per_dir,
+		exc_given = 1;
+		exclude_per_dir = ea->argv[0];)
+gitopt_eat(opt_full_name, prefix_offset = 0;)
+gitopt_eat(opt_error_unmatch, error_unmatch = 1;)
+gitopt_opt_abbrev(abbrev)
+
+static const struct opt_spec ls_files_ost[] = {
+	{ 0,			'z',	0,	0,	opt_z },
+	{ 0,			'v',	0,	0,	opt_v },
+	{ 0,			't',	0,	0,	opt_t },
+	{ "cached",		'c',	0,	0,	opt_c },
+	{ "deleted",		'd',	0,	0,	opt_d },
+	{ "modified",		'm',	0,	0,	opt_m },
+	{ "others",		'o',	0,	0,	opt_o },
+	{ "ignored",		'i',	0,	0,	opt_i },
+	{ "stage",		's',	0,	0,	opt_s },
+	{ "killed",		'k',	0,	0,	opt_k },
+	{ "directory",		0,	0,	0,	opt_directory },
+	{ "no-empty-directory",	0,	0,	0,	opt_no_empty_directory},
+	{ "unmerged",		'u',	0,	0,	opt_u },
+	{ "exclude",		'x',	"%s",	ARG_ONE,	opt_x },
+	{ "exclude-from",	'X',	"%s",	ARG_ONE,	opt_X },
+	{ "exclude-per-directory",0,	"%s",	ARG_ONE, opt_exclude_per_dir },
+	{ "full-name",		0,	0,	0,	opt_full_name },
+	{ "error-unmatch",	0,	0,	0,	opt_error_unmatch },
+	abbrev_ost_row,
+	{ 0, 0 }
+};
+
 int main(int argc, const char **argv)
 {
-	int i;
-	int exc_given = 0;
+	struct exec_args *a = new_exec_args(argc); /* argv[0] and options: */
+	struct exec_args *b = new_exec_args(argc); /* non-option args */
 
 	prefix = setup_git_directory();
 	if (prefix)
 		prefix_offset = strlen(prefix);
 	git_config(git_default_config);
 
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
-
-		if (!strcmp(arg, "--")) {
-			i++;
-			break;
-		}
-		if (!strcmp(arg, "-z")) {
-			line_terminator = 0;
-			continue;
-		}
-		if (!strcmp(arg, "-t") || !strcmp(arg, "-v")) {
-			tag_cached = "H ";
-			tag_unmerged = "M ";
-			tag_removed = "R ";
-			tag_modified = "C ";
-			tag_other = "? ";
-			tag_killed = "K ";
-			if (arg[1] == 'v')
-				show_valid_bit = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-c") || !strcmp(arg, "--cached")) {
-			show_cached = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-d") || !strcmp(arg, "--deleted")) {
-			show_deleted = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-m") || !strcmp(arg, "--modified")) {
-			show_modified = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-o") || !strcmp(arg, "--others")) {
-			show_others = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-i") || !strcmp(arg, "--ignored")) {
-			show_ignored = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-s") || !strcmp(arg, "--stage")) {
-			show_stage = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-k") || !strcmp(arg, "--killed")) {
-			show_killed = 1;
-			continue;
-		}
-		if (!strcmp(arg, "--directory")) {
-			show_other_directories = 1;
-			continue;
-		}
-		if (!strcmp(arg, "--no-empty-directory")) {
-			hide_empty_directories = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-u") || !strcmp(arg, "--unmerged")) {
-			/* There's no point in showing unmerged unless
-			 * you also show the stage information.
-			 */
-			show_stage = 1;
-			show_unmerged = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-x") && i+1 < argc) {
-			exc_given = 1;
-			add_exclude(argv[++i], "", 0, &exclude_list[EXC_CMDL]);
-			continue;
-		}
-		if (!strncmp(arg, "--exclude=", 10)) {
-			exc_given = 1;
-			add_exclude(arg+10, "", 0, &exclude_list[EXC_CMDL]);
-			continue;
-		}
-		if (!strcmp(arg, "-X") && i+1 < argc) {
-			exc_given = 1;
-			add_excludes_from_file(argv[++i]);
-			continue;
-		}
-		if (!strncmp(arg, "--exclude-from=", 15)) {
-			exc_given = 1;
-			add_excludes_from_file(arg+15);
-			continue;
-		}
-		if (!strncmp(arg, "--exclude-per-directory=", 24)) {
-			exc_given = 1;
-			exclude_per_dir = arg + 24;
-			continue;
-		}
-		if (!strcmp(arg, "--full-name")) {
-			prefix_offset = 0;
-			continue;
-		}
-		if (!strcmp(arg, "--error-unmatch")) {
-			error_unmatch = 1;
-			continue;
-		}
-		if (!strncmp(arg, "--abbrev=", 9)) {
-			abbrev = strtoul(arg+9, NULL, 10);
-			if (abbrev && abbrev < MINIMUM_ABBREV)
-				abbrev = MINIMUM_ABBREV;
-			else if (abbrev > 40)
-				abbrev = 40;
-			continue;
-		}
-		if (!strcmp(arg, "--abbrev")) {
-			abbrev = DEFAULT_ABBREV;
-			continue;
-		}
-		if (*arg == '-')
-			usage(ls_files_usage);
-		break;
-	}
+	if (gitopt_parse_ost_split(a, b, ls_files_ost, argc, argv) < 0)
+		usage(ls_files_usage);
+	free_exec_args(a);
 
-	pathspec = get_pathspec(prefix, argv + i);
+	pathspec = get_pathspec(prefix,b->argv);
 
 	/* Verify that the pathspec matches the prefix */
 	if (pathspec)
-- 
1.3.2.g0a3ae

^ permalink raw reply related

* [PATCH 3/6] ls-tree: convert to gitopt
From: Eric Wong @ 2006-05-09  5:06 UTC (permalink / raw)
  To: git; +Cc: Eric Wong
In-Reply-To: <11471512101532-git-send-email-normalperson@yhbt.net>

A pretty simple and straight forward conversion, imho.

Signed-off-by: Eric Wong <normalperson@yhbt.net>

---

 gitopt/abbrev.h |   24 +++++++++++++++++
 ls-tree.c       |   79 +++++++++++++++++++++++--------------------------------
 2 files changed, 57 insertions(+), 46 deletions(-)
 create mode 100644 gitopt/abbrev.h

06d5dc3649e7407f7ff8df0d42a55a906b39cb39
diff --git a/gitopt/abbrev.h b/gitopt/abbrev.h
new file mode 100644
index 0000000..c3b2353
--- /dev/null
+++ b/gitopt/abbrev.h
@@ -0,0 +1,24 @@
+#include "../cache.h"
+
+/* we specify rewrite_fmt here to make opt_abbrev() simpler: */
+#define abbrev_ost_row \
+	{ "abbrev",		0,	"a %s",	ARG_OPTINT,	opt_abbrev }
+
+#define gitopt_opt_abbrev(dest) \
+static struct exec_args *opt_abbrev(const struct opt_spec *s, \
+			const int argc, const char **argv, int *argc_pos) \
+{ \
+	struct exec_args *ea = optional_int_arg(s, argc, argv, argc_pos); \
+	if (!ea) return NULL; \
+	if (ea->argc == 1) \
+		dest = DEFAULT_ABBREV; \
+	else { \
+		dest = strtoul(ea->argv[1], NULL, 10); \
+		if (dest && dest < MINIMUM_ABBREV) \
+			dest = MINIMUM_ABBREV; \
+		else if (dest > 40) \
+			dest = 40; \
+	} \
+	return nil_exec_args(ea); \
+}
+
diff --git a/ls-tree.c b/ls-tree.c
index f2b3bc1..4fb6343 100644
--- a/ls-tree.c
+++ b/ls-tree.c
@@ -7,6 +7,8 @@ #include "cache.h"
 #include "blob.h"
 #include "tree.h"
 #include "quote.h"
+#include "gitopt.h"
+#include "gitopt/abbrev.h"
 
 static int line_termination = '\n';
 #define LS_RECURSIVE 1
@@ -84,68 +86,53 @@ static int show_tree(unsigned char *sha1
 	return retval;
 }
 
+gitopt_eat(opt_d, ls_options |= LS_TREE_ONLY;)
+gitopt_eat(opt_t, ls_options |= LS_SHOW_TREES;)
+gitopt_eat(opt_z, line_termination = 0;)
+gitopt_eat(opt_r, ls_options |= LS_RECURSIVE;)
+gitopt_eat(opt_name_only_status, ls_options |= LS_NAME_ONLY;)
+gitopt_eat(opt_full_name, chomp_prefix = 0;)
+gitopt_opt_abbrev(abbrev)
+
+static const struct opt_spec ls_tree_ost[] = {
+	{ 0,			'z',	0,	0,	opt_z },
+	{ 0,			'r',	0,	0,	opt_r },
+	{ 0,			'd',	0,	0,	opt_d },
+	{ 0,			't',	0,	0,	opt_t },
+	{ "name-only",		0,	0,	0,	opt_name_only_status },
+	{ "name-status",	0,	0,	0,	opt_name_only_status },
+	{ "full-name",		0,	0,	0,	opt_full_name },
+	abbrev_ost_row,
+	{ 0, 0 }
+};
+
 int main(int argc, const char **argv)
 {
 	unsigned char sha1[20];
 	struct tree *tree;
+	struct exec_args *a = new_exec_args(argc); /* argv[0] and options: */
+	struct exec_args *b = new_exec_args(argc); /* non-option args */
 
 	prefix = setup_git_directory();
 	git_config(git_default_config);
 	if (prefix && *prefix)
 		chomp_prefix = strlen(prefix);
-	while (1 < argc && argv[1][0] == '-') {
-		switch (argv[1][1]) {
-		case 'z':
-			line_termination = 0;
-			break;
-		case 'r':
-			ls_options |= LS_RECURSIVE;
-			break;
-		case 'd':
-			ls_options |= LS_TREE_ONLY;
-			break;
-		case 't':
-			ls_options |= LS_SHOW_TREES;
-			break;
-		case '-':
-			if (!strcmp(argv[1]+2, "name-only") ||
-			    !strcmp(argv[1]+2, "name-status")) {
-				ls_options |= LS_NAME_ONLY;
-				break;
-			}
-			if (!strcmp(argv[1]+2, "full-name")) {
-				chomp_prefix = 0;
-				break;
-			}
-			if (!strncmp(argv[1]+2, "abbrev=",7)) {
-				abbrev = strtoul(argv[1]+9, NULL, 10);
-				if (abbrev && abbrev < MINIMUM_ABBREV)
-					abbrev = MINIMUM_ABBREV;
-				else if (abbrev > 40)
-					abbrev = 40;
-				break;
-			}
-			if (!strcmp(argv[1]+2, "abbrev")) {
-				abbrev = DEFAULT_ABBREV;
-				break;
-			}
-			/* otherwise fallthru */
-		default:
-			usage(ls_tree_usage);
-		}
-		argc--; argv++;
-	}
+
+	if (gitopt_parse_ost_split(a, b, ls_tree_ost, argc, argv) < 0)
+		usage(ls_tree_usage);
+	free_exec_args(a);
+
 	/* -d -r should imply -t, but -d by itself should not have to. */
 	if ( (LS_TREE_ONLY|LS_RECURSIVE) ==
 	    ((LS_TREE_ONLY|LS_RECURSIVE) & ls_options))
 		ls_options |= LS_SHOW_TREES;
 
-	if (argc < 2)
+	if (b->argc < 1)
 		usage(ls_tree_usage);
-	if (get_sha1(argv[1], sha1))
-		die("Not a valid object name %s", argv[1]);
+	if (get_sha1(b->argv[0], sha1))
+		die("Not a valid object name %s", b->argv[0]);
 
-	pathspec = get_pathspec(prefix, argv + 2);
+	pathspec = get_pathspec(prefix, b->argv + 1);
 	tree = parse_tree_indirect(sha1);
 	if (!tree)
 		die("not a tree object");
-- 
1.3.2.g0a3ae

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox