* Make git diff-generation use a simpler spawn-like interface
From: Linus Torvalds @ 2006-02-26 23:51 UTC (permalink / raw)
To: Junio C Hamano, Git Mailing List
Instead of depending of fork() and execve() and doing things in between
the two, make the git diff functions do everything up front, and then do
a single "spawn_prog()" invocation to run the actual external diff
program (if any is even needed).
This actually ends up simplifying the code, and should make it much
easier to make it efficient under broken operating systems (read: Windows).
Signed-off-by: Linus Torvalds <torvalds@osdl.org>
---
Now, somebody else might disagree with my contention that this also makes
things simpler, but I like how we separate out the "run an external
program" phase from the actual setup and header printing phase.
Of course, it still uses fork/execvp here, but now it should be _trivial_
to just have a windows-optimized version of "spawn_prog()" that does that
Windows thang.
Linus
diff --git a/diff.c b/diff.c
index 804c08c..32da5d7 100644
--- a/diff.c
+++ b/diff.c
@@ -178,11 +178,12 @@ static void emit_rewrite_diff(const char
copy_file('+', temp[1].name);
}
-static void builtin_diff(const char *name_a,
+static const char *builtin_diff(const char *name_a,
const char *name_b,
struct diff_tempfile *temp,
const char *xfrm_msg,
- int complete_rewrite)
+ int complete_rewrite,
+ const char **args)
{
int i, next_at, cmd_size;
const char *const diff_cmd = "diff -L%s -L%s";
@@ -242,19 +243,24 @@ static void builtin_diff(const char *nam
}
if (xfrm_msg && xfrm_msg[0])
puts(xfrm_msg);
+ /*
+ * we do not run diff between different kind
+ * of objects.
+ */
if (strncmp(temp[0].mode, temp[1].mode, 3))
- /* we do not run diff between different kind
- * of objects.
- */
- exit(0);
+ return NULL;
if (complete_rewrite) {
- fflush(NULL);
emit_rewrite_diff(name_a, name_b, temp);
- exit(0);
+ return NULL;
}
}
- fflush(NULL);
- execlp("/bin/sh","sh", "-c", cmd, NULL);
+
+ /* This is disgusting */
+ *args++ = "sh";
+ *args++ = "-c";
+ *args++ = cmd;
+ *args = NULL;
+ return "/bin/sh";
}
struct diff_filespec *alloc_filespec(const char *path)
@@ -559,6 +565,40 @@ static void remove_tempfile_on_signal(in
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 \
@@ -573,9 +613,9 @@ static void run_external_diff(const char
const char *xfrm_msg,
int complete_rewrite)
{
+ const char *spawn_arg[10];
struct diff_tempfile *temp = diff_temp;
- pid_t pid;
- int status;
+ int retval;
static int atexit_asked = 0;
const char *othername;
@@ -592,59 +632,41 @@ static void run_external_diff(const char
signal(SIGINT, remove_tempfile_on_signal);
}
- fflush(NULL);
- pid = fork();
- if (pid < 0)
- die("unable to fork");
- if (!pid) {
- if (pgm) {
- if (one && two) {
- const char *exec_arg[10];
- const char **arg = &exec_arg[0];
- *arg++ = pgm;
- *arg++ = name;
- *arg++ = temp[0].name;
- *arg++ = temp[0].hex;
- *arg++ = temp[0].mode;
- *arg++ = temp[1].name;
- *arg++ = temp[1].hex;
- *arg++ = temp[1].mode;
- if (other) {
- *arg++ = other;
- *arg++ = xfrm_msg;
- }
- *arg = NULL;
- execvp(pgm, (char *const*) exec_arg);
+ if (pgm) {
+ const char **arg = &spawn_arg[0];
+ if (one && two) {
+ *arg++ = pgm;
+ *arg++ = name;
+ *arg++ = temp[0].name;
+ *arg++ = temp[0].hex;
+ *arg++ = temp[0].mode;
+ *arg++ = temp[1].name;
+ *arg++ = temp[1].hex;
+ *arg++ = temp[1].mode;
+ if (other) {
+ *arg++ = other;
+ *arg++ = xfrm_msg;
}
- else
- execlp(pgm, pgm, name, NULL);
+ } else {
+ *arg++ = pgm;
+ *arg++ = name;
}
- /*
- * otherwise we use the built-in one.
- */
- if (one && two)
- builtin_diff(name, othername, temp, xfrm_msg,
- complete_rewrite);
- else
+ *arg = NULL;
+ } else {
+ if (one && two) {
+ pgm = builtin_diff(name, othername, temp, xfrm_msg, complete_rewrite, spawn_arg);
+ } else
printf("* Unmerged path %s\n", name);
- exit(0);
}
- if (waitpid(pid, &status, 0) < 0 ||
- !WIFEXITED(status) || WEXITSTATUS(status)) {
- /* 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.
- */
- remove_tempfile();
+
+ retval = 0;
+ if (pgm)
+ retval = spawn_prog(pgm, spawn_arg);
+ remove_tempfile();
+ if (retval) {
fprintf(stderr, "external diff died, stopping at %s.\n", name);
exit(1);
}
- remove_tempfile();
}
static void diff_fill_sha1_info(struct diff_filespec *one)
^ permalink raw reply related
* Re: Teach the "git" command to handle some commands internally
From: Junio C Hamano @ 2006-02-26 23:46 UTC (permalink / raw)
To: Linus Torvalds
Cc: Git Mailing List, Andreas Ericsson, Alex Riesen, Michal Ostrowski
In-Reply-To: <Pine.LNX.4.64.0602261518110.22647@g5.osdl.org>
Linus Torvalds <torvalds@osdl.org> writes:
> On Sun, 26 Feb 2006, Junio C Hamano wrote:
>>
>> > There's one other change: the search order for external programs is
>> > modified slightly, so that the first entry remains GIT_EXEC_DIR, but the
>> > second entry is the same directory as the git wrapper itself was executed
>> > out of - if we can figure it out from argv[0], of course.
>>
>> I am not sure about this part, though.
>
> Well, what it means is that _if_ you install all your "git" binaries in
> some directory that is not in your patch and is not GIT_EXEC_DIR, they
> will still magically work, assuming you don't do something strange.
I understood that part. I was wondering if this change defeats
what Michal (you sensibly CC'ed your message to) wanted to do
earlier, going great length trying to avoid mucking with PATH
and "where-ever git itself is found" in the last round. After
reviewing the change in 77cb17 commit, I realize my worry was
unfounded.
^ permalink raw reply
* Re: [PATCH] contrib/git-svn: add show-ignore command
From: Eric Wong @ 2006-02-26 23:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vlkvx7mve.fsf@assigned-by-dhcp.cox.net>
Junio C Hamano <junkio@cox.net> wrote:
> After Andrew Morten raised the issue, I've made sure I _really_
> enable my pre-applypatch hook.
>
> Please enable your pre-commit hook (comes with the distribution
> as a sample hook) to catch these trailing whitespaces before
> they hit your commit objects.
Oops, sorry about this one. I forgot it on the directory I was working
on at the time. I've also added Dave Jones' whitespace hilighter lines
to my .vimrc as well.
--
Eric Wong
^ permalink raw reply
* Re: Teach the "git" command to handle some commands internally
From: Linus Torvalds @ 2006-02-26 23:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git Mailing List, Andreas Ericsson, Alex Riesen, Michal Ostrowski
In-Reply-To: <7vbqwt7m3t.fsf@assigned-by-dhcp.cox.net>
On Sun, 26 Feb 2006, Junio C Hamano wrote:
>
> > There's one other change: the search order for external programs is
> > modified slightly, so that the first entry remains GIT_EXEC_DIR, but the
> > second entry is the same directory as the git wrapper itself was executed
> > out of - if we can figure it out from argv[0], of course.
>
> I am not sure about this part, though.
Well, what it means is that _if_ you install all your "git" binaries in
some directory that is not in your patch and is not GIT_EXEC_DIR, they
will still magically work, assuming you don't do something strange.
IOW, you can do things like
alias git=/opt/my-git/git
and all the "git" commands will automatically work fine, even if you
didn't know at compile time where you would install them, and you didn't
set GIT_EXEC_DIR at run-time. It will still first look in GIT_EXEC_DIR,
but if that fails, it will take the git commands from /opt/my-git/ instead
of from /usr/bin or whatever.
It seemed a nice thing to try to make the git wrapper execute whatever git
version it was installed with, rather than depend on PATH.
But hey, it's only a couple of lines out of the whole thing, so you can
certainly skip it (remove the
if (exec_path)
prepend_path(exec_path)
lines to disable it).
Linus
^ permalink raw reply
* NT directory traversal speed on 25K files on Cygwin
From: Rutger Nijlunsing @ 2006-02-26 23:17 UTC (permalink / raw)
To: Christopher Faylor; +Cc: git
In-Reply-To: <20060226195552.GA30735@trixie.casa.cgf.cx>
On Sun, Feb 26, 2006 at 02:55:52PM -0500, Christopher Faylor wrote:
> On Thu, Feb 23, 2006 at 03:07:07PM +0100, Alex Riesen wrote:
> >filesystem is slow and locked down, and exec-attribute is NOT really
> >useful even on NTFS (it is somehow related to execute permission and
> >open files. I still cannot figure out how exactly are they related).
>
> Again, it's not clear if you're talking about Windows or Cygwin but
> under Cygwin, in the default configuration, the exec attribute means the
> same thing to cygwin as it does to linux.
I don't know about native Windows speed, but comparing NutCracker with
Cygwin on a simple 'find . | wc -l' already gives a clue that looking
at Cygwin to benchmark NT file inspection IO will give a skewed
picture:
##### NutCracker
$ time find . | wc -l
real 0m 1.44s
user 0m 0.45s
sys 0m 0.98s
25794
##### Cygwin
$ time c:\\cygwin\\bin\\find . | wc -l
real 0m 6.72s
user 0m 1.09s
sys 0m 5.59s
25794
##### CMD.EXE + DIR /S
C:\PROJECT> c:\cygwin\bin\time cmd /c dir /s >NUL
0.01user 0.01system 0:05.70elapsed 0%CPU (0avgtext+0avgdata 6320maxresident)k
0inputs+0outputs (395major+0minor)pagefaults 0swaps
##### Cygwin 'find -ls' (NutCracker doesn't have a '-ls')
C:\PROJECT> c:\cygwin\bin\time c:\cygwin\bin\find -ls | wc -l
2.79user 7.81system 0:10.60elapsed 100%CPU (0avgtext+0avgdata 14480maxresident)k
25794
Regards,
Rutger.
--
Rutger Nijlunsing ---------------------------------- eludias ed dse.nl
never attribute to a conspiracy which can be explained by incompetence
----------------------------------------------------------------------
^ permalink raw reply
* Re: Teach the "git" command to handle some commands internally
From: Junio C Hamano @ 2006-02-26 23:10 UTC (permalink / raw)
To: Linus Torvalds
Cc: Git Mailing List, Andreas Ericsson, Alex Riesen, Michal Ostrowski
In-Reply-To: <Pine.LNX.4.64.0602261225500.22647@g5.osdl.org>
Linus Torvalds <torvalds@osdl.org> writes:
> This also implies that to support the old "git-log" and "git-diff" syntax,
> the "git" wrapper now automatically looks at the name it was executed as,
> and if it is "git-xxxx", it will assume that it is to internally do what
> "git xxxx" would do.
>
> In other words, you can (once you implement an internal command) soft- or
> hard-link that command to the "git" wrapper command, and it will do the
> right thing, whether you use the "git xxxx" or the "git-xxxx" format.
I like this careful backward compatibility part.
> There's one other change: the search order for external programs is
> modified slightly, so that the first entry remains GIT_EXEC_DIR, but the
> second entry is the same directory as the git wrapper itself was executed
> out of - if we can figure it out from argv[0], of course.
I am not sure about this part, though.
^ permalink raw reply
* Re: [PATCH] contrib/git-svn: add show-ignore command
From: Junio C Hamano @ 2006-02-26 22:53 UTC (permalink / raw)
To: Eric Wong; +Cc: git
In-Reply-To: <11409493473353-git-send-email-normalperson@yhbt.net>
After Andrew Morten raised the issue, I've made sure I _really_
enable my pre-applypatch hook.
Please enable your pre-commit hook (comes with the distribution
as a sample hook) to catch these trailing whitespaces before
they hit your commit objects.
Applying 'contrib/git-svn: add show-ignore command'
*
* You have some suspicious patch lines:
*
* In contrib/git-svn/git-svn.perl
* trailing whitespace (line 268)
contrib/git-svn/git-svn.perl:268:
* trailing whitespace (line 276)
contrib/git-svn/git-svn.perl:276:
^ permalink raw reply
* Re: [PATCH] Add a Documentation/git-tools.txt
From: Junio C Hamano @ 2006-02-26 22:51 UTC (permalink / raw)
To: Marco Costalba; +Cc: git, Alexandre Julliard
In-Reply-To: <e5bfff550602260022jde1fe2n4ec117c609a5d22d@mail.gmail.com>
"Marco Costalba" <mcostalba@gmail.com> writes:
> A brief survey of useful git tools, including third-party
> and external projects.
>
> Signed-off-by: Marco Costalba <mcostalba@gmail.com>
> ---
>
> Suggestions/corrections from the list has been collected during the
> past week, so I resend
> the updated patch.
>
> Please consider for apply.
Thanks. I've considered it, but it is seriously linewrapped.
> + - *pcl-cvs* (contrib/)
> +
> + This is an Emacs interface for git. The user interface is
> modeled on
> + pcl-cvs.
Also is the emacs one really pcl-cvs? I thought it was modeled
after pcl-cvs, but this is a different implementation to deal
with git. If Alexandre does not have a name for it, I'd say
we'll list it as "git.el".
^ permalink raw reply
* [PATCH] annotate: Use qx{} for pipes on activestate.
From: Ryan Anderson @ 2006-02-26 21:09 UTC (permalink / raw)
To: junkio; +Cc: merlyn, git, Ryan Anderson
In-Reply-To: <1140922925166-git-send-email-ryan@michonline.com>
Note: This needs someone to tell me what the value of $^O is on ActiveState.
Signed-off-by: Ryan Anderson <ryan@michonline.com>
---
As always, available in
http://h4x0r5.com/~ryan/git/ryan.git/ annotate-upstream
Randal, does this look basically the right approach here?
(I would like to make that glob I use not a global, but I can't
seem to figure out how at the moment.)
git-annotate.perl | 43 ++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 42 insertions(+), 1 deletions(-)
a927bb08b9b319cc3832fcf354a75e3760af593c
diff --git a/git-annotate.perl b/git-annotate.perl
index ee8ff15..f9c2c6c 100755
--- a/git-annotate.perl
+++ b/git-annotate.perl
@@ -431,8 +431,20 @@ sub gitvar_name {
return join(' ', @field[0...(@field-4)]);
}
-
sub open_pipe {
+ if ($^O eq '##INSERT_ACTIVESTATE_STRING_HERE##') {
+ return open_pipe_activestate(@_);
+ } else {
+ return open_pipe_normal(@_);
+ }
+}
+
+sub open_pipe_activestate {
+ tie *fh, "Git::ActiveStatePipe", @_;
+ return *fh;
+}
+
+sub open_pipe_normal {
my (@execlist) = @_;
my $pid = open my $kid, "-|";
@@ -445,3 +457,32 @@ sub open_pipe {
return $kid;
}
+
+package Git::ActiveStatePipe;
+use strict;
+
+sub TIEHANDLE {
+ my ($class, @params) = @_;
+ my $cmdline = join " ", @params;
+ my @data = qx{$cmdline};
+ bless { i => 0, data => \@data }, $class;
+}
+
+sub READLINE {
+ my $self = shift;
+ if ($self->{i} >= scalar @{$self->{data}}) {
+ return undef;
+ }
+ return $self->{'data'}->[ $self->{i}++ ];
+}
+
+sub CLOSE {
+ my $self = shift;
+ delete $self->{data};
+ delete $self->{i};
+}
+
+sub EOF {
+ my $self = shift;
+ return ($self->{i} >= scalar @{$self->{data}});
+}
--
1.2.3.g9ca3
^ permalink raw reply related
* Re: [PATCH] fmt-merge-msg: avoid open "-|" list form for Perl 5.6
From: Christopher Faylor @ 2006-02-26 20:40 UTC (permalink / raw)
To: git
In-Reply-To: <Pine.LNX.4.64.0602261217080.22647@g5.osdl.org>
On Sun, Feb 26, 2006 at 12:18:19PM -0800, Linus Torvalds wrote:
>On Sun, 26 Feb 2006, Christopher Faylor wrote:
>>If the speed of cygwin's fork is an issue then I'd previously suggested
>>using spawn*. The spawn family of functions were designed to emulate
>>Windows functions of the same name. They start a new process without
>>the requirement of forking.
>
>I thought that cygwin didn't implement the posix_spawn*() family?
Right. It just implements the windows version of spawn. I looked more
closely at the posix_spawn functions after you last suggested it and,
while it would be possible to implement this in cygwin, these functions
are a lot more heavyweight than the windows-like implementation of spawn
that are already in cygwin. So, they would come with their own
performance penalty.
The cygwin/windows version of spawn is basically like an extended version
of exec*():
pid = spawnlp (P_NOWAIT, "/bin/ls", "ls", "-l", NULL);
will start "/bin/ls" and return a pid which can be used in waitpid.
There is still some overhead to this function but it basically is just a
wrapper around the Windows CreateProcess, which means that it doesn't
go through the annoying overhead of Cygwin's fork.
The posix_spawn stuff is in my todo list but the Windows spawn stuff
could be used now.
cgf
^ permalink raw reply
* Teach the "git" command to handle some commands internally
From: Linus Torvalds @ 2006-02-26 20:34 UTC (permalink / raw)
To: Junio C Hamano, Git Mailing List, Andreas Ericsson, Alex Riesen,
Michal Ostrowski
This is another patch in the "prepare to do more in C" series, where the
git wrapper command is taught about the notion of handling some
functionality internally.
Right now, the only internal commands are "version" and "help", but the
point being that we can now easily extend it to handle some of the trivial
scripts internally. Things like "git log" and "git diff" wouldn't need
separate external scripts any more.
This also implies that to support the old "git-log" and "git-diff" syntax,
the "git" wrapper now automatically looks at the name it was executed as,
and if it is "git-xxxx", it will assume that it is to internally do what
"git xxxx" would do.
In other words, you can (once you implement an internal command) soft- or
hard-link that command to the "git" wrapper command, and it will do the
right thing, whether you use the "git xxxx" or the "git-xxxx" format.
There's one other change: the search order for external programs is
modified slightly, so that the first entry remains GIT_EXEC_DIR, but the
second entry is the same directory as the git wrapper itself was executed
out of - if we can figure it out from argv[0], of course.
Signed-off-by: Linus Torvalds <torvalds@osdl.org>
---
I personally think this is also a cleanup, but I'm cc'ing other people who
have worked on the wrapper for comments. Maybe people hate it.
Linus
---
diff --git a/git.c b/git.c
index 4616df6..993cd0d 100644
--- a/git.c
+++ b/git.c
@@ -230,62 +230,141 @@ static void show_man_page(char *git_cmd)
execlp("man", "man", page, NULL);
}
+static int cmd_version(int argc, char **argv, char **envp)
+{
+ printf("git version %s\n", GIT_VERSION);
+ return 0;
+}
+
+static int cmd_help(int argc, char **argv, char **envp)
+{
+ char *help_cmd = argv[1];
+ if (!help_cmd)
+ cmd_usage(git_exec_path(), NULL);
+ show_man_page(help_cmd);
+ return 0;
+}
+
+#define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))
+
+static void handle_internal_command(int argc, char **argv, char **envp)
+{
+ const char *cmd = argv[0];
+ static struct cmd_struct {
+ const char *cmd;
+ int (*fn)(int, char **, char **);
+ } commands[] = {
+ { "version", cmd_version },
+ { "help", cmd_help },
+ };
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(commands); i++) {
+ struct cmd_struct *p = commands+i;
+ if (strcmp(p->cmd, cmd))
+ continue;
+ exit(p->fn(argc, argv, envp));
+ }
+}
+
int main(int argc, char **argv, char **envp)
{
+ char *cmd = argv[0];
+ char *slash = strrchr(cmd, '/');
char git_command[PATH_MAX + 1];
- char wd[PATH_MAX + 1];
- int i, show_help = 0;
- const char *exec_path;
+ const char *exec_path = NULL;
- getcwd(wd, PATH_MAX);
+ /*
+ * Take the basename of argv[0] as the command
+ * name, and the dirname as the default exec_path
+ * if it's an absolute path and we don't have
+ * anything better.
+ */
+ if (slash) {
+ *slash++ = 0;
+ if (*cmd == '/')
+ exec_path = cmd;
+ cmd = slash;
+ }
- for (i = 1; i < argc; i++) {
- char *arg = argv[i];
+ /*
+ * "git-xxxx" is the same as "git xxxx", but we obviously:
+ *
+ * - cannot take flags in between the "git" and the "xxxx".
+ * - cannot execute it externally (since it would just do
+ * the same thing over again)
+ *
+ * So we just directly call the internal command handler, and
+ * die if that one cannot handle it.
+ */
+ if (!strncmp(cmd, "git-", 4)) {
+ cmd += 4;
+ argv[0] = cmd;
+ handle_internal_command(argc, argv, envp);
+ die("cannot handle %s internally", cmd);
+ }
- if (!strcmp(arg, "help")) {
- show_help = 1;
- continue;
- }
+ /* Default command: "help" */
+ cmd = "help";
- if (strncmp(arg, "--", 2))
+ /* Look for flags.. */
+ while (argc > 1) {
+ cmd = *++argv;
+ argc--;
+
+ if (strncmp(cmd, "--", 2))
break;
- arg += 2;
+ cmd += 2;
+
+ /*
+ * For legacy reasons, the "version" and "help"
+ * commands can be written with "--" prepended
+ * to make them look like flags.
+ */
+ if (!strcmp(cmd, "help"))
+ break;
+ if (!strcmp(cmd, "version"))
+ break;
- if (!strncmp(arg, "exec-path", 9)) {
- arg += 9;
- if (*arg == '=') {
- exec_path = arg + 1;
- git_set_exec_path(exec_path);
- } else {
- puts(git_exec_path());
- exit(0);
+ /*
+ * Check remaining flags (which by now must be
+ * "--exec-path", but maybe we will accept
+ * other arguments some day)
+ */
+ if (!strncmp(cmd, "exec-path", 9)) {
+ cmd += 9;
+ if (*cmd == '=') {
+ git_set_exec_path(cmd + 1);
+ continue;
}
- }
- else if (!strcmp(arg, "version")) {
- printf("git version %s\n", GIT_VERSION);
+ puts(git_exec_path());
exit(0);
}
- else if (!strcmp(arg, "help"))
- show_help = 1;
- else if (!show_help)
- cmd_usage(NULL, NULL);
- }
-
- if (i >= argc || show_help) {
- if (i >= argc)
- cmd_usage(git_exec_path(), NULL);
-
- show_man_page(argv[i]);
+ cmd_usage(NULL, NULL);
}
+ argv[0] = cmd;
+ /*
+ * We search for git commands in the following order:
+ * - git_exec_path()
+ * - the path of the "git" command if we could find it
+ * in $0
+ * - the regular PATH.
+ */
+ if (exec_path)
+ prepend_to_path(exec_path, strlen(exec_path));
exec_path = git_exec_path();
prepend_to_path(exec_path, strlen(exec_path));
- execv_git_cmd(argv + i);
+ /* See if it's an internal command */
+ handle_internal_command(argc, argv, envp);
+
+ /* .. then try the external ones */
+ execv_git_cmd(argv);
if (errno == ENOENT)
- cmd_usage(exec_path, "'%s' is not a git-command", argv[i]);
+ cmd_usage(exec_path, "'%s' is not a git-command", cmd);
fprintf(stderr, "Failed to run command '%s': %s\n",
git_command, strerror(errno));
^ permalink raw reply related
* Re: [PATCH] fmt-merge-msg: avoid open "-|" list form for Perl 5.6
From: Christopher Faylor @ 2006-02-26 20:33 UTC (permalink / raw)
To: git
In-Reply-To: <43FDB8CC.5000503@op5.se>
On Thu, Feb 23, 2006 at 02:29:48PM +0100, Andreas Ericsson wrote:
>Alex Riesen wrote:
>>On 2/23/06, Andreas Ericsson <ae@op5.se> wrote:
>>
>>>Not to be unhelpful or anything, but activestate perl seems to be quite
>>>a lot of bother. Is it worth supporting it?
>>
>>
>>It's not activestate perl actually. It's only one platform it also
>>_has_ to support.
>>Is it worth supporting Windows?
>
>
>With or without cygwin? With cygwin, I'd say "yes, unless it makes
>things terribly difficult to maintain and so long as we don't take
>performance hits on unices". Without cygwin, I'd say "What? It runs on
>windows?".
>
>If we claim to support windows but do a poor job of it, no-one else will
>start working on a windows-port. If we don't claim to support windows
>but say that "it's known to work with cygwin, although be aware of these
>performance penalties...", eventually someone will come along with their
>shiny Visual Express and hack up support for it, even if some tools will
>be missing and others unnecessarily complicated.
Well, with Cygwin, you've at least got the ear of one of the Cygwin
maintainers, which should be worth something.
Even if I disappear, you can always send concerns to the Cygwin mailing
list. Do the ActiveState folks respond to complaints about things as
basic as pipes not working in perl?
Cygwin's goal is to make Windows look as much like Linux as we can
manage, so, unless we're total incompetents (which has been hinted in
this mailing list from time to time), it has *got* to be better,
source-code-wise to target Windows-running-Cygwin than
just-plain-Windows. However, as has been noted, that means that there
will be a speed tradeoff.
I think that, for most projects, the convenience of not having to
clutter the code with substantial accommodations for the windows/POSIX
mismatch usually offsets the annoyance of the speed penalty. Maybe
that's not the case for git, however.
Anyway, we're willing, within the limits of available time, to help out
where git uncovers issues with Cygwin. I just fixed some stuff in
dirent.h in the last Cygwin release, as a direct result of people noting
a problem here. Basically, I don't want git to be a morasse of #ifdef
__CYGWIN_'s and I'll do whatever I can to help.
We're always trying to tweak things to improve speed in Cygwin and am
open to intelligent suggestions about how we can make things better.
The dance between total linux compatibility and speed is one that we
struggle with all of the time and, sadly, over time, we've probably
sacrificed speed in the name of functionality. That's probably because
it's easy to fix a problem like "close-on-exec doesn't work for sockets"
and feel good that you've fixed a bug even if you've just added a few
microseconds to fork/exec.
cgf
^ permalink raw reply
* Re: the war on trailing whitespace
From: Dave Jones @ 2006-02-26 20:31 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Andrew Morton, junkio, git
In-Reply-To: <20060226202617.GH7851@redhat.com>
On Sun, Feb 26, 2006 at 03:26:17PM -0500, Dave Jones wrote:
> in my .vimrc, which highlights this (and other trailing whitespace) as
> a big red blob. I do this in part for the same reason Andrew does,
> so that when someone sends me a diff with a zillion spaces at the EOL,
> it screams at me, I spot them, and chop them out.
(seconds later, I find my .vimrc on master.k.o hasn't had this turned
on so I find a billion instances of this mess in agp/cpufreq -- Bah).
Dave
^ permalink raw reply
* Re: [PATCH] Use setenv(), fix warnings
From: Junio C Hamano @ 2006-02-26 20:29 UTC (permalink / raw)
To: Timo Hirvonen; +Cc: git
In-Reply-To: <20060226203756.05dcfb26.tihirvon@gmail.com>
Timo Hirvonen <tihirvon@gmail.com> writes:
> putenv(3):
> "If the argument `string` is of the form `name`, and does not
> contain an `=' character, then the variable `name` is removed from
> the environment."
>
> So the variable is emptied, not removed. But usually empty environment
> variables are treated as if they didn't exist...
Yes we were aware of that when we did it.
^ permalink raw reply
* Re: the war on trailing whitespace
From: Junio C Hamano @ 2006-02-26 20:29 UTC (permalink / raw)
To: Andrew Morton; +Cc: Linus Torvalds, junkio, git
In-Reply-To: <20060226103604.2d97696c.akpm@osdl.org>
Andrew Morton <akpm@osdl.org> writes:
> Thanks. But it defaults to nowarn. Nobody will turn it on and nothing
> improves.
This WS is clearly a policy, and while I personally agree that
it is a _good_ policy, I am a bit hesitant to hardcode this
stricter policy as the default to lower level tools.
I have a feeling that Linus is saying that pre-applypatch hook
is good enough, and you have to educate people who feed things
to you ;-)
^ permalink raw reply
* Re: the war on trailing whitespace
From: Dave Jones @ 2006-02-26 20:26 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Andrew Morton, junkio, git
In-Reply-To: <Pine.LNX.4.64.0602261213340.22647@g5.osdl.org>
On Sun, Feb 26, 2006 at 12:16:25PM -0800, Linus Torvalds wrote:
> Few enough people run "git-apply" on its own. Most people (certainly me)
> end up using it through some email-applicator script or other. So the plan
> was that the --whitespace=warn/error flag would go there, and that
> git-apply by default would work more like "patch".
>
> But hey, I have no strong preferences, and it's easy enough to make the
> default be warn (and add a "--whitespace=ok" flag to turn it off).
>
> Personally, I don't mind whitespace that much. In particular, I _suspect_
> I often have empty lines like
>
> int i;
>
> i = 10;
>
> where the "empty" line actually has the same indentation as the lines
> around it. Is that wrong? Perhaps.
I think I have the same anal-retentive problem Andrew has, because I have ..
highlight RedundantSpaces term=standout ctermbg=red guibg=red
match RedundantSpaces /\s\+$\| \+\ze\t/
in my .vimrc, which highlights this (and other trailing whitespace) as
a big red blob. I do this in part for the same reason Andrew does,
so that when someone sends me a diff with a zillion spaces at the EOL,
it screams at me, I spot them, and chop them out.
Dave
^ permalink raw reply
* Re: [PATCH] fmt-merge-msg: avoid open "-|" list form for Perl 5.6
From: Linus Torvalds @ 2006-02-26 20:18 UTC (permalink / raw)
To: Christopher Faylor; +Cc: git
In-Reply-To: <20060226195552.GA30735@trixie.casa.cgf.cx>
On Sun, 26 Feb 2006, Christopher Faylor wrote:
>
> If the speed of cygwin's fork is an issue then I'd previously suggested
> using spawn*. The spawn family of functions were designed to emulate
> Windows functions of the same name. They start a new process without
> the requirement of forking.
I thought that cygwin didn't implement the posix_spawn*() family?
Anyway, we probably _can_ use posix_spawn() in various places, and
especially if that helps windows performance, we should.
Linus
^ permalink raw reply
* Re: the war on trailing whitespace
From: Linus Torvalds @ 2006-02-26 20:16 UTC (permalink / raw)
To: Andrew Morton; +Cc: junkio, git
In-Reply-To: <20060226103604.2d97696c.akpm@osdl.org>
On Sun, 26 Feb 2006, Andrew Morton wrote:
>
> Thanks. But it defaults to nowarn. Nobody will turn it on and nothing
> improves.
Few enough people run "git-apply" on its own. Most people (certainly me)
end up using it through some email-applicator script or other. So the plan
was that the --whitespace=warn/error flag would go there, and that
git-apply by default would work more like "patch".
But hey, I have no strong preferences, and it's easy enough to make the
default be warn (and add a "--whitespace=ok" flag to turn it off).
Personally, I don't mind whitespace that much. In particular, I _suspect_
I often have empty lines like
int i;
i = 10;
where the "empty" line actually has the same indentation as the lines
around it. Is that wrong? Perhaps.
Linus
^ permalink raw reply
* Re: [PATCH] fmt-merge-msg: avoid open "-|" list form for Perl 5.6
From: Christopher Faylor @ 2006-02-26 19:55 UTC (permalink / raw)
To: git
In-Reply-To: <81b0412b0602230607n22146a77k36929f0ad9e44d53@mail.gmail.com>
On Thu, Feb 23, 2006 at 03:07:07PM +0100, Alex Riesen wrote:
>On 2/23/06, Andreas Ericsson <ae@op5.se> wrote:
>>>>Not to be unhelpful or anything, but activestate perl seems to be quite
>>>>a lot of bother. Is it worth supporting it?
>>>
>>>
>>>It's not activestate perl actually. It's only one platform it also
>>>_has_ to support. Is it worth supporting Windows?
>>
>>With or without cygwin? With cygwin, I'd say "yes, unless it makes
>>things terribly difficult to maintain and so long as we don't take
>>performance hits on unices". Without cygwin, I'd say "What? It runs
>>on windows?".
>
>There not much difference with or without cygwin. The penalties of
>doing any kind of support for it will pile up (as they started to do
>with pipes). Someday we'll have to start dropping features on Windows
>or restrict them beyond their usefullness. The fork emulation in
>cygwin isn't perfect,
If the speed of cygwin's fork is an issue then I'd previously suggested
using spawn*. The spawn family of functions were designed to emulate
Windows functions of the same name. They start a new process without
the requirement of forking.
>signals do not work reliably (if at all),
I'm not sure if you're mixing cygwin with windows here but if signals do
not work reliably in Cygwin then that is something that we'd like to
know about. Signals *obviously* have to work fairly well for programs
like ssh, bash, and X to work, however.
Native Windows, OTOH, hardly has any signals at all and deals with
signals in a way that is only vaguely like linux.
>filesystem is slow and locked down, and exec-attribute is NOT really
>useful even on NTFS (it is somehow related to execute permission and
>open files. I still cannot figure out how exactly are they related).
Again, it's not clear if you're talking about Windows or Cygwin but
under Cygwin, in the default configuration, the exec attribute means the
same thing to cygwin as it does to linux.
As always, if you have questions or problems with cygwin, you can ask in
the proper forum. The available cygwin mailing lists are here:
http://cygwin.com/lists.html.
Would getting git into the cygwin distribution solve any problems with
git adoption on Windows? This would get an automatic green light from
anyone who was interested, if so. Someone would just have to send an
"ITP" (Intent To Package) to the cygwin-apps mailing list and provide a
package using the guidelines here: http://cygwin.com/setup.html .
cgf
--
Christopher Faylor spammer? -> aaaspam@sourceware.org
Cygwin Co-Project Leader
TimeSys, Inc.
^ permalink raw reply
* Re: the war on trailing whitespace
From: Sam Ravnborg @ 2006-02-26 19:45 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Andrew Morton, Junio C Hamano, git
In-Reply-To: <Pine.LNX.4.64.0602260925170.22647@g5.osdl.org>
On Sun, Feb 26, 2006 at 09:29:00AM -0800, Linus Torvalds wrote:
>
>
> On Sat, 25 Feb 2006, Andrew Morton wrote:
> >
> > I'd suggest a) git will simply refuse to apply such a patch unless given a
> > special `forcing' flag, b) even when thus forced, it will still warn and c)
> > with a different flag, it will strip-then-apply, without generating a
> > warning.
>
> This doesn't do the "strip-then-apply" thing, but it allows you to make
> git-apply generate a warning or error on extraneous whitespace.
Can this somehow be done in a way so everyone that clones your tree
will inherit the warn/error on whitespace setting?
In this way we make sure it gets enabled automagically in many trees
and I do not have to remember yet another options.
Alternatively something that is enabled for a tree so I only have to do
something once - a trigger maybe?
Sam
^ permalink raw reply
* Re: [PATCH] Use setenv(), fix warnings
From: Jason Riedy @ 2006-02-26 19:38 UTC (permalink / raw)
To: Timo Hirvonen; +Cc: git
In-Reply-To: <20060226203756.05dcfb26.tihirvon@gmail.com>
And Timo Hirvonen writes:
- It appears that statically allocated strings are accepted but
- _automatic_ variables aren't.
The putenv()-ed string has to exist as long as the environment
does. You can twiddle the environment just by twiddling the
string you registered. What fun!
- I noticed setenv is now in compat/ so I though it was good idea
- to use it.
All uses of putenv() originally were setenv(). I finally realized
a compat/setenv.c was better than playing whack-a-setenv on every
release... (Ah, hindsight.)
Note that the current compat/setenv.c _LEAKS MEMORY_ on purpose.
Because putenv() requires the string to stay around, we can't
ever free it. I hope any library implementing bits of git
(libgitbit?) avoids setting environment variables.
- So the variable is emptied, not removed. But usually empty environment
- variables are treated as if they didn't exist...
More specifically, git treats both "" and NULL as empty, or at
least it did last time I checked.
Jason
^ permalink raw reply
* Re: the war on trailing whitespace
From: Andrew Morton @ 2006-02-26 18:36 UTC (permalink / raw)
To: Linus Torvalds; +Cc: junkio, git
In-Reply-To: <Pine.LNX.4.64.0602260925170.22647@g5.osdl.org>
Linus Torvalds <torvalds@osdl.org> wrote:
>
>
>
> On Sat, 25 Feb 2006, Andrew Morton wrote:
> >
> > I'd suggest a) git will simply refuse to apply such a patch unless given a
> > special `forcing' flag, b) even when thus forced, it will still warn and c)
> > with a different flag, it will strip-then-apply, without generating a
> > warning.
>
> This doesn't do the "strip-then-apply" thing, but it allows you to make
> git-apply generate a warning or error on extraneous whitespace.
>
> Use --whitespace=warn to warn, and (surprise, surprise) --whitespace=error
> to make it a fatal error to have whitespace at the end.
Thanks. But it defaults to nowarn. Nobody will turn it on and nothing
improves.
> Totally untested, of course. But it compiles, so it must be fine.
Who cares, as long as the patch doesn't add trailing whitespace? ) ;)
> HOWEVER! Note that this literally will check every single patch-line with
> "+" at the beginning. Which means that if you fix a simple typo, and the
> line had a space at the end before, and you didn't remove it, that's still
> considered a "new line with whitespace at the end", even though obviously
> the line wasn't really new.
>
> I assume this is what you wanted, and there isn't really any sane
> alternatives (you could make the warning activate only for _pure_
> additions with no deletions at all in that hunk, but that sounds a bit
> insane).
Yup. So by the time we've patched every line in the kernel, it's
trailing-whitespace-free.
^ permalink raw reply
* Re: [PATCH] Use setenv(), fix warnings
From: Timo Hirvonen @ 2006-02-26 18:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vmzge570u.fsf@assigned-by-dhcp.cox.net>
On Sun, 26 Feb 2006 10:06:41 -0800
Junio C Hamano <junkio@cox.net> wrote:
> Timo Hirvonen <tihirvon@gmail.com> writes:
>
> > - Use setenv() instead of putenv()
> > - Fix -Wundef -Wold-style-definition warnings
> > - Make pll_free() static
>
> I think the last one makes sense, and I can see why some people
> may prefer -Wundef but I am not sure about the first one. Care
> to defend why we should prefer setenv()? IIRC, initially we did
> not use setenv() anywhere because certain platforms only had
> putenv().
I was confused by putenv(3) man page. I thought it wanted malloc'ed
strings (no 'const' in the parameter -> warning). It appears that
statically allocated strings are accepted but _automatic_ variables
aren't. I noticed setenv is now in compat/ so I though it was good idea
to use it.
Sorry for the noise.
> > diff --git a/fsck-objects.c b/fsck-objects.c
> > @@ -483,7 +483,7 @@ int main(int argc, char **argv)
> > if (standalone && check_full)
> > die("Only one of --standalone or --full can be used.");
> > if (standalone)
> > - putenv("GIT_ALTERNATE_OBJECT_DIRECTORIES=");
> > + setenv("GIT_ALTERNATE_OBJECT_DIRECTORIES", "", 1);
>
> For platforms with only putenv we did this; here, what we really
> wanted to do was unsetenv.
putenv(3):
"If the argument `string` is of the form `name`, and does not
contain an `=' character, then the variable `name` is removed from
the environment."
So the variable is emptied, not removed. But usually empty environment
variables are treated as if they didn't exist...
--
http://onion.dynserv.net/~timo/
^ permalink raw reply
* Re: [PATCH] Use setenv(), fix warnings
From: Junio C Hamano @ 2006-02-26 18:06 UTC (permalink / raw)
To: Timo Hirvonen; +Cc: git
In-Reply-To: <20060226171346.33ad3e47.tihirvon@gmail.com>
Timo Hirvonen <tihirvon@gmail.com> writes:
> - Use setenv() instead of putenv()
> - Fix -Wundef -Wold-style-definition warnings
> - Make pll_free() static
I think the last one makes sense, and I can see why some people
may prefer -Wundef but I am not sure about the first one. Care
to defend why we should prefer setenv()? IIRC, initially we did
not use setenv() anywhere because certain platforms only had
putenv().
> diff --git a/fsck-objects.c b/fsck-objects.c
> @@ -483,7 +483,7 @@ int main(int argc, char **argv)
> if (standalone && check_full)
> die("Only one of --standalone or --full can be used.");
> if (standalone)
> - putenv("GIT_ALTERNATE_OBJECT_DIRECTORIES=");
> + setenv("GIT_ALTERNATE_OBJECT_DIRECTORIES", "", 1);
For platforms with only putenv we did this; here, what we really
wanted to do was unsetenv.
^ permalink raw reply
* Re: the war on trailing whitespace
From: Linus Torvalds @ 2006-02-26 17:29 UTC (permalink / raw)
To: Andrew Morton; +Cc: Junio C Hamano, git
In-Reply-To: <20060225210712.29b30f59.akpm@osdl.org>
On Sat, 25 Feb 2006, Andrew Morton wrote:
>
> I'd suggest a) git will simply refuse to apply such a patch unless given a
> special `forcing' flag, b) even when thus forced, it will still warn and c)
> with a different flag, it will strip-then-apply, without generating a
> warning.
This doesn't do the "strip-then-apply" thing, but it allows you to make
git-apply generate a warning or error on extraneous whitespace.
Use --whitespace=warn to warn, and (surprise, surprise) --whitespace=error
to make it a fatal error to have whitespace at the end.
Totally untested, of course. But it compiles, so it must be fine.
HOWEVER! Note that this literally will check every single patch-line with
"+" at the beginning. Which means that if you fix a simple typo, and the
line had a space at the end before, and you didn't remove it, that's still
considered a "new line with whitespace at the end", even though obviously
the line wasn't really new.
I assume this is what you wanted, and there isn't really any sane
alternatives (you could make the warning activate only for _pure_
additions with no deletions at all in that hunk, but that sounds a bit
insane).
Linus
---
diff --git a/apply.c b/apply.c
index 244718c..e7b3dca 100644
--- a/apply.c
+++ b/apply.c
@@ -34,6 +34,12 @@ static int line_termination = '\n';
static const char apply_usage[] =
"git-apply [--stat] [--numstat] [--summary] [--check] [--index] [--apply] [--no-add] [--index-info] [--allow-binary-replacement] [-z] [-pNUM] <patch>...";
+static enum whitespace_eol {
+ nowarn,
+ warn_on_whitespace,
+ error_on_whitespace
+} new_whitespace = nowarn;
+
/*
* For "diff-stat" like behaviour, we keep track of the biggest change
* we've seen, and the longest filename. That allows us to do simple
@@ -815,6 +821,22 @@ static int parse_fragment(char *line, un
oldlines--;
break;
case '+':
+ /*
+ * We know len is at least two, since we have a '+' and
+ * we checked that the last character was a '\n' above
+ */
+ if (isspace(line[len-2])) {
+ switch (new_whitespace) {
+ case nowarn:
+ break;
+ case warn_on_whitespace:
+ new_whitespace = nowarn; /* Just once */
+ error("Added whitespace at end of line at line %d", linenr);
+ break;
+ case error_on_whitespace:
+ die("Added whitespace at end of line at line %d", linenr);
+ }
+ }
added++;
newlines--;
break;
@@ -1839,6 +1861,17 @@ int main(int argc, char **argv)
line_termination = 0;
continue;
}
+ if (!strncmp(arg, "--whitespace=", 13)) {
+ if (strcmp(arg+13, "warn")) {
+ new_whitespace = warn_on_whitespace;
+ continue;
+ }
+ if (strcmp(arg+13, "error")) {
+ new_whitespace = error_on_whitespace;
+ continue;
+ }
+ die("unrecognixed whitespace option '%s'", arg+13);
+ }
if (check_index && prefix_length < 0) {
prefix = setup_git_directory();
^ 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