{"thread":{"id":"18997","subject":"[PATCH] Wait for git diff to finish in git difftool","startedAt":"2009-04-22T07:27:22Z","lastAt":"2009-04-23T19:18:09Z","messageCount":14,"participants":["Alex Riesen","David Aguilar","Junio C Hamano","Sverre Rabbelier"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"111926","messageId":"81b0412b0904220027t7afd1ed7xc57c479ef8cdb6b9@mail.gmail.com","threadId":"18997","inReplyTo":null,"subject":"[PATCH] Wait for git diff to finish in git difftool","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-04-22T07:27:22Z","receivedAt":"2009-04-22T07:27:22Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"In ActivetState Perl, exec does not wait for the started program. This\nbreaks difftool tests and may cause unexpected behaviour: git difftool\nhas returned, but the rest of code (diff and possibly the interactive\nprogram are still running in the background.\n\n---\n git-difftool.perl | 3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\n2009/4/6 David Aguilar :\n> +# Ensures that git-difftool ignores bogus --tool values\n> +test_expect_success 'difftool ignores bad --tool values' '\n> +       diff=$(git difftool --no-prompt --tool=bogus-tool branch)\n> +       test \"$?\" = 1 &&\n> +       test \"$diff\" = \"\"\n> +'\n\nThis breaks in that piece of ActiveState Perl if git-difftool is to\ncontinue to use exec: exec*(2) semantics are not available there\n(as they are not possible in Windows at all).\n\nIn this case the script will spawn git-diff and immediately exit with 0.\ngit-diff will run the bogus-tool in \"background\" later.\n\nI usually don't care for exit code in a pure UI tool, so the kill signal\nis just ORed together with the real exit code just to provide indication\nof error.\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 948ff7f..bd828c2 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -82,4 +82,5 @@ sub generate_command\n }\n\n setup_environment();\n-exec(generate_command());\n+my $rc = system(generate_command());\n+exit($rc | ($rc >> 8));\n-- \n1.6.3.rc0.45.g63634\n"},{"id":"111929","messageId":"20090422082652.GA32698@gmail.com","threadId":"18997","inReplyTo":"81b0412b0904220027t7afd1ed7xc57c479ef8cdb6b9@mail.gmail.com","subject":"Re: [PATCH] Wait for git diff to finish in git difftool","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2009-04-22T08:26:54Z","receivedAt":"2009-04-22T08:26:54Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On 0, Alex Riesen wrote:\n> In ActivetState Perl, exec does not wait for the started program. This\n> breaks difftool tests and may cause unexpected behaviour: git difftool\n> has returned, but the rest of code (diff and possibly the interactive\n> program are still running in the background.\n\nThanks for keeping an eye on portability.\n\nThere's a tiny typo in the commit message (Active't'State) that\nmaybe you can tweak before applying? ('you' being Junio)\n\nFor whatever it's worth,\n\nAcked-by: David Aguilar \n\n\n> I usually don't care for exit code in a pure UI tool, so the kill signal\n> is just ORed together with the real exit code just to provide indication\n> of error.\n\nThis seems reasonable. The exit code isn't very important in\nthe common 'show-me-the-diff' read-only scenario, and I wouldn't\nexpect anyone to rely on difftool being exactly exit-code\nequivalent to git-diff.\n\n\n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index 948ff7f..bd828c2 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -82,4 +82,5 @@ sub generate_command\n> }\n> \n> setup_environment();\n> -exec(generate_command());\n> +my $rc = system(generate_command());\n> +exit($rc | ($rc >> 8));\n> -- \n> 1.6.3.rc0.45.g63634\n\n-- \n\t\tDavid\n"},{"id":"111938","messageId":"81b0412b0904220404o30ce1413kcc4dcea1fa822841@mail.gmail.com","threadId":"18997","inReplyTo":"20090422082652.GA32698@gmail.com","subject":"Re: [PATCH] Wait for git diff to finish in git difftool","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-04-22T11:04:42Z","receivedAt":"2009-04-22T11:04:42Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2009/4/22 David Aguilar :\n> On  0, Alex Riesen wrote:\n\nI wasn't born on 0th.\n\n>\n> For whatever it's worth,\n>\n> Acked-by: David Aguilar \n\nYes, FWIW. I intentionally stopped signing off patches for\nWindows, ActiveState Perl and Cygwin: they usually harm the\nrest of the world, while just allowing Windows users limp along.\n"},{"id":"111976","messageId":"7vr5zk346d.fsf@gitster.siamese.dyndns.org","threadId":"18997","inReplyTo":"81b0412b0904220404o30ce1413kcc4dcea1fa822841@mail.gmail.com","subject":"Re: [PATCH] Wait for git diff to finish in git difftool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-22T18:59:38Z","receivedAt":"2009-04-22T18:59:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> 2009/4/22 David Aguilar :\n>> On  0, Alex Riesen wrote:\n>\n> I wasn't born on 0th.\n>\n>>\n>> For whatever it's worth,\n>>\n>> Acked-by: David Aguilar \n>\n> Yes, FWIW. I intentionally stopped signing off patches for\n> Windows, ActiveState Perl and Cygwin: they usually harm the\n> rest of the world, while just allowing Windows users limp along.\n\nI have to wonder if you are doing that with the full understanding of what\nSigned-off-by means. I do not think the provenance of your patch is\naffected by your distaste towards the system it has effects in any way.\n"},{"id":"112003","messageId":"81b0412b0904221340o6b2f1474oed2ad12e4f68cff3@mail.gmail.com","threadId":"18997","inReplyTo":"7vr5zk346d.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Wait for git diff to finish in git difftool","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-04-22T20:40:01Z","receivedAt":"2009-04-22T20:40:01Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2009/4/22 Junio C Hamano :\n>> Yes, FWIW. I intentionally stopped signing off patches for\n>> Windows, ActiveState Perl and Cygwin: they usually harm the\n>> rest of the world, while just allowing Windows users limp along.\n>\n> I have to wonder if you are doing that with the full understanding of what\n> Signed-off-by means.  I do not think the provenance of your patch is\n> affected by your distaste towards the system it has effects in any way.\n\nI'm trying to avoid being associated with development for the platform :)\n\nFrankly, I just always forget about it. The patches to my windows\nrepository are mostly just hacks to either make it pass test suite or\nworkaround another, the platforms specific, stupidity. Not sure anyone\nis actually interested in them (because, I repeat, they mostly hurt\neveryone else, while making the platforms port \"a little working\").\n"},{"id":"112041","messageId":"7v4owgyp8x.fsf@gitster.siamese.dyndns.org","threadId":"18997","inReplyTo":"81b0412b0904221340o6b2f1474oed2ad12e4f68cff3@mail.gmail.com","subject":"Re: [PATCH] Wait for git diff to finish in git difftool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-23T04:21:02Z","receivedAt":"2009-04-23T04:21:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> 2009/4/22 Junio C Hamano :\n>>> Yes, FWIW. I intentionally stopped signing off patches for\n>>> Windows, ActiveState Perl and Cygwin: they usually harm the\n>>> rest of the world, while just allowing Windows users limp along.\n>>\n>> I have to wonder if you are doing that with the full understanding of what\n>> Signed-off-by means.  I do not think the provenance of your patch is\n>> affected by your distaste towards the system it has effects in any way.\n>\n> I'm trying to avoid being associated with development for the platform :)\n>\n> Frankly, I just always forget about it. The patches to my windows\n> repository are mostly just hacks to either make it pass test suite or\n> workaround another, the platforms specific, stupidity. Not sure anyone\n> is actually interested in them (because, I repeat, they mostly hurt\n> everyone else, while making the platforms port \"a little working\").\n\nWhat do you really mean? These patches add maintenance burden but its\nbenefit nobody would care about, and should not be applied?\n"},{"id":"112053","messageId":"81b0412b0904230033n35f1117fmea4432a2f2140d25@mail.gmail.com","threadId":"18997","inReplyTo":"7v4owgyp8x.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Wait for git diff to finish in git difftool","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-04-23T07:33:48Z","receivedAt":"2009-04-23T07:33:48Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2009/4/23 Junio C Hamano :\n> Alex Riesen writes:\n>> Frankly, I just always forget about it. The patches to my windows\n>> repository are mostly just hacks to either make it pass test suite or\n>> workaround another, the platforms specific, stupidity. Not sure anyone\n>> is actually interested in them (because, I repeat, they mostly hurt\n>> everyone else, while making the platforms port \"a little working\").\n>\n> What do you really mean?  These patches add maintenance burden but its\n> benefit nobody would care about, and should not be applied?\n\nMaintenance _and_ run-time. It is your decision, but yes, I kind of hate\nthe idea of them being applied.\n\nIn plain words: please don't apply MY patches with words \"Windows\"\nor \"Cygwin\" in message body, unless I beg you to (and even then,\nconsider me heavily drugged).\n"},{"id":"112057","messageId":"7vtz4fwzaz.fsf@gitster.siamese.dyndns.org","threadId":"18997","inReplyTo":"81b0412b0904230033n35f1117fmea4432a2f2140d25@mail.gmail.com","subject":"Re: [PATCH] Wait for git diff to finish in git difftool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-23T08:26:44Z","receivedAt":"2009-04-23T08:26:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> 2009/4/23 Junio C Hamano :\n>> Alex Riesen writes:\n>>> Frankly, I just always forget about it. The patches to my windows\n>>> repository are mostly just hacks to either make it pass test suite or\n>>> workaround another, the platforms specific, stupidity. Not sure anyone\n>>> is actually interested in them (because, I repeat, they mostly hurt\n>>> everyone else, while making the platforms port \"a little working\").\n>>\n>> What do you really mean?  These patches add maintenance burden but its\n>> benefit nobody would care about, and should not be applied?\n>\n> Maintenance _and_ run-time. It is your decision, but yes, I kind of hate\n> the idea of them being applied.\n>\n> In plain words: please don't apply MY patches with words \"Windows\"\n> or \"Cygwin\" in message body, unless I beg you to (and even then,\n> consider me heavily drugged).\n\nHmm, what's the point then for me to spend time looking at them posted on\nthe list?\n"},{"id":"112072","messageId":"81b0412b0904230252k3e8197d3y8798d5b797a49c39@mail.gmail.com","threadId":"18997","inReplyTo":"7vtz4fwzaz.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Wait for git diff to finish in git difftool","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-04-23T09:52:14Z","receivedAt":"2009-04-23T09:52:14Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2009/4/23 Junio C Hamano :\n> Alex Riesen writes:\n>\n>> 2009/4/23 Junio C Hamano :\n>>> Alex Riesen writes:\n>>>> Frankly, I just always forget about it. The patches to my windows\n>>>> repository are mostly just hacks to either make it pass test suite or\n>>>> workaround another, the platforms specific, stupidity. Not sure anyone\n>>>> is actually interested in them (because, I repeat, they mostly hurt\n>>>> everyone else, while making the platforms port \"a little working\").\n>>>\n>>> What do you really mean?  These patches add maintenance burden but its\n>>> benefit nobody would care about, and should not be applied?\n>>\n>> Maintenance _and_ run-time. It is your decision, but yes, I kind of hate\n>> the idea of them being applied.\n>>\n>> In plain words: please don't apply MY patches with words \"Windows\"\n>> or \"Cygwin\" in message body, unless I beg you to (and even then,\n>> consider me heavily drugged).\n>\n> Hmm, what's the point then for me to spend time looking at them posted on\n> the list?\n>\n\nNone. But it still would be nice if you look at them when you're addressed\ndirectly. Please understand, maybe I'm abusing the list a little, but it looks\nlike the best place to _archive_ the workarounds for this obscure platform.\nThe code is of little use to general public, but in case one finds himself\nin the same situation as I am in, at least Google can help out. I think it\nhappens too rare to justify your looking at them, and especially including\nthem in the mainline, but hiding the code on my laptop isn't of any use\nto anyone (including me: I don't always have that laptop).\n"},{"id":"112087","messageId":"fabb9a1e0904230700l4c2123w663a2cfba65befcd@mail.gmail.com","threadId":"18997","inReplyTo":"81b0412b0904230252k3e8197d3y8798d5b797a49c39@mail.gmail.com","subject":"Re: [PATCH] Wait for git diff to finish in git difftool","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2009-04-23T14:00:06Z","receivedAt":"2009-04-23T14:00:06Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Thu, Apr 23, 2009 at 11:52, Alex Riesen wrote:\n> None. But it still would be nice if you look at them when you're addressed\n> directly. Please understand, maybe I'm abusing the list a little, but it looks\n> like the best place to _archive_ the workarounds for this obscure platform.\n\nIn that case you should note in your patch (preferrably in the\nsubject) that they are not meant for inclusion and merely for the that\npurpose so that those who are not interested in them need not waste\ntheir time reviewing the patch with the assumption that it _is_ meant\nfor inclusion.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"112090","messageId":"7vhc0fv2xb.fsf@gitster.siamese.dyndns.org","threadId":"18997","inReplyTo":"81b0412b0904230252k3e8197d3y8798d5b797a49c39@mail.gmail.com","subject":"Re: [PATCH] Wait for git diff to finish in git difftool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-23T14:51:28Z","receivedAt":"2009-04-23T14:51:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n>>> In plain words: please don't apply MY patches with words \"Windows\"\n>>> or \"Cygwin\" in message body, unless I beg you to (and even then,\n>>> consider me heavily drugged).\n>>\n>> Hmm, what's the point then for me to spend time looking at them posted on\n>> the list?\n>\n> None. But it still would be nice if you look at them when you're addressed\n> directly. Please understand, maybe I'm abusing the list a little, but it looks\n> like the best place to _archive_ the workarounds for this obscure platform.\n> The code is of little use to general public, but in case one finds himself\n> in the same situation as I am in, at least Google can help out. I think it\n> happens too rare to justify your looking at them, and especially including\n> them in the mainline, but hiding the code on my laptop isn't of any use\n> to anyone (including me: I don't always have that laptop).\n\nActually, I think I probably should have phrased it more positively.\n\nDo you think you are in a so minority situation that your \"workarounds\"\nare valuable for nobody else? Apparently you see value in letting them\nfinding your patches in the archive, so you do not expect them to be\n\"nobody else\" but rather \"a small minority\".\n\nPeople on minority platforms have smaller number of people in similar\nsituation to ask help from than people on mainstream platforms, and they\nsuffer from the minority status of their platforms not only with git but\nabout many other things. Worse, their chance of finding solution by\ndigging the archive is smaller, and they would need to spend a lot more\ntime, than the people on other platforms. And you know this a lot better\nthan I do ;-).\n\nEven though the suffering of these people as a whole may not be much, only\nbecause there are small number of them, as individuals they may suffer a\nlot. If we can help them without having to bend over backwards too much,\nit would be a good thing. I think we should at least try.\n\nFor example, I do not think conversion from Perl exec() to Perl system()\nis worsening the code in any way in difftool. Sure, it won't *improve*\nthings for general majority of people, but as long as it does not hurt\npeople (that includes me and others who pay maintenance cost), I do not\nthink we should just bury the patch in archive or in your laptop.\n"},{"id":"112111","messageId":"20090423185732.GA8433@blimp.localdomain","threadId":"18997","inReplyTo":"7vhc0fv2xb.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Wait for git diff to finish in git difftool","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-04-23T18:57:32Z","receivedAt":"2009-04-23T18:57:32Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Junio C Hamano, Thu, Apr 23, 2009 16:51:28 +0200:\n> Do you think you are in a so minority situation that your \"workarounds\"\n> are valuable for nobody else? Apparently you see value in letting them\n> finding your patches in the archive, so you do not expect them to be\n> \"nobody else\" but rather \"a small minority\".\n\nCorrect, \"small minority\".\n\n> People on minority platforms have smaller number of people in similar\n> situation to ask help from than people on mainstream platforms, and they\n> suffer from the minority status of their platforms not only with git but\n> about many other things. Worse, their chance of finding solution by\n> digging the archive is smaller, and they would need to spend a lot more\n> time, than the people on other platforms. And you know this a lot better\n> than I do ;-).\n\nWell, this particular minority is used to have marginal levels of\ntechnical support and .\n\n> For example, I do not think conversion from Perl exec() to Perl system()\n> is worsening the code in any way in difftool. Sure, it won't *improve*\n> things for general majority of people, but as long as it does not hurt\n> people (that includes me and others who pay maintenance cost), I do not\n> think we should just bury the patch in archive or in your laptop.\n\nIsn't it clearly marked in the archives with the keywords most often\nassociated with the platform? (they may seem a little... unprintable)\n\nStill, you are, as usual, right. Especially regarding this particular case.\n\nI'll add this comment regarding use of system in the case where exec\nis right choice on all accounts and resend the patch:\n\n # ActiveState Perl for Win32 does not implement POSIX semantics of\n # exec* system call. It just spawns the given executable and finishes\n # the starting program, exiting with code 0.\n # system will at least catch the errors in returned by git diff,\n # allowing the caller of git difftool better handling of failures.\n"},{"id":"112114","messageId":"20090423190835.GB8433@blimp.localdomain","threadId":"18997","inReplyTo":"20090423185732.GA8433@blimp.localdomain","subject":"Re: [PATCH] Wait for git diff to finish in git difftool","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-04-23T19:08:35Z","receivedAt":"2009-04-23T19:08:35Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Alex Riesen, Thu, Apr 23, 2009 20:57:32 +0200:\n> Junio C Hamano, Thu, Apr 23, 2009 16:51:28 +0200:\n> > For example, I do not think conversion from Perl exec() to Perl system()\n> > is worsening the code in any way in difftool. Sure, it won't *improve*\n> > things for general majority of people, but as long as it does not hurt\n> > people (that includes me and others who pay maintenance cost), I do not\n> > think we should just bury the patch in archive or in your laptop.\n> \n> Isn't it clearly marked in the archives with the keywords most often\n> associated with the platform? (they may seem a little... unprintable)\n> \n> Still, you are, as usual, right. Especially regarding this particular case.\n> \n> I'll add this comment regarding use of system in the case where exec\n> is right choice on all accounts and resend the patch:\n> \n> # ActiveState Perl for Win32 does not implement POSIX semantics of\n> # exec* system call. It just spawns the given executable and finishes\n> # the starting program, exiting with code 0.\n> # system will at least catch the errors in returned by git diff,\n> # allowing the caller of git difftool better handling of failures.\n\nOh... I'm too late...\n"},{"id":"112116","messageId":"20090423191809.GC8433@blimp.localdomain","threadId":"18997","inReplyTo":"20090423190835.GB8433@blimp.localdomain","subject":"[PATCH] Explain seemingly pointless use of system in difftool","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-04-23T19:18:09Z","receivedAt":"2009-04-23T19:18:09Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Portability reasons.\n\nSigned-off-by: Alex Riesen \n---\n\nAlex Riesen, Thu, Apr 23, 2009 21:08:35 +0200:\n> Alex Riesen, Thu, Apr 23, 2009 20:57:32 +0200:\n> > I'll add this comment regarding use of system in the case where exec\n> > is right choice on all accounts and resend the patch:\n> > \n> > # ActiveState Perl for Win32 does not implement POSIX semantics of\n> > # exec* system call. It just spawns the given executable and finishes\n> > # the starting program, exiting with code 0.\n> > # system will at least catch the errors in returned by git diff,\n> > # allowing the caller of git difftool better handling of failures.\n> \n> Oh... I'm too late...\n> \n\nThere.\n\n git-difftool.perl | 6 ++++++\n 1 files changed, 6 insertions(+), 0 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex bd828c2..9255d23 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -82,5 +82,11 @@ sub generate_command\n }\n \n setup_environment();\n+\n+# ActiveState Perl for Win32 does not implement POSIX semantics of\n+# exec* system call. It just spawns the given executable and finishes\n+# the starting program, exiting with code 0.\n+# system will at least catch the errors returned by git diff,\n+# allowing the caller of git difftool better handling of failures.\n my $rc = system(generate_command());\n exit($rc | ($rc >> 8));\n-- \n1.6.3.rc1.74.g42ff\n"}]}