Fix the problem where the cmd->err passed into start_command wasn't
being properly closed when certain types of errors occurr. (Compare
the affected code with the clean shutdown code later in the function.)
On Windows, this problem would be triggered if mingw_spawnvpe()
failed, which would happen if the command to be executed was malformed
(e.g. a text file that didn't start with a #! line). If cmd->err was
a pipe, the failure to close it could result in a hang while the other
side was waiting (forever) for either input or pipe close, e.g. while
trying to shove the output into the side band. On msysGit, this
problem was causing a hang in t5516-fetch-push.
[J6t: With a slight adjustment of the test case, the hang is also
observed on Linux.]
Signed-off-by: bert Dvornik <dvornik+git@gmail.com>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
A caller of start_command can set the member 'dir' to a directory to
request that the child process starts with that directory as CWD. The first
user of this feature was added recently in eee49b6 (Teach diff --submodule
and status to handle .git files in submodules).
On Windows, we have been lazy and had not implemented support for this
feature, yet. This fixes the shortcoming.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Async procedures are intended as helpers that perform a very restricted
task, and the caller usually has to manage them in a larger context.
Conceptually, the async procedure is not concerned with the "bigger
picture" in whose context it is run. When it dies, it is not supposed
to destroy this "bigger picture", but rather only its own limit view
of the world. On POSIX, the async procedure is run in its own process,
and exiting this process naturally had only these limited effects.
On Windows (or when ASYNC_AS_THREAD is set), calling die() exited the
whole process, destroying the caller (the "big picture") as well.
This fixes it to exit only the thread.
Without ASYNC_AS_THREAD, one particular effect of exiting the async
procedure process is that it automatically closes file descriptors, most
notably the writable end of the pipe that the async procedure writes to.
The async API already requires that the async procedure closes the pipe
ends when it exits normally. But for calls to die() no requirements are
imposed. In the non-threaded case the pipe ends are closed implicitly
by the exiting process, but in the threaded case, the die routine must
take care of closing them.
Now t5530-upload-pack-error.sh passes on Windows.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
On Windows, async procedures have always been run in threads, and the
implementation used Windows specific APIs. Rewrite the code to use pthreads.
A new configuration option is introduced so that the threaded implementation
can also be used on POSIX systems. Since this option is intended only as
playground on POSIX, but is mandatory on Windows, the option is not
documented.
One detail is that on POSIX it is necessary to set FD_CLOEXEC on the pipe
handles. On Windows, this is not needed because pipe handles are not
inherited to child processes, and the new calls to set_cloexec() are
effectively no-ops.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Building git on Ubuntu 9.10 warns that the return value of write(2)
isn't checked. These warnings were introduced in commits:
2b541bf8 ("start_command: detect execvp failures early")
a5487ddf ("start_command: report child process setup errors to the
parent's stderr")
GCC details:
$ gcc --version
gcc (Ubuntu 4.4.1-4ubuntu9) 4.4.1
Silence the warnings by reading (but not making use of) the return value
of write(2).
Signed-off-by: Michael Wookey <michaelwookey@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This patch adds the possibility to supply a set of non-0 file
descriptors for async process communication instead of the
default-created pipe.
Additionally, we now support bi-directional communiction with the
async procedure, by giving the async function both read and write
file descriptors.
To retain compatiblity and similar "API feel" with start_command,
we require start_async callers to set .out = -1 to get a readable
file descriptor. If either of .in or .out is 0, we supply no file
descriptor to the async process.
[sp: Note: Erik started this patch, and a huge bulk of it is
his work. All bugs were introduced later by Shawn.]
Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Like .out, .err may now be set to a file descriptor > 0, which
is a writable pipe/socket/file that the child's stderr will be
redirected into.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When stdin, stdout, or stderr must be redirected for a child process that
on Windows is spawned using one of the spawn() functions of Microsoft's
C runtime, then there is no choice other than to
1. make a backup copy of fd 0,1,2 with dup
2. dup2 the redirection source fd into 0,1,2
3. spawn
4. dup2 the backup back into 0,1,2
5. close the backup copy and the redirection source
We used this idiom as well -- but we are not using the spawn() functions
anymore!
Instead, we have our own implementation. We had hardcoded that stdin,
stdout, and stderr of the child process were inherited from the parent's
fds 0, 1, and 2. But we can actually specify any fd.
With this patch, the fds to inherit are passed from start_command()'s
WIN32 section to our spawn implementation. This way, we can avoid the
backup copies of the fds.
The backup copies were a bug waiting to surface: The OS handles underlying
the dup()ed fds were inherited by the child process (but were not
associated with a file descriptor in the child). Consequently, the file or
pipe represented by the OS handle remained open even after the backup copy
was closed in the parent process until the child exited.
Since our implementation of pipe() creates non-inheritable OS handles, we
still dup() file descriptors in start_command() because dup() happens to
create inheritable duplicates. (A nice side effect is that the fd cleanup
in start_command is the same for Windows and Unix and remains unchanged.)
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Previously, failures during execvp could be detected only by
finish_command. However, in some situations it is beneficial for the
parent process to know earlier that the child process will not run.
The idea to use a pipe to signal failures to the parent process and
the test case were lifted from patches by Ilari Liusvaara.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When the child process's environment is set up in start_command(), error
messages were written to wherever the parent redirected the child's stderr
channel. However, even if the parent redirected the child's stderr, errors
during this setup process, including the exec itself, are usually an
indication of a problem in the parent's environment. Therefore, the error
messages should go to the parent's stderr.
Redirection of the child's error messages is usually only used to redirect
hook error messages during client-server exchanges. In these cases, hook
setup errors could be regarded as information leak.
This patch makes a copy of stderr if necessary and uses a special
die routine that is used for all die() calls in the child that sends the
errors messages to the parent's stderr.
The trace call that reported a failed execvp is removed (because it writes
to stderr) and replaced by die_errno() with special treatment of ENOENT.
The improvement in the error message can be seen with this sequence:
mkdir .git/hooks/pre-commit
git commit
Previously, the error message was
error: cannot run .git/hooks/pre-commit: No such file or directory
and now it is
fatal: cannot exec '.git/hooks/pre-commit': Permission denied
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
If there are no metacharacters in the program to be run, we
can just skip running the shell entirely and directly exec
the program.
The metacharacter test is pulled verbatim from
launch_editor, which already implements this optimization.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Many callsites run "sh -c $CMD" to run $CMD. We can make it
a little simpler for them by factoring out the munging of
argv.
For simple cases with no arguments, this doesn't help much, but:
1. For cases with arguments, we save the caller from
having to build the appropriate shell snippet.
2. We can later optimize to avoid the shell when
there are no metacharacters in the program.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The code which is conditional on MinGW32 is actually conditional on Windows.
Use the WIN32 symbol, which is defined by the MINGW32 and MSVC environments,
but not by Cygwin.
Define SNPRINTF_SIZE_CORR=1 for MSVC too, as its vsnprintf function does
not add NUL at the end of the buffer if the result fits the buffer size
exactly.
Signed-off-by: Frank Li <lznuaa@gmail.com>
Signed-off-by: Marius Storm-Olsen <mstormo@gmail.com>
Acked-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
MSVC requires __stdcall to be between the functions return value and the
function name, and that the function pointer type is in the form of
return_type (WINAPI *function_name)(arguments...)
Signed-off-by: Frank Li <lznuaa@gmail.com>
Signed-off-by: Marius Storm-Olsen <mstormo@gmail.com>
Acked-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
MSVC does not understand this C99 style.
Signed-off-by: Frank Li <lznuaa@gmail.com>
Signed-off-by: Marius Storm-Olsen <mstormo@gmail.com>
Acked-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Previously, it would not be possible to call start_command twice for the
same struct child_process that has env set.
The fix is achieved by moving the loop that modifies the environment block
into a helper function. This also allows us to make two other helper
functions static.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
i686-apple-darwin9-gcc-4.0.1 (GCC) 4.0.1 (Apple Inc. build 5490) compiler
(and probably others) mistakenly thinks variable failed_errno is used
before assigned. Work it around by giving it a fake initialization.
Signed-off-by: David Soria Parra <dsp@php.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In the case where a program was not found, it was still the task of the
caller to report an error to the user. Usually, this is an interesting case
but only few callers actually reported a specific error (though many call
sites report a generic error message regardless of the cause).
With this change the error is reported by run_command, but since there is
one call site in git.c that does not want that, an option is added to
struct child_process, which is used to turn the error off.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We now write the signal number in the error message if the program
terminated by a signal. The negative return value is constructed such that
after truncation to 8 bits it looks like a POSIX shell's $?:
$ echo 0000 | { git upload-pack .; echo $? >&2; } | :
error: git-upload-pack died of signal 13
141
Previously, the exit code was 255 instead of 141.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The motivation for this change is that system call failures are serious
errors that should be reported to the user, but only few callers took the
burden to decode the error codes that the functions returned into error
messages.
If at all, then only an unspecific error message was given. A prominent
example is this:
$ git upload-pack . | :
fatal: unable to run 'git-upload-pack'
In this example, git-upload-pack, the external command invoked through the
git wrapper, dies due to SIGPIPE, but the git wrapper does not bother to
report the real cause. In fact, this very error message is copied to the
syslog if git-daemon's client aborts the connection early.
With this change, system call failures are reported immediately after the
failure and only a generic failure code is returned to the caller. In the
above example the error is now to the point:
$ git upload-pack . | :
error: git-upload-pack died of signal
Note that there is no error report if the invoked program terminated with
a non-zero exit code, because it is reasonable to expect that the invoked
program has already reported an error. (But many run_command call sites
nevertheless write a generic error message.)
There was one special return code that was used to identify the case where
run_command failed because the requested program could not be exec'd. This
special case is now treated like a system call failure with errno set to
ENOENT. No error is reported in this case, because the call site in git.c
expects this as a normal result. Therefore, the callers that carefully
decoded the return value still check for this condition.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
As a general guideline, functions in git's code return zero to indicate
success and negative values to indicate failure. The run_command family of
functions followed this guideline. But there are actually two different
kinds of failure:
- failures of system calls;
- non-zero exit code of the program that was run.
Usually, a non-zero exit code of the program is a failure and means a
failure to the caller. Except that sometimes it does not. For example, the
exit code of merge programs (e.g. external merge drivers) conveys
information about how the merge failed, and not all exit calls are
actually failures.
Furthermore, the return value of run_command is sometimes used as exit
code by the caller.
This change arranges that the exit code of the program is returned as a
positive value, which can now be regarded as the "result" of the function.
System call failures continue to be reported as negative values.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Change calls to die(..., strerror(errno)) to use the new die_errno().
In the process, also make slight style adjustments: at least state
_something_ about the function that failed (instead of just printing
the pathname), and put paths in single quotes.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Essentially; s/type* /type */ as per the coding guidelines.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When run_command() was asked to run a non-existant command, its behavior
varied depending on the platform:
- on POSIX systems, we would fork, and then after the execvp call
failed, we could call die(), which prints a message to stderr and
exits with code 128.
- on Windows, we do a PATH lookup, realize the program isn't there, and
then return ERR_RUN_COMMAND_FORK
The goal of this patch is to make it clear to callers that the specific
error was a missing command. To do this, we will return the error code
ERR_RUN_COMMAND_EXEC, which is already defined in run-command.h, checked
for in several places, but never actually gets set.
The new behavior is:
- on POSIX systems, we exit the forked process with code 127 (the same
as the shell uses to report missing commands). The parent process
recognizes this code and returns an EXEC error. The stderr message is
silenced, since the caller may be speculatively trying to run a
command. Instead, we use trace_printf so that somebody interested in
debugging can see the error that occured.
- on Windows, we check errno, which is already set correctly by
mingw_spawnvpe, and report an EXEC error instead of a FORK error
Thus it is safe to speculatively run a command:
int r = run_command_v_opt(argv, 0);
if (r == -ERR_RUN_COMMAND_EXEC)
/* oops, it wasn't found; try something else */
else
/* we failed for some other reason, error is in r */
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
A function that runs a hook is used in several Git commands.
builtin-commit.c has the one that is most general for cases without
piping. The one in builtin-gc.c prints some useful warnings.
This patch moves a merged version of these variants into libgit and
lets the other builtins use this libified run_hook().
The run_hook() function used in receive-pack.c feeds the standard
input of the pre-receive or post-receive hooks. This function is
renamed to run_receive_hook() because the libified run_hook() cannot
handle this.
Mentored-by: Daniel Barkalow <barkalow@iabervon.org>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This function is not used anywhere.
Johannes Sixt <johannes.sixt@telecom.at>:
> Future callers can use run_command_v_opt_cd_env() instead.
Signed-off-by: Nanako Shiraishi <nanako3@lavabit.com>
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
This adds fflush(NULL) before fork() in start_command(), to keep
the generic interface safe.
A remaining use of fork() with no flushing is in a comment in
show_tree(). Rewrite that comment to use start_command().
Signed-off-by: Anders Melchiorsen <mail@cup.kalibalik.dk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This prevents double output in case stdout is redirected.
Signed-off-by: Anders Melchiorsen <mail@cup.kalibalik.dk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We prefer running the dashless form, and POSIX side already does so; we
should use it in MinGW's start_command(), too.
Signed-off-by: Steffen Prohaska <prohaska@zib.de>
Acked-by: Johannes Sixt <johannes.sixt@telecom.at>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This is a function provided by the caller which is called
_after_ the process is forked, but before the spawned
program is executed. On platforms (like mingw) where
subprocesses are forked and executed in a single call, the
preexec callback is simply ignored.
This will be used in the following patch to do some setup
for 'less' that must happen in the forked child.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When GIT_TRACE is set, the user is most likely wanting to see an external
command that is about to be executed.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The problem with Windows's own implementation is that it tries to be
clever when a console program is invoked from a GUI application: In this
case it sometimes automatically allocates a new console window. As a
consequence, the IO channels of the spawned program are directed to the
console, but the invoking application listens on channels that are now
directed to nowhere.
In this implementation we use the lowlevel facilities of CreateProcess(),
which offers a flag to tell the system not to open a console. As a side
effect, only stdin, stdout, and stderr channels will be accessible from
C programs that are spawned. Other channels (file handles, pipe handles,
etc.) are still inherited by the spawned program, but it doesn't get
enough information to access them.
Johannes Schindelin integrated path quoting and unified the various
*execv* and *spawnv* helpers. Eric Raible suggested to also quote '{'.
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
In upload-pack we must explicitly close the output channel of rev-list.
(On Unix, the channel is closed automatically because process that runs
rev-list terminates.)
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
On Windows, we have spawnv() variants to run a child process instead of
fork()/exec(). In order to attach pipe ends to stdin, stdout, and stderr,
we have to use this idiom:
save1 = dup(1);
dup2(pipe[1], 1);
spawnv();
dup2(save1, 1);
close(pipe[1]);
assuming that the descriptors created by pipe() are not inheritable.
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
With this patch, in the 'start_command' function after forking
we now take care of stderr in the child process before stdout.
This way if 'start_command' is called with a 'child_process'
argument like this:
.err = -1;
.stdout_to_stderr = 1;
then stderr will be redirected to a pipe before stdout is
redirected to stderr. So we can now get the process' stdout
from the pipe (as well as its stderr).
Earlier such a call would have redirected stdout to stderr
before stderr was itself redirected, and therefore stdout
would not have followed stderr, which would not have been
very useful anyway.
Update documentation in 'api-run-command.txt' accordingly.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
Acked-by: Johannes Sixt <johannes.sixt@telecom.at>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Callers of start_command() can set the members .in and .out of struct
child_process to a value > 0 to specify that this descriptor is used as
the stdin or stdout of the child process.
Previously, if start_command() was successful, this descriptor was closed
upon return. Here we now make sure that the descriptor is also closed in
case of failures. All callers are updated not to close the file descriptor
themselves after start_command() was called.
Note that earlier run_gpg_verify() of git-verify-tag set .out = 1, which
worked because start_command() treated this as a special case, but now
this is incorrect because it closes the descriptor. The intent here is to
inherit stdout to the child, which is achieved by .out = 0.
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
By setting .in, .out, or .err members of struct child_process to -1, the
callers of start_command() can request that a pipe is allocated that talks
to the child process and one end is returned by replacing -1 with the
file descriptor.
Previously, a flag was set (for .in and .out, but not .err) to signal
finish_command() to close the pipe end that start_command() had handed out,
so it was optional for callers to close the pipe, and many already do so.
Now we make it mandatory to close the pipe.
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Some callers may wish to redirect stderr to /dev/null in some
contexts, such as if they are executing a command only to get
the exit status and don't want users to see whatever output it
may produce as a side-effect of computing that exit status.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
This adds start_async() and finish_async(), which runs a function
asynchronously. Communication with the caller happens only via pipes.
For this reason, this implementation forks off a child process that runs
the function.
[sp: Style nit fixed by removing unnecessary block on if condition
inside of start_async()]
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
This adds another stanza that allocates a pipe that is connected to the
child's stderr and that the caller can read from. In order to request this
pipe, the caller sets cmd->err to -1.
The implementation is not exactly modeled after the stdout case: For stdout
the caller can supply an existing file descriptor, but this facility is
nowhere needed in the stderr case. Additionally, the caller is required to
close cmd->err.
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
To unset a variable, just specify its name, without "=". For example:
const char *env[] = {"GIT_DIR=.git", "PWD", NULL};
const char *argv[] = {"git-ls-files", "-s", NULL};
int err = run_command_v_opt_cd_env(argv, RUN_GIT_CMD, ".", env);
The PWD will be unset before executing git-ls-files.
Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
Signed-off-by: Junio C Hamano <junkio@cox.net>
There is no way to specify and override for the environment:
there'd be no user for it yet.
Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
Signed-off-by: Junio C Hamano <junkio@cox.net>
It can make code simplier (no need to preserve cwd) and safer
(no chance the cwd of the current process is accidentally forgotten).
Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
Signed-off-by: Junio C Hamano <junkio@cox.net>
Some run-command callers may wish to just discard any data that
is sent to stdout from the child. This is a lot like our existing
no_stdin support, we just open /dev/null and duplicate the descriptor
into position.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
Signed-off-by: Junio C Hamano <junkio@cox.net>
Some potential callers of the run_command family of functions need
to control not only the stdin redirection of the child, but also
the stdout redirection of the child. This can now be setup much
like the already existing stdin redirection.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
Signed-off-by: Junio C Hamano <junkio@cox.net>
I started hacking on a change to add stdout redirection support to
the run_command family, but found I was using a lot of close calls
on two pipes in an array (such as for pipe). So I'm doing a tiny
bit of refactoring first to make the next set of changes clearer.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
Signed-off-by: Junio C Hamano <junkio@cox.net>
bert Dvornik