| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
Before the previous commit, get_pathname returns an array of PATH_MAX
length. Even if git_path() and similar functions does not use the
whole array, git_path() caller can, in theory.
After the commit, get_pathname() may return a buffer that has just
enough room for the returned string and git_path() caller should never
write beyond that.
Make git_path(), mkpath() and git_path_submodule() return a const
buffer to make sure callers do not write in it at all.
This could have been part of the previous commit, but the "const"
conversion is too much distraction from the core changes in path.c.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
Explicitly declare that git_atexit_dispatch() and git_atexit_clear()
take no parameters instead of leaving their parameter list empty and
thus unspecified.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
Allow us build with NO_PTHREADS=NoThanks compilation option.
* eb/no-pthreads:
Handle atexit list internaly for unthreaded builds
pack-objects: set number of threads before checking and warning
index-pack: fix compilation with NO_PTHREADS
|
|
Wrap atexit()s calls on unthreaded builds to handle callback list
internally.
This is needed because on unthreaded builds, asyncs inherits parent's
atexit() list, that gets run as soon as the async exit()s (and again at
the end of async's parent process). That led to remove temporary files
too early.
Also remove a by-atexit-callback guard against this kind of issue in
clone.c, as this patch makes it redundant.
Fixes test 5537 (temporary shallow file vanished before unpack-objects
could open it)
BTW remove an unused variable in shallow.c.
Helped-by: Duy Nguyen <pclouds@gmail.com>
Helped-by: Andreas Schwab <schwab@linux-m68k.org>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Etienne Buira <etienne.buira@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
Similar to args, add a struct argv_array member to struct child_process
that simplifies specifying the environment for children. It is freed
automatically by finish_command() or if start_command() encounters an
error.
Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
Merge prepare_run_command_v_opt() and its only caller. This removes a
pointer indirection and allows to initialize the struct child_process
using CHILD_PROCESS_INIT.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
Signed-off-by: Rene Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
Add a helper function for initializing those struct child_process
variables for which the macro CHILD_PROCESS_INIT can't be used.
Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
Most struct child_process variables are cleared using memset first after
declaration. Provide a macro, CHILD_PROCESS_INIT, that can be used to
initialize them statically instead. That's shorter, doesn't require a
function call and is slightly more readable (especially given that we
already have STRBUF_INIT, ARGV_ARRAY_INIT etc.).
Helped-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
Most of these are battle-tested in msysgit and are needed to
complete what has been merged to 'master' already.
* sk/mingw-uni-fix-more:
Win32: enable color output in Windows cmd.exe
Win32: patch Windows environment on startup
Win32: keep the environment sorted
Win32: use low-level memory allocation during initialization
Win32: reduce environment array reallocations
Win32: don't copy the environment twice when spawning child processes
Win32: factor out environment block creation
Win32: unify environment function names
Win32: unify environment case-sensitivity
Win32: fix environment memory leaks
Win32: Unicode environment (incoming)
Win32: Unicode environment (outgoing)
Revert "Windows: teach getenv to do a case-sensitive search"
tests: do not pass iso8859-1 encoded parameter
|
|
When spawning child processes via start_command(), the environment and all
environment entries are copied twice. First by make_augmented_environ /
copy_environ to merge with child_process.env. Then a second time by
make_environment_block to create a sorted environment block string as
required by CreateProcess.
Move the merge logic to make_environment_block so that we only need to copy
the environment once. This changes semantics of the env parameter: it now
expects a delta (such as child_process.env) rather than a full environment.
This is not a problem as the parameter is only used by start_command()
(all other callers previously passed char **environ, and now pass NULL).
The merge logic no longer xstrdup()s the environment strings, so do_putenv
must not free them. Add a parameter to distinguish this from normal putenv.
Remove the now unused make_augmented_environ / free_environ API.
Signed-off-by: Karsten Blees <blees@dcon.de>
Signed-off-by: Stepan Kasal <kasal@ucw.cz>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
Use the existing argv_array member instead of providing our own. This
way we don't have to initialize or clean it up explicitly.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
All child_process structs need to point to an argv. For
flexibility, we do not mandate the use of a dynamic
argv_array. However, because the child_process does not own
the memory, this can make memory management with a
separate argv_array difficult.
For example, if a function calls start_command but not
finish_command, the argv memory must persist. The code needs
to arrange to clean up the argv_array separately after
finish_command runs. As a result, some of our code in this
situation just leaks the memory.
To help such cases, this patch adds a built-in argv_array to
the child_process, which gets cleaned up automatically (both
in finish_command and when start_command fails). Callers
may use it if they choose, but can continue to use the raw
argv if they wish.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
Don't change git environment: move the GIT_EDITOR=":" override to the
hook command subprocess, like it's already done for GIT_INDEX_FILE.
Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
Two places we did not check return value (expected to be a file
descriptor) correctly.
* tr/fd-gotcha-fixes:
run-command: dup_devnull(): guard against syscalls failing
git_mkstemps: correctly test return value of open()
|
|
dup_devnull() did not check the return values of open() and dup2().
Fix this omission.
Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
Throughout git, it is assumed that the WIN32 preprocessor symbol is
defined on native Windows setups (mingw and msvc) and not on Cygwin.
On Cygwin, most of the time git can pretend this is just another Unix
machine, and Windows-specific magic is generally counterproductive.
Unfortunately Cygwin *does* define the WIN32 symbol in some headers.
Best to rely on a new git-specific symbol GIT_WINDOWS_NATIVE instead,
defined as follows:
#if defined(WIN32) && !defined(__CYGWIN__)
# define GIT_WINDOWS_NATIVE
#endif
After this change, it should be possible to drop the
CYGWIN_V15_WIN32API setting without any negative effect.
[rj: %s/WINDOWS_NATIVE/GIT_WINDOWS_NATIVE/g ]
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
A regression fix for the logic to detect die() handler triggering
itself recursively.
* jk/a-thread-only-dies-once:
run-command: use thread-aware die_is_recursing routine
usage: allow pluggable die-recursion checks
|
|
If we die from an async thread, we do not actually exit the
program, but just kill the thread. This confuses the static
counter in usage.c's default die_is_recursing function; it
updates the counter once for the thread death, and then when
the main program calls die() itself, it erroneously thinks
we are recursing. The end result is that we print "recursion
detected in die handler" instead of the real error in such a
case (the easiest way to trigger this is having a remote
connection hang up while running a sideband demultiplexer).
This patch solves it by using a per-thread counter when the
async_die function is installed; we detect recursion in each
thread (including the main one), but they do not step on
each other's toes.
Other threaded code does not need to worry about this, as
they do not install specialized die handlers; they just let
a die() from a sub-thread take down the whole program.
Since we are overriding the default recursion-check
function, there is an interesting corner case that is not a
problem, but bears some explanation. Imagine the main thread
calls die(), and then in the die_routine starts an async
call. We will switch to using thread-local storage, which
starts at 0, for the main thread's counter, even though
the original counter was actually at 1. That's OK, though,
for two reasons:
1. It would miss only the first level of recursion, and
would still find recursive failures inside the async
helper.
2. We do not currently and are not likely to start doing
anything as heavyweight as starting an async routine
from within a die routine or helper function.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
When we fail to fork, we set the failed_errno variable to
the value of errno so it is not clobbered by later syscalls.
However, we do so in a conditional, and it is hard to see
later under what conditions the variable has a valid value.
Instead of setting it only when fork fails, let's just
always set it after forking. This is more obvious for human
readers (as we are no longer setting it as a side effect of
a strerror call), and it is more obvious to gcc, which no
longer generates a spurious -Wuninitialized warning. It also
happens to match what the WIN32 half of the #ifdef does.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
* sb/run-command-fd-error-reporting:
run-command: be more informative about what failed
|
|
While debugging an error with verify_signed_buffer() the error
messages from run-command weren't very useful:
error: cannot create pipe for gpg: Too many open files
error: could not run gpg.
because they didn't indicate *which* pipe couldn't be created.
Print which pipe failed to be created in the error message so we
can more easily debug similar problems in the future.
For example, the above error now prints:
error: cannot create standard error pipe for gpg: Too many open files
error: could not run gpg.
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
Create find_hook() function to determine if a given hook exists and is
executable. If it is, the path to the script will be returned,
otherwise NULL is returned.
This encapsulates the tests that are used to check for the existence of
a hook in one place, making it easier to modify those checks if that is
found to be necessary. This also makes it simple for places that can
use a hook to check if a hook exists before doing, possibly lengthy,
setup work which would be pointless if no such hook is present.
The returned value is left as a static value from get_pathname() rather
than a duplicate because it is anticipated that the return value will
either be used as a boolean, immediately added to an argv_array list
which would result in it being duplicated at that point, or used to
actually run the command without much intervening work. Callers which
need to hold onto the returned value for a longer time are expected to
duplicate the return value themselves.
Signed-off-by: Aaron Schrab <aaron@schrab.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
When a sub-command dies due to a signal, we encode the
signal number into the numeric exit status as "signal -
128". This is easy to identify (versus a regular positive
error code), and when cast to an unsigned integer (e.g., by
feeding it to exit), matches what a POSIX shell would return
when reporting a signal death in $? or through its own exit
code.
So we have a negative value inside the code, but once it
passes across an exit() barrier, it looks positive (and any
code we receive from a sub-shell will have the positive
form). E.g., death by SIGPIPE (signal 13) will look like
-115 to us in inside git, but will end up as 141 when we
call exit() with it. And a program killed by SIGPIPE but run
via the shell will come to us with an exit code of 141.
Unfortunately, this means that when the "use_shell" option
is set, we need to be on the lookout for _both_ forms. We
might or might not have actually invoked the shell (because
we optimize out some useless shell calls). If we didn't invoke
the shell, we will will see the sub-process's signal death
directly, and run-command converts it into a negative value.
But if we did invoke the shell, we will see the shell's
128+signal exit status. To be thorough, we would need to
check both, or cast the value to an unsigned char (after
checking that it is not -1, which is a magic error value).
Fortunately, most callsites do not care at all whether the
exit was from a code or from a signal; they merely check for
a non-zero status, and sometimes propagate the error via
exit(). But for the callers that do care, we can make life
slightly easier by just using the consistent positive form.
This actually fixes two minor bugs:
1. In launch_editor, we check whether the editor died from
SIGINT or SIGQUIT. But we checked only the negative
form, meaning that we would fail to notice a signal
death exit code which was propagated through the shell.
2. In handle_alias, we assume that a negative return value
from run_command means that errno tells us something
interesting (like a fork failure, or ENOENT).
Otherwise, we simply propagate the exit code. Negative
signal death codes confuse us, and we print a useless
"unable to run alias 'foo': Success" message. By
encoding signal deaths using the positive form, the
existing code just propagates it as it would a normal
non-zero exit code.
The downside is that callers of run_command can no longer
differentiate between a signal received directly by the
sub-process, and one propagated. However, no caller
currently cares, and since we already optimize out some
calls to the shell under the hood, that distinction is not
something that should be relied upon by callers.
Fix the same logic in t/test-terminal.perl for consistency [jc:
raised by Jonathan in the discussion].
Signed-off-by: Jeff King <peff@peff.net>
Acked-by: Johannes Sixt <j6t@kdbg.org>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
Commit 1327452 cleaned up an unused parameter from
wait_or_whine, but forgot to update a caller that is inside
"#ifdef NO_PTHREADS".
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
SIGINT and SIGQUIT are not generally interesting signals to
the user, since they are typically caused by them hitting "^C"
or otherwise telling their terminal to send the signal.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
We do not actually use this parameter; instead we complain
from the child itself (for fork/exec) or from start_command
(if we are using spawn on Windows).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
Removes a workaround for buggy version of less older than version
406.
* jk/no-more-pre-exec-callback:
pager: drop "wait for output to run less" hack
|
|
* dg/run-command-child-cleanup:
run-command.c: fix broken list iteration in clear_child_for_cleanup
|
|
The code to wait for subprocess and remove it from our internal queue
wasn't quite right.
* dg/run-command-child-cleanup:
run-command.c: fix broken list iteration in clear_child_for_cleanup
|
|
* jc/maint-sane-execvp-notdir:
sane_execvp(): ignore non-directory on $PATH
|
|
Iterate through children_to_clean using 'next' fields but with an
extra level of indirection. This allows us to update the chain when
we remove a child and saves us managing several variables around
the loop mechanism.
Signed-off-by: David Gould <david@optimisefitness.com>
Acked-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
"git foo" errored out with "Not a directory" when the user had a non
directory on $PATH, and worse yet it masked an alias "foo" to run.
* jc/maint-sane-execvp-notdir:
sane_execvp(): ignore non-directory on $PATH
|
|
When you have a non-directory on your PATH, a funny thing happens:
$ PATH=$PATH:/bin/sh git foo
fatal: cannot exec 'git-foo': Not a directory?
Worse yet, as real commands always take precedence over aliases,
this behaviour interacts rather badly with them:
$ PATH=$PATH:/bin/sh git -c alias.foo=show git foo -s
fatal: cannot exec 'git-foo': Not a directory?
This is because an ENOTDIR error from the underlying execvp(2) is
reported back to the caller of our sane_execvp() wrapper as-is.
Translating it to ENOENT, just like the case where we _might_ have
the command in an unreadable directory, fixes it. Without an alias,
we would get
git: 'foo' is not a git command. See 'git --help'.
and we use the 'foo' alias when it is available, of course.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
Commit 35ce862 (pager: Work around window resizing bug in
'less', 2007-01-24) causes git's pager sub-process to wait
to receive input after forking but before exec-ing the
pager. To handle this, run-command had to grow a "pre-exec
callback" feature. Unfortunately, this feature does not work
at all on Windows (where we do not fork), and interacts
poorly with run-command's parent notification system. Its
use should be discouraged.
The bug in less was fixed in version 406, which was released
in June 2007. It is probably safe at this point to remove
our workaround. That lets us rip out the preexec_cb feature
entirely.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
Mops up an unfortunate fallout from bw/spawn-via-shell-path topic.
By Johannes Sixt
* js/spawn-via-shell-path-fix:
Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows
|
|
When PATH contains an unreadable directory, alias expansion code did not
kick in, and failed with an error that said "git-subcmd" was not found.
By Jeff King (1) and Ramsay Jones (1)
* jk/run-command-eacces:
run-command: treat inaccessible directories as ENOENT
compat/mingw.[ch]: Change return type of exec functions to int
|
|
The recent change to use SHELL_PATH instead of "sh" to spawn shell commands
is not suited for Windows:
- The default setting, "/bin/sh", does not work when git has to run the
shell because it is a POSIX style path, but not a proper Windows style
path.
- If it worked, it would hard-code a position in the files system where
the shell is expected, making git (more precisely, the POSIX toolset that
is needed alongside git) non-relocatable. But we cannot sacrifice
relocatability on Windows.
- Apart from that, even though the Makefile leaves SHELL_PATH set to
"/bin/sh" for the Windows builds, the build system passes a mangled path
to the compiler, and something like "D:/Src/msysgit/bin/sh" is used,
which is doubly bad because it points to where /bin/sh resolves to on
the system where git was built.
- Finally, the system's CreateProcess() function that is used under
mingw.c's hood does not work with forward slashes and cannot find the
shell.
Undo the earlier change on Windows.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
When execvp reports EACCES, it can be one of two things:
1. We found a file to execute, but did not have
permissions to do so.
2. We did not have permissions to look in some directory
in the $PATH.
In the former case, we want to consider this a
permissions problem and report it to the user as such (since
getting this for something like "git foo" is likely a
configuration error).
In the latter case, there is a good chance that the
inaccessible directory does not contain anything of
interest. Reporting "permission denied" is confusing to the
user (and prevents our usual "did you mean...?" lookup). It
also prevents git from trying alias lookup, since we do so
only when an external command does not exist (not when it
exists but has an error).
This patch detects EACCES from execvp, checks whether we are
in case (2), and if so converts errno to ENOENT. This
behavior matches that of "bash" (but not of simpler shells
that use execvp more directly, like "dash").
Test stolen from Junio.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
During the testing of the 1.7.10 rc series on Solaris for OpenCSW, it
was discovered that t7006-pager was failing due to finding a bad "sh"
in PATH after a call to execvp("sh", ...). This call was setup by
run_command.c:prepare_shell_cmd.
The PATH in use at the time saw /opt/csw/bin given precedence to
traditional Solaris paths such as /usr/bin and /usr/xpg4/bin. A
package named schilyutils (Joerg Schilling's utilities) was installed
on the build system and it delivered a modified version of the
traditional Solaris /usr/bin/sh as /opt/csw/bin/sh. This version of
sh suffers from many of the same problems as /usr/bin/sh.
The command-specific pager test failed due to the broken "sh" handling
^ as a pipe character. It tried to fork two processes when it
encountered "sed s/^/foo:/" as the pager command. This problem was
entirely dependent on the PATH of the user at runtime.
Possible fixes for this issue are:
1. Use the standard system() or popen() which both launch a POSIX
shell on Solaris as long as _POSIX_SOURCE is defined.
2. The git wrapper could prepend SANE_TOOL_PATH to PATH thus forcing
all unqualified commands run to use the known good tools on the
system.
3. The run_command.c:prepare_shell_command() could use the same
SHELL_PATH that is in the #! line of all all scripts and not rely
on PATH to find the sh to run.
Option 1 would preclude opening a bidirectional pipe to a filter
script and would also break git for Windows as cmd.exe is spawned from
system() (cf. v1.7.5-rc0~144^2, "alias: use run_command api to execute
aliases, 2011-01-07).
Option 2 is not friendly to users as it would negate their ability to
use tools of their choice in many cases. Alternately, injecting
SANE_TOOL_PATH such that it takes precedence over /bin and /usr/bin
(and anything with lower precedence than those paths) as
git-sh-setup.sh does would not solve the problem either as the user
environment could still allow a bad sh to be found. (Many OpenCSW
users will have /opt/csw/bin leading their PATH and some subset would
have schilyutils installed.)
Option 3 allows us to use a known good shell while still honouring the
users' PATH for the utilities being run. Thus, it solves the problem
while not negatively impacting either users or git's ability to run
external commands in convenient ways. Essentially, the shell is a
special case of tool that should not rely on SANE_TOOL_PATH and must
be called explicitly.
With this patch applied, any code path leading to
run_command.c:prepare_shell_cmd can count on using the same sane shell
that all shell scripts in the git suite use. Both the build system
and run_command.c will default this shell to /bin/sh unless
overridden.
Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
Several git commands are so-called dashed externals, that is commands
executed as a child process of the git wrapper command. If the git
wrapper is killed by a signal, the child process will continue to run.
This is different from internal commands, which always die with the git
wrapper command.
Enable the recently introduced cleanup mechanism for child processes in
order to make dashed externals act more in line with internal commands.
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
Acked-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
When we spawn a helper process, it should generally be done
and finish_command called before we exit. However, if we
exit abnormally due to an early return or a signal, the
helper may continue to run in our absence.
In the best case, this may simply be wasted CPU cycles or a
few stray messages on a terminal. But it could also mean a
process that the user thought was aborted continues to run
to completion (e.g., a push's pack-objects helper will
complete the push, even though you killed the push process).
This patch provides infrastructure for run-command to keep
track of PIDs to be killed, and clean them on signal
reception or input, just as we do with tempfiles. PIDs can
be added in two ways:
1. If NO_PTHREADS is defined, async helper processes are
automatically marked. By definition this code must be
ready to die when the parent dies, since it may be
implemented as a thread of the parent process.
2. If the run-command caller specifies the "clean_on_exit"
option. This is not the default, as there are cases
where it is OK for the child to outlive us (e.g., when
spawning a pager).
PIDs are cleared from the kill-list automatically during
wait_or_whine, which is called from finish_command and
finish_async.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
* jk/argv-array:
run_hook: use argv_array API
checkout: use argv_array API
bisect: use argv_array API
quote: provide sq_dequote_to_argv_array
refactor argv_array into generic code
quote.h: fix bogus comment
add sha1_array API docs
|
|
This was a pretty straightforward use, so it really doesn't
save that many lines. Still, perhaps it's a little bit more
readable.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
If the pager fails to run, git produces no output, e.g.:
$ GIT_PAGER=not-a-command git log
The error reporting fails for two reasons:
(1) start_command: There is a mechanism that detects errors during
execvp introduced in 2b541bf8 (start_command: detect execvp
failures early). The child writes one byte to a pipe only if
execvp fails. The parent waits for either EOF, when the
successful execvp automatically closes the pipe (see
FD_CLOEXEC in fcntl(1)), or it reads a single byte, in which
case it knows that the execvp failed. This mechanism is
incompatible with the workaround introduced in 35ce8622
(pager: Work around window resizing bug in 'less'), which
waits for input from the parent before the exec. Since both
the parent and the child are waiting for input from each
other, that would result in a deadlock. In order to avoid
that, the mechanism is disabled by closing the child_notifier
file descriptor.
(2) finish_command: The parent correctly detects the 127 exit
status from the child, but the error output goes nowhere,
since by that time it is already being redirected to the
child.
No simple solution for (1) comes to mind.
Number (2) can be solved by not sending error output to the pager.
Not redirecting error output to the pager can result in the pager
overwriting error output with standard output, however.
Since there is no reliable way to handle error reporting in the
parent, produce the output in the child instead.
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
The new process's error output may be redirected elsewhere, but if
the exec fails, output should still go to the parent's stderr. This
has already been done for the die_routine. Do the same for
error_routine.
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
If start_command fails after forking and before exec finishes, there
is not much use in noticing an I/O error on top of that.
finish_command will notice that the child exited with nonzero status
anyway. So as noted in v1.7.0.3~20^2 (run-command.c: fix build
warnings on Ubuntu, 2010-01-30) and v1.7.5-rc0~29^2 (2011-03-16), it
is safe to ignore errors from write in this codepath.
Even so, the result from write contains useful information: it tells
us if the write was cancelled by a signal (EINTR) or was only
partially completed (e.g., when writing to an almost-full pipe).
Let's use write_in_full to loop until the desired number of bytes have
been written (still ignoring errors if that fails).
As a happy side effect, the assignment to a dummy variable to appease
gcc -D_FORTIFY_SOURCE is no longer needed. xwrite and write_in_full
check the return value from write(2).
Noticed with gcc -Wunused-but-set-variable.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
This reverts commit ebec842773932e6f853acac70c80f84209b5f83e, which
somehow mistakenly thought that any non-zero return from write(2) is
an error.
|
|
Current gcc + glibc with -D_FORTIFY_SOURCE try very aggressively to
protect against a programming style which uses write(...) without
checking the return value for errors. Even the usual hint of casting
to (void) does not suppress the warning.
Sometimes when there is an output error, especially right before exit,
there really is nothing to be done. The obvious solution, adopted in
v1.7.0.3~20^2 (run-command.c: fix build warnings on Ubuntu,
2010-01-30), is to save the return value to a dummy variable:
ssize_t dummy;
dummy = write(...);
But that (1) is ugly and (2) triggers -Wunused-but-set-variable
warnings with gcc-4.6 -Wall, so we are not much better off than when
we started.
Instead, use an "if" statement with an empty body to make the intent
clear.
if (write(...))
; /* yes, yes, there was an error. */
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Improved-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
Nguyễn Thái Ngọc Duy
René Scharfe
Junio C Hamano
Etienne Buira
Karsten Blees
Jeff King
Benoit Pierre
Felipe Contreras
Thomas Rast
Jonathan Nieder
Stephen Boyd
Aaron Schrab
David Gould
Johannes Sixt
Ben Walton
Clemens Buchacher