summaryrefslogtreecommitdiff
path: root/run-command.c
Commit message (Collapse)AuthorAgeFilesLines
* Merge branch 'rs/daemon-plug-child-leak'Junio C Hamano2015-11-031-6/+9
|\ | | | | | | | | | | | | | | | | "git daemon" uses "run_command()" without "finish_command()", so it needs to release resources itself, which it forgot to do. * rs/daemon-plug-child-leak: daemon: plug memory leak run-command: factor out child_process_clear()
| * run-command: factor out child_process_clear()René Scharfe2015-11-021-6/+9
| | | | | | | | | | | | | | | | | | Avoid duplication by moving the code to release allocated memory for arguments and environment to its own function, child_process_clear(). Export it to provide a counterpart to child_process_init(). Signed-off-by: Rene Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | Merge branch 'ti/glibc-stdio-mutex-from-signal-handler'Junio C Hamano2015-10-071-8/+17
|\ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Allocation related functions and stdio are unsafe things to call inside a signal handler, and indeed killing the pager can cause glibc to deadlock waiting on allocation mutex as our signal handler tries to free() some data structures in wait_for_pager(). Reduce these unsafe calls. * ti/glibc-stdio-mutex-from-signal-handler: pager: don't use unsafe functions in signal handlers
| * | pager: don't use unsafe functions in signal handlersti/glibc-stdio-mutex-from-signal-handlerTakashi Iwai2015-09-041-8/+17
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Since the commit a3da8821208d (pager: do wait_for_pager on signal death), we call wait_for_pager() in the pager's signal handler. The recent bug report revealed that this causes a deadlock in glibc at aborting "git log" [*1*]. When this happens, git process is left unterminated, and it can't be killed by SIGTERM but only by SIGKILL. The problem is that wait_for_pager() function does more than waiting for pager process's termination, but it does cleanups and printing errors. Unfortunately, the functions that may be used in a signal handler are very limited [*2*]. Particularly, malloc(), free() and the variants can't be used in a signal handler because they take a mutex internally in glibc. This was the cause of the deadlock above. Other than the direct calls of malloc/free, many functions calling malloc/free can't be used. strerror() is such one, either. Also the usage of fflush() and printf() in a signal handler is bad, although it seems working so far. In a safer side, we should avoid them, too. This patch tries to reduce the calls of such functions in signal handlers. wait_for_signal() takes a flag and avoids the unsafe calls. Also, finish_command_in_signal() is introduced for the same reason. There the free() calls are removed, and only waits for the children without whining at errors. [*1*] https://bugzilla.opensuse.org/show_bug.cgi?id=942297 [*2*] http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03 Signed-off-by: Takashi Iwai <tiwai@suse.de> Reviewed-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | | Merge branch 'jk/async-pkt-line'Junio C Hamano2015-10-051-1/+15
|\ \ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | The debugging infrastructure for pkt-line based communication has been improved to mark the side-band communication specifically. * jk/async-pkt-line: pkt-line: show packets in async processes as "sideband" run-command: provide in_async query function
| * | | run-command: provide in_async query functionJeff King2015-09-011-1/+15
| |/ / | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | It's not easy for arbitrary code to find out whether it is running in an async process or not. A top-level function which is fed to start_async() can know (you just pass down an argument saying "you are async"). But that function may call other global functions, and we would not want to have to pass the information all the way through the call stack. Nor can we simply set a global variable, as those may be shared between async threads and the main thread (if the platform supports pthreads). We need pthread tricks _or_ a global variable, depending on how start_async is implemented. The callers don't have enough information to do this right, so let's provide a simple query function that does. Fortunately we can reuse the existing infrastructure to make the pthread case simple (and even simplify die_async() by using our new function). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | | Merge branch 'jk/long-error-messages'Junio C Hamano2015-08-251-15/+2
|\ \ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | The codepath to produce error messages had a hard-coded limit to the size of the message, primarily to avoid memory allocation while calling die(). * jk/long-error-messages: vreportf: avoid intermediate buffer vreportf: report to arbitrary filehandles
| * | | vreportf: report to arbitrary filehandlesJeff King2015-08-111-15/+2
| |/ / | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | The vreportf function always goes to stderr, but run-command wants child errors to go to the parent's original stderr. To solve this, commit a5487dd duplicates the stderr fd and installs die and error handlers to direct the output appropriately (which later turned into the vwritef function). This has two downsides, though: - we make multiple calls to write(), which contradicts the "write at once" logic from d048a96 (print warning/error/fatal messages in one shot, 2007-11-09). - the custom handlers basically duplicate the normal handlers. They're only a few lines of code, but we should not have to repeat the magic "exit(128)", for example. We can solve the first by using fdopen() on the duplicated descriptor. We can't pass this to vreportf, but we could introduce a new vreportf_to to handle it. However, to fix the second problem, we instead introduce a new "set_error_handle" function, which lets the normal vreportf calls output to a handle besides stderr. Thus we can get rid of our custom handlers entirely, and just ask the regular handlers to output to our new descriptor. And as vwritef has no more callers, it can just go away. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | | find_hook: keep our own static bufferJeff King2015-08-101-4/+6
|/ / | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | The find_hook function returns the results of git_path, which is a static buffer shared by other path-related calls. Returning such a buffer is slightly dangerous, because it can be overwritten by seemingly unrelated functions. Let's at least keep our _own_ static buffer, so you can only get in trouble by calling find_hook in quick succession, which is less likely to happen and more obvious to notice. While we're at it, let's add some documentation of the function's limitations. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | Merge branch 'nd/multiple-work-trees'Junio C Hamano2015-05-111-2/+2
|\ \ | |/ |/| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | A replacement for contrib/workdir/git-new-workdir that does not rely on symbolic links and make sharing of objects and refs safer by making the borrowee and borrowers aware of each other. * nd/multiple-work-trees: (41 commits) prune --worktrees: fix expire vs worktree existence condition t1501: fix test with split index t2026: fix broken &&-chain t2026 needs procondition SANITY git-checkout.txt: a note about multiple checkout support for submodules checkout: add --ignore-other-wortrees checkout: pass whole struct to parse_branchname_arg instead of individual flags git-common-dir: make "modules/" per-working-directory directory checkout: do not fail if target is an empty directory t2025: add a test to make sure grafts is working from a linked checkout checkout: don't require a work tree when checking out into a new one git_path(): keep "info/sparse-checkout" per work-tree count-objects: report unused files in $GIT_DIR/worktrees/... gc: support prune --worktrees gc: factor out gc.pruneexpire parsing code gc: style change -- no SP before closing parenthesis checkout: clean up half-prepared directories in --to mode checkout: reject if the branch is already checked out elsewhere prune: strategies for linked checkouts checkout: support checking out into a new working directory ...
| * path.c: make get_pathname() call sites return const char *Nguyễn Thái Ngọc Duy2014-12-011-2/+2
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | 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>
* | Merge branch 'jk/run-command-capture'Junio C Hamano2015-03-251-1/+22
|\ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | The run-command interface was easy to abuse and make a pipe for us to read from the process, wait for the process to finish and then attempt to read its output, which is a pattern that lead to a deadlock. Fix such uses by introducing a helper to do this correctly (i.e. we need to read first and then wait the process to finish) and also add code to prevent such abuse in the run-command helper. * jk/run-command-capture: run-command: forbid using run_command with piped output trailer: use capture_command submodule: use capture_command wt-status: use capture_command run-command: introduce capture_command helper wt_status: fix signedness mismatch in strbuf_read call wt-status: don't flush before running "submodule status"
| * | run-command: forbid using run_command with piped outputjk/run-command-captureJeff King2015-03-221-1/+6
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Because run_command both spawns and wait()s for the command before returning control to the caller, any reads from the pipes we open must necessarily happen after wait() returns. This can lead to deadlock, as the child process may block on writing to us while we are blocked waiting for it to exit. Worse, it only happens when the child fills the pipe buffer, which means that the problem may come and go depending on the platform and the size of the output produced by the child. Let's detect and flag this dangerous construct so that we can catch potential bugs early in the test suite rather than having them happen in the field. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * | run-command: introduce capture_command helperJeff King2015-03-221-0/+16
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Something as simple as reading the stdout from a command turns out to be rather hard to do right. Doing: cmd.out = -1; run_command(&cmd); strbuf_read(&buf, cmd.out, 0); can result in deadlock if the child process produces a large amount of output. What happens is: 1. The parent spawns the child with its stdout connected to a pipe, of which the parent is the sole reader. 2. The parent calls wait(), blocking until the child exits. 3. The child writes to stdout. If it writes more data than the OS pipe buffer can hold, the write() call will block. This is a deadlock; the parent is waiting for the child to exit, and the child is waiting for the parent to call read(). So we might try instead: start_command(&cmd); strbuf_read(&buf, cmd.out, 0); finish_command(&cmd); But that is not quite right either. We are examining cmd.out and running finish_command whether start_command succeeded or not, which is wrong. Moreover, these snippets do not do any error handling. If our read() fails, we must make sure to still call finish_command (to reap the child process). And both snippets failed to close the cmd.out descriptor, which they must do (provided start_command succeeded). Let's introduce a run-command helper that can make this a bit simpler for callers to get right. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | | git-compat-util.h: move SHELL_PATH default into headerKyle J. McKay2015-03-101-4/+0
|/ / | | | | | | | | | | | | | | | | If SHELL_PATH is not defined we use "/bin/sh". However, run-command.c is not the only file that needs to use the default value so move it into a common header. Signed-off-by: Kyle J. McKay <mackyle@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | Merge branch 'jc/hook-cleanup'Junio C Hamano2014-12-221-17/+0
|\ \ | |/ |/| | | | | | | | | Remove unused code. * jc/hook-cleanup: run-command.c: retire unused run_hook_with_custom_index()
| * run-command.c: retire unused run_hook_with_custom_index()jc/hook-cleanupJunio C Hamano2014-12-011-17/+0
| | | | | | | | | | | | | | | | | | | | | | | | This was originally meant to be used to rewrite run_commit_hook() that only special cases the GIT_INDEX_FILE environment, but the run_hook_ve() refactoring done earlier made the implementation of run_commit_hook() thin and clean enough. Nobody uses this, so retire it as an unfinished clean-up made unnecessary. Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | run-command: use void to declare that functions take no parametersRené Scharfe2014-11-101-2/+2
| | | | | | | | | | | | | | | | | | 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>
* | Merge branch 'eb/no-pthreads'Junio C Hamano2014-10-241-0/+40
|\ \ | | | | | | | | | | | | | | | | | | | | | | | | 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
| * | Handle atexit list internaly for unthreaded buildseb/no-pthreadsEtienne Buira2014-10-191-0/+40
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | 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>
* | | run-command: add env_array, an optional argv_array for envRené Scharfe2014-10-191-0/+6
|/ / | | | | | | | | | | | | | | | | | | | | 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>
* | run-command: inline prepare_run_command_v_opt()René Scharfe2014-08-201-16/+8
| | | | | | | | | | | | | | | | | | 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>
* | run-command: call run_command_v_opt_cd_env() instead of duplicating itRené Scharfe2014-08-201-3/+1
| | | | | | | | | | Signed-off-by: Rene Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | run-command: introduce child_process_init()René Scharfe2014-08-201-0/+6
| | | | | | | | | | | | | | | | | | 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>
* | run-command: introduce CHILD_PROCESS_INITRené Scharfe2014-08-201-2/+1
|/ | | | | | | | | | | | 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>
* Merge branch 'sk/mingw-uni-fix-more'Junio C Hamano2014-07-301-8/+2
|\ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | 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
| * Win32: don't copy the environment twice when spawning child processesKarsten Blees2014-07-211-8/+2
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | 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>
* | run-command: use internal argv_array of struct child_process in run_hook_ve()René Scharfe2014-07-171-11/+4
|/ | | | | | | | 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>
* run-command: store an optional argv_arrayJeff King2014-05-151-1/+8
| | | | | | | | | | | | | | | | | | | | | | | 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>
* commit: fix patch hunk editing with "commit -p -m"Benoit Pierre2014-03-181-12/+32
| | | | | | | | 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>
* run-command: trivial style fixesFelipe Contreras2013-10-311-8/+5
| | | | | Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* Merge branch 'tr/fd-gotcha-fixes'Junio C Hamano2013-07-221-1/+4
|\ | | | | | | | | | | | | | | | | 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()
| * run-command: dup_devnull(): guard against syscalls failingThomas Rast2013-07-121-1/+4
| | | | | | | | | | | | | | | | 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>
* | mingw: rename WIN32 cpp macro to GIT_WINDOWS_NATIVEJonathan Nieder2013-05-081-5/+5
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | 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>
* | Merge branch 'jk/a-thread-only-dies-once'Junio C Hamano2013-04-191-0/+11
|\ \ | | | | | | | | | | | | | | | | | | | | | | | | 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
| * | run-command: use thread-aware die_is_recursing routineJeff King2013-04-161-0/+11
| |/ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | 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>
* | run-command: always set failed_errno in start_commandJeff King2013-03-211-2/+3
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | 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>
* | Merge branch 'sb/run-command-fd-error-reporting'Junio C Hamano2013-02-071-2/+6
|\ \ | | | | | | | | | | | | * sb/run-command-fd-error-reporting: run-command: be more informative about what failed
| * | run-command: be more informative about what failedStephen Boyd2013-02-011-2/+6
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | 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>
| * | Merge branch 'dg/run-command-child-cleanup' into maintJunio C Hamano2012-09-201-6/+7
| |\ \ | | | | | | | | | | | | | | | | * dg/run-command-child-cleanup: run-command.c: fix broken list iteration in clear_child_for_cleanup
| * \ \ Merge branch 'jc/maint-sane-execvp-notdir' into maint-1.7.11Junio C Hamano2012-09-111-0/+2
| |\ \ \ | | | | | | | | | | | | | | | | | | | | * jc/maint-sane-execvp-notdir: sane_execvp(): ignore non-directory on $PATH
* | | | | hooks: Add function to check if a hook existsAaron Schrab2013-01-141-2/+13
| |_|_|/ |/| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | 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>
* | | | run-command: encode signal death as a positive integerJeff King2013-01-061-1/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | 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>
* | | | fix compilation with NO_PTHREADSJeff King2013-01-051-1/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | 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>
* | | | run-command: do not warn about child death from terminalJeff King2012-12-021-1/+2
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | 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>
* | | | run-command: drop silent_exec_failure arg from wait_or_whineJeff King2012-12-021-4/+3
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | 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>
* | | | Merge branch 'jk/no-more-pre-exec-callback'Jeff King2012-10-251-10/+0
|\ \ \ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | 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
| * | | | pager: drop "wait for output to run less" hackJeff King2012-06-051-10/+0
| |/ / / | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | 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>
* | | | Merge branch 'dg/run-command-child-cleanup'Junio C Hamano2012-09-141-6/+7
|\ \ \ \ | | |_|/ | |/| | | | | | | | | | | | | | | | | | | | | | 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
| * | | run-command.c: fix broken list iteration in clear_child_for_cleanupDavid Gould2012-09-111-6/+7
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | 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>