{"thread":{"id":"25704","subject":"Scripted clone generating an incomplete, unusable .git/config","startedAt":"2010-11-10T23:21:37Z","lastAt":"2010-11-12T05:18:04Z","messageCount":17,"participants":["Dun Peal","Stefan Naewe","Jonathan Nieder","Nguyen Thai Ngoc Duy","Andreas Schwab","Daniel Barkalow","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"155631","messageId":"AANLkTik7-QzrMKDpV=W4dqpuguZsAr5yrMELmHu5NZMd@mail.gmail.com","threadId":"25704","inReplyTo":null,"subject":"Scripted clone generating an incomplete, unusable .git/config","fromName":"Dun Peal","fromEmail":"dunpealer@gmail.com","sentAt":"2010-11-10T23:21:37Z","receivedAt":"2010-11-10T23:21:37Z","isPatch":false,"sender":{"key":"dunpealer@gmail.com","avatar":"https://gravatar.com/avatar/42f3e6a1166eb44e33f24c20ccbe809fa8591413f366dfe3755c8a8d4935ace9?d=mp&s=160"},"body":"This is a weird issue I ran into while scripting some Git operations\nwith git 1.7.2 on a Linux server.\n\nWhen running the git-clone command manually from the command line, the\nresulting repo/.git/config had all three required sections: core,\nremote (origin), branch (master).\n\nWhen running the exact same git-clone command manually from the Python\nscripted, the resulting repo/.git/config was missing the `core` and\n`remote` sections.\n\nHere's a bash log fully demonstrating the issue:\n\n  $ python -c \"import os; os.popen('git clone\ngit@git.domain.com:repos/repo.git')\"\n  [...]\n  $ cat repo/.git/config\n  [branch \"master\"]\n          remote = origin\n          merge = refs/heads/master\n  $ rm -Rf repo\n  $ git clone git@git.domain.com:repos/repo.git\n  $ cat repo/.git/config\n  [core]\n          repositoryformatversion = 0\n          filemode = true\n          bare = false\n          logallrefupdates = true\n  [remote \"origin\"]\n          fetch = +refs/heads/*:refs/remotes/origin/*\n          url = git@git.domain.com:repo/repo.git\n  [branch \"master\"]\n          remote = origin\n          merge = refs/heads/master\n\nWhat's causing this?  Is it a bug?\n\nThanks, D\n"},{"id":"155651","messageId":"4CDBA15A.6090301@atlas-elektronik.com","threadId":"25704","inReplyTo":"AANLkTik7-QzrMKDpV=W4dqpuguZsAr5yrMELmHu5NZMd@mail.gmail.com","subject":"Re: Scripted clone generating an incomplete, unusable .git/config","fromName":"Stefan Naewe","fromEmail":"stefan.naewe@atlas-elektronik.com","sentAt":"2010-11-11T07:55:06Z","receivedAt":"2010-11-11T07:55:06Z","isPatch":false,"sender":{"key":"stefan.naewe@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4468?v=4"},"body":"On 11/11/2010 12:21 AM, Dun Peal wrote:\n> This is a weird issue I ran into while scripting some Git operations\n> with git 1.7.2 on a Linux server.\n> \n> When running the git-clone command manually from the command line, the\n> resulting repo/.git/config had all three required sections: core,\n> remote (origin), branch (master).\n> \n> When running the exact same git-clone command manually from the Python\n> scripted, the resulting repo/.git/config was missing the `core` and\n> `remote` sections.\n> \n> Here's a bash log fully demonstrating the issue:\n> \n>   $ python -c \"import os; os.popen('git clone\n> git@git.domain.com:repos/repo.git')\"\n>   [...]\n>   $ cat repo/.git/config\n>   [branch \"master\"]\n>           remote = origin\n>           merge = refs/heads/master\n>   $ rm -Rf repo\n>   $ git clone git@git.domain.com:repos/repo.git\n>   $ cat repo/.git/config\n>   [core]\n>           repositoryformatversion = 0\n>           filemode = true\n>           bare = false\n>           logallrefupdates = true\n>   [remote \"origin\"]\n>           fetch = +refs/heads/*:refs/remotes/origin/*\n>           url = git@git.domain.com:repo/repo.git\n>   [branch \"master\"]\n>           remote = origin\n>           merge = refs/heads/master\n> \n> What's causing this?  Is it a bug?\n\nSame for me with git version 1.7.3.2 on Debian Etch.\nSeems to be a problem with the popen() returning too early or\nthe interpreter dying too early.\n\nThis works though:\n\n$ python -c \"import subprocess; subprocess.call(['git', 'clone', 'git://host/repoo.git'])\"\n\nRegards,\n  Stefan\n-- \n----------------------------------------------------------------\n/dev/random says: I is knot dain bramaged!\n"},{"id":"155652","messageId":"4CDBA2AB.1020207@atlas-elektronik.com","threadId":"25704","inReplyTo":"4CDBA15A.6090301@atlas-elektronik.com","subject":"Re: Scripted clone generating an incomplete, unusable .git/config","fromName":"Stefan Naewe","fromEmail":"stefan.naewe@atlas-elektronik.com","sentAt":"2010-11-11T08:00:43Z","receivedAt":"2010-11-11T08:00:43Z","isPatch":false,"sender":{"key":"stefan.naewe@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4468?v=4"},"body":"On 11/11/2010 8:55 AM, Stefan Naewe wrote:\n> On 11/11/2010 12:21 AM, Dun Peal wrote:\n>>\n>> Here's a bash log fully demonstrating the issue:\n>>\n>>   $ python -c \"import os; os.popen('git clone\n>> git@git.domain.com:repos/repo.git')\"\n>>   [...]\n>> What's causing this?  Is it a bug?\n> \n> Same for me with git version 1.7.3.2 on Debian Etch.\n\nMake that 'Debian Lenny (5.0.6)' FWIW...\n\n> Seems to be a problem with the popen() returning too early or\n> the interpreter dying too early.\n\nI forgot to say:\n\n...because it works if I do it interactivley in the python shell.\n \n> This works though:\n> \n> $ python -c \"import subprocess; subprocess.call(['git', 'clone', 'git://host/repoo.git'])\"\n \nRegards,\n  Stefan\n-- \n----------------------------------------------------------------\n/dev/random says: I is knot dain bramaged!\n"},{"id":"155659","messageId":"20101111103742.GA16422@burratino","threadId":"25704","inReplyTo":"AANLkTik7-QzrMKDpV=W4dqpuguZsAr5yrMELmHu5NZMd@mail.gmail.com","subject":"Re: Scripted clone generating an incomplete, unusable .git/config","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-11T10:37:42Z","receivedAt":"2010-11-11T10:37:42Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nDun Peal wrote:\n\n>   $ python -c \"import os; os.popen('git clone git@git.domain.com:repos/repo.git')\"\n>   [...]\n>   $ cat repo/.git/config\n>   [branch \"master\"]\n>           remote = origin\n>           merge = refs/heads/master\n\nIt looks like you've probably figured it out already, but for\ncompleteness:\n\nMost likely the clone is terminating when Python exits, perhaps due to\nSIGPIPE.  It doesn't look like a bug to me; I suspect you meant to use\nos.system(), which is synchronous, instead.  You might be able to get\na better sense of what happened with GIT_TRACE=1 or strace.\n\nHope that helps,\nJonathan\n"},{"id":"155668","messageId":"AANLkTinzotA4TSjMjjmW--gw7ST3dXMyHzPveGynaVmZ@mail.gmail.com","threadId":"25704","inReplyTo":"20101111103742.GA16422@burratino","subject":"Re: Scripted clone generating an incomplete, unusable .git/config","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-11-11T12:16:27Z","receivedAt":"2010-11-11T12:16:27Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Nov 11, 2010 at 5:37 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Hi,\n>\n> Dun Peal wrote:\n>\n>>   $ python -c \"import os; os.popen('git clone git@git.domain.com:repos/repo.git')\"\n>>   [...]\n>>   $ cat repo/.git/config\n>>   [branch \"master\"]\n>>           remote = origin\n>>           merge = refs/heads/master\n>\n> It looks like you've probably figured it out already, but for\n> completeness:\n>\n> Most likely the clone is terminating when Python exits, perhaps due to\n> SIGPIPE.  It doesn't look like a bug to me; I suspect you meant to use\n> os.system(), which is synchronous, instead.  You might be able to get\n> a better sense of what happened with GIT_TRACE=1 or strace.\n\nIf \"git clone\" is terminated before it completes, shouldn't it clean\nthe uncompleted repo?\n-- \nDuy\n"},{"id":"155710","messageId":"20101111173253.GC16972@burratino","threadId":"25704","inReplyTo":"AANLkTinzotA4TSjMjjmW--gw7ST3dXMyHzPveGynaVmZ@mail.gmail.com","subject":"Re: Scripted clone generating an incomplete, unusable .git/config","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-11T17:32:53Z","receivedAt":"2010-11-11T17:32:53Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"On Thu, Nov 11, 2010 at 07:16:27PM +0700, Nguyen Thai Ngoc Duy wrote:\n> On Thu, Nov 11, 2010 at 5:37 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n\n>> Most likely the clone is terminating when Python exits, perhaps due to\n>> SIGPIPE.  It doesn't look like a bug to me; I suspect you meant to use\n>> os.system(), which is synchronous, instead.\n[...]\n> If \"git clone\" is terminated before it completes, shouldn't it clean\n> the uncompleted repo?\n\nAh, so it should.\n\n trace: built-in: git clone jrn@localhost:/home/jrn/src/xz\n trace: run_command: ssh jrn@localhost git-upload-pack '/home/jrn/src/xz'\n trace: remove junk called\n jrn@localhosts password: \n trace: run_command: index-pack --stdin -v --fix-thin --keep=fetch-pack 19314 on burratino\n trace: exec: git index-pack --stdin -v --fix-thin --keep=fetch-pack 19314 on burratino\n trace: built-in: git index-pack --stdin -v --fix-thin --keep=fetch-pack 19314 on burratino\n remote: Counting objects: 7299, done.\n remote: Compressing objects: 100% (1826/1826), done.\n remote: Total 7299 (delta 5421), reused 7274 (delta 5401)\n Receiving objects: 100% (7299/7299), 2.36 MiB | 4.43 MiB/s, done.\n Resolving deltas: 100% (5421/5421), done.\n trace: exited with status 0\n trace: exited with status 0\n trace: remove junk called\n trace: remove_junk: pid != 0\n\nAre there any downside to the following?\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 19ed640..af6b40a 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -667,6 +667,5 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tstrbuf_release(&branch_top);\n \tstrbuf_release(&key);\n \tstrbuf_release(&value);\n-\tjunk_pid = 0;\n \treturn err;\n }\n"},{"id":"155712","messageId":"m2eiarwyip.fsf@igel.home","threadId":"25704","inReplyTo":"20101111103742.GA16422@burratino","subject":"Re: Scripted clone generating an incomplete, unusable .git/config","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2010-11-11T17:39:10Z","receivedAt":"2010-11-11T17:39:10Z","isPatch":false,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Hi,\n>\n> Dun Peal wrote:\n>\n>>   $ python -c \"import os; os.popen('git clone git@git.domain.com:repos/repo.git')\"\n>>   [...]\n>>   $ cat repo/.git/config\n>>   [branch \"master\"]\n>>           remote = origin\n>>           merge = refs/heads/master\n>\n> It looks like you've probably figured it out already, but for\n> completeness:\n>\n> Most likely the clone is terminating when Python exits, perhaps due to\n> SIGPIPE.  It doesn't look like a bug to me; I suspect you meant to use\n> os.system(), which is synchronous, instead.  You might be able to get\n> a better sense of what happened with GIT_TRACE=1 or strace.\n\nI think it would be a bug in python not wait(2)ing for the subprocess\nduring the implicit close when exiting.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"155718","messageId":"alpine.LNX.2.00.1011111241360.14365@iabervon.org","threadId":"25704","inReplyTo":"20101111173253.GC16972@burratino","subject":"Re: Scripted clone generating an incomplete, unusable .git/config","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2010-11-11T17:55:48Z","receivedAt":"2010-11-11T17:55:48Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Thu, 11 Nov 2010, Jonathan Nieder wrote:\n\n> On Thu, Nov 11, 2010 at 07:16:27PM +0700, Nguyen Thai Ngoc Duy wrote:\n> > On Thu, Nov 11, 2010 at 5:37 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> \n> >> Most likely the clone is terminating when Python exits, perhaps due to\n> >> SIGPIPE.  It doesn't look like a bug to me; I suspect you meant to use\n> >> os.system(), which is synchronous, instead.\n> [...]\n> > If \"git clone\" is terminated before it completes, shouldn't it clean\n> > the uncompleted repo?\n> \n> Ah, so it should.\n> \n>  trace: built-in: git clone jrn@localhost:/home/jrn/src/xz\n>  trace: run_command: ssh jrn@localhost git-upload-pack '/home/jrn/src/xz'\n>  trace: remove junk called\n>  jrn@localhosts password: \n>  trace: run_command: index-pack --stdin -v --fix-thin --keep=fetch-pack 19314 on burratino\n>  trace: exec: git index-pack --stdin -v --fix-thin --keep=fetch-pack 19314 on burratino\n>  trace: built-in: git index-pack --stdin -v --fix-thin --keep=fetch-pack 19314 on burratino\n>  remote: Counting objects: 7299, done.\n>  remote: Compressing objects: 100% (1826/1826), done.\n>  remote: Total 7299 (delta 5421), reused 7274 (delta 5401)\n>  Receiving objects: 100% (7299/7299), 2.36 MiB | 4.43 MiB/s, done.\n>  Resolving deltas: 100% (5421/5421), done.\n>  trace: exited with status 0\n>  trace: exited with status 0\n>  trace: remove junk called\n>  trace: remove_junk: pid != 0\n> \n> Are there any downside to the following?\n> \n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n> ---\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 19ed640..af6b40a 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -667,6 +667,5 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \tstrbuf_release(&branch_top);\n>  \tstrbuf_release(&key);\n>  \tstrbuf_release(&value);\n> -\tjunk_pid = 0;\n>  \treturn err;\n>  }\n\nI believe that would cause it to remove the repository when it terminates, \nregardless of whether it completed or not. That line is after all of the \nclone's code. I'm a bit suspicious that it's not flushing some stdio \nbuffer and relying on the C library doing it on an orderly exit.\n\n\t-Daniel\n*This .sig left intentionally blank*"},{"id":"155726","messageId":"20101111184829.GG16972@burratino","threadId":"25704","inReplyTo":"alpine.LNX.2.00.1011111241360.14365@iabervon.org","subject":"Re: Scripted clone generating an incomplete, unusable .git/config","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-11T18:48:29Z","receivedAt":"2010-11-11T18:48:29Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"On Thu, Nov 11, 2010 at 12:55:48PM -0500, Daniel Barkalow wrote:\n> On Thu, 11 Nov 2010, Jonathan Nieder wrote:\n\n>> --- a/builtin/clone.c\n>> +++ b/builtin/clone.c\n>> @@ -667,6 +667,5 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>>  \tstrbuf_release(&branch_top);\n>>  \tstrbuf_release(&key);\n>>  \tstrbuf_release(&value);\n>> -\tjunk_pid = 0;\n>>  \treturn err;\n>>  }\n>\n> I believe that would cause it to remove the repository when it terminates, \n> regardless of whether it completed or not.\n\nAh, right, the second remove_junk() call is because of atexit().\n\nSo why does git clone keep running after the first remove_junk() call?\nIt seems that the signal is initially set up (by Python's popen()?) as\nSIG_IGN.  I guess \"git clone\" should explicitly override that to be\nSIG_DFL?\n\nHere's a proof of concept.  It is not very good because it overrides\nany previously set sigchain handlers (in case the \"git\" wrappers\nstart to use one) and because using SIG_DFL as a sigchain_fun feels\nlike violating an abstraction.\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 19ed640..2f21a91 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -458,6 +458,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t}\n \tjunk_git_dir = git_dir;\n \tatexit(remove_junk);\n+\tsigchain_push_common(SIG_DFL);\n \tsigchain_push_common(remove_junk_on_signal);\n \n \tsetenv(CONFIG_ENVIRONMENT, mkpath(\"%s/config\", git_dir), 1);\n"},{"id":"155730","messageId":"20101111190508.GA3038@sigill.intra.peff.net","threadId":"25704","inReplyTo":"20101111184829.GG16972@burratino","subject":"Re: Scripted clone generating an incomplete, unusable .git/config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-11-11T19:05:08Z","receivedAt":"2010-11-11T19:05:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 11, 2010 at 12:48:29PM -0600, Jonathan Nieder wrote:\n\n> So why does git clone keep running after the first remove_junk() call?\n> It seems that the signal is initially set up (by Python's popen()?) as\n> SIG_IGN.  I guess \"git clone\" should explicitly override that to be\n> SIG_DFL?\n\nI was tracing this earlier today, too, and got sidetracked. But I got to\nthe same confusing point: why doesn't it die after cleaning up? It looks\nlike we inherit SIG_IGN for SIGPIPE from the parent python process.\n\nI don't think it makes sense for git-clone to do this itself. If we are\ngoing to say \"SIGPIPE should default to SIG_DFL on startup\" then we\nshould do it as the very first thing in the git wrapper, not just for\ngit-clone. That gives each git program a known starting point of\nbehavior.\n\nBut I wonder if we should perhaps just be ignoring SIGPIPE in this\ninstance instead.  There isn't a real error here; we just ended up not\nbeing able to write some useless progress report to stdout. There's no\nreason to fail.\n\nNote that we probably don't want to ignore SIGPIPE for all of git; many\nof the output-producing programs rely on it for early termination when\nthe user closes the pager. But for clone, it makes sense.\n\n> Here's a proof of concept.  It is not very good because it overrides\n> any previously set sigchain handlers (in case the \"git\" wrappers\n> start to use one) and because using SIG_DFL as a sigchain_fun feels\n> like violating an abstraction.\n> [...]\n> +\tsigchain_push_common(SIG_DFL);\n\nI don't think your patch is the right solution, but FWIW, sigchain was\nexplicitly intended to be able to take SIG_DFL and SIG_IGN. Probably\nsigchain_fun should be removed and we should just use sighandler_t\nexplicitly (I think in initial versions they were not the same thing,\nand I realized the error of my ways, but the sigchain_fun typedef hung\naround anyway).\n\n-Peff\n"},{"id":"155754","messageId":"20101112021602.GA10765@burratino","threadId":"25704","inReplyTo":"20101111190508.GA3038@sigill.intra.peff.net","subject":"Re: Scripted clone generating an incomplete, unusable .git/config","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-12T02:16:02Z","receivedAt":"2010-11-12T02:16:02Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> I don't think your patch is the right solution, but FWIW, sigchain was\n> explicitly intended to be able to take SIG_DFL and SIG_IGN. Probably\n> sigchain_fun should be removed and we should just use sighandler_t\n> explicitly\n\nSorry, that was lazy of me.  The name sighandler_t is a GNU extension[1].\n\nThe following addresses my confusion but I doubt it's worth the\nsyntactic ugliness.\n\n-- 8< --\nSubject: sigchain: hide sigchain_fun type\n\nSignal handlers that might be passed to signal() must be pointers to\nfunction with the prototype\n\n\tvoid handler(int signum);\n\nIn glibc this type is called sighandler_t; in the sigchain lib,\nsigchain_fun.\n\nThese really represent the same type in all respects: even special\nvalues like SIG_IGN and SIG_DFL are perfectly reasonable arguments\nfor a function accepting values of one of the two types.  Avoid\nconfusion by eliminating the sigchain_fun name from sigchain.h.\n\nIt would be nice to instead use sighandler_t everywhere, but\nunfortunately that name is a GNU extension.\n\nSuggested-by: Jeff King <peff@peff.net>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n [1] http://www.delorie.com/gnu/docs/glibc/libc_481.html\n\n sigchain.c |    2 ++\n sigchain.h |    6 ++----\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/sigchain.h b/sigchain.h\nindex 618083b..571d148 100644\n--- a/sigchain.h\n+++ b/sigchain.h\n@@ -1,11 +1,9 @@\n #ifndef SIGCHAIN_H\n #define SIGCHAIN_H\n \n-typedef void (*sigchain_fun)(int);\n-\n-int sigchain_push(int sig, sigchain_fun f);\n+int sigchain_push(int sig, void (*f)(int));\n int sigchain_pop(int sig);\n \n-void sigchain_push_common(sigchain_fun f);\n+void sigchain_push_common(void (*f)(int));\n \n #endif /* SIGCHAIN_H */\ndiff --git a/sigchain.c b/sigchain.c\nindex 1118b99..f837f61 100644\n--- a/sigchain.c\n+++ b/sigchain.c\n@@ -3,6 +3,8 @@\n \n #define SIGCHAIN_MAX_SIGNALS 32\n \n+typedef void (*sigchain_fun)(int);\n+\n struct sigchain_signal {\n \tsigchain_fun *old;\n \tint n;\n"},{"id":"155757","messageId":"20101112042455.GA20555@sigill.intra.peff.net","threadId":"25704","inReplyTo":"20101112021602.GA10765@burratino","subject":"Re: Scripted clone generating an incomplete, unusable .git/config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-11-12T04:24:56Z","receivedAt":"2010-11-12T04:24:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 11, 2010 at 08:16:02PM -0600, Jonathan Nieder wrote:\n\n> > I don't think your patch is the right solution, but FWIW, sigchain was\n> > explicitly intended to be able to take SIG_DFL and SIG_IGN. Probably\n> > sigchain_fun should be removed and we should just use sighandler_t\n> > explicitly\n> \n> Sorry, that was lazy of me.  The name sighandler_t is a GNU extension[1].\n\nAh, you're right. ANSI C defines signal() without using a typedef at\nall. Maybe that is why I didn't use it in the first place. I don't\nrecall.\n\n> The following addresses my confusion but I doubt it's worth the\n> syntactic ugliness.\n>\n> -- 8< --\n> Subject: sigchain: hide sigchain_fun type\n\nI think it makes the code uglier. Maybe this is a better solution:\n\n-- >8 --\nSubject: [PATCH] document sigchain api\n\nIt's pretty straightforward, but a stripped-down example\nnever hurts. And we should make clear that it is explicitly\nOK to use SIG_DFL and SIG_IGN.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/technical/api-sigchain.txt |   41 ++++++++++++++++++++++++++++++\n 1 files changed, 41 insertions(+), 0 deletions(-)\n create mode 100644 Documentation/technical/api-sigchain.txt\n\ndiff --git a/Documentation/technical/api-sigchain.txt b/Documentation/technical/api-sigchain.txt\nnew file mode 100644\nindex 0000000..535cdff\n--- /dev/null\n+++ b/Documentation/technical/api-sigchain.txt\n@@ -0,0 +1,41 @@\n+sigchain API\n+============\n+\n+Code often wants to set a signal handler to clean up temporary files or\n+other work-in-progress when we die unexpectedly. For multiple pieces of\n+code to do this without conflicting, each piece of code must remember\n+the old value of the handler and restore it either when:\n+\n+  1. The work-in-progress is finished, and the handler is no longer\n+     necessary. The handler should revert to the original behavior\n+     (either another handler, SIG_DFL, or SIG_IGN).\n+\n+  2. The signal is received. We should then do our cleanup, then chain\n+     to the next handler (or die if it is SIG_DFL).\n+\n+Sigchain is a tiny library for keeping a stack of handlers. Your handler\n+and installation code should look something like:\n+\n+------------------------------------------\n+  void clean_foo_on_signal(int sig)\n+  {\n+\t  clean_foo();\n+\t  sigchain_pop(sig);\n+\t  raise(sig);\n+  }\n+\n+  void other_func()\n+  {\n+\t  sigchain_push_common(clean_foo_on_signal);\n+\t  mess_up_foo();\n+\t  clean_foo();\n+  }\n+------------------------------------------\n+\n+Handlers are given the typdef of sigchain_fun. This is the same type\n+that is given to signal() or sigaction(). It is perfectly reasonable to\n+push SIG_DFL or SIG_IGN onto the stack.\n+\n+You can sigchain_push and sigchain_pop individual signals. For\n+convenience, sigchain_push_common will push the handler onto the stack\n+for many common signals.\n-- \n1.7.3.2.362.g0e229.dirty\n"},{"id":"155758","messageId":"20101112043229.GB10765@burratino","threadId":"25704","inReplyTo":"20101111190508.GA3038@sigill.intra.peff.net","subject":"Re: Scripted clone generating an incomplete, unusable .git/config","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-12T04:32:30Z","receivedAt":"2010-11-12T04:32:30Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> I don't think it makes sense for git-clone to do this itself. If we are\n> going to say \"SIGPIPE should default to SIG_DFL on startup\" then we\n> should do it as the very first thing in the git wrapper, not just for\n> git-clone. That gives each git program a known starting point of\n> behavior.\n\nHere's what that might look like.\n\n-- 8< --\nSubject: SIGPIPE and other fatal signals should default to SIG_DFL\n\nThe intuitive behavior when a git command receives a fatal\nsignal is for it to die.\n\nIndeed, that is the default handling.  However, POSIX semantics\nallow the parent of a process to override that default by\nignoring a signal, since ignored signals are preserved by fork() and\nexec().  For example, Python 2.6 and 2.7's os.popen() runs a shell\nwith SIGPIPE ignored (Python issue 1736483).\n\nWorst of all, in such a situation, many git commands use\nsigchain_push_common() to run cleanup actions (removing temporary\nfiles, discarding locks, that kind of thing) before passing control to\nthe original handler; if that handler ignores the signal rather than\nexiting, then execution will continue in an unplanned-for and unusual\nstate.\n\nSo give the signals in question default handling at the start of\nmain(), before sigchain_push_common can be called.\n\nNEEDSWORK:\n\n - imposes an obnoxious maintenance burden.  New non-builtins\n   that might call sigchain_push_common() would need to have\n   default handling restored at the start of main().\n\n - needs tests, especially for interaction with \"git daemon\"\n   client shutdown\n\nSuggested-by: Jeff King <peff@peff.net>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nCurrently git daemon uses SIG_IGN state on SIGTERM to protect\nchildren with active connections.  Why isn't that causing the same\nsort of problems as os.popen() causes?\n\n check-racy.c   |    1 +\n daemon.c       |    1 +\n fast-import.c  |    1 +\n git.c          |    2 ++\n http-backend.c |    1 +\n http-fetch.c   |    1 +\n http-push.c    |    1 +\n imap-send.c    |    1 +\n remote-curl.c  |    1 +\n shell.c        |    2 ++\n show-index.c   |    2 ++\n upload-pack.c  |    1 +\n 12 files changed, 15 insertions(+), 0 deletions(-)\n\ndiff --git a/check-racy.c b/check-racy.c\nindex 00d92a1..f1ad9d5 100644\n--- a/check-racy.c\n+++ b/check-racy.c\n@@ -6,6 +6,7 @@ int main(int ac, char **av)\n \tint dirty, clean, racy;\n \n \tdirty = clean = racy = 0;\n+\tsigchain_push_common(SIG_DFL);\n \tread_cache();\n \tfor (i = 0; i < active_nr; i++) {\n \t\tstruct cache_entry *ce = active_cache[i];\ndiff --git a/daemon.c b/daemon.c\nindex 7ccd097..7719f33 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -1001,6 +1001,7 @@ int main(int argc, char **argv)\n \tgid_t gid = 0;\n \tint i;\n \n+\tsigchain_push_common(SIG_DFL);\n \tgit_extract_argv0_path(argv[0]);\n \n \tfor (i = 1; i < argc; i++) {\ndiff --git a/fast-import.c b/fast-import.c\nindex dc48245..ce28759 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -3027,6 +3027,7 @@ int main(int argc, const char **argv)\n {\n \tunsigned int i;\n \n+\tsigchain_push_common(SIG_DFL);\n \tgit_extract_argv0_path(argv[0]);\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\ndiff --git a/git.c b/git.c\nindex e08d0f1..a8677fb 100644\n--- a/git.c\n+++ b/git.c\n@@ -502,6 +502,8 @@ int main(int argc, const char **argv)\n \tif (!cmd)\n \t\tcmd = \"git-help\";\n \n+\tsigchain_push_common(SIG_DFL);\n+\n \t/*\n \t * \"git-xxxx\" is the same as \"git xxxx\", but we obviously:\n \t *\ndiff --git a/http-backend.c b/http-backend.c\nindex 14c90c2..b03455a 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -550,6 +550,7 @@ int main(int argc, char **argv)\n \tchar *cmd_arg = NULL;\n \tint i;\n \n+\tsigchain_push_common(SIG_DFL);\n \tgit_extract_argv0_path(argv[0]);\n \tset_die_routine(die_webcgi);\n \ndiff --git a/http-fetch.c b/http-fetch.c\nindex 762c750..aca4e8f 100644\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -24,6 +24,7 @@ int main(int argc, const char **argv)\n \tint get_verbosely = 0;\n \tint get_recover = 0;\n \n+\tsigchain_push_common(SIG_DFL);\n \tgit_extract_argv0_path(argv[0]);\n \n \twhile (arg < argc && argv[arg][0] == '-') {\ndiff --git a/http-push.c b/http-push.c\nindex c9bcd11..a9d0894 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1791,6 +1791,7 @@ int main(int argc, char **argv)\n \tstruct remote *remote;\n \tchar *rewritten_url = NULL;\n \n+\tsigchain_push_common(SIG_DFL);\n \tgit_extract_argv0_path(argv[0]);\n \n \trepo = xcalloc(sizeof(*repo), 1);\ndiff --git a/imap-send.c b/imap-send.c\nindex 71506a8..eb13adc 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1538,6 +1538,7 @@ int main(int argc, char **argv)\n \tint nongit_ok;\n \n+\tsigchain_push_common(SIG_DFL);\n \tgit_extract_argv0_path(argv[0]);\n \n \tif (argc != 1)\n \t\tusage(imap_send_usage);\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 04d4813..804492d 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -787,6 +787,7 @@ int main(int argc, const char **argv)\n \tstruct strbuf buf = STRBUF_INIT;\n \tint nongit;\n \n+\tsigchain_push_common(SIG_DFL);\n \tgit_extract_argv0_path(argv[0]);\n \tsetup_git_directory_gently(&nongit);\n \tif (argc < 2) {\ndiff --git a/shell.c b/shell.c\nindex dea4cfd..9798b99 100644\n--- a/shell.c\n+++ b/shell.c\n@@ -137,6 +137,8 @@ int main(int argc, char **argv)\n \tint devnull_fd;\n \tint count;\n \n+\tsigchain_push_common(SIG_DFL);\n+\n \t/*\n \t * Always open file descriptors 0/1/2 to avoid clobbering files\n \t * in die().  It also avoids not messing up when the pipes are\ndiff --git a/show-index.c b/show-index.c\nindex 4c0ac13..7d48c88 100644\n--- a/show-index.c\n+++ b/show-index.c\n@@ -11,6 +11,8 @@ int main(int argc, char **argv)\n \tunsigned int version;\n \tstatic unsigned int top_index[256];\n \n+\tsigchain_push_common(SIG_DFL);\n+\n \tif (argc != 1)\n \t\tusage(show_index_usage);\n \tif (fread(top_index, 2 * 4, 1, stdin) != 1)\ndiff --git a/upload-pack.c b/upload-pack.c\nindex f05e422..4c764f7 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -682,6 +682,7 @@ int main(int argc, char **argv)\n \tint i;\n \tint strict = 0;\n \n+\tsigchain_push_common(SIG_DFL);\n \tgit_extract_argv0_path(argv[0]);\n \tread_replace_refs = 0;\n \n"},{"id":"155759","messageId":"20101112043500.GC10765@burratino","threadId":"25704","inReplyTo":"20101112042455.GA20555@sigill.intra.peff.net","subject":"Re: Scripted clone generating an incomplete, unusable .git/config","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-12T04:35:00Z","receivedAt":"2010-11-12T04:35:00Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Subject: [PATCH] document sigchain api\n> \n> It's pretty straightforward, but a stripped-down example\n> never hurts. And we should make clear that it is explicitly\n> OK to use SIG_DFL and SIG_IGN.\n\nNice, thanks.\n"},{"id":"155760","messageId":"20101112044137.GA24915@sigill.intra.peff.net","threadId":"25704","inReplyTo":"20101112043229.GB10765@burratino","subject":"Re: Scripted clone generating an incomplete, unusable .git/config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-11-12T04:41:38Z","receivedAt":"2010-11-12T04:41:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 11, 2010 at 10:32:30PM -0600, Jonathan Nieder wrote:\n\n> Subject: SIGPIPE and other fatal signals should default to SIG_DFL\n> \n> The intuitive behavior when a git command receives a fatal\n> signal is for it to die.\n> \n> Indeed, that is the default handling.  However, POSIX semantics\n> allow the parent of a process to override that default by\n> ignoring a signal, since ignored signals are preserved by fork() and\n> exec().  For example, Python 2.6 and 2.7's os.popen() runs a shell\n> with SIGPIPE ignored (Python issue 1736483).\n>\n> [...]\n>\n>  check-racy.c   |    1 +\n>  daemon.c       |    1 +\n>  fast-import.c  |    1 +\n>  git.c          |    2 ++\n>  http-backend.c |    1 +\n>  http-fetch.c   |    1 +\n>  http-push.c    |    1 +\n>  imap-send.c    |    1 +\n>  remote-curl.c  |    1 +\n>  shell.c        |    2 ++\n>  show-index.c   |    2 ++\n>  upload-pack.c  |    1 +\n>  12 files changed, 15 insertions(+), 0 deletions(-)\n\nDo we need to have it in every command? Is calling git-foo deprecated\nenough that we can just put it in git.c?\n\nI guess there are still a few commands that get installed explicitly in\n.../bin (upload-pack, for example). They would need a separate one.\nPerhaps it doesn't hurt to just put it in all of the non-builtins as you\ndid. It's not that big a maintenance issue.\n\n-Peff\n"},{"id":"155761","messageId":"20101112051234.GD10765@burratino","threadId":"25704","inReplyTo":"20101112043229.GB10765@burratino","subject":"[RFC/PATCH] daemon, tag, verify-tag: do not pass ignored signals to child (Re: Scripted clone generating an incomplete, unusable .git/config)","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-12T05:12:34Z","receivedAt":"2010-11-12T05:12:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> Currently git daemon uses SIG_IGN state on SIGTERM to protect\n> children with active connections.  Why isn't that causing the same\n> sort of problems as os.popen() causes?\n\nIt's late so please do not trust me, but I think the following would\nfix that.\n\n-- 8< --\nSubject: daemon, tag, verify-tag: do not pass ignored signals to child\n\nIt is bad practice to have signals ignored or blocked while creating a\nchild process, since to do so triggers not-so-well-tested code paths\nin many programs.\n\ntag and verify-tag block SIGPIPE to avoid termination from writing\nafter gpg fails and closes its pipe early.  Ignoring SIGPIPE in the\nchild is an unintended side-effect; avoid it by narrowing the scope\nof the request to ignore SIGPIPE to encompass only the write() (and\nin particular, not the fork()).\n\nConnection handling threads in daemon block SIGTERM to avoid\ntermination of active connections when the number of connections gets\ntoo high.  Use a signal handling function instead of SIG_IGN to\navoid passing the ignored signal to the child.  Ignoring SIGTERM in\nthe request-handling child is not necessary, since kill_some_child()\nnever tries to kill those.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nNeeds tests.\n\n builtin/tag.c        |   11 +++++++----\n builtin/verify-tag.c |   10 +++++++---\n daemon.c             |    6 +++++-\n 3 files changed, 19 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex d311491..efc9b93 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -173,10 +173,6 @@ static int do_sign(struct strbuf *buffer)\n \t\t\tbracket[1] = '\\0';\n \t}\n \n-\t/* When the username signingkey is bad, program could be terminated\n-\t * because gpg exits without reading and then write gets SIGPIPE. */\n-\tsignal(SIGPIPE, SIG_IGN);\n-\n \tmemset(&gpg, 0, sizeof(gpg));\n \tgpg.argv = args;\n \tgpg.in = -1;\n@@ -189,9 +185,14 @@ static int do_sign(struct strbuf *buffer)\n \tif (start_command(&gpg))\n \t\treturn error(\"could not run gpg.\");\n \n+\t/* When the username signingkey is bad, program could be terminated\n+\t * because gpg exits without reading and then write gets SIGPIPE. */\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n+\n \tif (write_in_full(gpg.in, buffer->buf, buffer->len) != buffer->len) {\n \t\tclose(gpg.in);\n \t\tclose(gpg.out);\n+\t\tsigchain_pop(SIGPIPE);\n \t\tfinish_command(&gpg);\n \t\treturn error(\"gpg did not accept the tag data\");\n \t}\n@@ -199,6 +200,8 @@ static int do_sign(struct strbuf *buffer)\n \tlen = strbuf_read(buffer, gpg.out, 1024);\n \tclose(gpg.out);\n \n+\tsigchain_pop(SIGPIPE);\n+\n \tif (finish_command(&gpg) || !len || len < 0)\n \t\treturn error(\"gpg failed to sign the tag\");\n \ndiff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\nindex 9f482c2..5361017 100644\n--- a/builtin/verify-tag.c\n+++ b/builtin/verify-tag.c\n@@ -54,8 +54,15 @@ static int run_gpg_verify(const char *buf, unsigned long size, int verbose)\n \t\treturn error(\"could not run gpg.\");\n \t}\n \n+\t/* sometimes the program was terminated because this signal\n+\t * was received in the process of writing the gpg input: */\n+\tsigchain_push(SIGPIPE, ignore_signal);\n+\n \twrite_in_full(gpg.in, buf, len);\n \tclose(gpg.in);\n+\n+\tsigchain_pop(SIGPIPE);\n+\n \tret = finish_command(&gpg);\n \n \tunlink_or_warn(path);\n@@ -104,9 +111,6 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n \tif (argc <= i)\n \t\tusage_with_options(verify_tag_usage, verify_tag_options);\n \n-\t/* sometimes the program was terminated because this signal\n-\t * was received in the process of writing the gpg input: */\n-\tsignal(SIGPIPE, SIG_IGN);\n \twhile (i < argc)\n \t\tif (verify_tag(argv[i++], verbose))\n \t\t\thad_error = 1;\ndiff --git a/daemon.c b/daemon.c\nindex 7719f33..ccc560b 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -243,6 +243,10 @@ static int git_daemon_config(const char *var, const char *value, void *cb)\n \treturn 0;\n }\n \n+static void ignore_termination_signal(int sig)\n+{\n+}\n+\n static int run_service(char *dir, struct daemon_service *service)\n {\n \tconst char *path;\n@@ -294,7 +298,7 @@ static int run_service(char *dir, struct daemon_service *service)\n \t * We'll ignore SIGTERM from now on, we have a\n \t * good client.\n \t */\n-\tsignal(SIGTERM, SIG_IGN);\n+\tsignal(SIGTERM, ignore_termination_signal);\n \n \treturn service->fn();\n }\n-- \n1.7.2.3.557.gab647.dirty\n"},{"id":"155762","messageId":"20101112051804.GE10765@burratino","threadId":"25704","inReplyTo":"20101112044137.GA24915@sigill.intra.peff.net","subject":"Re: Scripted clone generating an incomplete, unusable .git/config","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-12T05:18:04Z","receivedAt":"2010-11-12T05:18:04Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Thu, Nov 11, 2010 at 10:32:30PM -0600, Jonathan Nieder wrote:\n\n>> Subject: SIGPIPE and other fatal signals should default to SIG_DFL\n>> \n>> The intuitive behavior when a git command receives a fatal\n>> signal is for it to die.\n[...]\n> Do we need to have it in every command? Is calling git-foo deprecated\n> enough that we can just put it in git.c?\n> \n> I guess there are still a few commands that get installed explicitly in\n> .../bin (upload-pack, for example). They would need a separate one.\n> Perhaps it doesn't hurt to just put it in all of the non-builtins as you\n> did. It's not that big a maintenance issue.\n\nOkay.  Here's a hunk I forgot to add.  The next challenge is how to\ntest this mess. :)\n\ndiff --git a/cache.h b/cache.h\nindex d85ce86..8088e26 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -5,6 +5,7 @@\n #include \"strbuf.h\"\n #include \"hash.h\"\n #include \"advice.h\"\n+#include \"sigchain.h\"\n \n #include SHA1_HEADER\n #ifndef git_SHA_CTX\n"}]}