{"thread":{"id":"42195","subject":"[PATCH] t5510: run auto-gc in the foreground","startedAt":"2016-05-01T15:37:43Z","lastAt":"2016-05-07T14:44:51Z","messageCount":9,"participants":["SZEDER Gábor","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"285116","messageId":"20160501153743.323-1-szeder@ira.uka.de","threadId":"42195","inReplyTo":null,"subject":"[PATCH] t5510: run auto-gc in the foreground","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-05-01T15:37:43Z","receivedAt":"2016-05-01T15:37:43Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The last test added to 't5510-fetch' in 0898c9628104 (fetch: release\npack files before garbage-collecting, 2016-01-13) may sporadically\ntrigger following error message from the test harness:\n\n  rm: cannot remove 'trash directory.t5510-fetch/auto-gc/.git': Directory not empty\n\nThe test in question forces an auto-gc, which, if the system supports\nit, runs in the background by default, and occasionally takes long\nenough for the test to finish and for 'test_done' to start\nhousekeeping.  This can lead to the test's 'git gc --auto' in the\nbackground and 'test_done's 'rm -rf $trash' in the foreground racing\neach other to create and delete files and directories.  It might just\nhappen that 'git gc' re-creates a directory that 'rm -rf' already\nvisited and removed, which ultimately triggers the above error.\n\nDisable detaching the auto-gc process to ensure that it finishes\nbefore the test can continue, thus avoiding this racy situation.\n\nSigned-off-by: SZEDER Gábor <szeder@ira.uka.de>\n---\n t/t5510-fetch.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 38321d19efbe..454d896390c0 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -682,6 +682,7 @@ test_expect_success 'fetching with auto-gc does not lock up' '\n \t(\n \t\tcd auto-gc &&\n \t\tgit config gc.autoPackLimit 1 &&\n+\t\tgit config gc.autoDetach false &&\n \t\tGIT_ASK_YESNO=\"$D/askyesno\" git fetch >fetch.out 2>&1 &&\n \t\t! grep \"Should I try again\" fetch.out\n \t)\n-- \n2.8.2.356.ge684b1d\n"},{"id":"285143","messageId":"alpine.DEB.2.20.1605020859131.9313@virtualbox","threadId":"42195","inReplyTo":"20160501153743.323-1-szeder@ira.uka.de","subject":"Re: [PATCH] t5510: run auto-gc in the foreground","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-02T07:01:08Z","receivedAt":"2016-05-02T07:01:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Gábor,\n\nOn Sun, 1 May 2016, SZEDER Gábor wrote:\n\n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index 38321d19efbe..454d896390c0 100755\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -682,6 +682,7 @@ test_expect_success 'fetching with auto-gc does not lock up' '\n>  \t(\n>  \t\tcd auto-gc &&\n>  \t\tgit config gc.autoPackLimit 1 &&\n> +\t\tgit config gc.autoDetach false &&\n>  \t\tGIT_ASK_YESNO=\"$D/askyesno\" git fetch >fetch.out 2>&1 &&\n>  \t\t! grep \"Should I try again\" fetch.out\n>  \t)\n\nSounds good to me.\n\nAlternatively, we could consider passing `-c gc.autoDetach=false` instead,\nto limit the scope. I am not insisting on it, of course ;-)\n\nCiao,\nDscho"},{"id":"285229","messageId":"20160503015526.Horde.e0uZ0P4BqpNnwx_zmhu3WfE@webmail.informatik.kit.edu","threadId":"42195","inReplyTo":"alpine.DEB.2.20.1605020859131.9313@virtualbox","subject":"Re: [PATCH] t5510: run auto-gc in the foreground","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-05-02T23:55:26Z","receivedAt":"2016-05-02T23:55:26Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting Johannes Schindelin <Johannes.Schindelin@gmx.de>:\n\n> Hi Gábor,\n>\n> On Sun, 1 May 2016, SZEDER Gábor wrote:\n>\n>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n>> index 38321d19efbe..454d896390c0 100755\n>> --- a/t/t5510-fetch.sh\n>> +++ b/t/t5510-fetch.sh\n>> @@ -682,6 +682,7 @@ test_expect_success 'fetching with auto-gc does\n>> not lock up' '\n>> \t(\n>> \t\tcd auto-gc &&\n>> \t\tgit config gc.autoPackLimit 1 &&\n>> +\t\tgit config gc.autoDetach false &&\n>> \t\tGIT_ASK_YESNO=\"$D/askyesno\" git fetch >fetch.out 2>&1 &&\n>> \t\t! grep \"Should I try again\" fetch.out\n>> \t)\n>\n> Sounds good to me.\n\nThere is something still bothering me, though.\n\nI take this was a Windows-specific issue; deleting open files on Linux is\nno brainer.  According to a comment on the original Git for Windows issue\nat github[1], 'git gc' runs in the background by default on Windows as well.\n\nNow, it's 'git gc --auto' that's trying to delete pack and index files that\nbecame redundant after repacking, and when it can't delete those files,\nthen complains.  I.e. those \"Should I try again\" questions the test looks\nfor come from 'git gc', not from 'git fetch'.  So far so good.\n\nLet's assume that someone inconsiderately removes that closed_all_pack()\ncall added to cmd_fetch(), basically reverting the fix in 0898c9628104.\nThe test added in the same commit (i.e. not including my fix here) should\nstill be able to catch it, but I think it's possible that it sometimes\nremains unnoticed. Consider the following sequence:\n\n    1. 'git fetch' does its thing, including opening pack and index files.\n    2. 'git fetch' launches 'git gc --auto' which then goes into background.\n    3. The scheduler happens to decide that it's 'git fetch's turn again,\n       and it finishes, including closing all opened pack and index files.\n    4. 'git gc' gets a chance to proceed, repacks and then manages to delete\n       all redundant pack and index files successfully.\n    5. 'grep' doesn't find the string it looks for.\n    6. The test succeeds.\n\nAnd the background 'git gc --auto' doesn't even have to be delayed that\nlong, because 'git fetch' exits immediately after launching it.  That's\nconsiderably shorter than the delay necessary for the 'rm -rf' error I\ndescribed in the commit message, because for the latter 'git fetch' must\nfinish, 'grep' must run, and 'test_done' must write the test results and\nstart 'rm -rf $trash' while 'git gc' is still running in the background.\n\nSo, if I'm right, then my fix is not just about avoiding a sporadic error\nfrom the test harness, but it's also important for the test's\ncorrectness.  But am I right?  Alas I don't have a Git for Windows dev\nenvironment to play around with this.\n\n[1] - https://github.com/git-for-windows/git/issues/500#issuecomment-149933531\n\n\nGábor\n\n\n> Alternatively, we could consider passing `-c gc.autoDetach=false` instead,\n> to limit the scope. I am not insisting on it, of course ;-)\n>\n> Ciao,\n> Dscho\n"},{"id":"285274","messageId":"20160503135028.Horde.jeJdKT1kb2NTVAS1HpcsQh2@webmail.informatik.kit.edu","threadId":"42195","inReplyTo":"20160503015526.Horde.e0uZ0P4BqpNnwx_zmhu3WfE@webmail.informatik.kit.edu","subject":"Re: [PATCH] t5510: run auto-gc in the foreground","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-05-03T11:50:28Z","receivedAt":"2016-05-03T11:50:28Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting SZEDER Gábor <szeder@ira.uka.de>:\n\n> Quoting Johannes Schindelin <Johannes.Schindelin@gmx.de>:\n>\n>> Hi Gábor,\n>>\n>> On Sun, 1 May 2016, SZEDER Gábor wrote:\n>>\n>>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n>>> index 38321d19efbe..454d896390c0 100755\n>>> --- a/t/t5510-fetch.sh\n>>> +++ b/t/t5510-fetch.sh\n>>> @@ -682,6 +682,7 @@ test_expect_success 'fetching with auto-gc does\n>>> not lock up' '\n>>> \t(\n>>> \t\tcd auto-gc &&\n>>> \t\tgit config gc.autoPackLimit 1 &&\n>>> +\t\tgit config gc.autoDetach false &&\n>>> \t\tGIT_ASK_YESNO=\"$D/askyesno\" git fetch >fetch.out 2>&1 &&\n>>> \t\t! grep \"Should I try again\" fetch.out\n>>> \t)\n>>\n>> Sounds good to me.\n>\n> There is something still bothering me, though.\n>\n> I take this was a Windows-specific issue; deleting open files on Linux is\n> no brainer.  According to a comment on the original Git for Windows issue\n> at github[1], 'git gc' runs in the background by default on Windows as well.\n\nOk, having slept on it, it was a false alarm.\n\nThough 'git gc --auto' claims \"Auto packing the repository in background for\noptimum performance.\" on Windows, it does in fact runs in the foreground.\n\n'git gc --auto' first prints that message, unless gc.autoDetach is disabled,\nand then calls daemonize() to go to the background.  However, daemonize() is\nbasically a no-op on Windows, thus 'git gc' will remain in the foreground and\nthe sequence I described below is impossible.  Good.\n\nPerhaps it would be worth updating 'git gc' to not lie about going to the\nbackground when we can already know in advance that it won't.\n\n\n\n> Now, it's 'git gc --auto' that's trying to delete pack and index files that\n> became redundant after repacking, and when it can't delete those files,\n> then complains.  I.e. those \"Should I try again\" questions the test looks\n> for come from 'git gc', not from 'git fetch'.  So far so good.\n>\n> Let's assume that someone inconsiderately removes that closed_all_pack()\n> call added to cmd_fetch(), basically reverting the fix in 0898c9628104.\n> The test added in the same commit (i.e. not including my fix here) should\n> still be able to catch it, but I think it's possible that it sometimes\n> remains unnoticed. Consider the following sequence:\n>\n>    1. 'git fetch' does its thing, including opening pack and index files.\n>    2. 'git fetch' launches 'git gc --auto' which then goes into background.\n>    3. The scheduler happens to decide that it's 'git fetch's turn again,\n>       and it finishes, including closing all opened pack and index files.\n>    4. 'git gc' gets a chance to proceed, repacks and then manages to delete\n>       all redundant pack and index files successfully.\n>    5. 'grep' doesn't find the string it looks for.\n>    6. The test succeeds.\n>\n> And the background 'git gc --auto' doesn't even have to be delayed that\n> long, because 'git fetch' exits immediately after launching it.  That's\n> considerably shorter than the delay necessary for the 'rm -rf' error I\n> described in the commit message, because for the latter 'git fetch' must\n> finish, 'grep' must run, and 'test_done' must write the test results and\n> start 'rm -rf $trash' while 'git gc' is still running in the background.\n>\n> So, if I'm right, then my fix is not just about avoiding a sporadic error\n> from the test harness, but it's also important for the test's\n> correctness.  But am I right?  Alas I don't have a Git for Windows dev\n> environment to play around with this.\n>\n> [1] -  \n> https://github.com/git-for-windows/git/issues/500#issuecomment-149933531\n>\n>\n> Gábor\n>\n>\n>> Alternatively, we could consider passing `-c gc.autoDetach=false` instead,\n>> to limit the scope. I am not insisting on it, of course ;-)\n>>\n>> Ciao,\n>> Dscho\n"},{"id":"285415","messageId":"alpine.DEB.2.20.1605040741500.9313@virtualbox","threadId":"42195","inReplyTo":"20160503135028.Horde.jeJdKT1kb2NTVAS1HpcsQh2@webmail.informatik.kit.edu","subject":"Re: [PATCH] t5510: run auto-gc in the foreground","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-04T05:48:25Z","receivedAt":"2016-05-04T05:48:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Gábor,\n\nOn Tue, 3 May 2016, SZEDER Gábor wrote:\n\n> Quoting SZEDER Gábor <szeder@ira.uka.de>:\n> \n> >Quoting Johannes Schindelin <Johannes.Schindelin@gmx.de>:\n> >\n> > >Hi Gábor,\n> > >\n> > >On Sun, 1 May 2016, SZEDER Gábor wrote:\n> > >\n> > > >diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> > > >index 38321d19efbe..454d896390c0 100755\n> > > >--- a/t/t5510-fetch.sh\n> > > >+++ b/t/t5510-fetch.sh\n> > > >@@ -682,6 +682,7 @@ test_expect_success 'fetching with auto-gc does\n> > > >not lock up' '\n> > > > (\n> > > >  cd auto-gc &&\n> > > >  git config gc.autoPackLimit 1 &&\n> > > >+\t\tgit config gc.autoDetach false &&\n> > > >  GIT_ASK_YESNO=\"$D/askyesno\" git fetch >fetch.out 2>&1 &&\n> > > >  ! grep \"Should I try again\" fetch.out\n> > > > )\n> > >\n> > >Sounds good to me.\n> >\n> >There is something still bothering me, though.\n> >\n> >I take this was a Windows-specific issue; deleting open files on Linux is\n> >no brainer.  According to a comment on the original Git for Windows issue\n> >at github[1], 'git gc' runs in the background by default on Windows as well.\n> \n> Ok, having slept on it, it was a false alarm.\n> \n> Though 'git gc --auto' claims \"Auto packing the repository in background for\n> optimum performance.\" on Windows, it does in fact runs in the foreground.\n\nThanks for checking. I ran out of time yesterday.\n\n> 'git gc --auto' first prints that message, unless gc.autoDetach is disabled,\n> and then calls daemonize() to go to the background.  However, daemonize() is\n> basically a no-op on Windows, thus 'git gc' will remain in the foreground and\n> the sequence I described below is impossible.  Good.\n\nOh, right. I think this will take a lot of energy to fix: daemonize()'s\nfunctionality is not really available, indeed. What *is* available is a\nspawn() that detaches the new process.\n\n> Perhaps it would be worth updating 'git gc' to not lie about going to the\n> background when we can already know in advance that it won't.\n\nHmm.  https://github.com/git/git/blob/master/builtin/gc.c#L372-L373\nalready looks correct (should it really know that we cannot daemonize()?\nWhat about other code paths using that function?):\n\n\t\t\tif (detach_auto)\n\t\t\t\tfprintf(stderr, _(\"Auto packing the repository in background for optimum performance.\\n\"));\n\t\t\telse\n\t\t\t\tfprintf(stderr, _(\"Auto packing the repository for optimum performance.\\n\"));\n\nCiao,\nDscho"},{"id":"285571","messageId":"20160505171430.Horde.-GuvDpZBfS8VI1Zcfn4bJQI@webmail.informatik.kit.edu","threadId":"42195","inReplyTo":"alpine.DEB.2.20.1605040741500.9313@virtualbox","subject":"Re: [PATCH] t5510: run auto-gc in the foreground","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-05-05T15:14:30Z","receivedAt":"2016-05-05T15:14:30Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting Johannes Schindelin <Johannes.Schindelin@gmx.de>:\n\n> Hi Gábor,\n>\n> On Tue, 3 May 2016, SZEDER Gábor wrote:\n>\n>> Quoting SZEDER Gábor <szeder@ira.uka.de>:\n>>\n>> >Quoting Johannes Schindelin <Johannes.Schindelin@gmx.de>:\n>> >\n>> > >Hi Gábor,\n>> > >\n>> > >On Sun, 1 May 2016, SZEDER Gábor wrote:\n>> > >\n>> > > >diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n>> > > >index 38321d19efbe..454d896390c0 100755\n>> > > >--- a/t/t5510-fetch.sh\n>> > > >+++ b/t/t5510-fetch.sh\n>> > > >@@ -682,6 +682,7 @@ test_expect_success 'fetching with auto-gc does\n>> > > >not lock up' '\n>> > > > (\n>> > > >  cd auto-gc &&\n>> > > >  git config gc.autoPackLimit 1 &&\n>> > > >+\t\tgit config gc.autoDetach false &&\n>> > > >  GIT_ASK_YESNO=\"$D/askyesno\" git fetch >fetch.out 2>&1 &&\n>> > > >  ! grep \"Should I try again\" fetch.out\n>> > > > )\n>> > >\n>> > >Sounds good to me.\n>> >\n>> >There is something still bothering me, though.\n>> >\n>> >I take this was a Windows-specific issue; deleting open files on Linux is\n>> >no brainer.  According to a comment on the original Git for Windows issue\n>> >at github[1], 'git gc' runs in the background by default on  \n>> Windows as well.\n>>\n>> Ok, having slept on it, it was a false alarm.\n>>\n>> Though 'git gc --auto' claims \"Auto packing the repository in background for\n>> optimum performance.\" on Windows, it does in fact runs in the foreground.\n>\n> Thanks for checking. I ran out of time yesterday.\n>\n>> 'git gc --auto' first prints that message, unless gc.autoDetach is disabled,\n>> and then calls daemonize() to go to the background.  However, daemonize() is\n>> basically a no-op on Windows, thus 'git gc' will remain in the  \n>> foreground and\n>> the sequence I described below is impossible.  Good.\n>\n> Oh, right. I think this will take a lot of energy to fix: daemonize()'s\n> functionality is not really available, indeed. What *is* available is a\n> spawn() that detaches the new process.\n>\n>> Perhaps it would be worth updating 'git gc' to not lie about going to the\n>> background when we can already know in advance that it won't.\n>\n> Hmm.  https://github.com/git/git/blob/master/builtin/gc.c#L372-L373\n> already looks correct (should it really know that we cannot daemonize()?\n>\n> \t\t\tif (detach_auto)\n> \t\t\t\tfprintf(stderr, _(\"Auto packing the repository in background for  \n> optimum performance.\\n\"));\n> \t\t\telse\n> \t\t\t\tfprintf(stderr, _(\"Auto packing the repository for optimum  \n> performance.\\n\"));\n\nIt is only correct as far as the value of the 'gc.autoDetach' config\nvariable is concerned, which is enabled by default, even on Windows, where\nit can't go to the background.\n\n> (should it really know that we cannot daemonize()?\n> What about other code paths using that function?):\n\ndaemonize() is currently only used in the 'git gc --auto' code path and in\n'git daemon', which doesn't announce that it is about to go in the\nbackground.  Then there is the WIP index-helper, which will use daemonize()\nas well, but it won't announce that either.\n\nBeing the sole callsite that makes such an announcement, I think it should\nknow or at least should check whether \"daemonizing\" is possible. (as\nopposed to e.g. teaching daemonize() to print such messages)\n\n\nGábor\n"},{"id":"285572","messageId":"20160505151646.13189-1-szeder@ira.uka.de","threadId":"42195","inReplyTo":"20160505171430.Horde.-GuvDpZBfS8VI1Zcfn4bJQI@webmail.informatik.kit.edu","subject":"[RFC PATCH] gc --auto: don't lie about running in background on Windows","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-05-05T15:16:46Z","receivedAt":"2016-05-05T15:16:46Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When 'git gc --auto' is invoked it tells the user whether it is about\nto auto pack the repository in the background or in the foreground\ndepending on 'gc.autoDetach' (enabled by default).  However, going to\nthe background is not supported at all on Windows, yet 'git gc --auto'\nby default claims that it will do so.\n\nAdd a helper function that can tell whether the platform supports\ngoing to the background and use it in the 'git gc --auto' codepath to\nprint an honest message.\n\nSigned-off-by: SZEDER Gábor <szeder@ira.uka.de>\n---\n\nI built and tested with 'NO_POSIX_GOODIES=UnfortunatelyYes': it\nworked.  However, I did not test it on Windows due to lack of\nresources, hence the RFC out of caution.\n\nArguably this helper function could be just a simple variable.  I\nopted for a function because:\n\n  - I preferred a single '#ifdef NO_POSIX_GOODIES', and putting a\n    static variable so near to EOF felt just wrong.  (And this is why\n    it's not an inline-able function defined in a header file.)\n\n  - currently we know already at compile time that Windows can't\n    daemonize, but in the future we might want to extend this helper\n    function to perform some runtime checks, too.  But this is perhaps\n    like preparing for crossing a bridge where we'll never get to.\n\n builtin/gc.c |  1 +\n cache.h      |  1 +\n setup.c      | 17 +++++++++++++++--\n 3 files changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex c583aad6ec28..79a0f6dc1126 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -368,6 +368,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t */\n \t\tif (!need_to_gc())\n \t\t\treturn 0;\n+\t\tdetach_auto &= can_daemonize();\n \t\tif (!quiet) {\n \t\t\tif (detach_auto)\n \t\t\t\tfprintf(stderr, _(\"Auto packing the repository in background for optimum performance.\\n\"));\ndiff --git a/cache.h b/cache.h\nindex fd728f079320..3662e5aabb98 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -524,6 +524,7 @@ extern int set_git_dir_init(const char *git_dir, const char *real_git_dir, int);\n extern int init_db(const char *template_dir, unsigned int flags);\n \n extern void sanitize_stdfds(void);\n+extern int can_daemonize(void);\n extern int daemonize(void);\n \n #define alloc_nr(x) (((x)+16)*3/2)\ndiff --git a/setup.c b/setup.c\nindex c86bf5c9fabe..6187a4ad9c47 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1033,12 +1033,25 @@ void sanitize_stdfds(void)\n \t\tclose(fd);\n }\n \n+#ifdef NO_POSIX_GOODIES\n+int can_daemonize(void)\n+{\n+\treturn 0;\n+}\n+\n int daemonize(void)\n {\n-#ifdef NO_POSIX_GOODIES\n \terrno = ENOSYS;\n \treturn -1;\n+}\n #else\n+int can_daemonize(void)\n+{\n+\treturn 1;\n+}\n+\n+int daemonize(void)\n+{\n \tswitch (fork()) {\n \t\tcase 0:\n \t\t\tbreak;\n@@ -1054,5 +1067,5 @@ int daemonize(void)\n \tclose(2);\n \tsanitize_stdfds();\n \treturn 0;\n-#endif\n }\n+#endif /* #ifdef NO_POSIX_GOODIES */\n-- \n2.8.2.356.ge684b1d\n"},{"id":"285573","messageId":"xmqqbn4kcvuc.fsf@gitster.mtv.corp.google.com","threadId":"42195","inReplyTo":"20160505151646.13189-1-szeder@ira.uka.de","subject":"Re: [RFC PATCH] gc --auto: don't lie about running in background on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-05T16:28:59Z","receivedAt":"2016-05-05T16:28:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder@ira.uka.de> writes:\n\n> Arguably this helper function could be just a simple variable.  I\n> opted for a function because:\n>\n>   - I preferred a single '#ifdef NO_POSIX_GOODIES', and putting a\n>     static variable so near to EOF felt just wrong.  (And this is why\n>     it's not an inline-able function defined in a header file.)\n>\n>   - currently we know already at compile time that Windows can't\n>     daemonize, but in the future we might want to extend this helper\n>     function to perform some runtime checks, too.  But this is perhaps\n>     like preparing for crossing a bridge where we'll never get to.\n\nAlternatively, the implementation of daemonize() and can_daemonize()\ncan live in compat/ and have the #ifdef switch in git-compat-util.h,\ne.g. something along the lines of these:\n\n\t<< git-compat-util.h >>\n\t... after conditional inclusion of compat/mingw.h ...\n\t#ifndef can_daemonize\n        #define can_daemonize() 1\n\t#endif\n        \n\t<< compat/mingw.h >>\n\t#define can_daemonize() 0\n\t#define daemonize() mingw_daemonize()\n\n\t<< setup.c >>\n        ...\n        #ifndef NO_POSIX_GOODIES\n        int daemonize(void)\n        {\n            ... no ifdef around here ...\n\t}\n\t#endif\n\nWe can be even more purist and move the daemonize() implementation\nfor POSIX to compat/posix.c to keep the generic part even more\nplatform agnostic, which would remove the only #ifdef in setup.c,\nbut that might be going a bit too far.\n\nThese possible implementation choices aside, the goal of this patch\nis a worthwhile thing to do, I would think.\n\nThanks.\n\n>  builtin/gc.c |  1 +\n>  cache.h      |  1 +\n>  setup.c      | 17 +++++++++++++++--\n>  3 files changed, 17 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index c583aad6ec28..79a0f6dc1126 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -368,6 +368,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n>  \t\t */\n>  \t\tif (!need_to_gc())\n>  \t\t\treturn 0;\n> +\t\tdetach_auto &= can_daemonize();\n>  \t\tif (!quiet) {\n>  \t\t\tif (detach_auto)\n>  \t\t\t\tfprintf(stderr, _(\"Auto packing the repository in background for optimum performance.\\n\"));\n> diff --git a/cache.h b/cache.h\n> index fd728f079320..3662e5aabb98 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -524,6 +524,7 @@ extern int set_git_dir_init(const char *git_dir, const char *real_git_dir, int);\n>  extern int init_db(const char *template_dir, unsigned int flags);\n>  \n>  extern void sanitize_stdfds(void);\n> +extern int can_daemonize(void);\n>  extern int daemonize(void);\n>  \n>  #define alloc_nr(x) (((x)+16)*3/2)\n> diff --git a/setup.c b/setup.c\n> index c86bf5c9fabe..6187a4ad9c47 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -1033,12 +1033,25 @@ void sanitize_stdfds(void)\n>  \t\tclose(fd);\n>  }\n>  \n> +#ifdef NO_POSIX_GOODIES\n> +int can_daemonize(void)\n> +{\n> +\treturn 0;\n> +}\n> +\n>  int daemonize(void)\n>  {\n> -#ifdef NO_POSIX_GOODIES\n>  \terrno = ENOSYS;\n>  \treturn -1;\n> +}\n>  #else\n> +int can_daemonize(void)\n> +{\n> +\treturn 1;\n> +}\n> +\n> +int daemonize(void)\n> +{\n>  \tswitch (fork()) {\n>  \t\tcase 0:\n>  \t\t\tbreak;\n> @@ -1054,5 +1067,5 @@ int daemonize(void)\n>  \tclose(2);\n>  \tsanitize_stdfds();\n>  \treturn 0;\n> -#endif\n>  }\n> +#endif /* #ifdef NO_POSIX_GOODIES */\n"},{"id":"285821","messageId":"20160507164451.Horde.S4kJBIYRkpnmQt7tw82MHDH@webmail.informatik.kit.edu","threadId":"42195","inReplyTo":"xmqqbn4kcvuc.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH] gc --auto: don't lie about running in background on Windows","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-05-07T14:44:51Z","receivedAt":"2016-05-07T14:44:51Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting Junio C Hamano <gitster@pobox.com>:\n\n> SZEDER Gábor <szeder@ira.uka.de> writes:\n>\n>> Arguably this helper function could be just a simple variable.  I\n>> opted for a function because:\n>>\n>>   - I preferred a single '#ifdef NO_POSIX_GOODIES', and putting a\n>>     static variable so near to EOF felt just wrong.  (And this is why\n>>     it's not an inline-able function defined in a header file.)\n>>\n>>   - currently we know already at compile time that Windows can't\n>>     daemonize, but in the future we might want to extend this helper\n>>     function to perform some runtime checks, too.  But this is perhaps\n>>     like preparing for crossing a bridge where we'll never get to.\n>\n> Alternatively, the implementation of daemonize() and can_daemonize()\n> can live in compat/ and have the #ifdef switch in git-compat-util.h,\n> e.g. something along the lines of these:\n>\n> \t<< git-compat-util.h >>\n> \t... after conditional inclusion of compat/mingw.h ...\n> \t#ifndef can_daemonize\n>         #define can_daemonize() 1\n> \t#endif\n>\n> \t<< compat/mingw.h >>\n> \t#define can_daemonize() 0\n> \t#define daemonize() mingw_daemonize()\n>\n> \t<< setup.c >>\n>         ...\n>         #ifndef NO_POSIX_GOODIES\n>         int daemonize(void)\n>         {\n>             ... no ifdef around here ...\n> \t}\n> \t#endif\n\nconfig.mak.uname sets NO_POSIX_GOODIES only for Windows builds, but\nNO_POSIX_GOODIES doesn't sound Windows-specific at all.  Currently if\nsomebody were to decide to build with NO_POSIX_GOODIES=UnfortunatelyYes,\nthen he would get a working git in the end, even on non-Windows\nplatforms.  With the proposed alternative above we would only provide an\nalternative daemonize() implementation for MINGW, breaking the build of\nthose setting NO_POSIX_GOODIES by themselves.\n\nI don't know whether there are platforms out there besides Windows that\nneed the NO_POSIX_GOODIES treatment, but we don't explicitly support\nthem in config.mak.uname, or people who for whatever reason build with\nthis knob turned on.\n"}]}