* Re: [RFC] CLI option parsing and usage generation for porcelains
From: Wincent Colaiuta @ 2007-10-13 14:53 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: git
In-Reply-To: <1192282153-26684-1-git-send-email-madcoder@debian.org>
El 13/10/2007, a las 15:29, Pierre Habouzit escribió:
> The following series is open for comments, it's not 100% ready for
> inclusion IMHO, as some details may need to be sorted out first, and
> that I've not re-read the patches thoroughly yet. Though I uses the
> tip
> of that branch as my everyday git for 2 weeks or so without any
> noticeable issues.
Great to see two things:
- the simplification in the commands switched over to use the options
parser
- the improved readability and usefulness of the options help
Great work, Pierre! I'll take a closer look at this and trial it in
my local Git install for a while to see if any issues come up.
Wincent
^ permalink raw reply
* Re: [PATCH] Add a simple option parser.
From: Pierre Habouzit @ 2007-10-13 14:58 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <Pine.LNX.4.64.0710131519510.25221@racer.site>
[-- Attachment #1: Type: text/plain, Size: 4865 bytes --]
On Sat, Oct 13, 2007 at 02:39:10PM +0000, Johannes Schindelin wrote:
> Hi,
>
> On Sat, 13 Oct 2007, Pierre Habouzit wrote:
>
> > Aggregation of single switches is allowed:
> > -rC0 is the same as -r -C 0 (supposing that -C wants an arg).
>
> I'd be more interested in "-rC 0" working... Is that supported, too?
yes it is.
> > +static inline const char *get_arg(struct optparse_t *p)
> > +{
> > + if (p->opt) {
> > + const char *res = p->opt;
> > + p->opt = NULL;
> > + return res;
> > + }
> > + p->argc--;
> > + return *++p->argv;
> > +}
>
> This is only used once; I wonder if it is really that more readable having
> this as a function in its own right.
it's used twice, and it makes the code more readable I believe.
> > +static inline const char *skippfx(const char *str, const char *prefix)
>
> Personally, I do not like abbreviations like that. They do not save that
> much screen estate (skip_prefix is only 4 characters longer, but much more
> readable). Same goes for "cnt" later.
Ack I'll fix that.
> > +static int parse_long_opt(struct optparse_t *p, const char *arg,
> > + struct option *options, int count)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < count; i++) {
> > + const char *rest;
> > + int flags = 0;
> > +
> > + if (!options[i].long_name)
> > + continue;
> > +
> > + rest = skippfx(arg, options[i].long_name);
> > + if (!rest && !strncmp(arg, "no-", 3)) {
> > + rest = skippfx(arg + 3, options[i].long_name);
> > + flags |= OPT_SHORT;
> > + }
>
> Would this not be more intuitive as
>
> if (!prefixcmp(arg, "no-")) {
> arg += 3;
> flags |= OPT_UNSET;
> }
> rest = skip_prefix(arg, options[i].long_name);
Yes, that can be done indeed, but the point is, we have sometimes
option whose long-name is "no-foo" (because it's what makes sense) but I
can rework that.
> Hm? (Note that I say UNSET, not SHORT... ;-)
fsck, good catch.
> > + if (!rest)
> > + continue;
> > + if (*rest) {
> > + if (*rest != '=')
> > + continue;
>
> Is this really no error? For example, "git log
> --decorate-walls-and-roofs" would not fail...
it would be an error, it will yield a "option not found".
> > +int parse_options(int argc, const char **argv,
> > + struct option *options, int count,
> > + const char * const usagestr[], int flags)
>
> Please indent by the same amount.
oops, stupid space vs. tab thing.
> > + if (arg[1] != '-') {
> > + optp.opt = arg + 1;
> > + do {
> > + if (*optp.opt == 'h')
> > + make_usage(usagestr, options, count);
>
> How about calling this "usage_with_options()"? With that name I expected
> make_usage() to return a strbuf.
will do.
> > + if (!arg[2]) { /* "--" */
> > + if (!(flags & OPT_COPY_DASHDASH))
> > + optp.argc--, optp.argv++;
>
> I would prefer this as
>
> if (!(flags & OPT_COPY_DASHDASH)) {
> optp.argc--;
> optp.argv++;
> }
>
> While I'm at it: could you use "args" instead of "optp"? It is misleading
> both in that it not only contains options (but other arguments, too) as in
> that it is not a pointer (the trailing "p" is used as an indicator of that
> very often, including git's source code).
okay.
> In the same vein, OPT_COPY_DASHDASH should be named
> PARSE_OPT_KEEP_DASHDASH.
okay.
>
> > + if (opts->short_name) {
> > + strbuf_addf(&sb, "-%c", opts->short_name);
> > + }
> > + if (opts->long_name) {
> > + strbuf_addf(&sb, opts->short_name ? ", --%s" : "--%s",
> > + opts->long_name);
> > + }
>
> Please lose the curly brackets.
>
> > + if (sb.len - pos <= USAGE_OPTS_WIDTH) {
> > + int pad = USAGE_OPTS_WIDTH - (sb.len - pos) + USAGE_GAP;
> > + strbuf_addf(&sb, "%*s%s\n", pad, "", opts->help);
> > + } else {
> > + strbuf_addf(&sb, "\n%*s%s\n", USAGE_OPTS_WIDTH + USAGE_GAP, "",
> > + opts->help);
> > + }
>
> Same here. (And I'd also make sure that the lines are not that long.)
okay.
>
> > diff --git a/parse-options.h b/parse-options.h
> > new file mode 100644
> > index 0000000..4b33d17
> > --- /dev/null
> > +++ b/parse-options.h
> > @@ -0,0 +1,37 @@
> > +#ifndef PARSE_OPTIONS_H
> > +#define PARSE_OPTIONS_H
> > +
> > +enum option_type {
> > + OPTION_BOOLEAN,
>
> I know that I proposed "BOOLEAN", but actually, you use it more like an
> "INCREMENTAL", right?
yes, I don't like _BOOLEAN either, I would have prefered _FLAG or sth
like that. INCREMENTAL is just too long.
> Other than that: I like it very much.
:P
--
·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] Port builtin-add.c to use the new option parser.
From: Pierre Habouzit @ 2007-10-13 15:03 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Kristian Høgsberg
In-Reply-To: <Pine.LNX.4.64.0710131544030.25221@racer.site>
[-- Attachment #1: Type: text/plain, Size: 1873 bytes --]
On Sat, Oct 13, 2007 at 02:47:20PM +0000, Johannes Schindelin wrote:
> Hi,
>
> On Sat, 13 Oct 2007, Pierre Habouzit wrote:
>
> > +static struct option builtin_add_options[] = {
> > + OPT_BOOLEAN('i', "interactive", &add_interactive, "interactive picking"),
> > + OPT_BOOLEAN('n', NULL, &show_only, "dry-run"),
> > + OPT_BOOLEAN('f', NULL, &ignored_too, "allow adding otherwise ignored files"),
> > + OPT_BOOLEAN('v', NULL, &verbose, "be verbose"),
> > + OPT_BOOLEAN('u', NULL, &take_worktree_changes, "update only files that git already knows about"),
> > + OPT_BOOLEAN( 0 , "refresh", &refresh_only, "don't add, only refresh stat() informations in the index"),
> > +};
>
> I see you terminate the list by a ",". How does this play with the option
> parser?
>
> Thinking about this more, I am reverting my stance on the ARRAY_SIZE()
> issue. I think if you introduce a "OPTION_NONE = 0" in the enum, then
> this single last comma should be enough.
adding a trailing comma does not add a NULL after that, it's ignored,
you're confused.
┌─(17:00)────
└[artemis] cat a.c
#include <stdio.h>
int main(void) {
const char * const arr[] = { "1", "2", };
printf("%d\n", sizeof(arr) / sizeof(arr[0]));
return 0;
};
┌─(17:00)────
└[artemis] ./a
2
Very few compilers do not grok trailing commas, I always put them
because it avoids spurious diffs for nothing, and that you can reorder
lines easily too.
Note that I don't really like using ARRAY_SIZE either, I kept it that
way, but my taste would rather be to have an "empty" option, and
explicitely mark the end of the array.
--
·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] Color support added to git-add--interactive.
From: Jean-Luc Herren @ 2007-10-13 16:38 UTC (permalink / raw)
To: Wincent Colaiuta, git
In-Reply-To: <19271E58-5C4F-41AF-8F9D-F114F36A34AC@wincent.com>
Hi!
I really like the idea of colorizing git add -i, especially the
prompt. Here are my two cents.
Wincent Colaiuta wrote:
> +sub print_ansi_color($$;$) {
> + my $color = shift;
> + my $string = shift;
> + my $trailer = shift;
None of the other subs in this file have a prototype, so for
consistency I'd suggest to not add it on this function either.
However maybe a patch that adds it to all subs would be welcome.
(I wouldn't see the necessity though.)
And the common way of getting the arguments is reading @_ (see all
other subs in the file). So maybe instead write:
[...]
sub print_ansi_color {
my ($color, $string, $trailer) = @_;
[...]
> + if ($use_color) {
> + printf '%s%s%s', Term::ANSIColor::color($color), $string,
> + Term::ANSIColor::color('clear');
> + } else {
Why use printf when you could directly use print here? It's only
used for concatenating.
> + if ($trailer) {
> + print $trailer;
> + }
This will fail to print $trailer when $trailer happens to be a
string that evaluates to false in bool context, like '0'. Write
this as:
if (defined $trailer) {
print $trailer;
}
IMHO, parsing the output of 'git diff-files --color' is a very bad
idea and it makes all regexes uglier and more difficult to read.
You're much better off recolorizing it yourself, which makes it a
more localized change. Especially, I don't think that you have
any guarantee that escape sequences won't ever contain the
characters '+', '-' or ' ' (space), which would break your code on
lines like this:
> + if ($line =~ /^[^-+ ]*\+/) {
Finally -- and this might be just my eyes -- blue is a very nice
color, but it looks a bit too dark on black background. Maybe
choose a default color that looks reasonable on black *and* white
background.
Cheers,
jlh
^ permalink raw reply
* Re: [PATCH] Color support added to git-add--interactive.
From: Johannes Schindelin @ 2007-10-13 16:38 UTC (permalink / raw)
To: Wincent Colaiuta
Cc: Dan Zwell, Git Mailing List, Jeff King, Jonathan del Strother,
Frank Lichtenheld
In-Reply-To: <19271E58-5C4F-41AF-8F9D-F114F36A34AC@wincent.com>
Hi,
On Sat, 13 Oct 2007, Wincent Colaiuta wrote:
> Didn't even think about implementing user-settable colors.
FWIW, this is what Documentation/config.txt has to say about it:
color.diff.<slot>::
Use customized color for diff colorization. `<slot>` specifies
which part of the patch to use the specified color, and is one
of `plain` (context text), `meta` (metainformation), `frag`
(hunk header), `old` (removed lines), `new` (added lines),
`commit` (commit headers), or `whitespace` (highlighting dubious
whitespace). The values of these variables may be specified as
in color.branch.<slot>.
Hth,
Dscho
^ permalink raw reply
* Re: push fails with unexpected 'matches more than one'
From: Steffen Prohaska @ 2007-10-13 16:51 UTC (permalink / raw)
To: Git Mailing List; +Cc: Shawn O. Pearce
In-Reply-To: <20071013032100.GK27899@spearce.org>
On Oct 13, 2007, at 5:21 AM, Shawn O. Pearce wrote:
> Steffen Prohaska <prohaska@zib.de> wrote:
>> I read carefully through the documentation of git-send-pack and
>> git-rev-parse. The current implementation of git-send-pack is in line
>> with the documented behaviour, as is the implementation of git-rev-
>> parse.
>>
>> So formally everything is correct.
>>
>> But it is completely against my expectation that git-push <remote>
>> <head>
>> can successfully resolve a <head> that git-rev-parse fails to
>> parse. I
>> understand that refs are not revs ;). But nonetheless, I'd expect
>> that a
>> local ref that cannot be parsed by git-rev-parse should also fail
>> to be
>> pushed by git-send-pack. I didn't expect that git-send-pack would
>> locate
>> <head> as someprefix/<head>.
>>
>> Why is my expectation wrong?
>> Or is the current specification of git-send-pack's ref parsing wrong?
>
> I think its a bug. But I didn't write the original code.
>
> Meaning I think what happened here was someone wanted to enable
> git-send-pack to match "master" here with "refs/heads/master" on
> the remote side. One easy way to do that was to see if any ref
> ended with "/master", as that was what the ref here was called.
>
> Way back when that code was written most Git repositories probably
> only ever had that one branch anyway, or maybe two (refs/heads/master
> and refs/heads/origin) so matching the trailing suffix never came
> up as a bug. Nobody ever had two refs that could possibly match.
>
> Then the documentation got expanded to actually document the behavior
> that git-send-pack implemented. Unfortunately that codified the
> bug as documented behavior.
>
>
> So I agree with you Steffen, this is a bug in send-pack, and I run
> up against it every once in a while. I've specifically told my
> coworkers "NEVER, EVER, EVER, create a branch called 'master' that
> isn't exactly refs/heads/master OR ELSE I WILL COME BEAT YOU WITH A
> CLUE STICK". They still create "refs/heads/experiments/master".
> *sigh*.
>
> I think we should fix it. Anyone that is relying on "git push
> $url master" to resolve to "refs/heads/experimental/master" on the
> remote side is already playing with fire. But Junio is (rightfully
> so) very conservative and doesn't like to break a user's scripts.
> We may not be able to fix this until Git 1.6.
I'm working on this and will send patches tomorrow.
Steffen
^ permalink raw reply
* Re: [PATCH] Color support added to git-add--interactive.
From: Wincent Colaiuta @ 2007-10-13 17:14 UTC (permalink / raw)
To: Jean-Luc Herren; +Cc: git
In-Reply-To: <4710F47D.2070306@gmx.ch>
El 13/10/2007, a las 18:38, Jean-Luc Herren escribió:
> Here are my two cents.
>
> Wincent Colaiuta wrote:
>> +sub print_ansi_color($$;$) {
>> + my $color = shift;
>> + my $string = shift;
>> + my $trailer = shift;
>
> None of the other subs in this file have a prototype, so for
> consistency I'd suggest to not add it on this function either.
> However maybe a patch that adds it to all subs would be welcome.
> (I wouldn't see the necessity though.)
Yes, I saw that the other functions didn't use prototypes and I agree
that consistency would be a good thing. I liked the idea of it in
this case because it makes explicit the fact that the function takes
two params plus a third, optional one. So definitely not necessary,
but a preference of mine. In any case, a change to all the other
functions is a question for a separate patch.
> And the common way of getting the arguments is reading @_ (see all
> other subs in the file). So maybe instead write:
>
> [...]
> sub print_ansi_color {
> my ($color, $string, $trailer) = @_;
> [...]
Yes, I actually did write it that way first but then my doubts about
Perl made me write it the longer way; but if they are equivalent then
I prefer the shorter way.
>> + if ($use_color) {
>> + printf '%s%s%s', Term::ANSIColor::color($color), $string,
>> + Term::ANSIColor::color('clear');
>> + } else {
>
> Why use printf when you could directly use print here? It's only
> used for concatenating.
True. I had may brain in the C-world.
>> + if ($trailer) {
>> + print $trailer;
>> + }
>
> This will fail to print $trailer when $trailer happens to be a
> string that evaluates to false in bool context, like '0'. Write
> this as:
>
> if (defined $trailer) {
> print $trailer;
> }
Again, I actually wrote it that way the first time, and then changed
it, this time because I thought they were the same. Like I said, not
a perl hacker.
> IMHO, parsing the output of 'git diff-files --color' is a very bad
> idea and it makes all regexes uglier and more difficult to read.
> You're much better off recolorizing it yourself, which makes it a
> more localized change.
You're probably right, although it is also duplicating the work
that's already done elsewhere. In general I favor making the simplest
change that would work, and tweaking a few of the regexes did look
simpler than re-implementing the colorization logic.
But the approach you suggest might be more robust, perhaps, seeing as
there's not much to the diff output. As far as I can tell there are
really only five or six different things to look for, and they'd be
fairly easy to catch:
- lines beginning with "@@ " (hunk headers)
- lines beginning with "+" (insertions)
- lines beginning with "-" (deletions)
- lines beginning with " " (context lines, no color)
- lines beginning with "\" (things like "\ No newline at end of
file", again, no color)
- everything else; ie. the diff header stuff (eg "diff --git a/foo b/
foo")
The only special cases seem to be the "+++" and "---" lines in the
header, which look like insertions and deletions when they're not.
Trickier would be the highlighting of dubious whitespace, and that's
when it starts to sound like re-inventing the wheel and duplicating
the logic for the detection that's defined elsewhere (possibly in
diff-lib.c? haven't found the exact spot yet).
> Especially, I don't think that you have
> any guarantee that escape sequences won't ever contain the
> characters '+', '-' or ' ' (space)
Yes, that was one of the things I didn't like about the sloppy
regexes. I couldn't really make them any stricter though because I
wasn't confident about the range of possible characters that might be
included in the escape sequences.
> Finally -- and this might be just my eyes -- blue is a very nice
> color, but it looks a bit too dark on black background. Maybe
> choose a default color that looks reasonable on black *and* white
> background.
Yeah, well I didn't choose the colours and I didn't really want to
get into it. Before being considered for inclusion a patch like this
would need to tap in to the existing config settings for color.diff
and color.diff.<slot> anyway...
Wincent
^ permalink raw reply
* Re: [PATCH] Color support added to git-add--interactive.
From: Jeff King @ 2007-10-13 17:27 UTC (permalink / raw)
To: Wincent Colaiuta
Cc: Dan Zwell, Git Mailing List, Jonathan del Strother,
Johannes Schindelin, Frank Lichtenheld
In-Reply-To: <19271E58-5C4F-41AF-8F9D-F114F36A34AC@wincent.com>
On Sat, Oct 13, 2007 at 04:45:41PM +0200, Wincent Colaiuta wrote:
> - as Johannes pointed out, "clear" and "reset" are not used consistently
> even though the Term::ANSIColor documentation says that they're the same, so
> settled on "clear"; although in any case, the changes to the
> print_ansi_color function mean that it is now the only site where clearing
> takes place
Please use "reset", as that is the term used by the C color code.
> - changed the regex as suggested by Johannes, and a couple of others that
> are used when splitting hunks
I believe there are other places where the diff output is parsed, and
the colors will mess that up, too (e.g., split_hunk). All of those
regexes need to be changed, too. I am a bit concerned that we are
putting intimate knowledge of the colorization scheme here. As much as
it pains me to have two diff colorizers, I wonder if that would be a
better solution than having a diff colorizer, and a colorized diff
parser.
-Peff
^ permalink raw reply
* [PATCH] gitk: Added support for OS X mouse wheel
From: Jonathan del Strother @ 2007-10-13 17:33 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Väinö Järvelä
(Väinö Järvelä supplied this patch a while ago for 1.5.2. It no longer
applied cleanly, so I'm reposting it.)
MacBook doesn't seem to recognize MouseRelease-4 and -5 events, at all.
So i added a support for the MouseWheel event, which i limited to Tcl/tk
aqua, as i couldn't test it neither on Linux or Windows. Tcl/tk needs to
be updated from the version that is shipped with OS X 10.4 Tiger, for
this patch to work.
Signed-off-by: Jonathan del Strother <jon.delStrother@bestbefore.tv>
---
gitk | 14 ++++++++++----
1 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/gitk b/gitk
index 300fdce..9b3e627 100755
--- a/gitk
+++ b/gitk
@@ -838,11 +838,17 @@ proc makewindow {} {
bindall <1> {selcanvline %W %x %y}
#bindall <B1-Motion> {selcanvline %W %x %y}
if {[tk windowingsystem] == "win32"} {
- bind . <MouseWheel> { windows_mousewheel_redirector %W %X %Y %D }
- bind $ctext <MouseWheel> { windows_mousewheel_redirector %W %X %Y %
D ; break }
+ bind . <MouseWheel> { windows_mousewheel_redirector %W %X %Y
%D }
+ bind $ctext <MouseWheel> { windows_mousewheel_redirector %W %
X %Y %D ; break }
} else {
- bindall <ButtonRelease-4> "allcanvs yview scroll -5 units"
- bindall <ButtonRelease-5> "allcanvs yview scroll 5 units"
+ bindall <ButtonRelease-4> "allcanvs yview scroll -5 units"
+ bindall <ButtonRelease-5> "allcanvs yview scroll 5 units"
+ if {[tk windowingsystem] eq "aqua"} {
+ bindall <MouseWheel> {
+ set delta [expr {- (%D)}]
+ allcanvs yview scroll $delta units
+ }
+ }
}
bindall <2> "canvscan mark %W %x %y"
bindall <B2-Motion> "canvscan dragto %W %x %y"
--
1.5.3.1
^ permalink raw reply related
* Re: [PATCH] Color support added to git-add--interactive.
From: Jeff King @ 2007-10-13 17:51 UTC (permalink / raw)
To: Wincent Colaiuta
Cc: Dan Zwell, Git Mailing List, Jonathan del Strother,
Johannes Schindelin, Frank Lichtenheld
In-Reply-To: <20071013172745.GA2624@coredump.intra.peff.net>
On Sat, Oct 13, 2007 at 01:27:45PM -0400, Jeff King wrote:
> I believe there are other places where the diff output is parsed, and
> the colors will mess that up, too (e.g., split_hunk). All of those
Oops, I see you actually dealt with those already (I just responded to
your cover letter first). Though I am still concerned about the
robustness of the re-parsing scheme.
-Peff
^ permalink raw reply
* Re: Git User's Survey 2007 unfinished summary continued
From: Andreas Ericsson @ 2007-10-13 18:00 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jakub Narebski, git
In-Reply-To: <Pine.LNX.4.64.0710130130380.25221@racer.site>
Johannes Schindelin wrote:
>
> At the GSoC mentor summit, I encountered a rather different stance: people
> did not _know_ what distributed SCM means, and were rather afraid of the
> concept. Some of them seemed to fight changing their known procedures
> tooth and nail. Which is fine by me (I don't have to force anybody to
> use git, thankyouverymuch).
>
I recently attended a fairly large opensource conference in germany, where
companies that somehow do business around the opensource network
monitoring tool named Nagios gather and drink themselves and each other
into insensibility once a year. I ran into much the same issues there, even though
it would have made all our lives a whole lot easier if we could pull patches from
each others repositories.
--
Andreas Ericsson andreas.ericsson@op5.se
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-230231
^ permalink raw reply
* Re: [PATCH] Fixing path quoting issues
From: Jonathan del Strother @ 2007-10-13 18:12 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, Junio C Hamano
In-Reply-To: <470DC05A.8020209@viscovery.net>
On 11 Oct 2007, at 07:19, Johannes Sixt wrote:
>> - git-commit -F msg -m amending ."
>> + git-commit -F msg -m amending ."
>
> You fix whitespace...
>
>> test_expect_success \
>> - "using message from other commit" \
>> - "git-commit -C HEAD^ ."
>> + "using message from other commit" \
>> + "git-commit -C HEAD^ ."
>
> ... and you break it. More of these follow. Don't do that, it makes
> patch review unnecessarily hard.
I'm just preparing to release this patch... was that "don't break
whitespace", or "don't try to fix whitespace in a patch that's has
nothing to do with whitespacing-fixing" ?
And while I'm here - tabs are preferred, are they? There seem to be
a mixture of tabs & 4 space indentation.
^ permalink raw reply
* Re: [PATCH] Color support added to git-add--interactive.
From: Andreas Ericsson @ 2007-10-13 18:31 UTC (permalink / raw)
To: Jean-Luc Herren; +Cc: Wincent Colaiuta, git
In-Reply-To: <4710F47D.2070306@gmx.ch>
Jean-Luc Herren wrote:
> Hi!
>
> I really like the idea of colorizing git add -i, especially the
> prompt. Here are my two cents.
>
> Wincent Colaiuta wrote:
>> +sub print_ansi_color($$;$) {
>> + my $color = shift;
>> + my $string = shift;
>> + my $trailer = shift;
>
> None of the other subs in this file have a prototype, so for
> consistency I'd suggest to not add it on this function either.
> However maybe a patch that adds it to all subs would be welcome.
> (I wouldn't see the necessity though.)
>
> And the common way of getting the arguments is reading @_ (see all
> other subs in the file). So maybe instead write:
>
> [...]
> sub print_ansi_color {
> my ($color, $string, $trailer) = @_;
> [...]
>
>> + if ($use_color) {
>> + printf '%s%s%s', Term::ANSIColor::color($color), $string,
>> + Term::ANSIColor::color('clear');
>> + } else {
>
> Why use printf when you could directly use print here? It's only
> used for concatenating.
>
>> + if ($trailer) {
>> + print $trailer;
>> + }
>
> This will fail to print $trailer when $trailer happens to be a
> string that evaluates to false in bool context, like '0'. Write
> this as:
>
> if (defined $trailer) {
> print $trailer;
> }
>
> IMHO, parsing the output of 'git diff-files --color' is a very bad
> idea and it makes all regexes uglier and more difficult to read.
> You're much better off recolorizing it yourself, which makes it a
> more localized change. Especially, I don't think that you have
> any guarantee that escape sequences won't ever contain the
> characters '+', '-' or ' ' (space), which would break your code on
> lines like this:
>
>> + if ($line =~ /^[^-+ ]*\+/) {
>
> Finally -- and this might be just my eyes -- blue is a very nice
> color, but it looks a bit too dark on black background. Maybe
> choose a default color that looks reasonable on black *and* white
> background.
>
Red for removed and green for added seems to be the standard, although
I know it makes life terribly difficult for red-green colorblind people,
who usually prefer yellow/lightblue for black bg, or beige/blue for
white background.
--
Andreas Ericsson andreas.ericsson@op5.se
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-230231
^ permalink raw reply
* Re: [PATCH] Add a simple option parser.
From: Alex Riesen @ 2007-10-13 19:16 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: git, Junio C Hamano
In-Reply-To: <1192282153-26684-2-git-send-email-madcoder@debian.org>
Pierre Habouzit, Sat, Oct 13, 2007 15:29:03 +0200:
> +static int opterror(struct option *opt, const char *reason, int flags)
"const struct option *opt"? You never modify the struct option itself,
only the values under the pointers it contains. Using const here will
allow the compiler to reuse string constants (not that there will be
much of the opportunity, but anyway) in the option arrays.
> +static int get_value(struct optparse_t *p, struct option *opt, int flags)
"const struct option *opt"?
> +static int parse_short_opt(struct optparse_t *p, struct option *options, int count)
"const struct option *options"?
> +int parse_options(int argc, const char **argv,
> + struct option *options, int count,
> + const char * const usagestr[], int flags)
"const struct option *options"?
> +void make_usage(const char * const usagestr[], struct option *opts, int cnt)
"const struct option *opts"?
Why not "const char *const *usagestr"? Especially if you change
"usagestr" (the pointer itself) later. "[]" is sometimes a hint that
the pointer itself should not be changed, being an array.
And you want make opts const.
BTW, it does not "make" usage. It calls the usage() or prints a usage
description. "make" implies it creates the "usage", which according to
the prototype is later nowhere to be found.
> +{
> + struct strbuf sb;
> +
> + strbuf_init(&sb, 4096);
> + do {
> + strbuf_addstr(&sb, *usagestr++);
> + strbuf_addch(&sb, '\n');
> + } while (*usagestr);
This will crash for empty usagestr, like "{ NULL }". Was it
deliberately? (I'd make it deliberately, if I were you. I'd even used
cnt of opts, to force people to document all options).
> + strbuf_addf(&sb, "\n%*s%s\n", USAGE_OPTS_WIDTH + USAGE_GAP, "",
> + opts->help);
...
> + usage(sb.buf);
BTW, if you just printed the usage message out (it is about usage of a
program, isn't it?) and called exit() everyone would be just as happy.
And you wouldn't have to include strbuf (it is the only use of it),
less code, too. It'd make simplier to stea^Wcopy your implementation,
which I like :)
^ permalink raw reply
* Re: [PATCH] Port builtin-add.c to use the new option parser.
From: Alex Riesen @ 2007-10-13 19:22 UTC (permalink / raw)
To: Pierre Habouzit, Johannes Schindelin, git, Junio C Hamano
In-Reply-To: <20071013150306.GH7110@artemis.corp>
Pierre Habouzit, Sat, Oct 13, 2007 17:03:06 +0200:
> On Sat, Oct 13, 2007 at 02:47:20PM +0000, Johannes Schindelin wrote:
> > Thinking about this more, I am reverting my stance on the ARRAY_SIZE()
> > issue. I think if you introduce a "OPTION_NONE = 0" in the enum, then
> > this single last comma should be enough.
>
> adding a trailing comma does not add a NULL after that, it's ignored,
> you're confused.
Yep
> Note that I don't really like using ARRAY_SIZE either, I kept it that
> way, but my taste would rather be to have an "empty" option, and
> explicitely mark the end of the array.
You can have both. Just stop at NULL-entry or when the 'size' elements
passed, whatever happens first.
^ permalink raw reply
* Re: Git User's Survey 2007 unfinished summary continued
From: David Kastrup @ 2007-10-13 19:59 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jakub Narebski, git
In-Reply-To: <Pine.LNX.4.64.0710130130380.25221@racer.site>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Jakub, thank you very much for doing this. It is a very tedious
> work, and I deem it invaluable.
And yet you trash the results.
>> Git is just too complicated for a typical project. I understand
>> it's probably great for the Linux kernel but for a smaller
>> project like mine (Mesa) it's overkill and a frustration. (...)
>> With git everything seems hard. (...) I've _wasted_ hours
>> trying to figure out git. That alone is a huge issue. I guess I
>> could go into specific details about my problems with git but
>> I've already spent enough time on this survey.
>
> I find it always a little strange how people want to use something
> like git, but are unwilling to ask. Is this such a big attack on
> the manliness to admist one needs help or what?
Not everybody likes getting the kind of treatment you find fit to dish
out.
>> Figure out why people find git hard to learn and eliminate those
>> barriers to entry. Make git more task-oriented rather than
>> data-model-oriented the way it is now.
>
> Frankly, expectations like these make me want to bang somebody's
> head on the wall.
And you wonder that people are unwilling to ask for things on the
list? When even mentioning something in a _survey_ makes core
developers want to bang their heads against a wall?
I find it a pity that my suggestion to ask about how comfortable
people are with the tone on the list did not make it into the survey.
Enough core developers make the tone sufficiently unconstructive to
make it quite understandable that people are unwilling to ask
questions here, in order to avoid getting their heads banged against a
wall, virtual or not.
--
David Kastrup, Kriemhildstr. 15, 44793 Bochum
^ permalink raw reply
* Re: [PATCH] Color support added to git-add--interactive.
From: Dan Zwell @ 2007-10-13 20:03 UTC (permalink / raw)
To: Jeff King
Cc: Wincent Colaiuta, Git Mailing List, Jonathan del Strother,
Johannes Schindelin, Frank Lichtenheld
In-Reply-To: <20071013175127.GA3183@coredump.intra.peff.net>
Jeff King wrote:
> On Sat, Oct 13, 2007 at 01:27:45PM -0400, Jeff King wrote:
>
> <snip> Though I am still concerned about the
> robustness of the re-parsing scheme.
>
> -Peff
>
The importance of the diff coloring pales in comparison to the prompt
coloring. Diff coloring is useful, but prompt coloring is a basic
usability concern (if people can't easily tell where a hunk begins, the
tool becomes annoying). Perhaps we could split this into two patches,
merging the first after a few small changes can be taken care of, while
the second may need more discussion and testing. The coloring of the
prompts is relatively low risk. It just needs to be modified to take
color settings from .git/config. I was thinking that this might be the
example that I would take settings from:
[color]
add-interactive = auto
[color "add-interactive"]
prompt = bold blue
header = bold
help = blue
For the sake of a unified interface, the "Stage this hunk?" prompt
should be colored the same as the other prompts. I will give a bit of
thought to the default colors, though it's important to avoid red and
green (as those will look like diff output when the second patch is
applied).
Also needed is some command line parsing so that "--color" can be
specified on the command line (very small change), and all of this
should be added to the documentation.
Obviously, the suggestions/fixes from other parts of this thread must be
taken into account, as well. I can probably do all this tomorrow (and
send low-risk/high-risk patches), unless someone takes it before me.
Dan
^ permalink raw reply
* [PATCH 0/14] fork/exec removal series
From: Johannes Sixt @ 2007-10-13 20:06 UTC (permalink / raw)
To: gitster; +Cc: git, Johannes Sixt
Junio,
here is a series of patches that removes a number fork/exec pairs.
The series consists of 2 parts:
- The first half replaces a number of fork/exec pairs by start_command/
finish_command or run_command.
- The second half introduces a new framework that runs a function
asynchronously. New functions start_async and finish_async are implemented
similarly to start_command and run_command. They are used to replace
occurrences of fork() that does not exec() in the child. Such code
could in principle be run in a thread, and on MinGW port we will go this
route, but on Posix we stay with fork().
The series can be applied on top of 2b5a06e (Restore default verbosity for
http fetches), as you reqested, but that commit does not compile, so I
developed it on 90446a00 (bundle transport: fix an alloc_ref() call),
which is a few commits earlier.
There will be some strbuf related merge conflicts once you merge this into
master.
builtin-archive.c | 8 +-
builtin-fetch-pack.c | 101 +++++++++----------------
cache.h | 4 +-
connect.c | 131 ++++++++++++++++-----------------
convert.c | 87 ++++++++--------------
diff.c | 38 +---------
peek-remote.c | 8 +-
run-command.c | 79 +++++++++++++++++---
run-command.h | 24 ++++++
send-pack.c | 8 +-
t/t0021-conversion.sh | 7 ++-
transport.c | 9 +--
upload-pack.c | 199 ++++++++++++++++++++++---------------------------
13 files changed, 334 insertions(+), 369 deletions(-)
-- Hannes
^ permalink raw reply
* [PATCH 02/14] Use start_command() in git_connect() instead of explicit fork/exec.
From: Johannes Sixt @ 2007-10-13 20:06 UTC (permalink / raw)
To: gitster; +Cc: git, Johannes Sixt
In-Reply-To: <1192305984-22594-2-git-send-email-johannes.sixt@telecom.at>
The child process handling is delegated to start_command() and
finish_command().
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
connect.c | 108 ++++++++++++++++++++++++++++--------------------------------
1 files changed, 50 insertions(+), 58 deletions(-)
diff --git a/connect.c b/connect.c
index b156f92..21597bf 100644
--- a/connect.c
+++ b/connect.c
@@ -484,11 +484,15 @@ struct child_process *git_connect(int fd[2], char *url,
char *host, *path = url;
char *end;
int c;
- int pipefd[2][2];
struct child_process *conn;
enum protocol protocol = PROTO_LOCAL;
int free_path = 0;
char *port = NULL;
+ const char **arg;
+ char command[MAX_CMD_LEN];
+ char *posn = command;
+ int size = MAX_CMD_LEN;
+ int of = 0;
/* Without this we cannot rely on waitpid() to tell
* what happened to our children.
@@ -574,62 +578,51 @@ struct child_process *git_connect(int fd[2], char *url,
return NULL;
}
- if (pipe(pipefd[0]) < 0 || pipe(pipefd[1]) < 0)
- die("unable to create pipe pair for communication");
conn = xcalloc(1, sizeof(*conn));
- conn->pid = fork();
- if (conn->pid < 0)
- die("unable to fork");
- if (!conn->pid) {
- char command[MAX_CMD_LEN];
- char *posn = command;
- int size = MAX_CMD_LEN;
- int of = 0;
-
- of |= add_to_string(&posn, &size, prog, 0);
- of |= add_to_string(&posn, &size, " ", 0);
- of |= add_to_string(&posn, &size, path, 1);
-
- if (of)
- die("command line too long");
-
- dup2(pipefd[1][0], 0);
- dup2(pipefd[0][1], 1);
- close(pipefd[0][0]);
- close(pipefd[0][1]);
- close(pipefd[1][0]);
- close(pipefd[1][1]);
- if (protocol == PROTO_SSH) {
- const char *ssh, *ssh_basename;
- ssh = getenv("GIT_SSH");
- if (!ssh) ssh = "ssh";
- ssh_basename = strrchr(ssh, '/');
- if (!ssh_basename)
- ssh_basename = ssh;
- else
- ssh_basename++;
- if (!port)
- execlp(ssh, ssh_basename, host, command, NULL);
- else
- execlp(ssh, ssh_basename, "-p", port, host,
- command, NULL);
- }
- else {
- unsetenv(ALTERNATE_DB_ENVIRONMENT);
- unsetenv(DB_ENVIRONMENT);
- unsetenv(GIT_DIR_ENVIRONMENT);
- unsetenv(GIT_WORK_TREE_ENVIRONMENT);
- unsetenv(GRAFT_ENVIRONMENT);
- unsetenv(INDEX_ENVIRONMENT);
- execlp("sh", "sh", "-c", command, NULL);
+ of |= add_to_string(&posn, &size, prog, 0);
+ of |= add_to_string(&posn, &size, " ", 0);
+ of |= add_to_string(&posn, &size, path, 1);
+
+ if (of)
+ die("command line too long");
+
+ conn->in = conn->out = -1;
+ conn->argv = arg = xcalloc(6, sizeof(*arg));
+ if (protocol == PROTO_SSH) {
+ const char *ssh = getenv("GIT_SSH");
+ if (!ssh) ssh = "ssh";
+
+ *arg++ = ssh;
+ if (port) {
+ *arg++ = "-p";
+ *arg++ = port;
}
- die("exec failed");
+ *arg++ = host;
+ }
+ else {
+ /* remove these from the environment */
+ const char *env[] = {
+ ALTERNATE_DB_ENVIRONMENT,
+ DB_ENVIRONMENT,
+ GIT_DIR_ENVIRONMENT,
+ GIT_WORK_TREE_ENVIRONMENT,
+ GRAFT_ENVIRONMENT,
+ INDEX_ENVIRONMENT,
+ NULL
+ };
+ conn->env = env;
+ *arg++ = "sh";
+ *arg++ = "-c";
}
- fd[0] = pipefd[0][0];
- fd[1] = pipefd[1][1];
- close(pipefd[0][1]);
- close(pipefd[1][0]);
+ *arg++ = command;
+ *arg = NULL;
+
+ if (start_command(conn))
+ die("unable to fork");
+
+ fd[0] = conn->out; /* read from child's stdout */
+ fd[1] = conn->in; /* write to child's stdin */
if (free_path)
free(path);
return conn;
@@ -637,13 +630,12 @@ struct child_process *git_connect(int fd[2], char *url,
int finish_connect(struct child_process *conn)
{
+ int code;
if (conn == NULL)
return 0;
- while (waitpid(conn->pid, NULL, 0) < 0) {
- if (errno != EINTR)
- return -1;
- }
+ code = finish_command(conn);
+ free(conn->argv);
free(conn);
- return 0;
+ return code;
}
--
1.5.3.3.1134.gee562
^ permalink raw reply related
* [PATCH 03/14] Use start_command() to run content filters instead of explicit fork/exec.
From: Johannes Sixt @ 2007-10-13 20:06 UTC (permalink / raw)
To: gitster; +Cc: git, Johannes Sixt
In-Reply-To: <1192305984-22594-3-git-send-email-johannes.sixt@telecom.at>
The previous code already used finish_command() to wait for the process
to terminate, but did not use start_command() to run it.
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
convert.c | 30 +++++++-----------------------
1 files changed, 7 insertions(+), 23 deletions(-)
diff --git a/convert.c b/convert.c
index d77c8eb..6d64994 100644
--- a/convert.c
+++ b/convert.c
@@ -208,34 +208,18 @@ static int filter_buffer(const char *path, const char *src,
* Spawn cmd and feed the buffer contents through its stdin.
*/
struct child_process child_process;
- int pipe_feed[2];
int write_err, status;
+ const char *argv[] = { "sh", "-c", cmd, NULL };
memset(&child_process, 0, sizeof(child_process));
+ child_process.argv = argv;
+ child_process.in = -1;
- if (pipe(pipe_feed) < 0) {
- error("cannot create pipe to run external filter %s", cmd);
- return 1;
- }
-
- child_process.pid = fork();
- if (child_process.pid < 0) {
- error("cannot fork to run external filter %s", cmd);
- close(pipe_feed[0]);
- close(pipe_feed[1]);
- return 1;
- }
- if (!child_process.pid) {
- dup2(pipe_feed[0], 0);
- close(pipe_feed[0]);
- close(pipe_feed[1]);
- execlp("sh", "sh", "-c", cmd, NULL);
- return 1;
- }
- close(pipe_feed[0]);
+ if (start_command(&child_process))
+ return error("cannot fork to run external filter %s", cmd);
- write_err = (write_in_full(pipe_feed[1], src, size) < 0);
- if (close(pipe_feed[1]))
+ write_err = (write_in_full(child_process.in, src, size) < 0);
+ if (close(child_process.in))
write_err = 1;
if (write_err)
error("cannot feed the input to external filter %s", cmd);
--
1.5.3.3.1134.gee562
^ permalink raw reply related
* [PATCH 01/14] Change git_connect() to return a struct child_process instead of a pid_t.
From: Johannes Sixt @ 2007-10-13 20:06 UTC (permalink / raw)
To: gitster; +Cc: git, Johannes Sixt
In-Reply-To: <1192305984-22594-1-git-send-email-johannes.sixt@telecom.at>
This prepares the API of git_connect() and finish_connect() to operate on
a struct child_process. Currently, we just use that object as a placeholder
for the pid that we used to return. A follow-up patch will change the
implementation of git_connect() and finish_connect() to make full use
of the object.
Old code had early-return-on-error checks at the calling sites of
git_connect(), but since git_connect() dies on errors anyway, these checks
were removed.
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
builtin-archive.c | 8 +++-----
builtin-fetch-pack.c | 8 +++-----
cache.h | 4 ++--
connect.c | 31 +++++++++++++++++--------------
peek-remote.c | 8 +++-----
send-pack.c | 8 +++-----
transport.c | 9 ++-------
7 files changed, 33 insertions(+), 43 deletions(-)
diff --git a/builtin-archive.c b/builtin-archive.c
index a90c65c..346444b 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -30,7 +30,7 @@ static int run_remote_archiver(const char *remote, int argc,
{
char *url, buf[LARGE_PACKET_MAX];
int fd[2], i, len, rv;
- pid_t pid;
+ struct child_process *conn;
const char *exec = "git-upload-archive";
int exec_at = 0;
@@ -46,9 +46,7 @@ static int run_remote_archiver(const char *remote, int argc,
}
url = xstrdup(remote);
- pid = git_connect(fd, url, exec, 0);
- if (pid < 0)
- return pid;
+ conn = git_connect(fd, url, exec, 0);
for (i = 1; i < argc; i++) {
if (i == exec_at)
@@ -76,7 +74,7 @@ static int run_remote_archiver(const char *remote, int argc,
rv = recv_sideband("archive", fd[0], 1, 2);
close(fd[0]);
close(fd[1]);
- rv |= finish_connect(pid);
+ rv |= finish_connect(conn);
return !!rv;
}
diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index 8f25d50..f56592f 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -762,7 +762,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
{
int i, ret;
int fd[2];
- pid_t pid;
+ struct child_process *conn;
struct ref *ref;
struct stat st;
@@ -773,16 +773,14 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
st.st_mtime = 0;
}
- pid = git_connect(fd, (char *)dest, uploadpack,
+ conn = git_connect(fd, (char *)dest, uploadpack,
args.verbose ? CONNECT_VERBOSE : 0);
- if (pid < 0)
- return NULL;
if (heads && nr_heads)
nr_heads = remove_duplicates(nr_heads, heads);
ref = do_fetch_pack(fd, nr_heads, heads, pack_lockfile);
close(fd[0]);
close(fd[1]);
- ret = finish_connect(pid);
+ ret = finish_connect(conn);
if (!ret && nr_heads) {
/* If the heads to pull were given, we should have
diff --git a/cache.h b/cache.h
index 16bdbb2..c428f87 100644
--- a/cache.h
+++ b/cache.h
@@ -502,8 +502,8 @@ struct ref {
#define REF_TAGS (1u << 2)
#define CONNECT_VERBOSE (1u << 0)
-extern pid_t git_connect(int fd[2], char *url, const char *prog, int flags);
-extern int finish_connect(pid_t pid);
+extern struct child_process *git_connect(int fd[2], char *url, const char *prog, int flags);
+extern int finish_connect(struct child_process *conn);
extern int path_match(const char *path, int nr, char **match);
extern int get_ack(int fd, unsigned char *result_sha1);
extern struct ref **get_remote_heads(int in, struct ref **list, int nr_match, char **match, unsigned int flags);
diff --git a/connect.c b/connect.c
index aee78ff..b156f92 100644
--- a/connect.c
+++ b/connect.c
@@ -470,21 +470,22 @@ char *get_port(char *host)
}
/*
- * This returns 0 if the transport protocol does not need fork(2),
- * or a process id if it does. Once done, finish the connection
+ * This returns NULL if the transport protocol does not need fork(2), or a
+ * struct child_process object if it does. Once done, finish the connection
* with finish_connect() with the value returned from this function
- * (it is safe to call finish_connect() with 0 to support the former
+ * (it is safe to call finish_connect() with NULL to support the former
* case).
*
- * Does not return a negative value on error; it just dies.
+ * If it returns, the connect is successful; it just dies on errors.
*/
-pid_t git_connect(int fd[2], char *url, const char *prog, int flags)
+struct child_process *git_connect(int fd[2], char *url,
+ const char *prog, int flags)
{
char *host, *path = url;
char *end;
int c;
int pipefd[2][2];
- pid_t pid;
+ struct child_process *conn;
enum protocol protocol = PROTO_LOCAL;
int free_path = 0;
char *port = NULL;
@@ -570,15 +571,16 @@ pid_t git_connect(int fd[2], char *url, const char *prog, int flags)
free(target_host);
if (free_path)
free(path);
- return 0;
+ return NULL;
}
if (pipe(pipefd[0]) < 0 || pipe(pipefd[1]) < 0)
die("unable to create pipe pair for communication");
- pid = fork();
- if (pid < 0)
+ conn = xcalloc(1, sizeof(*conn));
+ conn->pid = fork();
+ if (conn->pid < 0)
die("unable to fork");
- if (!pid) {
+ if (!conn->pid) {
char command[MAX_CMD_LEN];
char *posn = command;
int size = MAX_CMD_LEN;
@@ -630,17 +632,18 @@ pid_t git_connect(int fd[2], char *url, const char *prog, int flags)
close(pipefd[1][0]);
if (free_path)
free(path);
- return pid;
+ return conn;
}
-int finish_connect(pid_t pid)
+int finish_connect(struct child_process *conn)
{
- if (pid == 0)
+ if (conn == NULL)
return 0;
- while (waitpid(pid, NULL, 0) < 0) {
+ while (waitpid(conn->pid, NULL, 0) < 0) {
if (errno != EINTR)
return -1;
}
+ free(conn);
return 0;
}
diff --git a/peek-remote.c b/peek-remote.c
index ceb7871..8d20f7c 100644
--- a/peek-remote.c
+++ b/peek-remote.c
@@ -25,7 +25,7 @@ int main(int argc, char **argv)
int i, ret;
char *dest = NULL;
int fd[2];
- pid_t pid;
+ struct child_process *conn;
int nongit = 0;
unsigned flags = 0;
@@ -64,12 +64,10 @@ int main(int argc, char **argv)
if (!dest || i != argc - 1)
usage(peek_remote_usage);
- pid = git_connect(fd, dest, uploadpack, 0);
- if (pid < 0)
- return 1;
+ conn = git_connect(fd, dest, uploadpack, 0);
ret = peek_remote(fd, flags);
close(fd[0]);
close(fd[1]);
- ret |= finish_connect(pid);
+ ret |= finish_connect(conn);
return !!ret;
}
diff --git a/send-pack.c b/send-pack.c
index 4533d1b..b562953 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -362,7 +362,7 @@ int main(int argc, char **argv)
char *dest = NULL;
char **heads = NULL;
int fd[2], ret;
- pid_t pid;
+ struct child_process *conn;
char *remote_name = NULL;
struct remote *remote = NULL;
@@ -426,12 +426,10 @@ int main(int argc, char **argv)
}
}
- pid = git_connect(fd, dest, receivepack, verbose ? CONNECT_VERBOSE : 0);
- if (pid < 0)
- return 1;
+ conn = git_connect(fd, dest, receivepack, verbose ? CONNECT_VERBOSE : 0);
ret = send_pack(fd[0], fd[1], remote, nr_heads, heads);
close(fd[0]);
close(fd[1]);
- ret |= finish_connect(pid);
+ ret |= finish_connect(conn);
return !!ret;
}
diff --git a/transport.c b/transport.c
index 3475cca..5d723b0 100644
--- a/transport.c
+++ b/transport.c
@@ -279,18 +279,13 @@ static struct ref *get_refs_via_connect(const struct transport *transport)
struct git_transport_data *data = transport->data;
struct ref *refs;
int fd[2];
- pid_t pid;
char *dest = xstrdup(transport->url);
-
- pid = git_connect(fd, dest, data->uploadpack, 0);
-
- if (pid < 0)
- die("Failed to connect to \"%s\"", transport->url);
+ struct child_process *conn = git_connect(fd, dest, data->uploadpack, 0);
get_remote_heads(fd[0], &refs, 0, NULL, 0);
packet_flush(fd[1]);
- finish_connect(pid);
+ finish_connect(conn);
free(dest);
--
1.5.3.3.1134.gee562
^ permalink raw reply related
* [PATCH 04/14] Use run_command() to spawn external diff programs instead of fork/exec.
From: Johannes Sixt @ 2007-10-13 20:06 UTC (permalink / raw)
To: gitster; +Cc: git, Johannes Sixt
In-Reply-To: <1192305984-22594-4-git-send-email-johannes.sixt@telecom.at>
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
diff.c | 38 +++-----------------------------------
1 files changed, 3 insertions(+), 35 deletions(-)
diff --git a/diff.c b/diff.c
index 0ee9ea1..d03dc6a 100644
--- a/diff.c
+++ b/diff.c
@@ -9,6 +9,7 @@
#include "xdiff-interface.h"
#include "color.h"
#include "attr.h"
+#include "run-command.h"
#ifdef NO_FAST_WORKING_DIRECTORY
#define FAST_WORKING_DIRECTORY 0
@@ -1791,40 +1792,6 @@ static void remove_tempfile_on_signal(int signo)
raise(signo);
}
-static int spawn_prog(const char *pgm, const char **arg)
-{
- pid_t pid;
- int status;
-
- fflush(NULL);
- pid = fork();
- if (pid < 0)
- die("unable to fork");
- if (!pid) {
- execvp(pgm, (char *const*) arg);
- exit(255);
- }
-
- while (waitpid(pid, &status, 0) < 0) {
- if (errno == EINTR)
- continue;
- return -1;
- }
-
- /* Earlier we did not check the exit status because
- * diff exits non-zero if files are different, and
- * we are not interested in knowing that. It was a
- * mistake which made it harder to quit a diff-*
- * session that uses the git-apply-patch-script as
- * the GIT_EXTERNAL_DIFF. A custom GIT_EXTERNAL_DIFF
- * should also exit non-zero only when it wants to
- * abort the entire diff-* session.
- */
- if (WIFEXITED(status) && !WEXITSTATUS(status))
- return 0;
- return -1;
-}
-
/* An external diff command takes:
*
* diff-cmd name infile1 infile1-sha1 infile1-mode \
@@ -1877,7 +1844,8 @@ static void run_external_diff(const char *pgm,
*arg++ = name;
}
*arg = NULL;
- retval = spawn_prog(pgm, spawn_arg);
+ fflush(NULL);
+ retval = run_command_v_opt(spawn_arg, 0);
remove_tempfile();
if (retval) {
fprintf(stderr, "external diff died, stopping at %s.\n", name);
--
1.5.3.3.1134.gee562
^ permalink raw reply related
* [PATCH 05/14] Use start_comand() in builtin-fetch-pack.c instead of explicit fork/exec.
From: Johannes Sixt @ 2007-10-13 20:06 UTC (permalink / raw)
To: gitster; +Cc: git, Johannes Sixt
In-Reply-To: <1192305984-22594-5-git-send-email-johannes.sixt@telecom.at>
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
builtin-fetch-pack.c | 56 ++++++++++++++-----------------------------------
1 files changed, 16 insertions(+), 40 deletions(-)
diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index f56592f..871b704 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -7,6 +7,7 @@
#include "pack.h"
#include "sideband.h"
#include "fetch-pack.h"
+#include "run-command.h"
static int transfer_unpack_limit = -1;
static int fetch_unpack_limit = -1;
@@ -491,18 +492,19 @@ static pid_t setup_sideband(int fd[2], int xd[2])
static int get_pack(int xd[2], char **pack_lockfile)
{
- int status;
- pid_t pid, side_pid;
+ pid_t side_pid;
int fd[2];
const char *argv[20];
char keep_arg[256];
char hdr_arg[256];
const char **av;
int do_keep = args.keep_pack;
- int keep_pipe[2];
+ struct child_process cmd;
side_pid = setup_sideband(fd, xd);
+ memset(&cmd, 0, sizeof(cmd));
+ cmd.argv = argv;
av = argv;
*hdr_arg = 0;
if (!args.keep_pack && unpack_limit) {
@@ -519,8 +521,8 @@ static int get_pack(int xd[2], char **pack_lockfile)
}
if (do_keep) {
- if (pack_lockfile && pipe(keep_pipe))
- die("fetch-pack: pipe setup failure: %s", strerror(errno));
+ if (pack_lockfile)
+ cmd.out = -1;
*av++ = "index-pack";
*av++ = "--stdin";
if (!args.quiet && !args.no_progress)
@@ -544,43 +546,17 @@ static int get_pack(int xd[2], char **pack_lockfile)
*av++ = hdr_arg;
*av++ = NULL;
- pid = fork();
- if (pid < 0)
+ cmd.in = fd[0];
+ cmd.git_cmd = 1;
+ if (start_command(&cmd))
die("fetch-pack: unable to fork off %s", argv[0]);
- if (!pid) {
- dup2(fd[0], 0);
- if (do_keep && pack_lockfile) {
- dup2(keep_pipe[1], 1);
- close(keep_pipe[0]);
- close(keep_pipe[1]);
- }
- close(fd[0]);
- close(fd[1]);
- execv_git_cmd(argv);
- die("%s exec failed", argv[0]);
- }
- close(fd[0]);
close(fd[1]);
- if (do_keep && pack_lockfile) {
- close(keep_pipe[1]);
- *pack_lockfile = index_pack_lockfile(keep_pipe[0]);
- close(keep_pipe[0]);
- }
- while (waitpid(pid, &status, 0) < 0) {
- if (errno != EINTR)
- die("waiting for %s: %s", argv[0], strerror(errno));
- }
- if (WIFEXITED(status)) {
- int code = WEXITSTATUS(status);
- if (code)
- die("%s died with error code %d", argv[0], code);
- return 0;
- }
- if (WIFSIGNALED(status)) {
- int sig = WTERMSIG(status);
- die("%s died of signal %d", argv[0], sig);
- }
- die("%s died of unnatural causes %d", argv[0], status);
+ if (do_keep && pack_lockfile)
+ *pack_lockfile = index_pack_lockfile(cmd.out);
+
+ if (finish_command(&cmd))
+ die("%s failed", argv[0]);
+ return 0;
}
static struct ref *do_fetch_pack(int fd[2],
--
1.5.3.3.1134.gee562
^ permalink raw reply related
* [PATCH 11/14] upload-pack: Run rev-list in an asynchronous function.
From: Johannes Sixt @ 2007-10-13 20:06 UTC (permalink / raw)
To: gitster; +Cc: git, Johannes Sixt
In-Reply-To: <1192305984-22594-11-git-send-email-johannes.sixt@telecom.at>
This gets rid of an explicit fork().
Since upload-pack has to coordinate two processes (rev-list and
pack-objects), we cannot use the normal finish_async(), but have to monitor
the process explicitly. Hence, there are no changes at this front.
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
upload-pack.c | 46 ++++++++++++++++++----------------------------
1 files changed, 18 insertions(+), 28 deletions(-)
diff --git a/upload-pack.c b/upload-pack.c
index c5aa0ea..6799468 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -97,11 +97,12 @@ static void show_edge(struct commit *commit)
fprintf(pack_pipe, "-%s\n", sha1_to_hex(commit->object.sha1));
}
-static void do_rev_list(int create_full_pack)
+static int do_rev_list(int fd, void *create_full_pack)
{
int i;
struct rev_info revs;
+ pack_pipe = fdopen(fd, "w");
if (create_full_pack)
use_thin_pack = 0; /* no point doing it */
init_revisions(&revs, NULL);
@@ -131,14 +132,12 @@ static void do_rev_list(int create_full_pack)
prepare_revision_walk(&revs);
mark_edges_uninteresting(revs.commits, &revs, show_edge);
traverse_commit_list(&revs, show_commit, show_object);
+ return 0;
}
static void create_pack_file(void)
{
- /* Pipe from rev-list to pack-objects
- */
- int lp_pipe[2];
- pid_t pid_rev_list;
+ struct async rev_list;
struct child_process pack_objects;
int create_full_pack = (nr_our_refs == want_obj.nr && !have_obj.nr);
char data[8193], progress[128];
@@ -148,22 +147,12 @@ static void create_pack_file(void)
const char *argv[10];
int arg = 0;
- if (pipe(lp_pipe) < 0)
- die("git-upload-pack: unable to create pipe");
- pid_rev_list = fork();
- if (pid_rev_list < 0)
+ rev_list.proc = do_rev_list;
+ /* .data is just a boolean: any non-NULL value will do */
+ rev_list.data = create_full_pack ? &rev_list : NULL;
+ if (start_async(&rev_list))
die("git-upload-pack: unable to fork git-rev-list");
- if (!pid_rev_list) {
- close(lp_pipe[0]);
- pack_pipe = fdopen(lp_pipe[1], "w");
- do_rev_list(create_full_pack);
- exit(0);
- }
-
- /* close this so that it is not inherited by pack_objects */
- close(lp_pipe[1]);
-
argv[arg++] = "pack-objects";
argv[arg++] = "--stdout";
if (!no_progress)
@@ -173,14 +162,15 @@ static void create_pack_file(void)
argv[arg++] = NULL;
memset(&pack_objects, 0, sizeof(pack_objects));
- pack_objects.in = lp_pipe[0]; /* start_command closes it */
+ pack_objects.in = rev_list.out; /* start_command closes it */
pack_objects.out = -1;
pack_objects.err = -1;
pack_objects.git_cmd = 1;
pack_objects.argv = argv;
+
if (start_command(&pack_objects)) {
/* daemon sets things up to ignore TERM */
- kill(pid_rev_list, SIGKILL);
+ kill(rev_list.pid, SIGKILL);
die("git-upload-pack: unable to fork git-pack-objects");
}
@@ -280,11 +270,11 @@ static void create_pack_file(void)
}
/* See if the children are still there */
- if (pid_rev_list || pack_objects.pid) {
+ if (rev_list.pid || pack_objects.pid) {
pid = waitpid(-1, &status, WNOHANG);
if (!pid)
continue;
- who = ((pid == pid_rev_list) ? "git-rev-list" :
+ who = ((pid == rev_list.pid) ? "git-rev-list" :
(pid == pack_objects.pid) ? "git-pack-objects" :
NULL);
if (!who) {
@@ -302,11 +292,11 @@ static void create_pack_file(void)
who);
goto fail;
}
- if (pid == pid_rev_list)
- pid_rev_list = 0;
+ if (pid == rev_list.pid)
+ rev_list.pid = 0;
if (pid == pack_objects.pid)
pack_objects.pid = 0;
- if (pid_rev_list || pack_objects.pid)
+ if (rev_list.pid || pack_objects.pid)
continue;
}
@@ -329,8 +319,8 @@ static void create_pack_file(void)
fail:
if (pack_objects.pid)
kill(pack_objects.pid, SIGKILL);
- if (pid_rev_list)
- kill(pid_rev_list, SIGKILL);
+ if (rev_list.pid)
+ kill(rev_list.pid, SIGKILL);
send_client_data(3, abort_msg, sizeof(abort_msg));
die("git-upload-pack: %s", abort_msg);
}
--
1.5.3.3.1134.gee562
^ permalink raw reply related
* [PATCH 13/14] Avoid a dup2(2) in apply_filter() - start_command() can do it for us.
From: Johannes Sixt @ 2007-10-13 20:06 UTC (permalink / raw)
To: gitster; +Cc: git, Johannes Sixt
In-Reply-To: <1192305984-22594-13-git-send-email-johannes.sixt@telecom.at>
When apply_filter() runs the external (clean or smudge) filter program, it
needs to pass the writable end of a pipe as its stdout. For this purpose,
it used to dup2(2) the file descriptor explicitly to stdout. Now we use
the facilities of start_command() to do it for us.
Furthermore, the path argument of a subordinate function, filter_buffer(),
was not used, so here we replace it to pass the fd instead.
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
convert.c | 7 +++----
1 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/convert.c b/convert.c
index 6d64994..c870817 100644
--- a/convert.c
+++ b/convert.c
@@ -201,7 +201,7 @@ static char *crlf_to_worktree(const char *path, const char *src, unsigned long *
return buffer;
}
-static int filter_buffer(const char *path, const char *src,
+static int filter_buffer(int fd, const char *src,
unsigned long size, const char *cmd)
{
/*
@@ -214,6 +214,7 @@ static int filter_buffer(const char *path, const char *src,
memset(&child_process, 0, sizeof(child_process));
child_process.argv = argv;
child_process.in = -1;
+ child_process.out = fd;
if (start_command(&child_process))
return error("cannot fork to run external filter %s", cmd);
@@ -265,10 +266,8 @@ static char *apply_filter(const char *path, const char *src,
return NULL;
}
if (!child_process.pid) {
- dup2(pipe_feed[1], 1);
close(pipe_feed[0]);
- close(pipe_feed[1]);
- exit(filter_buffer(path, src, *sizep, cmd));
+ exit(filter_buffer(pipe_feed[1], src, *sizep, cmd));
}
close(pipe_feed[1]);
--
1.5.3.3.1134.gee562
^ 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