* [PATCH] t7501-commit.sh: Add test case for fixing author in amend commit.
From: Kristian Høgsberg @ 2007-11-05 17:17 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: gitster, git, Kristian Høgsberg
In-Reply-To: <Pine.LNX.4.64.0711051020330.4362@racer.site>
Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---
How about this?
t/t7501-commit.sh | 15 +++++++++++++++
1 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index b151b51..c5d122f 100644
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -163,4 +163,19 @@ test_expect_success 'partial commit that involves removal (3)' '
'
+author="The Real Author <someguy@his.email.org>"
+test_expect_success 'amend commit to fix author' '
+
+ oldtick=$GIT_AUTHOR_DATE &&
+ test_tick &&
+ git reset --hard &&
+ git cat-file -p HEAD |
+ sed -e "s/author.*/author $author $oldtick/" \
+ -e "s/^\(committer.*> \).*$/\1$GIT_COMMITTER_DATE/" > \
+ expected &&
+ git commit --amend --author="$author" &&
+ git cat-file -p HEAD > current &&
+ diff expected current
+
+'
test_done
--
1.5.3.4
^ permalink raw reply related
* Re: [PATCH] Use parseopts in builtin-fetch
From: Daniel Barkalow @ 2007-11-05 17:14 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Junio C Hamano, git
In-Reply-To: <20071105084333.GA25574@artemis.corp>
On Mon, 5 Nov 2007, Pierre Habouzit wrote:
> On Mon, Nov 05, 2007 at 03:35:34AM +0000, Daniel Barkalow wrote:
> > Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
> > ---
> > I mostly did this and the next one for practice with the API. I'm
> > impressed that "git fetch -vv" is even handled correctly without anything
> > special. Now that I've done it, assuming I did it right, it might as well
> > get added to the series.
>
> I believe the same patches (or very similar ones) are in pu but are
> not in next yet because they conflict with the builtin-fetch recent
> series.
>
> see http://git.madism.org/?p=git.git;a=blobdiff;f=builtin-fetch.c;h=12b1c4;hp=6b1750d;hb=7407915;hpb=61610e6
Ah, okay, forgot to look there. In any case, I was mostly looking for what
mistakes I shouldn't make in future conversions.
> > + OPT_BOOLEAN('q', "quiet", &quiet, "fetch silently"),
>
> there is an OPT__QUIET(&quiet) for this one.
>
> > + i = 1;
> > if (i < argc) {
> > int j = 0;
> > refs = xcalloc(argc - i + 1, sizeof(const char *));
>
> this is wrong, you meant i = 0, and frankly, it's better to just strip
> i altogether.
I didn't consume the remote name's slot, and started at the next one. But
you're right that it's probably nicest to do to *argv++, argc-- thing and
be zero-based for the list afterwards. I think I need the 'i' in this
case, because the names get somewhat converted from the exact list given.
-Daniel
*This .sig left intentionally blank*
^ permalink raw reply
* Re: [PATCH] Use parseopts in builtin-fetch
From: Kristian Høgsberg @ 2007-11-05 17:02 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Daniel Barkalow, Junio C Hamano, git
In-Reply-To: <20071105085513.GB25574@artemis.corp>
On Mon, 2007-11-05 at 09:55 +0100, Pierre Habouzit wrote:
> On Mon, Nov 05, 2007 at 03:35:34AM +0000, Daniel Barkalow wrote:
> > I mostly did this and the next one for practice with the API. I'm
> > impressed that "git fetch -vv" is even handled correctly without anything
> > special.
>
> About that: OPTION_BOOLEAN increments the associated variable, to
> support this case specifically.
>
> The last thing that really miss in parse-options is a way to recurse
> into a sub-array of struct option, to be able to port the generic diff
> and revision arguments.
>
> Though, there is a difficulty here that I've not yet found how to
> circumvent tastefully: right now options take an absolute pointer to
> _the_ variable that will be filled with values. I need to be able to
> relocate such a structure for sub-arrays for quite obvious reasons, and
> that is quite hard to achieve without hazardous APIs. I currently lean
> in the direction of simply memdup-ing the array and do fix-ups on
> *values pointers. Though how to do that in a graceful way is not obvious
> to me yet :)
What about requiring sub-arrays entries to take a pointer to a struct as
their 'value' and requiring that all value pointers in the sub-array be
relative to this struct, ie
(void *) offsetof(struct diff_options, detect_rename)
Then you can have something like
OPT__SUBARRAY(diff_options, &rev.diff_opt);
in your options array and it will fill out the specified my_diff_opts.
cheers,
Kristian
^ permalink raw reply
* Re: [PATCH 2/3] parseopt: introduce OPT_RECURSE to specify shared options
From: Pierre Habouzit @ 2007-11-05 16:53 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Linus Torvalds, git, Junio C Hamano
In-Reply-To: <Pine.LNX.4.64.0711051623450.4362@racer.site>
[-- Attachment #1: Type: text/plain, Size: 2297 bytes --]
On Mon, Nov 05, 2007 at 04:29:43PM +0000, Johannes Schindelin wrote:
> Hi,
>
> On Mon, 5 Nov 2007, Linus Torvalds wrote:
>
> > On Mon, 5 Nov 2007, Johannes Schindelin wrote:
> > >
> > > After kicking this around a bit more on IRC, we had another idea.
> > > Instead of introducing OPT_RECURSE(), do something like OPT__QUIET(),
> > > only this time in diff.h: ....
> >
> > I think the preprocessor approach would tend to be simpler, which is an
> > advantage. But whichever approach is chosen, I think one important issue
> > is to make sure that options that *hide* other options are correctly
> > handled in the help printout..
>
> Yep. See my patch 3/3, which just used a char[256] for the short names,
> and a path-list for the long names.
>
> > But that's an implementation issue. The same certainly *can* be done
> > with a recursive setup, just passing a linked list of what the earlier
> > levels were (which is what we do in other places). And it's not like the
> > recursion is going to be very deep or complex.
>
> Exactly.
>
> The more pressing issue is that we have pointers in the option structure,
> which point back to the variables expected to hold the option values.
>
> The recurse approach would need fixing up those (or some ugly copying of
> a struct diff_options).
>
> But the preprocessor approach means wasting space (since we basically have
> the same options in different builtins),
The "lost" space is the number of options x sizeof(struct option), the
latter being (if I'm correct):
on i386: 9 * 4 = 36 octets
on amd64: 4 x 2 + 8 * 4 + 8 (padding) + 8 * 2 = 64 octets.
It's not even near being an issue :)
> and it means that the callback
> functions needed to parse e.g. the diff colour names need to be public.
> Which is not the worst thing, of course.
Well it's certainly less ugly than copying the diff_options or
reseting it or anything like that. I don't care if we need to make a
couple of opt-parsing function public more than what we could have
needed.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] git-svn: fix dcommit clobbering when committing a series of diffs
From: Eric Wong @ 2007-11-05 16:40 UTC (permalink / raw)
To: Peter Baumann; +Cc: Junio C Hamano, git
In-Reply-To: <20071105142254.GA20277@xp.machine.xx>
Peter Baumann <waste.manager@gmx.de> wrote:
> On Mon, Nov 05, 2007 at 03:21:47AM -0800, Eric Wong wrote:
> > +
> > +test_expect_success 'unrelated some unrelated changes to git' "
>
> The first unrelated seems odd here.
Oops, must have been a brain glitch. Good catch, thanks.
--
Eric Wong
^ permalink raw reply
* Re: [PATCH 2/3] parseopt: introduce OPT_RECURSE to specify shared options
From: Johannes Schindelin @ 2007-11-05 16:29 UTC (permalink / raw)
To: Linus Torvalds; +Cc: git, Junio C Hamano, Pierre Habouzit
In-Reply-To: <alpine.LFD.0.999.0711050755340.15101@woody.linux-foundation.org>
Hi,
On Mon, 5 Nov 2007, Linus Torvalds wrote:
> On Mon, 5 Nov 2007, Johannes Schindelin wrote:
> >
> > After kicking this around a bit more on IRC, we had another idea.
> > Instead of introducing OPT_RECURSE(), do something like OPT__QUIET(),
> > only this time in diff.h: ....
>
> I think the preprocessor approach would tend to be simpler, which is an
> advantage. But whichever approach is chosen, I think one important issue
> is to make sure that options that *hide* other options are correctly
> handled in the help printout..
Yep. See my patch 3/3, which just used a char[256] for the short names,
and a path-list for the long names.
> But that's an implementation issue. The same certainly *can* be done
> with a recursive setup, just passing a linked list of what the earlier
> levels were (which is what we do in other places). And it's not like the
> recursion is going to be very deep or complex.
Exactly.
The more pressing issue is that we have pointers in the option structure,
which point back to the variables expected to hold the option values.
The recurse approach would need fixing up those (or some ugly copying of
a struct diff_options).
But the preprocessor approach means wasting space (since we basically have
the same options in different builtins), and it means that the callback
functions needed to parse e.g. the diff colour names need to be public.
Which is not the worst thing, of course.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 2/3] parseopt: introduce OPT_RECURSE to specify shared options
From: Linus Torvalds @ 2007-11-05 16:15 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Pierre Habouzit
In-Reply-To: <Pine.LNX.4.64.0711051340490.4362@racer.site>
On Mon, 5 Nov 2007, Johannes Schindelin wrote:
>
> After kicking this around a bit more on IRC, we had another idea. Instead
> of introducing OPT_RECURSE(), do something like OPT__QUIET(), only this
> time in diff.h: ....
I think the preprocessor approach would tend to be simpler, which is an
advantage. But whichever approach is chosen, I think one important issue
is to make sure that options that *hide* other options are correctly
handled in the help printout..
We have a few cases where a "recursive" option is hidden. For example, the
option "-n" can mean different things in different contexts: in
git-format-patch, for example, the "-n" is handled *before* calling
setup_revisions() and allt he normal revision flags, which means that the
format-patch -specific meaning of "-n" overrides the normal "revision"
meaning.
I suspect that in this kind of situation, it's actually easier to have a
single linear array of options, because the option parser can just walk
back and check "oh, I already saw this short option, so I should not
output it as documentation because this version of it is hidden by the
earlier one".
But that's an implementation issue. The same certainly *can* be done with
a recursive setup, just passing a linked list of what the earlier levels
were (which is what we do in other places). And it's not like the
recursion is going to be very deep or complex.
Linus
^ permalink raw reply
* Re: [PATCH] Implement selectable group ownership in git-init
From: Wincent Colaiuta @ 2007-11-05 15:26 UTC (permalink / raw)
To: Francesco Pretto; +Cc: Junio C Hamano, git
In-Reply-To: <472F3212.4090800@gmail.com>
El 5/11/2007, a las 16:09, Francesco Pretto escribió:
> Wincent Colaiuta ha scritto:
>>
>> using the administration tools designed for managing access, just
>> like
>> every other SCM (and every server, and every piece of software
>> which can
>> be accessed by many on a multi-user system).
>>
>
> I don't agree in general: in SCMs and other multi-user softwares,
> the access
> control configuration can be safely postponed just because it's in
> their
> standard usage pattern that the access should be conditioned by a
> daemon
> to be configured later. It's not the case of git, just because git
> is very
> tied to *nix permissions.
> But as it is now, it could seems that it's good to put committers in
> the (for
> example) git group, just because you have a git administrative account
> git:git . This is caused, imo, by the fact that the flow of creating
> a shared
> repository for a specific work/project group with git-init run by an
> administrative user (as it should be) is something like this:
>
> - Do it wrong;
> - Fix it immediately.
>
> I don't like the "Do it wrong" part. I'm trying to produce a sane and
> transparent patch to implement the selectable group just in case of
> repository
> first initialization. Why do I care so much of first time users?
> Dunno, but
> I think it's important.
What's stopping you from using "sudo -u"?
Wincent
^ permalink raw reply
* Re: [PATCH] Implement selectable group ownership in git-init
From: Francesco Pretto @ 2007-11-05 15:09 UTC (permalink / raw)
To: Wincent Colaiuta; +Cc: Junio C Hamano, git
In-Reply-To: <8EF5148D-C1F0-4329-A221-82D0B7E9932C@wincent.com>
Wincent Colaiuta ha scritto:
>
> using the administration tools designed for managing access, just like
> every other SCM (and every server, and every piece of software which can
> be accessed by many on a multi-user system).
>
I don't agree in general: in SCMs and other multi-user softwares, the access
control configuration can be safely postponed just because it's in their
standard usage pattern that the access should be conditioned by a daemon
to be configured later. It's not the case of git, just because git is very
tied to *nix permissions.
But as it is now, it could seems that it's good to put committers in the (for
example) git group, just because you have a git administrative account
git:git . This is caused, imo, by the fact that the flow of creating a shared
repository for a specific work/project group with git-init run by an
administrative user (as it should be) is something like this:
- Do it wrong;
- Fix it immediately.
I don't like the "Do it wrong" part. I'm trying to produce a sane and
transparent patch to implement the selectable group just in case of repository
first initialization. Why do I care so much of first time users? Dunno, but
I think it's important.
^ permalink raw reply
* Re: [PATCH 1/2] git-svn: fix dcommit clobbering when committing a series of diffs
From: Peter Baumann @ 2007-11-05 14:22 UTC (permalink / raw)
To: Eric Wong; +Cc: Junio C Hamano, git
In-Reply-To: <1194261708-32256-1-git-send-email-normalperson@yhbt.net>
On Mon, Nov 05, 2007 at 03:21:47AM -0800, Eric Wong wrote:
> Our revision number sent to SVN is set to the last revision we
> committed if we've made any previous commits in a dcommit
> invocation.
>
> Although our SVN Editor code uses the delta of two (old) trees
> to generate information to send upstream, it'll still send
> complete resultant files upstream; even if the tree they're
> based against is out-of-date.
>
> The combination of sending a file that does not include the
> latest changes, but set with a revision number of a commit we
> just made will cause SVN to accept the resultant file even if it
> was generated against an old tree.
>
> More trouble was caused when fixing this because we were
> rebasing uncessarily at times. We used git-diff-tree to check
> the imported SVN revision against our HEAD, not the last tree we
> committed to SVN. The unnecessary rebasing caused merge commits
> upstream to SVN to fail.
>
> Signed-off-by: Eric Wong <normalperson@yhbt.net>
> ---
>
[...]
> diff --git a/t/t9106-git-svn-dcommit-clobber-series.sh b/t/t9106-git-svn-dcommit-clobber-series.sh
> new file mode 100755
> index 0000000..4216a88
> --- /dev/null
> +++ b/t/t9106-git-svn-dcommit-clobber-series.sh
> @@ -0,0 +1,56 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2007 Eric Wong
> +test_description='git-svn dcommit clobber series'
> +. ./lib-git-svn.sh
> +
> +test_expect_success 'initialize repo' "
> + mkdir import &&
> + cd import &&
> + awk 'BEGIN { for (i = 1; i < 64; i++) { print i } }' > file
> + svn import -m 'initial' . $svnrepo &&
> + cd .. &&
> + git svn init $svnrepo &&
> + git svn fetch &&
> + test -e file
> + "
> +
> +test_expect_success '(supposedly) non-conflicting change from SVN' "
> + test x\"\`sed -n -e 58p < file\`\" = x58 &&
> + test x\"\`sed -n -e 61p < file\`\" = x61 &&
> + svn co $svnrepo tmp &&
> + cd tmp &&
> + perl -i -p -e 's/^58\$/5588/' file &&
> + perl -i -p -e 's/^61\$/6611/' file &&
> + test x\"\`sed -n -e 58p < file\`\" = x5588 &&
> + test x\"\`sed -n -e 61p < file\`\" = x6611 &&
> + svn commit -m '58 => 5588, 61 => 6611' &&
> + cd ..
> + "
> +
> +test_expect_success 'unrelated some unrelated changes to git' "
The first unrelated seems odd here.
-Peter
> + echo hi > life &&
> + git update-index --add life &&
> + git commit -m hi-life &&
> + echo bye >> life &&
> + git commit -m bye-life life
> + "
> +
[...]
^ permalink raw reply
* Re: [PATCH] parseopt: introduce OPT_RECURSE to specify shared options
From: Pierre Habouzit @ 2007-11-05 13:53 UTC (permalink / raw)
To: Johannes Schindelin, git, gitster
In-Reply-To: <20071105123923.GC25574@artemis.corp>
[-- Attachment #1: Type: text/plain, Size: 1096 bytes --]
On Mon, Nov 05, 2007 at 12:39:23PM +0000, Pierre Habouzit wrote:
> I like the kind of code that I allow to write better (I tend to
> dislike big fat global variables), though it's obvious that Johannes
> patch is a lot simpler and I like that.
We discussed it further, and what came out is that instead of
supporting quite complicated recursion mechanisms (or even a non so
complicated one), we can just define an
OPT__DIFFOPTIONS(diffopts)/OPT__REVOPTIONS(rev) macro that would inline
the needed options.
That's an idea I had but dismissed. Though, maybe it's not _that_
ugly, is clearly simpler, and one can argue that it's in the logical
continuation of OPT__QUIET and friends. What do you think ?
If it's the road we decide to take, then my documentation patch (2/4),
the parseopt fix (my 1/4 or johannes 1/3) and Johannes usage generator
enhancement (his 3/3) are still to be taken.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: [PATCH] Implement selectable group ownership in git-init
From: Wincent Colaiuta @ 2007-11-05 13:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Francesco Pretto, git
In-Reply-To: <7vabpvx8uu.fsf@gitster.siamese.dyndns.org>
El 3/11/2007, a las 23:27, Junio C Hamano escribió:
> I think what the patch attempts to achieve may be good, but only
> to reduce a few keystrokes of doing the "chgrp". Is it really
> worth it, I have to wonder...
I think this proposal adds unnecessary clutter to the codebase for
something that can easily be achieved (and *should*) using chown,
chgrp, or "sudo -u" etc.
The permissions of your Git installation and your repositories are
completely outside of the domain of Git itself, and should be
controlled using the administration tools designed for managing
access, just like every other SCM (and every server, and every piece
of software which can be accessed by many on a multi-user system).
Cheers,
Wincent
^ permalink raw reply
* Re: [PATCH 2/3] parseopt: introduce OPT_RECURSE to specify shared options
From: Johannes Schindelin @ 2007-11-05 13:46 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Pierre Habouzit
In-Reply-To: <Pine.LNX.4.64.0711051315300.4362@racer.site>
Hi,
On Mon, 5 Nov 2007, Johannes Schindelin wrote:
> The diff options should not need to be defined in every user of the
> diffcore. This provides the framework:
>
> extern struct option *diff_options;
>
> struct option options[] = {
> OPT_RECURSE(diff_options),
> ...
> OPT_END(),
> };
After kicking this around a bit more on IRC, we had another idea. Instead
of introducing OPT_RECURSE(), do something like OPT__QUIET(), only this
time in diff.h:
#define OPT__DIFF(opt) \
OPT_BOOLEAN('p', NULL, &opt.format_patch, "show a patch"), \
...
Pierre said this feels a bit "80s", so I'd like to hear other people's
opinions.
Hmm?
Ciao,
Dscho
^ permalink raw reply
* [PATCH 3/3] parseopt: do not list options with the same name twice
From: Johannes Schindelin @ 2007-11-05 13:15 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Junio C Hamano, git
In-Reply-To: <Pine.LNX.4.64.0711051237420.4362@racer.site>
The option parser will take the first exact match, and happily ignore
it when a later option has the same name. For example, when you have
something like
OPT_BOOLEAN('i', NULL, &troz, "blah"),
OPT_BOOLEAN('i', NULL, &brain, "blub"),
in your options array, a command line "prog -i" will increment the
variable "troz", and leave the variable "brain" alone.
This behavior is all good and well, especially since we plan to
have recursive options, e.g. for the diff options, and in some cases
options should be overridden (see for example format-patch's "-n").
This patch matches the usage to this behavior: whenever an option name
was overridden by another option, it is no longer shown. In the
example above, that means that the usage would only show
-i blah
and not print anything about "blub".
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Okay, my plan was borked: the options struct is passed as a
const. So this is plan B.
parse-options.c | 37 ++++++++++++++++++++++++++-----------
test-parse-options.c | 2 ++
2 files changed, 28 insertions(+), 11 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index ed8e951..83a221b 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1,5 +1,6 @@
#include "git-compat-util.h"
#include "parse-options.h"
+#include "path-list.h"
#define OPT_SHORT 1
#define OPT_UNSET 2
@@ -261,15 +262,18 @@ int parse_options(int argc, const char **argv, const struct option *options,
#define USAGE_OPTS_WIDTH 24
#define USAGE_GAP 2
-static void usage_show_options(const struct option *opts)
+static void usage_show_options(const struct option *opts,
+ char *seen_short_names, struct path_list *seen_long_names)
{
for (; opts->type != OPTION_END; opts++) {
- size_t pos;
+ const char *before_option = " ";
+ size_t pos = 0;
int pad;
if (opts->type == OPTION_RECURSE) {
const struct option *sub = opts->value;
- usage_show_options(sub);
+ usage_show_options(sub,
+ seen_short_names, seen_long_names);
continue;
}
@@ -280,13 +284,19 @@ static void usage_show_options(const struct option *opts)
continue;
}
- pos = fprintf(stderr, " ");
- if (opts->short_name)
- pos += fprintf(stderr, "-%c", opts->short_name);
- if (opts->long_name && opts->short_name)
- pos += fprintf(stderr, ", ");
- if (opts->long_name)
- pos += fprintf(stderr, "--%s", opts->long_name);
+ if (opts->short_name && !seen_short_names[opts->short_name]) {
+ pos += fprintf(stderr, "%s-%c",
+ before_option, opts->short_name);
+ seen_short_names[opts->short_name] = 1;
+ before_option = ", ";
+ }
+ if (opts->long_name && !path_list_has_path(seen_long_names,
+ opts->long_name)) {
+ pos += fprintf(stderr, "%s--%s",
+ before_option, opts->long_name);
+ path_list_insert(opts->long_name, seen_long_names);
+ } else if (*before_option != ',')
+ continue;
switch (opts->type) {
case OPTION_INTEGER:
@@ -329,6 +339,9 @@ static void usage_show_options(const struct option *opts)
void usage_with_options(const char * const *usagestr,
const struct option *opts)
{
+ char seen_short_names[256];
+ struct path_list seen_long_names = { NULL, 0, 0, 0 };
+
fprintf(stderr, "usage: %s\n", *usagestr++);
while (*usagestr && **usagestr)
fprintf(stderr, " or: %s\n", *usagestr++);
@@ -338,7 +351,9 @@ void usage_with_options(const char * const *usagestr,
if (opts->type != OPTION_GROUP)
fputc('\n', stderr);
- usage_show_options(opts);
+ memset(seen_short_names, 0, sizeof(seen_short_names));
+ usage_show_options(opts, seen_short_names, &seen_long_names);
+ path_list_clear(&seen_long_names, 0);
fputc('\n', stderr);
diff --git a/test-parse-options.c b/test-parse-options.c
index ee64fb3..56f6f24 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -16,6 +16,7 @@ int main(int argc, const char **argv)
struct option sub[] = {
OPT_BOOLEAN('n', "narf", &narf, "what are we doing tonight?"),
OPT_INTEGER('z', "zort", &zort, "try to take over the world"),
+ OPT_INTEGER('j', NULL, &boolean, "ignored option"),
OPT_END(),
};
struct option options[] = {
@@ -27,6 +28,7 @@ int main(int argc, const char **argv)
OPT_STRING('s', "string", &string, "string", "get a string"),
OPT_STRING(0, "string2", &string, "str", "get another string"),
OPT_STRING(0, "st", &string, "st", "get another string (pervert ordering)"),
+ OPT_INTEGER(0, "string", &boolean, "ignored option"),
OPT_END(),
};
int i;
--
1.5.3.5.1549.g91a3
^ permalink raw reply related
* [PATCH 2/3] parseopt: introduce OPT_RECURSE to specify shared options
From: Johannes Schindelin @ 2007-11-05 13:15 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Junio C Hamano, git
In-Reply-To: <Pine.LNX.4.64.0711051237420.4362@racer.site>
The diff options should not need to be defined in every user of the
diffcore. This provides the framework:
extern struct option *diff_options;
struct option options[] = {
OPT_RECURSE(diff_options),
...
OPT_END(),
};
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
parse-options.c | 68 +++++++++++++++++++++++++++++++++++-----------
parse-options.h | 2 +
t/t0040-parse-options.sh | 26 +++++++++++++++++
test-parse-options.c | 10 +++++++
4 files changed, 90 insertions(+), 16 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 15b32f7..ed8e951 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -4,6 +4,8 @@
#define OPT_SHORT 1
#define OPT_UNSET 2
+#define OPT_NOT_FOUND -2
+
struct optparse_t {
const char **argv;
int argc;
@@ -111,8 +113,14 @@ static int parse_short_opt(struct optparse_t *p, const struct option *options)
p->opt = p->opt[1] ? p->opt + 1 : NULL;
return get_value(p, options, OPT_SHORT);
}
+ if (options->type == OPTION_RECURSE) {
+ const struct option *sub = options->value;
+ int result = parse_short_opt(p, sub);
+ if (result != OPT_NOT_FOUND)
+ return result;
+ }
}
- return error("unknown switch `%c'", *p->opt);
+ return OPT_NOT_FOUND;
}
static int parse_long_opt(struct optparse_t *p, const char *arg,
@@ -129,6 +137,13 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,
const char *rest;
int flags = 0;
+ if (options->type == OPTION_RECURSE) {
+ const struct option *sub = options->value;
+ int result = parse_long_opt(p, arg, sub);
+ if (result != OPT_NOT_FOUND)
+ return result;
+ }
+
if (!options->long_name)
continue;
@@ -187,14 +202,14 @@ is_abbreviated:
abbrev_option->long_name);
if (abbrev_option)
return get_value(p, abbrev_option, abbrev_flags);
- return error("unknown option `%s'", arg);
+ return OPT_NOT_FOUND;
}
int parse_options(int argc, const char **argv, const struct option *options,
const char * const usagestr[], int flags)
{
struct optparse_t args = { argv + 1, argc - 1, NULL };
- int j = 0;
+ int j = 0, result;
for (; args.argc; args.argc--, args.argv++) {
const char *arg = args.argv[0];
@@ -209,8 +224,13 @@ int parse_options(int argc, const char **argv, const struct option *options,
do {
if (*args.opt == 'h')
usage_with_options(usagestr, options);
- if (parse_short_opt(&args, options) < 0)
+ result = parse_short_opt(&args, options);
+ if (result < 0) {
+ if (result == OPT_NOT_FOUND)
+ error("unknown switch `%c'",
+ arg[1]);
usage_with_options(usagestr, options);
+ }
} while (args.opt);
continue;
}
@@ -225,8 +245,12 @@ int parse_options(int argc, const char **argv, const struct option *options,
if (!strcmp(arg + 2, "help"))
usage_with_options(usagestr, options);
- if (parse_long_opt(&args, arg + 2, options))
+ result = parse_long_opt(&args, arg + 2, options);
+ if (result) {
+ if (result == OPT_NOT_FOUND)
+ error("unknown option `%s'", arg + 2);
usage_with_options(usagestr, options);
+ }
}
memmove(argv + j, args.argv, args.argc * sizeof(*argv));
@@ -237,22 +261,18 @@ int parse_options(int argc, const char **argv, const struct option *options,
#define USAGE_OPTS_WIDTH 24
#define USAGE_GAP 2
-void usage_with_options(const char * const *usagestr,
- const struct option *opts)
+static void usage_show_options(const struct option *opts)
{
- fprintf(stderr, "usage: %s\n", *usagestr++);
- while (*usagestr && **usagestr)
- fprintf(stderr, " or: %s\n", *usagestr++);
- while (*usagestr)
- fprintf(stderr, " %s\n", *usagestr++);
-
- if (opts->type != OPTION_GROUP)
- fputc('\n', stderr);
-
for (; opts->type != OPTION_END; opts++) {
size_t pos;
int pad;
+ if (opts->type == OPTION_RECURSE) {
+ const struct option *sub = opts->value;
+ usage_show_options(sub);
+ continue;
+ }
+
if (opts->type == OPTION_GROUP) {
fputc('\n', stderr);
if (*opts->help)
@@ -304,6 +324,22 @@ void usage_with_options(const char * const *usagestr,
}
fprintf(stderr, "%*s%s\n", pad + USAGE_GAP, "", opts->help);
}
+}
+
+void usage_with_options(const char * const *usagestr,
+ const struct option *opts)
+{
+ fprintf(stderr, "usage: %s\n", *usagestr++);
+ while (*usagestr && **usagestr)
+ fprintf(stderr, " or: %s\n", *usagestr++);
+ while (*usagestr)
+ fprintf(stderr, " %s\n", *usagestr++);
+
+ if (opts->type != OPTION_GROUP)
+ fputc('\n', stderr);
+
+ usage_show_options(opts);
+
fputc('\n', stderr);
exit(129);
diff --git a/parse-options.h b/parse-options.h
index 3a470e5..9ec1e35 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -8,6 +8,7 @@ enum parse_opt_type {
OPTION_STRING,
OPTION_INTEGER,
OPTION_CALLBACK,
+ OPTION_RECURSE,
};
enum parse_opt_flags {
@@ -44,6 +45,7 @@ struct option {
#define OPT_STRING(s, l, v, a, h) { OPTION_STRING, (s), (l), (v), (a), (h) }
#define OPT_CALLBACK(s, l, v, a, h, f) \
{ OPTION_CALLBACK, (s), (l), (v), (a), (h), 0, (f) }
+#define OPT_RECURSE(options) { OPTION_RECURSE, 0, NULL, options }
/* parse_options() will filter out the processed options and leave the
* non-option argments in argv[].
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 462fdf2..9c3efaa 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -13,6 +13,8 @@ usage: test-parse-options <options>
-b, --boolean get a boolean
-i, --integer <n> get a integer
-j <n> get a integer, too
+ -n, --narf what are we doing tonight?
+ -z, --zort <n> try to take over the world
string options
-s, --string <string>
@@ -29,6 +31,8 @@ test_expect_success 'test help' '
'
cat > expect << EOF
+narf: 0
+zort: 1
boolean: 2
integer: 1729
string: 123
@@ -40,6 +44,8 @@ test_expect_success 'short options' '
test ! -s output.err
'
cat > expect << EOF
+narf: 0
+zort: 1
boolean: 2
integer: 1729
string: 321
@@ -53,6 +59,8 @@ test_expect_success 'long options' '
'
cat > expect << EOF
+narf: 0
+zort: 1
boolean: 1
integer: 13
string: 123
@@ -69,6 +77,8 @@ test_expect_success 'intermingled arguments' '
'
cat > expect << EOF
+narf: 0
+zort: 1
boolean: 0
integer: 2
string: (not set)
@@ -92,6 +102,22 @@ test_expect_failure 'ambiguously abbreviated option' '
'
cat > expect << EOF
+narf: 1
+zort: 5
+boolean: 0
+integer: 3
+string: (not set)
+EOF
+
+test_expect_success 'recurse works' '
+ test-parse-options --narf -z5 --int=3 > output 2> output.err &&
+ test ! -s output.err &&
+ git diff expect output
+'
+
+cat > expect << EOF
+narf: 0
+zort: 1
boolean: 0
integer: 0
string: 123
diff --git a/test-parse-options.c b/test-parse-options.c
index 4d3e2ec..ee64fb3 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -1,6 +1,8 @@
#include "cache.h"
#include "parse-options.h"
+static int narf = 0;
+static int zort = 1;
static int boolean = 0;
static int integer = 0;
static char *string = NULL;
@@ -11,10 +13,16 @@ int main(int argc, const char **argv)
"test-parse-options <options>",
NULL
};
+ struct option sub[] = {
+ OPT_BOOLEAN('n', "narf", &narf, "what are we doing tonight?"),
+ OPT_INTEGER('z', "zort", &zort, "try to take over the world"),
+ OPT_END(),
+ };
struct option options[] = {
OPT_BOOLEAN('b', "boolean", &boolean, "get a boolean"),
OPT_INTEGER('i', "integer", &integer, "get a integer"),
OPT_INTEGER('j', NULL, &integer, "get a integer, too"),
+ OPT_RECURSE(sub),
OPT_GROUP("string options"),
OPT_STRING('s', "string", &string, "string", "get a string"),
OPT_STRING(0, "string2", &string, "str", "get another string"),
@@ -25,6 +33,8 @@ int main(int argc, const char **argv)
argc = parse_options(argc, argv, options, usage, 0);
+ printf("narf: %d\n", narf);
+ printf("zort: %d\n", zort);
printf("boolean: %d\n", boolean);
printf("integer: %d\n", integer);
printf("string: %s\n", string ? string : "(not set)");
--
1.5.3.5.1549.g91a3
^ permalink raw reply related
* [PATCH 1/3] parse-options: abbreviation engine fix.
From: Johannes Schindelin @ 2007-11-05 13:15 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Junio C Hamano, git
In-Reply-To: <Pine.LNX.4.64.0711051237420.4362@racer.site>
When an option could be an ambiguous abbreviation of two options, the code
used to error out. Even if an exact match would have occured later.
Test and original patch by Pierre Habouzit.
Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
parse-options.c | 33 +++++++++++++++++++++------------
t/t0040-parse-options.sh | 13 +++++++++++++
test-parse-options.c | 1 +
3 files changed, 35 insertions(+), 12 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index cc09c98..15b32f7 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -119,8 +119,8 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,
const struct option *options)
{
const char *arg_end = strchr(arg, '=');
- const struct option *abbrev_option = NULL;
- int abbrev_flags = 0;
+ const struct option *abbrev_option = NULL, *ambiguous_option = NULL;
+ int abbrev_flags = 0, ambiguous_flags = 0;
if (!arg_end)
arg_end = arg + strlen(arg);
@@ -137,16 +137,16 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,
/* abbreviated? */
if (!strncmp(options->long_name, arg, arg_end - arg)) {
is_abbreviated:
- if (abbrev_option)
- return error("Ambiguous option: %s "
- "(could be --%s%s or --%s%s)",
- arg,
- (flags & OPT_UNSET) ?
- "no-" : "",
- options->long_name,
- (abbrev_flags & OPT_UNSET) ?
- "no-" : "",
- abbrev_option->long_name);
+ if (abbrev_option) {
+ /*
+ * If this is abbreviated, it is
+ * ambiguous. So when there is no
+ * exact match later, we need to
+ * error out.
+ */
+ ambiguous_option = abbrev_option;
+ ambiguous_flags = abbrev_flags;
+ }
if (!(flags & OPT_UNSET) && *arg_end)
p->opt = arg_end + 1;
abbrev_option = options;
@@ -176,6 +176,15 @@ is_abbreviated:
}
return get_value(p, options, flags);
}
+
+ if (ambiguous_option)
+ return error("Ambiguous option: %s "
+ "(could be --%s%s or --%s%s)",
+ arg,
+ (ambiguous_flags & OPT_UNSET) ? "no-" : "",
+ ambiguous_option->long_name,
+ (abbrev_flags & OPT_UNSET) ? "no-" : "",
+ abbrev_option->long_name);
if (abbrev_option)
return get_value(p, abbrev_option, abbrev_flags);
return error("unknown option `%s'", arg);
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index ae49424..462fdf2 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -18,6 +18,7 @@ string options
-s, --string <string>
get a string
--string2 <str> get another string
+ --st <st> get another string (pervert ordering)
EOF
@@ -90,4 +91,16 @@ test_expect_failure 'ambiguously abbreviated option' '
test $? != 129
'
+cat > expect << EOF
+boolean: 0
+integer: 0
+string: 123
+EOF
+
+test_expect_success 'non ambiguous option (after two options it abbreviates)' '
+ test-parse-options --st 123 > output 2> output.err &&
+ test ! -s output.err &&
+ git diff expect output
+'
+
test_done
diff --git a/test-parse-options.c b/test-parse-options.c
index 277cfe4..4d3e2ec 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -18,6 +18,7 @@ int main(int argc, const char **argv)
OPT_GROUP("string options"),
OPT_STRING('s', "string", &string, "string", "get a string"),
OPT_STRING(0, "string2", &string, "str", "get another string"),
+ OPT_STRING(0, "st", &string, "st", "get another string (pervert ordering)"),
OPT_END(),
};
int i;
--
1.5.3.5.1549.g91a3
^ permalink raw reply related
* Re: [StGit PATCH] Cogito is deprecated, so don't point to it
From: Karl Hasselström @ 2007-11-05 13:11 UTC (permalink / raw)
To: David Kågedal; +Cc: Matthieu Moy, git, Catalin Marinas
In-Reply-To: <87zlxsannq.fsf@lysator.liu.se>
On 2007-11-05 13:21:13 +0100, David Kågedal wrote:
> Don't forget your new assimilate implementation.
That's in Catalin's master branch already, so there can't possibly be
a release that has this patch and not the assimilate improvements. But
yes, that's what makes it possible to easily recover from just about
anything the git commands can do.
--
Karl Hasselström, kha@treskal.com
www.treskal.com/kalle
^ permalink raw reply
* Re: [PATCH] errors: "strict subset" -> "ancestor"
From: Wincent Colaiuta @ 2007-11-05 13:06 UTC (permalink / raw)
To: David Symonds; +Cc: Steffen Prohaska, J. Bruce Fields, Junio C Hamano, git
In-Reply-To: <ee77f5c20711030014x23ac6206rec81fe5968992147@mail.gmail.com>
El 3/11/2007, a las 8:14, David Symonds escribió:
> On 11/3/07, Steffen Prohaska <prohaska@zib.de> wrote:
>>
>> On Nov 3, 2007, at 3:39 AM, J. Bruce Fields wrote:
>>
>>> diff --git a/send-pack.c b/send-pack.c
>>> index 5e127a1..b74fd45 100644
>>> --- a/send-pack.c
>>> +++ b/send-pack.c
>>> @@ -297,9 +297,9 @@ static int send_pack(int in, int out, struct
>>> remote *remote, int nr_refspec, cha
>>> * commits at the remote end and likely
>>> * we were not up to date to begin
>>> with.
>>> */
>>> - error("remote '%s' is not a strict "
>>> - "subset of local ref '%s'. "
>>> - "maybe you are not up-to-date
>>> and "
>>> + error("remote '%s' is not an
>>> ancestor of\n"
>>> + " local '%s'.\n"
>>
>> Two spaces in a row after local and before '%s'.
>
> So? That's presumably to align the remote and local strings.
Kind of: it aligns "error:" with "local":
error: remote 'refs/heads/master' is not an ancestor of
local 'refs/heads/master'.
Personal I think it would be better to align the right edges of
"remote" and "local" so that it looks like the following; this more
clearly shows the correspondence between the remote and local refs:
error: remote 'refs/heads/master' is not an ancestor of
local 'refs/heads/master'.
Or alternatively:
error: remote 'refs/heads/master' is not an ancestor of
local 'refs/heads/master'.
Cheers,
Wincent
^ permalink raw reply
* Re: [PATCH] parse-options: abbreviation engine fix.
From: Pierre Habouzit @ 2007-11-05 12:59 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <Pine.LNX.4.64.0711051230020.4362@racer.site>
[-- Attachment #1: Type: text/plain, Size: 1689 bytes --]
On lun, nov 05, 2007 at 12:34:00 +0000, Johannes Schindelin wrote:
>
> When an option could be an ambiguous abbreviation of two options, the code
> used to error out. Even if an exact match would have occured later.
>
> Test and original patch by Pierre Habouzit.
>
> Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> ---
>
> On Mon, 5 Nov 2007, Pierre Habouzit wrote:
>
> > When we had at least two long option then followed by another
> > one that was a prefix of both of them, then the abbreviation
> > detector failed.
>
> Yeah, I assumed that you would never do such a thing, but I agree
> that with recursing option parsing it is much more likely.
>
> It took me some time to understand your patch, and that the moving
> of the UNSET handling was unnecessary.
yeah, that's because I dislike where it's done. Each time I read it,
I'm under the impression that it clobbers p->opt if this case is not the
one used.
In fact, I believe that for code clarity we should do what you
proposed earlier: disallow `=` in option names (that's rather sane
anyway) and drop the `rest` variable altogether, just use your current
arg_end and prefixcmp/strncmps. That will incidentally also allow to
remove skip_prefix while we're at it.
This way we could set p->opt first thing in the function and be done
with it instead of doing it two different ways in the function, hence
making the reader assume one is wrong or at least questionable.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: [PATCH] git-mailsplit: with maildirs try to process new/ if cur/ is empty
From: Jakub Narebski @ 2007-11-05 12:58 UTC (permalink / raw)
To: git
In-Reply-To: <20071105124920.17726.qmail@746e9cce42b49f.315fe32.mid.smarden.org>
[Cc: Gerrit Pape <pape@smarden.org>, git@vger.kernel.org]
Gerrit Pape wrote:
> + char *sub[] = { "cur", "new" };
[...]
> + for (i = 0; i < 2; ++i) {
Wouldn't it be better to use sizeof(sub)/sizeof(sub[0]) or it's macro
equivalent ARRAY_SIZE(sub) instead of hardcoding 2 to avoid errors?
--
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
^ permalink raw reply
* [PATCH] Add support for Apple Mail's Maildir format to builtin-mailsplit.
From: Michael J. Cohen @ 2007-11-05 12:57 UTC (permalink / raw)
To: Git Mailing List
Add support for Apple Mail's Maildir format to builtin-mailsplit.
Currently, mailsplit only checks for the directory 'cur' inside of the
specified directory, whereas Apple Mail's Maildirs have a Messages
directory.
Signed-off-by: Michael Cohen <mjc@kernel.org>
---
Another patch will follow to allow builtin-mailinfo to parse Apple
Mail's extra header and footers.
builtin-mailsplit.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/builtin-mailsplit.c b/builtin-mailsplit.c
index 74b0470..6256351 100644
--- a/builtin-mailsplit.c
+++ b/builtin-mailsplit.c
@@ -129,8 +129,11 @@ static int split_maildir(const char *maildir,
const char *dir,
struct path_list list = {NULL, 0, 0, 1};
snprintf(curdir, sizeof(curdir), "%s/cur", maildir);
- if (populate_maildir_list(&list, curdir) < 0)
- goto out;
+ if (populate_maildir_list(&list, curdir) < 0) {
+ snprintf(curdir, sizeof(curdir), "%s/Messages", maildir);
+ if (populate_maildir_list(&list, curdir) < 0)
+ goto out;
+ }
for (i = 0; i < list.nr; i++) {
FILE *f;
--
1.5.3.5.562.g45f4-dirty
^ permalink raw reply related
* [PATCH] git-mailsplit: with maildirs try to process new/ if cur/ is empty
From: Gerrit Pape @ 2007-11-05 12:49 UTC (permalink / raw)
To: Fernando J. Pereda, git, Junio C Hamano
In-Reply-To: <20071026160118.GA5076@ferdyx.org>
When saving patches to a maildir with e.g. mutt, the files are put into
the new/ subdirectory of the maildir, not cur/. This makes git-am state
"Nothing to do.". This patch lets git-mailsplit additional check new/
after reading cur/.
This was reported by Joey Hess through
http://bugs.debian.org/447396
Signed-off-by: Gerrit Pape <pape@smarden.org>
---
On Fri, Oct 26, 2007 at 06:01:18PM +0200, Fernando J. Pereda wrote:
> By that reasoning, you should make it parse both cur/ and new/.
Okay.
builtin-mailsplit.c | 36 ++++++++++++++++++++----------------
1 files changed, 20 insertions(+), 16 deletions(-)
diff --git a/builtin-mailsplit.c b/builtin-mailsplit.c
index 74b0470..79e8ee0 100644
--- a/builtin-mailsplit.c
+++ b/builtin-mailsplit.c
@@ -101,19 +101,26 @@ static int populate_maildir_list(struct path_list *list, const char *path)
{
DIR *dir;
struct dirent *dent;
+ char name[PATH_MAX];
+ char *sub[] = { "cur", "new" };
+ int i;
- if ((dir = opendir(path)) == NULL) {
- error("cannot opendir %s (%s)", path, strerror(errno));
- return -1;
- }
+ for (i = 0; i < 2; ++i) {
+ snprintf(name, sizeof(name), "%s/%s", path, sub[i]);
+ if ((dir = opendir(name)) == NULL) {
+ error("cannot opendir %s (%s)", name, strerror(errno));
+ return -1;
+ }
- while ((dent = readdir(dir)) != NULL) {
- if (dent->d_name[0] == '.')
- continue;
- path_list_insert(dent->d_name, list);
- }
+ while ((dent = readdir(dir)) != NULL) {
+ if (dent->d_name[0] == '.')
+ continue;
+ snprintf(name, sizeof(name), "%s/%s", sub[i], dent->d_name);
+ path_list_insert(name, list);
+ }
- closedir(dir);
+ closedir(dir);
+ }
return 0;
}
@@ -122,19 +129,17 @@ static int split_maildir(const char *maildir, const char *dir,
int nr_prec, int skip)
{
char file[PATH_MAX];
- char curdir[PATH_MAX];
char name[PATH_MAX];
int ret = -1;
int i;
struct path_list list = {NULL, 0, 0, 1};
- snprintf(curdir, sizeof(curdir), "%s/cur", maildir);
- if (populate_maildir_list(&list, curdir) < 0)
+ if (populate_maildir_list(&list, maildir) < 0)
goto out;
for (i = 0; i < list.nr; i++) {
FILE *f;
- snprintf(file, sizeof(file), "%s/%s", curdir, list.items[i].path);
+ snprintf(file, sizeof(file), "%s/%s", maildir, list.items[i].path);
f = fopen(file, "r");
if (!f) {
error("cannot open mail %s (%s)", file, strerror(errno));
@@ -152,10 +157,9 @@ static int split_maildir(const char *maildir, const char *dir,
fclose(f);
}
- path_list_clear(&list, 1);
-
ret = skip;
out:
+ path_list_clear(&list, 1);
return ret;
}
--
1.5.3.5
^ permalink raw reply related
* Re: [PATCH] parse-options: abbreviation engine fix.
From: Johannes Schindelin @ 2007-11-05 12:38 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Junio C Hamano, git
In-Reply-To: <Pine.LNX.4.64.0711051230020.4362@racer.site>
Hi,
On Mon, 5 Nov 2007, Johannes Schindelin wrote:
> +cat > expect << EOF
> +boolean: 0
> +integer: 2
> +string: 123
> +EOF
> +
> +test_expect_failure 'non ambiguous option (after two options it abbreviates)' '
> + test-parse-options --st 123 &&
> + test ! -s output.err &&
> + git diff expect output
> +'
> +
Aaargh! Yet another instance of test_expect_failure being wrong, wrong,
wrong.
I'll come up with a replacement patch.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] parseopt: introduce OPT_RECURSE to specify shared options
From: Pierre Habouzit @ 2007-11-05 12:39 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, gitster
In-Reply-To: <Pine.LNX.4.64.0711051209061.4362@racer.site>
[-- Attachment #1: Type: text/plain, Size: 2886 bytes --]
On Mon, Nov 05, 2007 at 12:09:41PM +0000, Johannes Schindelin wrote:
>
> The diff options should not need to be defined in every user of the
> diffcore. This provides the framework:
>
> extern struct option *diff_options;
>
> struct option options[] = {
> OPT_RECURSE(diff_options),
> ...
> OPT_END(),
> };
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>
> This is not yet clever enough to show the correct usage when
> some sub option uses the same short or long name as one that
> has already been printed.
>
> Add the superlevel options struct as another parameter to
> usage_show_options(), and write an unset_option() function
> which takes a short name, a long name, and that superlevel
> options struct, and unsets all options which match.
Alternatively you can have a 256-bit bitfield to mark short options
that have already been seen and a hashtable of long options already
printed out, and while outputting an option, one should check that
either the short option or the long option is new.
This is nicer as some of the struct option may live in .rodata
> parse-options.c | 68 +++++++++++++++++++++++++++++++++++-----------
> parse-options.h | 2 +
> t/t0040-parse-options.sh | 24 ++++++++++++++++
> test-parse-options.c | 10 +++++++
> 4 files changed, 88 insertions(+), 16 deletions(-)
Okay, we discussed this with Johannes on IRC. 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.
I like the kind of code that I allow to write better (I tend to
dislike big fat global variables), though it's obvious that Johannes
patch is a lot simpler and I like that.
[0] actually we don't *need* to remove the struct diff_options argument
from many functions except from diff_opt_parse, it's just that for
real, everybody will work on the same global structure anyway.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* [PATCH] parse-options: abbreviation engine fix.
From: Johannes Schindelin @ 2007-11-05 12:34 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Junio C Hamano, git
In-Reply-To: <1194264204-3475-2-git-send-email-madcoder@debian.org>
When an option could be an ambiguous abbreviation of two options, the code
used to error out. Even if an exact match would have occured later.
Test and original patch by Pierre Habouzit.
Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
On Mon, 5 Nov 2007, Pierre Habouzit wrote:
> When we had at least two long option then followed by another
> one that was a prefix of both of them, then the abbreviation
> detector failed.
Yeah, I assumed that you would never do such a thing, but I agree
that with recursing option parsing it is much more likely.
It took me some time to understand your patch, and that the moving
of the UNSET handling was unnecessary.
IMHO this patch is easier to read.
parse-options.c | 33 +++++++++++++++++++++------------
t/t0040-parse-options.sh | 13 +++++++++++++
test-parse-options.c | 1 +
3 files changed, 35 insertions(+), 12 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index cc09c98..15b32f7 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -119,8 +119,8 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,
const struct option *options)
{
const char *arg_end = strchr(arg, '=');
- const struct option *abbrev_option = NULL;
- int abbrev_flags = 0;
+ const struct option *abbrev_option = NULL, *ambiguous_option = NULL;
+ int abbrev_flags = 0, ambiguous_flags = 0;
if (!arg_end)
arg_end = arg + strlen(arg);
@@ -137,16 +137,16 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,
/* abbreviated? */
if (!strncmp(options->long_name, arg, arg_end - arg)) {
is_abbreviated:
- if (abbrev_option)
- return error("Ambiguous option: %s "
- "(could be --%s%s or --%s%s)",
- arg,
- (flags & OPT_UNSET) ?
- "no-" : "",
- options->long_name,
- (abbrev_flags & OPT_UNSET) ?
- "no-" : "",
- abbrev_option->long_name);
+ if (abbrev_option) {
+ /*
+ * If this is abbreviated, it is
+ * ambiguous. So when there is no
+ * exact match later, we need to
+ * error out.
+ */
+ ambiguous_option = abbrev_option;
+ ambiguous_flags = abbrev_flags;
+ }
if (!(flags & OPT_UNSET) && *arg_end)
p->opt = arg_end + 1;
abbrev_option = options;
@@ -176,6 +176,15 @@ is_abbreviated:
}
return get_value(p, options, flags);
}
+
+ if (ambiguous_option)
+ return error("Ambiguous option: %s "
+ "(could be --%s%s or --%s%s)",
+ arg,
+ (ambiguous_flags & OPT_UNSET) ? "no-" : "",
+ ambiguous_option->long_name,
+ (abbrev_flags & OPT_UNSET) ? "no-" : "",
+ abbrev_option->long_name);
if (abbrev_option)
return get_value(p, abbrev_option, abbrev_flags);
return error("unknown option `%s'", arg);
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index ae49424..ee758e5 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -18,6 +18,7 @@ string options
-s, --string <string>
get a string
--string2 <str> get another string
+ --st <st> get another string (pervert ordering)
EOF
@@ -90,4 +91,16 @@ test_expect_failure 'ambiguously abbreviated option' '
test $? != 129
'
+cat > expect << EOF
+boolean: 0
+integer: 2
+string: 123
+EOF
+
+test_expect_failure 'non ambiguous option (after two options it abbreviates)' '
+ test-parse-options --st 123 &&
+ test ! -s output.err &&
+ git diff expect output
+'
+
test_done
diff --git a/test-parse-options.c b/test-parse-options.c
index 277cfe4..4d3e2ec 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -18,6 +18,7 @@ int main(int argc, const char **argv)
OPT_GROUP("string options"),
OPT_STRING('s', "string", &string, "string", "get a string"),
OPT_STRING(0, "string2", &string, "str", "get another string"),
+ OPT_STRING(0, "st", &string, "st", "get another string (pervert ordering)"),
OPT_END(),
};
int i;
--
1.5.3.5.1549.g91a3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox