Re: [PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias
Jeff King writes: > The outer git wrapper doesn't start the pager, so its stderr still gets > seen by the user. But the _inner_ git-log does start the pager, and then > dies of SIGPIPE. > > So yeah, I think we want something like this on top of > nd/clear-gitenv-upon-use-of-alias. That makes sense to me. > -- >8 -- > Subject: [PATCH] run-command: don't warn on SIGPIPE deaths > > When git executes a sub-command, we print a warning if the > command dies due to a signal, but make an exception for > "uninteresting" cases like SIGINT and SIGQUIT (since the > user presumably just hit ^C). > > We should make a similar exception for SIGPIPE, because it's > an expected and uninteresting return in most cases; it > generally means the user quit the pager before git had > finished generating all output. This used to be very hard > to trigger in practice, because: > > 1. We only complain if we see a real SIGPIPE death, not > the shell-induced 141 exit code. This means that > anything we run via the shell does not trigger the > warning, which includes most non-trivial aliases. > > 2. The common case for SIGPIPE is the user quitting the > pager before git has finished generating all output. > But if the user triggers a pager with "-p", we redirect > the git wrapper's stderr to that pager, too. Since the > pager is dead, it means that the message goes nowhere. > > 3. You can see it if you run your own pager, like > "git foo | head". But that only happens if "foo" is a > non-builtin (so it doesn't work with "log", for > example). > > However, it may become more common after 86d26f2, which > teaches alias to re-exec builtins rather than running them > in the same process. This case doesn't trigger (1), as we > don't need a shell to run a git command. It doesn't trigger > (2), because the pager is not started by the original git, > but by the inner re-exec of git. And it doesn't trigger (3), > because builtins are treated more like non-builtins in this > case. > > Given how flaky this message already is (e.g., you cannot > even know whether you will see it, as git optimizes out some > shell invocations behind the scenes based on the contents of > the command!), and that it is unlikely to ever provide > useful information, let's suppress it for all cases of > SIGPIPE. > > Signed-off-by: Jeff King > --- > run-command.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/run-command.c b/run-command.c > index 13fa452..694a6ff 100644 > --- a/run-command.c > +++ b/run-command.c > @@ -245,7 +245,7 @@ static int wait_or_whine(pid_t pid, const char *argv0, > int in_signal) > error("waitpid is confused (%s)", argv0); > } else if (WIFSIGNALED(status)) { > code = WTERMSIG(status); > - if (code != SIGINT && code != SIGQUIT) > + if (code != SIGINT && code != SIGQUIT && code != SIGPIPE) > error("%s died of signal %d", argv0, code); > /* >* This return value is chosen so that code & 0xff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias
On Thu, Dec 24, 2015 at 04:35:33PM +0700, Duy Nguyen wrote: > On Thu, Dec 24, 2015 at 4:31 AM, Jeff King wrote: > > 2. I doubt anybody is actually seeing this in practice anymore. But > > maybe I am misunderstanding something in Duy's series that changes > > this. > > There are two parts in your patch, one (that you two seemed to focus > on) about return code with "!" aliases. Another, suppressing SIGPIPE, > affects more than "!" aliases. Sorry if I was confusing; most of the examples in my previous message are about the SIGPIPE thing. I was having trouble triggering the message in practice, even for externals (because the error message goes to the pager, too!). > In my case it's execv_dashed_external(). Non-"!" aliases are now > forced to use that function. Thanks, this is the part I was missing. The outer git wrapper doesn't start the pager, so its stderr still gets seen by the user. But the _inner_ git-log does start the pager, and then dies of SIGPIPE. So yeah, I think we want something like this on top of nd/clear-gitenv-upon-use-of-alias. -- >8 -- Subject: [PATCH] run-command: don't warn on SIGPIPE deaths When git executes a sub-command, we print a warning if the command dies due to a signal, but make an exception for "uninteresting" cases like SIGINT and SIGQUIT (since the user presumably just hit ^C). We should make a similar exception for SIGPIPE, because it's an expected and uninteresting return in most cases; it generally means the user quit the pager before git had finished generating all output. This used to be very hard to trigger in practice, because: 1. We only complain if we see a real SIGPIPE death, not the shell-induced 141 exit code. This means that anything we run via the shell does not trigger the warning, which includes most non-trivial aliases. 2. The common case for SIGPIPE is the user quitting the pager before git has finished generating all output. But if the user triggers a pager with "-p", we redirect the git wrapper's stderr to that pager, too. Since the pager is dead, it means that the message goes nowhere. 3. You can see it if you run your own pager, like "git foo | head". But that only happens if "foo" is a non-builtin (so it doesn't work with "log", for example). However, it may become more common after 86d26f2, which teaches alias to re-exec builtins rather than running them in the same process. This case doesn't trigger (1), as we don't need a shell to run a git command. It doesn't trigger (2), because the pager is not started by the original git, but by the inner re-exec of git. And it doesn't trigger (3), because builtins are treated more like non-builtins in this case. Given how flaky this message already is (e.g., you cannot even know whether you will see it, as git optimizes out some shell invocations behind the scenes based on the contents of the command!), and that it is unlikely to ever provide useful information, let's suppress it for all cases of SIGPIPE. Signed-off-by: Jeff King --- run-command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/run-command.c b/run-command.c index 13fa452..694a6ff 100644 --- a/run-command.c +++ b/run-command.c @@ -245,7 +245,7 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal) error("waitpid is confused (%s)", argv0); } else if (WIFSIGNALED(status)) { code = WTERMSIG(status); - if (code != SIGINT && code != SIGQUIT) + if (code != SIGINT && code != SIGQUIT && code != SIGPIPE) error("%s died of signal %d", argv0, code); /* * This return value is chosen so that code & 0xff -- 2.7.0.rc3.367.g09631da -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias
On Thu, Dec 24, 2015 at 4:31 AM, Jeff King wrote: > 2. I doubt anybody is actually seeing this in practice anymore. But > maybe I am misunderstanding something in Duy's series that changes > this. There are two parts in your patch, one (that you two seemed to focus on) about return code with "!" aliases. Another, suppressing SIGPIPE, affects more than "!" aliases. In my case it's execv_dashed_external(). Non-"!" aliases are now forced to use that function. This is the output with 'pu'. Notice git-log is executed separately > ~/w/git $ GIT_TRACE=2 PAGER=true ./git --exec-path=. l 16:28:19.167040 git.c:567 trace: exec: 'git-l' 16:28:19.167088 run-command.c:345 trace: run_command: 'git-l' 16:28:19.168599 git.c:286 trace: alias expansion: l => 'log' '--stat' 16:28:19.168633 git.c:567 trace: exec: 'git-log' '--stat' 16:28:19.168649 run-command.c:345 trace: run_command: 'git-log' '--stat' 16:28:19.170420 git.c:350 trace: built-in: git 'log' '--stat' 16:28:19.210074 run-command.c:345 trace: run_command: 'true' 16:28:19.210382 run-command.c:204 trace: exec: 'true' error: git-log died of signal 13 With your patch on top, "git-log died of.." goes away. And this is with 'master', where the builtin version of git-log is used > ~/w/git $ GIT_TRACE=2 PAGER=true ./git --exec-path=. l 16:29:17.546382 git.c:561 trace: exec: 'git-l' 16:29:17.546428 run-command.c:343 trace: run_command: 'git-l' 16:29:17.547485 git.c:282 trace: alias expansion: l => 'log' '--stat' 16:29:17.548482 git.c:348 trace: built-in: git 'log' '--stat' 16:29:17.586457 run-command.c:343 trace: run_command: 'true' 16:29:17.586770 run-command.c:202 trace: exec: 'true' So, in practice, people will see "died of signal 13" a lot more often if my series is merged and released (I assume that people often use aliases for paged commands like git-log, git-diff...) -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias
On Wed, Dec 23, 2015 at 09:37:04PM +0100, Johannes Sixt wrote: > >--- a/git.c > >+++ b/git.c > >@@ -252,7 +252,7 @@ static int handle_alias(int *argcp, const char ***argv) > > alias_argv[argc] = NULL; > > > > ret = run_command_v_opt(alias_argv, RUN_USING_SHELL); > >-if (ret >= 0) /* normal exit */ > >+if (ret != -1) /* normal exit */ > > Why does this make a difference? We only ever return -1, zero, or a positive > value from run_command/finish_command/wait_or_whine, as far as I can see. Yeah, you're right. This bit predates 709ca73 (run-command: encode signal death as a positive integer, 2013-01-05), which came out of the same discussion. So I'd agree this hunk can simply be dropped. That leaves the ignoring of SIGPIPE in wait_or_whine. I started to rewrite the commit message to drop the first hunk, but I found I couldn't replicate the problem in the second either! Doing: GIT_PAGER=false git -c alias.foo='!git log -p' foo doesn't trigger it. We run the alias through a shell, so we see only the munged "141" value from the shell's exit code. Something like: GIT_PAGER=false git -p -c alias.foo='!yes' foo does generate the error message. But we've redirected stderr into the pager at that point, so by definition it can never be shown. So I think we would need a case where: - the outer git doesn't run the pager that dies; instead the pager is run inside the alias. But... - inside the alias cannot be a shell pipeline, since "foo | less" will report the exit code of "less", not "foo" (we make special arrangements in git to propagate the exit code of "foo"). So it pretty much has to be a git invocation inside the alias. But... - The git invocation will convert signal death in the sub-process into 141, like a shell would. So I'm not sure if this is triggerable at all with an alias. I did manage to trigger it with an external command, like: $ cat $(which git-yes) #!/bin/sh # This _has_ to be exec, otherwise the shell converts SIGPIPE death # into 141. exec yes and then if you run your _own_ pager, like this: $ git yes | false error: git-yes died of signal 13 you see it. But if git starts the pager, you don't: $ GIT_PAGER=false git -p yes Because the stderr of the outer git process is going to the same dead pipe. So my takeaways are: 1. Complaining about signal death in general is going to be flaky, because it's so easy for shells or git to rewrite the exit code and not trigger WIFSIGNALED() in the first place. 2. I doubt anybody is actually seeing this in practice anymore. But maybe I am misunderstanding something in Duy's series that changes this. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias
Am 23.12.2015 um 10:37 schrieb Jeff King: The second line comes from handle_alias itself. It calls die_errno whenever run_command returns a negative value. However, only -1 indicates a syscall error where errno has something useful (note that it says the useless "success" above). Instead, we treat negative returns from run_command (except for -1) as a normal code to be passed to exit. Signed-off-by: Jeff King --- git.c | 2 +- run-command.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/git.c b/git.c index 6ed824c..34a18a3 100644 --- a/git.c +++ b/git.c @@ -252,7 +252,7 @@ static int handle_alias(int *argcp, const char ***argv) alias_argv[argc] = NULL; ret = run_command_v_opt(alias_argv, RUN_USING_SHELL); - if (ret >= 0) /* normal exit */ + if (ret != -1) /* normal exit */ Why does this make a difference? We only ever return -1, zero, or a positive value from run_command/finish_command/wait_or_whine, as far as I can see. exit(ret); die_errno("While expanding alias '%s': '%s'", -- Hannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias
On Wed, Dec 23, 2015 at 4:37 AM, Jeff King wrote: > Subject: [PATCH] avoid SIGPIPE warnings for aliases > > When git executes an alias that specifies an external > command, it will complain if the alias dies due to a signal. > This is usually a good thing, as signal deaths are > unexpected. However, SIGPIPE is not unexpected for many > commands which produce a lot of output; it is intended that > the user closing the pager would kill them them via SIGPIPE. s/them them/them/ > As a result, the user might see annoying messages in a > scenario like this: > > $ cat ~/.gitconfig > [alias] > lgbase = log --some-options > lg = !git lgbase --more-options > lg2 = !git lgbase --other-options > > $ git lg -p > [user hits 'q' to exit pager] > error: git lgbase --more-options died of signal 13 > fatal: While expanding alias 'lg': 'git lgbase --more-options': Success > > Many users won't see this, because we execute the external > command with the shell, and a POSIX shell will silently > rewrite the signal-death exit code into 128+signal, and we > will treat it like a normal exit code. However, this does > not always happen: > > 1. If the sub-command does not have shell metacharacters, > we will skip the shell and exec it directly, getting > the signal code. > > 2. Some shells do not do this rewriting; for example, > setting SHELL_PATH set to zsh will reveal this problem. > > This patch squelches the message, and lets git exit silently > (with the same exit code that a shell would use in case of a > signal). > > The first line of the message comes from run-command's > wait_or_whine. To silence that message, we remain quiet > anytime we see SIGPIPE. > > The second line comes from handle_alias itself. It calls > die_errno whenever run_command returns a negative value. > However, only -1 indicates a syscall error where errno has > something useful (note that it says the useless "success" > above). Instead, we treat negative returns from run_command > (except for -1) as a normal code to be passed to exit. > > Signed-off-by: Jeff King -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias
On Wed, Dec 23, 2015 at 4:37 PM, Jeff King wrote: > On Tue, Dec 22, 2015 at 10:13:03AM -0800, Junio C Hamano wrote: > >> > Another by the way, this "forcing aliases as external commands" now >> > shows something like "error: git-log died of signal 13" when the pager >> > exits early, for an alias like "l1 = log --oneline". >> >> ... and we do not show that when we directly call "git log" is...? >> >> We do signal this with non-zero exit status like so: >> >> $ GIT_PAGER=true git log --oneline ; echo $? >> 141 >> >> and it is not surprising that the one that is catching the exit >> status of what was spawned and reporting "signal 13". > > This sounded very familiar. Apparently I've been running with the patch > below for almost 3 years and never got around to re-examining it. > > The original discussion is in: > > http://thread.gmane.org/gmane.comp.version-control.git/212630 > > Having skimmed through the arguments there, and given that we applied > the patch from the middle of the thread, I think this is a good change. Yep. The new thing here is the "died of" message now also comes from execv_dashed_external() for non-external aliases, so it can show up a lot more often. I wasn't sure if excluding SIGPIPE in wait_and_whine was safe. But I _guess_ it is, based on your having no problem with the patch for a long time and the past discussion. > > -- >8 -- > Date: Fri, 4 Jan 2013 07:19:35 -0500 > Subject: [PATCH] avoid SIGPIPE warnings for aliases > > When git executes an alias that specifies an external > command, it will complain if the alias dies due to a signal. > This is usually a good thing, as signal deaths are > unexpected. However, SIGPIPE is not unexpected for many > commands which produce a lot of output; it is intended that > the user closing the pager would kill them them via SIGPIPE. > > As a result, the user might see annoying messages in a > scenario like this: > > $ cat ~/.gitconfig > [alias] > lgbase = log --some-options > lg = !git lgbase --more-options > lg2 = !git lgbase --other-options > > $ git lg -p > [user hits 'q' to exit pager] > error: git lgbase --more-options died of signal 13 > fatal: While expanding alias 'lg': 'git lgbase --more-options': Success > > Many users won't see this, because we execute the external > command with the shell, and a POSIX shell will silently > rewrite the signal-death exit code into 128+signal, and we > will treat it like a normal exit code. However, this does > not always happen: > > 1. If the sub-command does not have shell metacharacters, > we will skip the shell and exec it directly, getting > the signal code. > > 2. Some shells do not do this rewriting; for example, > setting SHELL_PATH set to zsh will reveal this problem. > > This patch squelches the message, and lets git exit silently > (with the same exit code that a shell would use in case of a > signal). > > The first line of the message comes from run-command's > wait_or_whine. To silence that message, we remain quiet > anytime we see SIGPIPE. > > The second line comes from handle_alias itself. It calls > die_errno whenever run_command returns a negative value. > However, only -1 indicates a syscall error where errno has > something useful (note that it says the useless "success" > above). Instead, we treat negative returns from run_command > (except for -1) as a normal code to be passed to exit. > > Signed-off-by: Jeff King > --- > git.c | 2 +- > run-command.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/git.c b/git.c > index 6ed824c..34a18a3 100644 > --- a/git.c > +++ b/git.c > @@ -252,7 +252,7 @@ static int handle_alias(int *argcp, const char ***argv) > alias_argv[argc] = NULL; > > ret = run_command_v_opt(alias_argv, RUN_USING_SHELL); > - if (ret >= 0) /* normal exit */ > + if (ret != -1) /* normal exit */ > exit(ret); > > die_errno("While expanding alias '%s': '%s'", > diff --git a/run-command.c b/run-command.c > index 13fa452..694a6ff 100644 > --- a/run-command.c > +++ b/run-command.c > @@ -245,7 +245,7 @@ static int wait_or_whine(pid_t pid, const char *argv0, > int in_signal) > error("waitpid is confused (%s)", argv0); > } else if (WIFSIGNALED(status)) { > code = WTERMSIG(status); > - if (code != SIGINT && code != SIGQUIT) > + if (code != SIGINT && code != SIGQUIT && code != SIGPIPE) > error("%s died of signal %d", argv0, code); > /* > * This return value is chosen so that code & 0xff > -- > 2.7.0.rc2.368.g1cbb535 > -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias
On Tue, Dec 22, 2015 at 10:13:03AM -0800, Junio C Hamano wrote: > > Another by the way, this "forcing aliases as external commands" now > > shows something like "error: git-log died of signal 13" when the pager > > exits early, for an alias like "l1 = log --oneline". > > ... and we do not show that when we directly call "git log" is...? > > We do signal this with non-zero exit status like so: > > $ GIT_PAGER=true git log --oneline ; echo $? > 141 > > and it is not surprising that the one that is catching the exit > status of what was spawned and reporting "signal 13". This sounded very familiar. Apparently I've been running with the patch below for almost 3 years and never got around to re-examining it. The original discussion is in: http://thread.gmane.org/gmane.comp.version-control.git/212630 Having skimmed through the arguments there, and given that we applied the patch from the middle of the thread, I think this is a good change. -- >8 -- Date: Fri, 4 Jan 2013 07:19:35 -0500 Subject: [PATCH] avoid SIGPIPE warnings for aliases When git executes an alias that specifies an external command, it will complain if the alias dies due to a signal. This is usually a good thing, as signal deaths are unexpected. However, SIGPIPE is not unexpected for many commands which produce a lot of output; it is intended that the user closing the pager would kill them them via SIGPIPE. As a result, the user might see annoying messages in a scenario like this: $ cat ~/.gitconfig [alias] lgbase = log --some-options lg = !git lgbase --more-options lg2 = !git lgbase --other-options $ git lg -p [user hits 'q' to exit pager] error: git lgbase --more-options died of signal 13 fatal: While expanding alias 'lg': 'git lgbase --more-options': Success Many users won't see this, because we execute the external command with the shell, and a POSIX shell will silently rewrite the signal-death exit code into 128+signal, and we will treat it like a normal exit code. However, this does not always happen: 1. If the sub-command does not have shell metacharacters, we will skip the shell and exec it directly, getting the signal code. 2. Some shells do not do this rewriting; for example, setting SHELL_PATH set to zsh will reveal this problem. This patch squelches the message, and lets git exit silently (with the same exit code that a shell would use in case of a signal). The first line of the message comes from run-command's wait_or_whine. To silence that message, we remain quiet anytime we see SIGPIPE. The second line comes from handle_alias itself. It calls die_errno whenever run_command returns a negative value. However, only -1 indicates a syscall error where errno has something useful (note that it says the useless "success" above). Instead, we treat negative returns from run_command (except for -1) as a normal code to be passed to exit. Signed-off-by: Jeff King --- git.c | 2 +- run-command.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/git.c b/git.c index 6ed824c..34a18a3 100644 --- a/git.c +++ b/git.c @@ -252,7 +252,7 @@ static int handle_alias(int *argcp, const char ***argv) alias_argv[argc] = NULL; ret = run_command_v_opt(alias_argv, RUN_USING_SHELL); - if (ret >= 0) /* normal exit */ + if (ret != -1) /* normal exit */ exit(ret); die_errno("While expanding alias '%s': '%s'", diff --git a/run-command.c b/run-command.c index 13fa452..694a6ff 100644 --- a/run-command.c +++ b/run-command.c @@ -245,7 +245,7 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal) error("waitpid is confused (%s)", argv0); } else if (WIFSIGNALED(status)) { code = WTERMSIG(status); - if (code != SIGINT && code != SIGQUIT) + if (code != SIGINT && code != SIGQUIT && code != SIGPIPE) error("%s died of signal %d", argv0, code); /* * This return value is chosen so that code & 0xff -- 2.7.0.rc2.368.g1cbb535 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias
Duy Nguyen writes: > On Tue, Dec 22, 2015 at 5:57 PM, Duy Nguyen wrote: >> On Tue, Dec 22, 2015 at 4:18 AM, Junio C Hamano wrote: >>> Thanks. I wiggled these three on top of the "Revert the earlier >>> one"; while I think the result is correct, I'd appreciate if you can >>> double check the result when I push the topic out later today. >> >> Looks good. "prove" passed too by the way. > > Another by the way, this "forcing aliases as external commands" now > shows something like "error: git-log died of signal 13" when the pager > exits early, for an alias like "l1 = log --oneline". ... and we do not show that when we directly call "git log" is...? We do signal this with non-zero exit status like so: $ GIT_PAGER=true git log --oneline ; echo $? 141 and it is not surprising that the one that is catching the exit status of what was spawned and reporting "signal 13". -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias
On Tue, Dec 22, 2015 at 5:57 PM, Duy Nguyen wrote: > On Tue, Dec 22, 2015 at 4:18 AM, Junio C Hamano wrote: >> Thanks. I wiggled these three on top of the "Revert the earlier >> one"; while I think the result is correct, I'd appreciate if you can >> double check the result when I push the topic out later today. > > Looks good. "prove" passed too by the way. Another by the way, this "forcing aliases as external commands" now shows something like "error: git-log died of signal 13" when the pager exits early, for an alias like "l1 = log --oneline". It's probably not a regression because other external git commands with pager should show the same message. But I haven't checked thoroughly yet. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias
On Tue, Dec 22, 2015 at 4:18 AM, Junio C Hamano wrote: > Thanks. I wiggled these three on top of the "Revert the earlier > one"; while I think the result is correct, I'd appreciate if you can > double check the result when I push the topic out later today. Looks good. "prove" passed too by the way. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias
Thanks. I wiggled these three on top of the "Revert the earlier one"; while I think the result is correct, I'd appreciate if you can double check the result when I push the topic out later today. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias
Changes since v1: - make sure we save/restore env for external commands in 2/3 - fix t0001 test in 3/3 Interdiff: diff --git b/git.c a/git.c index 83b6c56..da278c3 100644 --- b/git.c +++ a/git.c @@ -229,7 +229,6 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) static int handle_alias(int *argcp, const char ***argv) { int envchanged = 0, ret = 0, saved_errno = errno; - const char *subdir; int count, option_count; const char **new_argv; const char *alias_command; @@ -237,7 +236,7 @@ static int handle_alias(int *argcp, const char ***argv) int unused_nongit; save_env_before_alias(); - subdir = setup_git_directory_gently(&unused_nongit); + setup_git_directory_gently(&unused_nongit); alias_command = (*argv)[0]; alias_string = alias_lookup(alias_command); @@ -296,8 +295,7 @@ static int handle_alias(int *argcp, const char ***argv) ret = 1; } - if (subdir && chdir(subdir)) - die_errno("Cannot change to '%s'", subdir); + restore_env(0); errno = saved_errno; @@ -534,9 +532,13 @@ static void handle_builtin(int argc, const char **argv) builtin = get_builtin(cmd); if (builtin) { - if (saved_env_before_alias) - restore_env(0); - else + /* +* XXX: if we can figure out cases where it is _safe_ +* to do, we can avoid spawning a new process when +* saved_env_before_alias is true +* (i.e. setup_git_dir* has been run once) +*/ + if (!saved_env_before_alias) exit(run_builtin(builtin, argc, argv)); } } diff --git b/t/t0001-init.sh a/t/t0001-init.sh index 19539fc..295aa59 100755 --- b/t/t0001-init.sh +++ a/t/t0001-init.sh @@ -88,24 +88,14 @@ test_expect_success 'plain nested in bare through aliased command' ' ' test_expect_success 'No extra GIT_* on alias scripts' ' - cat <<-\EOF >expected && - GIT_ATTR_NOSYSTEM - GIT_AUTHOR_EMAIL - GIT_AUTHOR_NAME - GIT_COMMITTER_EMAIL - GIT_COMMITTER_NAME - GIT_CONFIG_NOSYSTEM - GIT_EXEC_PATH - GIT_MERGE_AUTOEDIT - GIT_MERGE_VERBOSITY - GIT_PREFIX - GIT_TEMPLATE_DIR - GIT_TEXTDOMAINDIR - GIT_TRACE_BARE - EOF + ( + env | sed -ne "/^GIT_/s/=.*//p" && + echo GIT_PREFIX & setup.c + echo GIT_TEXTDOMAINDIR# wrapper-for-bin.sh + ) | sort | uniq >expected && cat <<-\EOF >script && #!/bin/sh - env | grep GIT_ | sed "s/=.*//" | sort >actual + env | sed -ne "/^GIT_/s/=.*//p" | sort >actual exit 0 EOF chmod 755 script && Nguyễn Thái Ngọc Duy (3): git.c: make it clear save_env() is for alias handling only setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE when .. git.c: make sure we do not leak GIT_* to alias scripts environment.c| 2 -- git.c| 41 +++-- t/t0001-init.sh | 17 + t/t5601-clone.sh | 23 +++ 4 files changed, 63 insertions(+), 20 deletions(-) -- 2.3.0.rc1.137.g477eb31 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html