* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Junio C Hamano @ 2008-06-23 21:23 UTC (permalink / raw)
To: Pierre Habouzit
Cc: Linus Torvalds, Jeff King, Johannes Schindelin, Git Mailing List
In-Reply-To: <20080623210935.GC13395@artemis.madism.org>
Pierre Habouzit <madcoder@debian.org> writes:
> With that, you write parsers this way:
>
> {
> struct parse_opt_ctx_t ctx;
>
> parse_options_start(&ctx, argc, argv, 0);
>
> for (;;) {
> const char *arg;
>
> switch (parse_options_step(&ctx, options, usagestr)) {
> case PARSE_OPT_HELP:
> /* dump your help here, the one for options/usagestr is already dumped */
> exit(129);
> case PARSE_OPT_DONE:
> goto done;
> }
>
> arg = *ctx->argv++;
> ctx->argc--;
>
> if (strcmp(arg, "-")) {
> /* you're on baby ! */
> } else if ....
> } else {
> error("unknown option %s", arg);
> parse_options_usage(options, usagestr);
> /* dump your help here */
> exit(129);
> }
> }
>
> done:
> argc = parse_options_end(&ctx);
> }
Nice. I have started doing the same (insignificant details are different;
e.g. I used "positive is unknown" convention instead ) and then the
solution is sitting in my mbox ;-)
^ permalink raw reply
* Re: [PATCH v2/RFC] git.el: Commands for committing patches
From: Junio C Hamano @ 2008-06-23 21:27 UTC (permalink / raw)
To: Nikolaj Schumacher; +Cc: git, Alexandre Julliard
In-Reply-To: <m263s0vbri.fsf_-_@nschum.de>
Nikolaj Schumacher <n_schumacher@web.de> writes:
> From 609f6fca6c70919036d41e1e4034b6e4de2e7ea2 Mon Sep 17 00:00:00 2001
> From: Nikolaj Schumacher <git@nschum.de>
> Date: Mon, 23 Jun 2008 09:34:14 +0200
> Subject: [PATCH] git.el: Added command for committing patches.
>
> This adds commands for committing patches from files, buffers and email
> buffers.
>
> In order to minimize code duplication, git-start-log-edit and
> git-prepare-log-buffer have been extracted from git-commit-file.
>
> Signed-off-by: Nikolaj Schumacher <git@nschum.de>
> ---
> contrib/emacs/git.el | 200 +++++++++++++++++++++++++++++++++++++++++++------
> 1 files changed, 175 insertions(+), 25 deletions(-)
I do not use things in contrib/emacs/git.el myself, but this looks like a
lot of code to do what "M-| git am <Enter>" already does...
^ permalink raw reply
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Linus Torvalds @ 2008-06-23 21:26 UTC (permalink / raw)
To: Pierre Habouzit
Cc: Jeff King, Johannes Schindelin, Git Mailing List, Junio C Hamano
In-Reply-To: <20080623210935.GC13395@artemis.madism.org>
On Mon, 23 Jun 2008, Pierre Habouzit wrote:
>
> Let's see if I can catch it elegantly.
No.
Look at builtin-blame.c.
THEN get back to me.
Trust me, you need what I wrote. Something that parses all the options in
one go, and ignores the ones it cannot parse, because a TOTALLY DIFFERENT
function than the caller will call it!
Why is that so hard to accept? Especially since I already wrote the code
and sent it out?
Linus
^ permalink raw reply
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Pierre Habouzit @ 2008-06-23 21:28 UTC (permalink / raw)
To: Junio C Hamano
Cc: Linus Torvalds, Jeff King, Johannes Schindelin, Git Mailing List
In-Reply-To: <7vprq7hmkx.fsf@gitster.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 1625 bytes --]
On Mon, Jun 23, 2008 at 09:23:58PM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
>
> > With that, you write parsers this way:
> >
> > {
> > struct parse_opt_ctx_t ctx;
> >
> > parse_options_start(&ctx, argc, argv, 0);
> >
> > for (;;) {
> > const char *arg;
> >
> > switch (parse_options_step(&ctx, options, usagestr)) {
> > case PARSE_OPT_HELP:
> > /* dump your help here, the one for options/usagestr is already dumped */
> > exit(129);
> > case PARSE_OPT_DONE:
> > goto done;
> > }
> >
> > arg = *ctx->argv++;
> > ctx->argc--;
> >
> > if (strcmp(arg, "-")) {
> > /* you're on baby ! */
> > } else if ....
> > } else {
> > error("unknown option %s", arg);
> > parse_options_usage(options, usagestr);
> > /* dump your help here */
> > exit(129);
> > }
> > }
> >
> > done:
> > argc = parse_options_end(&ctx);
> > }
>
> Nice. I have started doing the same (insignificant details are different;
> e.g. I used "positive is unknown" convention instead ) and then the
> solution is sitting in my mbox ;-)
Well it's still rough on the edges, totally untested (I didn't bother to
run the testsuite), but I wanted some feedback before I cook this to
something nicer.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: git svn --add-author-from implies --use-log-author
From: Avery Pennarun @ 2008-06-23 21:31 UTC (permalink / raw)
To: Mircea Bardac; +Cc: Git Mailing List
In-Reply-To: <48601432.2090707@mircea.bardac.net>
On 6/23/08, Mircea Bardac <dev@mircea.bardac.net> wrote:
> Avery Pennarun wrote:
>
> > You can set config options for these, however:
> >
> > git config svn.addAuthorFrom true
> > git config svn.useLogAuthor true
> >
> > (I actually use "git config --global" to set these on my system so
> > they apply to all my git-svn repositories.)
>
> Oh great. This is what I actually wanted. Makes a lot more sense to have
> them as (global) variables.
>
> My initial thought was that there are (somewhere) some variables being set
> for the repository by using --add-author-from and --use-log-author with "git
> svn clone ...". I find this quite intuitive (and maybe this should be
> default?). I can't see a reason for using these options once and not using
> them later, but it might just be me.
I agree, it's a good idea to save them to the config at git svn clone time.
When I added --add-author-from I just did it like --use-log-author,
then I had to read the source code to find out how to set them as
config variables :)
I'm sure some patches (at least for the documentation) would be welcomed.
> I have found that Documentation/SubmittingPatches contains info on how to
> use "Signed-off-by:"/"Acked-by:"/a little bit of "From:" but, as far as I
> remember, others have been/are used. Are they all gathered somewhere, as
> recommendations?
I don't know of any definitive reference, but Signed-off-by and
Acked-by seem to be the important ones. From: (as part of the commit
message) seems to be a git-svn extension that I invented. From: (as a
header in the commit message) is interpreted as the committer name by
git-am, I think, but you almost never need that.
Have fun,
Avery
^ permalink raw reply
* [PATCH/RFC] Bugfix for AIX tmpfile creation
From: Patrick Higgins @ 2008-06-23 21:33 UTC (permalink / raw)
To: git; +Cc: Patrick Higgins
The AIX mkstemp will modify it's template parameter to an empty string if
the call fails. This caused a subsequent mkdir to fail.
Signed-off-by: Patrick Higgins <patrick.higgins@cexp.com>
---
sha1_file.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 191f814..92299ed 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2118,6 +2118,7 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename)
fd = mkstemp(buffer);
if (fd < 0 && dirlen) {
/* Make sure the directory exists */
+ memcpy(buffer, filename, dirlen);
buffer[dirlen-1] = 0;
if (mkdir(buffer, 0777) || adjust_shared_perm(buffer))
return -1;
--
1.5.6.dirty
^ permalink raw reply related
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Linus Torvalds @ 2008-06-23 21:41 UTC (permalink / raw)
To: Pierre Habouzit
Cc: Jeff King, Johannes Schindelin, Git Mailing List, Junio C Hamano
In-Reply-To: <alpine.LFD.1.10.0806231425270.2926@woody.linux-foundation.org>
On Mon, 23 Jun 2008, Linus Torvalds wrote:
>
> Trust me, you need what I wrote. Something that parses all the options in
> one go, and ignores the ones it cannot parse, because a TOTALLY DIFFERENT
> function than the caller will call it!
Side note: of course you can always just queue them up yourself, and then
pass those off to the later stage. But where do you queue them? Can you
re-use argv[]? If so, what's the difference with the simpler patch that I
already posted? (simpler both from a usage standpoint _and_ from a patch
size standpoint).
That said, I *really* don't care what the end result is, but I would
strongly urge people to actually look at what existing code does. That
includes at a minimum builtin-blame.c, but also all the existing users
where _most_ of the arguments are just trivial. builtin-apply.c is a great
example of something where it's almost all trivial, but a lot of the
options set more than one value.
The guiding light here should not be how pretty or clean option-parse.c
is, but how easy it is to convert the hundreds of existing options that
haven't been converted yet. And a lot of them haven't been converted
because it's currently very painful to do multi-valued things, even if
most of those values really are pretty simple.
In that vein - maybe it would be a good idea to allow multiples of the
same option, so that things like builtin-apply.c can handle the "-p"
option (to give an example of this behaviour) as a set of TWO options that
do
OPT_INTEGER('p', NULL, &p_value, "Patch depth"),
OPT_BOOLEAN('p', NULL, &p_value_known),
where "-p3" would then trigger on both, once setting "p_value" to 3, and
once setting p_value_known to 1.
I dunno. But _this_ is the real problem with parse-options.c. It's hard to
convert existing really simple parsers. Whether the problem is writing a
sane set of rules for it without using callbacks (callbacks are neither
easy nor clean), or whether the problem is that the parsing is done in
multiple different places..
Linus
^ permalink raw reply
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Pierre Habouzit @ 2008-06-23 21:47 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jeff King, Johannes Schindelin, Git Mailing List, Junio C Hamano
In-Reply-To: <alpine.LFD.1.10.0806231425270.2926@woody.linux-foundation.org>
[-- Attachment #1: Type: text/plain, Size: 2298 bytes --]
On Mon, Jun 23, 2008 at 09:26:39PM +0000, Linus Torvalds wrote:
>
>
> On Mon, 23 Jun 2008, Pierre Habouzit wrote:
> >
> > Let's see if I can catch it elegantly.
>
> No.
>
> Look at builtin-blame.c.
>
> THEN get back to me.
>
> Trust me, you need what I wrote. Something that parses all the options in
> one go, and ignores the ones it cannot parse, because a TOTALLY DIFFERENT
> function than the caller will call it!
Well I really believe that with an incremental parse_revisions (I have
sent the patch to split {init,parse,setup}_revisions already) instead of
the 'do all at once' version, you can write the code in builtin-blame in
one pass exactly with the series I wrote.
> Why is that so hard to accept? Especially since I already wrote the code
> and sent it out?
I don't find it "hard to accept", I just don't like the idea because
it's broken wrt some parse-options mechanisms that I like a lot, and
would be sad to see broken, and that with a little work can be
overcomed. Note that builtin-blame is contains probably the most
involved option parser in git.git, and isn't really what one could call
representative of what awaits us on the path of migrating git.git to
parse-options fully.
I don't expect it to be easy to migrate at all. I do believe your code
works too, it's obvious to see it does actually. But it could be made
better, with real code that I will provide, just not tonight because I'm
exhausted.
Note that an incremental parse_revisions/parse_diff_opts/.. are a
precodition to many CLI parsers conversions anyways, so if the task
seems a bit exagerated for the sake of git-blame only, it actually helps
for dozens of other commands, where the conversion will just be trivial.
I fully understand you frustration wrt the poor handling of the command
line arguments in many git commands. Believe me I really do. I know I
haven't been active on this front for something like 6 months, but I'm
sadly not payed to work on git.git ;p Though things are moving right
now, so why don't we wait another week to see what other propose ?
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: [PATCH] parse-opt: do not pring errors on unknown options, return -2 intead.
From: Junio C Hamano @ 2008-06-23 22:08 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: git, peff, Johannes.Schindelin
In-Reply-To: <1214255482-2086-4-git-send-email-madcoder@debian.org>
You'd need something like this on top.
Documentation/technical/api-parse-options says that non-zero return is
used to signal an error.
parse-options.c | 16 ++++++++--------
1 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 132d34c..71a8056 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -94,14 +94,14 @@ static int get_value(struct parse_opt_ctx_t *p,
case OPTION_CALLBACK:
if (unset)
- return (*opt->callback)(opt, NULL, 1);
+ return (*opt->callback)(opt, NULL, 1) ? (-1) : 0;
if (opt->flags & PARSE_OPT_NOARG)
- return (*opt->callback)(opt, NULL, 0);
+ return (*opt->callback)(opt, NULL, 0) ? (-1) : 0;
if (opt->flags & PARSE_OPT_OPTARG && !p->opt)
- return (*opt->callback)(opt, NULL, 0);
+ return (*opt->callback)(opt, NULL, 0) ? (-1) : 0;
if (!arg)
return opterror(opt, "requires a value", flags);
- return (*opt->callback)(opt, get_arg(p), 0);
+ return (*opt->callback)(opt, get_arg(p), 0) ? (-1) : 0;
case OPTION_INTEGER:
if (unset) {
@@ -276,9 +276,9 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
if (*ctx->opt == 'h')
return parse_options_usage(usagestr, options);
switch (parse_short_opt(ctx, options)) {
- case -1:
+ case -1:
return parse_options_usage(usagestr, options);
- case -2:
+ case -2:
goto unknown_fixup;
}
if (ctx->opt)
@@ -318,9 +318,9 @@ unknown_fixup:
if (!strcmp(arg + 2, "help"))
return parse_options_usage(usagestr, options);
switch (parse_long_opt(ctx, arg + 2, options)) {
- case -1:
+ case -1:
return parse_options_usage(usagestr, options);
- case -2:
+ case -2:
return PARSE_OPT_UNKNOWN;
}
}
^ permalink raw reply related
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Junio C Hamano @ 2008-06-23 22:11 UTC (permalink / raw)
To: Linus Torvalds
Cc: Pierre Habouzit, Jeff King, Johannes Schindelin, Git Mailing List
In-Reply-To: <alpine.LFD.1.10.0806231425270.2926@woody.linux-foundation.org>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Mon, 23 Jun 2008, Pierre Habouzit wrote:
>>
>> Let's see if I can catch it elegantly.
>
> No.
>
> Look at builtin-blame.c.
>
> THEN get back to me.
>
> Trust me, you need what I wrote. Something that parses all the options in
> one go, and ignores the ones it cannot parse, because a TOTALLY DIFFERENT
> function than the caller will call it!
I do not think you two are saying vastly different things.
The series Pierre has just posted is prerequisite for teaching
parse_options() to queue the unknown ones (besides allowing a finer
grained stepwise parsing), so that the caller like cmd_blame() can call
revision parser with the remainder.
^ permalink raw reply
* Re: [PATCH] parse-opt: do not pring errors on unknown options, return -2 intead.
From: Pierre Habouzit @ 2008-06-23 22:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, peff, Johannes.Schindelin
In-Reply-To: <7vd4m7hkjf.fsf@gitster.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 541 bytes --]
On Mon, Jun 23, 2008 at 10:08:04PM +0000, Junio C Hamano wrote:
> You'd need something like this on top.
>
> Documentation/technical/api-parse-options says that non-zero return is
> used to signal an error.
Oh right, like said, it's still rough on the edges :)
I'm not 100% convinced by the current usage generation yet. But I'll
sleep on it first.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: [PATCH v6] gitweb: add test suite with Test::WWW::Mechanize::CGI
From: Jakub Narebski @ 2008-06-23 22:18 UTC (permalink / raw)
To: Lea Wiemann; +Cc: git
In-Reply-To: <485FE3F7.4040102@gmail.com>
On Mon, 23 Jun 2008, Lea Wiemann wrote:
> Jakub Narebski wrote:
>> Lea Wiemann wrote:
>>
>>> +# We don't count properly when skipping, so no_plan is necessary.
>>> +use Test::More qw(no_plan);
>>
>> I'd rather either skip this comment all together, or change it [...]
>
> I'll just remove it. Anybody trying to add a test count will give up
> within 30 seconds. ;-)
I just think that not having comment is better than have comment which
is not fully true, or five lines of explanations which could be
expanded further.
Simply: planning (coming with number of tests upfront) is hard, and for
this situation it doesn't give us anything.
>>> +our $long_tests = $ENV{GIT_TEST_LONG};
>>
>> Here also it could be "my $long_tests";
>
> This one is to make "local $long_tests = 0" work -- it wouldn't work
> with a 'my' declaration.
Hmmmm...
Siednote: some time ago I though that I understood difference between
lexical scoping (my) and dynamical scoping (local)...
>>> +chomp(my @heads = map { (split('/', $_))[2] }
>>> + `git-for-each-ref --sort=-committerdate refs/heads`);
>>
>> Wouldn't be easier to use '--format=%(refname)' in git-for-each-ref
>
> *checks* No, since I want to strip the leading refs/heads/ anyway (in
> order to test the page text, which mentions heads and tags without the
> directory prefix) -- hence the 'split' wouldn't go away with
> --format=%(refname).
Well, you could have replaced 'split' by s!^refs/!! ;-)))
>>> +# Thus subroutine was copied (and modified to work with blanks in the
>>> +# application path) from WWW::Mechanize::CGI 0.3, which is licensed
>>> +# 'under the same terms as perl itself' and thus GPL compatible.
>
> That's a tyop: s/Thus/This/ (fixed). It refers to the following "my
> $cgi = sub". Does the comment make sense now?
It makes sense now, thanks.
Although I guess it would be nice to have link to the ticket to the bug
(report) in WWW::Mechanize::CGI, so we could use cgi_application()
"again" if it gets fixed...
> + my $status = system $ENV{GITPERL}, $gitweb;
...if not for the trick with explicitly calling Perl (GITPERL), to
allow testing different Perl versions with gitweb.
>>> + # WebService::Validator::Feed::W3C would be nice to
>>> + # use, but it doesn't support direct input (as opposed
>>> + # to URIs) long enough for our feeds.
>>
>> Could you tell us here what are the limitations? Are those limits
>> of W3C SOAP interface for web validation, or the CPAN package mentioned?
>
> No idea. I tried it and it told me that the URL was too long -- I
> suspect it's the W3C server that rejected it. I wouldn't spend more
> effort trying to get online validation to work; it's probably easier to
> just occasionally validate manually. ;) XML well-formedness tests
> should catch most occasional breakages just fine.
Right.
Well, W3C feed validator provides SOAP interface we could use, using
POST of course and not GET (but it requires net connection), and also
standalone CLI validator (unfortunately in Python).
>>> + # Broken link in Atom/RSS view -- cannot spider:
>>> + # http://mid.gmane.org/485EB333.5070108@gmail.com
>>> + local $long_tests = 0;
>>> + test_page('?p=.git;a=atom', 'Atom feed');
>>
>> This I think should be written using Test::More equivalent of
>> test_expect_failure in t/test-lib.sh, namely TODO block,
>
> Right -- here's my new version (which still fails if the feeds die or
> are not well-formed XML -- I'll want that when I change gitweb):
>
> # RSS/Atom/OPML view
> # Simply retrieve and verify well-formedness, but don't spider.
> $mech->get_ok('?p=.git;a=atom', 'Atom feed') and _verify_page;
> $mech->get_ok('?p=.git;a=rss', 'RSS feed') and _verify_page;
> TODO: {
> # Now spider -- but there are broken links.
> # http://mid.gmane.org/485EB333.5070108@gmail.com
> local $TODO = "fix broken links in Atom/RSS feeds";
> test_page('?p=.git;a=atom', 'Atom feed');
> test_page('?p=.git;a=rss', 'RSS feed');
> }
> test_page('?a=opml', 'OPML outline');
Does it make t9503-gitweb.Mechanize.sh fail?
>>> [in t/test-lib.sh:]
>>> +GITPERL=${GITPERL:-perl}
>>> +export GITPERL
>>> [The idea is to easily run the test suite with different perl versions.]
>>
>> How it is different from PERL_PATH?
>
> Right, I didn't think of that. PERL_PATH isn't available in the tests
> though, it's only used internally by the Makefile to generate (among
> other things) gitweb.cgi. [...]
That's what I was asking about.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Pierre Habouzit @ 2008-06-23 22:24 UTC (permalink / raw)
To: Junio C Hamano
Cc: Linus Torvalds, Jeff King, Johannes Schindelin, Git Mailing List
In-Reply-To: <7v3an3hke8.fsf@gitster.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]
On Mon, Jun 23, 2008 at 10:11:11PM +0000, Junio C Hamano wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
> > On Mon, 23 Jun 2008, Pierre Habouzit wrote:
> >>
> >> Let's see if I can catch it elegantly.
> >
> > No.
> >
> > Look at builtin-blame.c.
> >
> > THEN get back to me.
> >
> > Trust me, you need what I wrote. Something that parses all the options in
> > one go, and ignores the ones it cannot parse, because a TOTALLY DIFFERENT
> > function than the caller will call it!
>
> I do not think you two are saying vastly different things.
>
> The series Pierre has just posted is prerequisite for teaching
> parse_options() to queue the unknown ones (besides allowing a finer
> grained stepwise parsing), so that the caller like cmd_blame() can call
> revision parser with the remainder.
Yeah actually one can use it this way, what I wrote can be used to do
the very same thing as your _KEEP_UNKNOWN flag, but also more.
Though I didn't fixed the fact that parse_options clobbers argv[0],
which can be easily fixed. The issue with that is that _some_ callers
use the fact that the filtered argv is NULL terminated. I'm unsure if
posix says that argv[argc] is readable and NULL at all, if it isn't,
then changing that would break git for some commands on some OSes other
than Linux and BSD where AFAICT argv[argc] == NULL holds.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: [PATCH/RFC] Bugfix for AIX tmpfile creation
From: Junio C Hamano @ 2008-06-23 22:29 UTC (permalink / raw)
To: Patrick Higgins; +Cc: git
In-Reply-To: <1214256821-3169-1-git-send-email-patrick.higgins@cexp.com>
Hmm, we have quite a few calls to mkstemp(), so adding the workaround to
this specific call site does not scale well as a fix, I am afraid.
Some callers do not want to use the xmkstemp() (because the function
always dies upon failure), so working around this bug in xmkstemp() won't
solve it either, unfortunately. We would probably need compat/mkstemp.c
that wraps problematic mkstemp() on platforms with broken mkstemp()
implementation.
^ permalink raw reply
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Pierre Habouzit @ 2008-06-23 22:36 UTC (permalink / raw)
To: Junio C Hamano, Linus Torvalds, Jeff King, Johannes Schindelin,
Git Mailing List <git
In-Reply-To: <20080623222404.GM13395@artemis.madism.org>
[-- Attachment #1: Type: text/plain, Size: 2768 bytes --]
On Mon, Jun 23, 2008 at 10:24:05PM +0000, Pierre Habouzit wrote:
> Yeah actually one can use it this way, what I wrote can be used to do
> the very same thing as your _KEEP_UNKNOWN flag, but also more.
>
> Though I didn't fixed the fact that parse_options clobbers argv[0],
> which can be easily fixed. The issue with that is that _some_ callers
> use the fact that the filtered argv is NULL terminated. I'm unsure if
> posix says that argv[argc] is readable and NULL at all, if it isn't,
> then changing that would break git for some commands on some OSes other
> than Linux and BSD where AFAICT argv[argc] == NULL holds.
Actually one can do it this way without breaking "backward
compatibility" here:
----8<---
From 8955adf557fded857eacc6c03c885a3d6334580f Mon Sep 17 00:00:00 2001
From: Pierre Habouzit <madcoder@debian.org>
Date: Tue, 24 Jun 2008 00:31:31 +0200
Subject: [PATCH] parse-opt: add PARSE_OPT_KEEP_ARGV0 parser option.
This way, argv[0] isn't clobbered, to the cost of maybe not having a
resulting NULL terminated argv array.
Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
parse-options.c | 8 +++++---
parse-options.h | 2 ++
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 1ade2ac..c1c5a94 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -246,7 +246,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
memset(ctx, 0, sizeof(*ctx));
ctx->argc = argc - 1;
ctx->argv = argv + 1;
- ctx->out = argv;
+ ctx->out = argv + ((flags & PARSE_OPT_KEEP_ARGV0) != 0);
ctx->flags = flags;
strbuf_init(&ctx->buf, 0);
}
@@ -329,10 +329,12 @@ unknown_fixup:
int parse_options_end(struct parse_opt_ctx_t *ctx)
{
+ int res = ctx->cpidx + ctx->argc;
memmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * sizeof(*ctx->out));
- ctx->out[ctx->cpidx + ctx->argc] = NULL;
+ if (ctx->out + res < ctx->argv + ctx->argc)
+ ctx->out[res] = NULL;
strbuf_release(&ctx->buf);
- return ctx->cpidx + ctx->argc;
+ return res;
}
int parse_options(int argc, const char **argv, const struct option *options,
diff --git a/parse-options.h b/parse-options.h
index 14447d5..6745c7d 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -22,6 +22,8 @@ enum parse_opt_type {
enum parse_opt_flags {
PARSE_OPT_KEEP_DASHDASH = 1,
PARSE_OPT_STOP_AT_NON_OPTION = 2,
+ /* using that option, the filtered argv may not be NULL terminated */
+ PARSE_OPT_KEEP_ARGV0 = 4,
};
enum parse_opt_option_flags {
--
1.5.6.117.g4959d.dirty
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply related
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Junio C Hamano @ 2008-06-23 22:38 UTC (permalink / raw)
To: Pierre Habouzit
Cc: Linus Torvalds, Jeff King, Johannes Schindelin, Git Mailing List
In-Reply-To: <20080623222404.GM13395@artemis.madism.org>
Pierre Habouzit <madcoder@debian.org> writes:
> Though I didn't fixed the fact that parse_options clobbers argv[0],
> which can be easily fixed. The issue with that is that _some_ callers
> use the fact that the filtered argv is NULL terminated.
Isn't it just the matter of (1) leave argv[0] as is, (2) start copy dest
at argv[1] not argv[0], and (3) terminate argv[nargc] = NULL where nargc
is what you did not handle?
You have argc args in incoming argv[], you consume zero or more of them
starting from argv[1] up to potentially argv[argc-1] (and argv[argc] is
NULL). So why is it an issue? Sorry, I do not understand.
^ permalink raw reply
* Re: [PATCH/RFC] Bugfix for AIX tmpfile creation
From: Linus Torvalds @ 2008-06-23 22:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Patrick Higgins, git
In-Reply-To: <7vy74vg4zq.fsf@gitster.siamese.dyndns.org>
On Mon, 23 Jun 2008, Junio C Hamano wrote:
>
> Hmm, we have quite a few calls to mkstemp(), so adding the workaround to
> this specific call site does not scale well as a fix, I am afraid.
All other callers of mkstemp() always rewrite the _entire_ path and don't
care about the result after failure (since it failed)
The new 'create_tmpfile()' usage was/is special in that it re-uses the
path without recreating it all, just the final part.
So I think Patrick's patch is fine and sufficient. Maybe I missed some in
my quick grep, but it does look ok, and the create_tmpfile() usage really
was pretty special.
Linus
^ permalink raw reply
* RE: [PATCH/RFC] Bugfix for AIX tmpfile creation
From: Patrick.Higgins @ 2008-06-23 23:07 UTC (permalink / raw)
To: torvalds, gitster; +Cc: git
In-Reply-To: <alpine.LFD.1.10.0806231552510.2926@woody.linux-foundation.org>
On Behalf Of Linus Torvalds
> To: Junio C Hamano
>
> On Mon, 23 Jun 2008, Junio C Hamano wrote:
> >
> > Hmm, we have quite a few calls to mkstemp(), so adding the
> workaround to
> > this specific call site does not scale well as a fix, I am afraid.
>
> All other callers of mkstemp() always rewrite the _entire_
> path and don't
> care about the result after failure (since it failed)
>
> The new 'create_tmpfile()' usage was/is special in that it
> re-uses the
> path without recreating it all, just the final part.
>
> So I think Patrick's patch is fine and sufficient. Maybe I
> missed some in
> my quick grep, but it does look ok, and the create_tmpfile()
> usage really
> was pretty special.
This is the only problem I've seen with it so far on AIX. I was really surprised because I hadn't seen any problems with any of the release candidates and had been using them all as they came out so I got lazy and didn't test the final release, but one of my co-workers let me know that it is broken this morning. Even the most basic things like git-add stopped working, and applying this small patch fixed all test cases. The irony is of course that I wanted 1.5.6 for the improved AIX support and the final release is completely broken on AIX!
^ permalink raw reply
* Re: [PATCH/RFC] Bugfix for AIX tmpfile creation
From: Junio C Hamano @ 2008-06-23 23:10 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Patrick Higgins, git
In-Reply-To: <alpine.LFD.1.10.0806231552510.2926@woody.linux-foundation.org>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Mon, 23 Jun 2008, Junio C Hamano wrote:
>>
>> Hmm, we have quite a few calls to mkstemp(), so adding the workaround to
>> this specific call site does not scale well as a fix, I am afraid.
>
> All other callers of mkstemp() always rewrite the _entire_ path and don't
> care about the result after failure (since it failed)
Yeah, I realize I grepped wrong X-<.
^ permalink raw reply
* Re: [PATCH/RFC] Bugfix for AIX tmpfile creation
From: Junio C Hamano @ 2008-06-23 23:14 UTC (permalink / raw)
To: Patrick.Higgins; +Cc: torvalds, gitster, git
In-Reply-To: <911589C97062424796D53B625CEC0025E46183@USCOBRMFA-SE-70.northamerica.cexp.com>
<Patrick.Higgins@cexp.com> writes:
> The irony is of course that I wanted 1.5.6 for the improved AIX support
> and the final release is completely broken on AIX!
Yeah, queued to 'maint' for 1.5.6.1. Thanks
^ permalink raw reply
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Pierre Habouzit @ 2008-06-23 23:31 UTC (permalink / raw)
To: Junio C Hamano
Cc: Linus Torvalds, Jeff King, Johannes Schindelin, Git Mailing List
In-Reply-To: <7vmylbg4ks.fsf@gitster.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 1318 bytes --]
On Mon, Jun 23, 2008 at 10:38:11PM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
>
> > Though I didn't fixed the fact that parse_options clobbers argv[0],
> > which can be easily fixed. The issue with that is that _some_ callers
> > use the fact that the filtered argv is NULL terminated.
>
> Isn't it just the matter of (1) leave argv[0] as is, (2) start copy dest
> at argv[1] not argv[0], and (3) terminate argv[nargc] = NULL where nargc
> is what you did not handle?
>
> You have argc args in incoming argv[], you consume zero or more of them
> starting from argv[1] up to potentially argv[argc-1] (and argv[argc] is
> NULL). So why is it an issue? Sorry, I do not understand.
Are we sure argv[argc] is NULL when those are main() arguments ? If
yes there is no issue, and I should read posix more carefully, but I was
under the impression that POSIX wasn't enforcing that.
Unrelated but worth to note: many parse_options users just don't care
about argv[0] and having it kept each time would rather be a pain for
them (they would need to call argv++, argc-- themselves).
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Linus Torvalds @ 2008-06-23 23:40 UTC (permalink / raw)
To: Pierre Habouzit
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Git Mailing List
In-Reply-To: <20080623233146.GP13395@artemis.madism.org>
On Tue, 24 Jun 2008, Pierre Habouzit wrote:
>
> Are we sure argv[argc] is NULL when those are main() arguments ?
Yes. There's tons of code that ends up not caring about argc at all, so
any system that doesn't NULL-terminate would break.
In fact, git itself already depends on it, because it turns argv directly
into the pathspec name pattern array.
Linus
^ permalink raw reply
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Junio C Hamano @ 2008-06-23 23:51 UTC (permalink / raw)
To: Pierre Habouzit
Cc: Linus Torvalds, Jeff King, Johannes Schindelin, Git Mailing List
In-Reply-To: <20080623233146.GP13395@artemis.madism.org>
Pierre Habouzit <madcoder@debian.org> writes:
> Unrelated but worth to note: many parse_options users just don't care
> about argv[0] and having it kept each time would rather be a pain for
> them (they would need to call argv++, argc-- themselves).
Not necessarily. If they were parsing by hand, they were written to deal
with the fact that argv[0] is not the program argument (iow, they start
counting from one). And before and after calling parse_options(), they
need to change that assumption anyway, because parse_options() makes
argv[0] disappear.
See for example these:
* 5eee6b2 (Make builtin-reset.c use parse_options., 2008-03-04)
The caller used to start counting from 1, after the conversion it
has to count from 0.
* 8320199 (Rewrite builtin-fetch option parsing to use parse_options()., 2007-12-04)
* 3968658 (Make builtin-tag.c use parse_options., 2007-11-09)
The caller used to say (i==argc) is "no param" case, now it has to
say "!argc" is the equivalent condition.
I am not saying that forcing these callers to change their loop invariants
in order to use parse_options() was bad. I am saying that your "would
rather be a pain for them" is a bogus argument --- they needed to pay that
price when converting to parse_options() that munges argv[0] anyway.
But this argv[0] munging exactly is why parse_options() hurts users who
want to cascade option parsing.
Didn't one of the recent series have an option to tell parse_options() to
start parsing from argv[0] (persumably because it got an argument array
from somebody who munges it to drop argv[0])? I think it was merge-in-C
from Miklos, but the approach feels very much backwards. The caller
should be (at least) able to choose to say KEEP_ARGV0 like one of your
patch does.
^ permalink raw reply
* [PATCH v2] git-svn: make rebuild respect rewriteRoot option
From: Jan Krüger @ 2008-06-24 0:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git mailing list, Eric Wong
Suppose someone fetches git-svn-ified commits from another repo and then
attempts to use 'git-svn init --rewrite-root=foo bar'. Using git svn rebase
after that will fail badly:
* For each commit tried by working_head_info, rebuild is called indirectly.
* rebuild will iterate over all commits and skip all of them because the
URL does not match. Because of that no rev_map file is generated at all.
* Thus, rebuild will run once for every commit. This takes ages.
* In the end there still isn't any rev_map file and thus working_head_info
fails.
Addressing this behaviour fixes an apparently not too uncommon problem with
providing git-svn mirrors of Subversion repositories. Some repositories are
accessed using different URLs depending on whether the user has push
privileges or not. In the latter case, an anonymous URL is often used that
differs from the push URL. Providing a mirror that is usable in both cases
becomes a lot more possible with this change.
Signed-off-by: Jan Krüger <jk@jk.gs>
---
For reference, the previous version of this patch at
<http://thread.gmane.org/gmane.comp.version-control.git/85551> was
acked by Eric Wong (see that thread); the only change here is the added
test case as suggested by Eric.
The test doesn't exactly reproduce the original situation but it does
trigger the rebuild process to ensure it works correctly despite
'rewriteRoot' being set to something not equal to 'url', which is what
we are after in the first place.
git-svn.perl | 6 ++--
t/t9123-git-svn-rebuild-with-rewriteroot.sh | 32 +++++++++++++++++++++++++++
2 files changed, 35 insertions(+), 3 deletions(-)
create mode 100755 t/t9123-git-svn-rebuild-with-rewriteroot.sh
diff --git a/git-svn.perl b/git-svn.perl
index a54979d..4c9c59b 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -2577,8 +2577,8 @@ sub rebuild {
my ($log, $ctx) =
command_output_pipe(qw/rev-list --pretty=raw --no-color --reverse/,
$self->refname, '--');
- my $full_url = $self->full_url;
- remove_username($full_url);
+ my $metadata_url = $self->metadata_url;
+ remove_username($metadata_url);
my $svn_uuid = $self->ra_uuid;
my $c;
while (<$log>) {
@@ -2596,7 +2596,7 @@ sub rebuild {
# if we merged or otherwise started elsewhere, this is
# how we break out of it
if (($uuid ne $svn_uuid) ||
- ($full_url && $url && ($url ne $full_url))) {
+ ($metadata_url && $url && ($url ne $metadata_url))) {
next;
}
diff --git a/t/t9123-git-svn-rebuild-with-rewriteroot.sh b/t/t9123-git-svn-rebuild-with-rewriteroot.sh
new file mode 100755
index 0000000..cf3f64d
--- /dev/null
+++ b/t/t9123-git-svn-rebuild-with-rewriteroot.sh
@@ -0,0 +1,32 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Jan Krüger
+#
+
+test_description='git-svn respects rewriteRoot during rebuild'
+
+. ./lib-git-svn.sh
+
+mkdir import
+cd import
+ touch foo
+ svn import -m 'import for git-svn' . "$svnrepo" >/dev/null
+cd ..
+rm -rf import
+
+test_expect_success 'init, fetch and checkout repository' '
+ git svn init --rewrite-root=http://invalid.invalid/ "$svnrepo" &&
+ git svn fetch
+ git checkout -b mybranch remotes/git-svn
+ '
+
+test_expect_success 'remove rev_map' '
+ rm "$GIT_SVN_DIR"/.rev_map.*
+ '
+
+test_expect_success 'rebuild rev_map' '
+ git svn rebase >/dev/null
+ '
+
+test_done
+
--
1.5.6.2.g4316
^ permalink raw reply related
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Junio C Hamano @ 2008-06-24 0:30 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jeff King, Johannes Schindelin, Pierre Habouzit, Git Mailing List
In-Reply-To: <alpine.LFD.1.10.0806231114180.2926@woody.linux-foundation.org>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Mon, 23 Jun 2008, Linus Torvalds wrote:
>>
>> Umm. Helloo, reality.. There are actually very few options that take a
>> flag for their arguments. In particular, the option parsing we really
>> _care_ about (revision parsing - see builtin-blame.c which is exactly
>> where I wanted to convert things) very much DOES NOT.
>
> Actually, I guess "--default" does, but if you try to mix that up with (a)
> a default head that starts with a dash and (b) git-blame, you're doing
> something pretty odd.
>
> And yes, "-n" does too, but if you pass it negative numbers you get what
> you deserve.
>
> The point being, we really _do_ have a real-life existing case for
> PARSE_OPT_CONTINUE_ON_UNKNOWN, which is hard to handle any other way.
> Currently you can literally do
>
> git blame --since=April -b Makefile
>
> and while it's a totally made-up example, it's one I've picked to show
> exactly what does *not* work with my patch that started this whole thread.
>
> And guess what you need to fix it?
>
> If you guessed PARSE_OPT_CONTINUE_ON_UNKNOWN, you win a prize.
With this on top of Pierre's series, and adding PARSE_OPT_SKIP_UNKNOWN to
the obvious place in your patch, "blame --since=April -b Makefile" would
work.
-- >8 --
Subject: [PATCH] parse-options: PARSE_OPT_SKIP_UNKNOWN
---
parse-options.c | 14 ++++++++++----
parse-options.h | 1 +
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 71a8056..9f1eb65 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -341,20 +341,26 @@ int parse_options(int argc, const char **argv, const struct option *options,
struct parse_opt_ctx_t ctx;
parse_options_start(&ctx, argc, argv, flags);
+
+ again:
switch (parse_options_step(&ctx, options, usagestr)) {
case PARSE_OPT_HELP:
exit(129);
case PARSE_OPT_DONE:
break;
default: /* PARSE_OPT_UNKNOWN */
- if (ctx.argv[0][1] == '-') {
+ if (flags & PARSE_OPT_KEEP_UNKNOWN) {
+ ctx.out[ctx.cpidx++] = ctx.argv[0];
+ ctx.argc--;
+ ctx.argv++;
+ goto again;
+ }
+ if (ctx.argv[0][1] == '-')
error("unknown option `%s'", ctx.argv[0] + 2);
- } else {
+ else
error("unknown switch `%c'", *ctx.opt);
- }
usage_with_options(usagestr, options);
}
-
return parse_options_end(&ctx);
}
diff --git a/parse-options.h b/parse-options.h
index 403794f..30fbf7e 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -22,6 +22,7 @@ enum parse_opt_type {
enum parse_opt_flags {
PARSE_OPT_KEEP_DASHDASH = 1,
PARSE_OPT_STOP_AT_NON_OPTION = 2,
+ PARSE_OPT_KEEP_UNKNOWN = 4,
};
enum parse_opt_option_flags {
--
1.5.6.49.g112db
^ 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