{"thread":{"id":"38726","subject":"[PATCH v4] git: treat \"-C \" as a no-op when is empty","startedAt":"2015-03-06T11:18:08Z","lastAt":"2015-03-08T10:15:16Z","messageCount":8,"participants":["Karthik Nayak","Junio C Hamano","Eric Sunshine","karthik nayak"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"257148","messageId":"1425640688-26513-1-git-send-email-karthik.188@gmail.com","threadId":"38726","inReplyTo":null,"subject":"[PATCH v4] git: treat \"-C \" as a no-op when is empty","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-03-06T11:18:08Z","receivedAt":"2015-03-06T11:18:08Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"'git -C \"\"' unhelpfully dies with error \"Cannot change to ''\",\nwhereas the shell treats `cd \"\"' as a no-op. Taking the shell's\nbehavior as a precedent, teach git to treat `-C \"\"' as a no-op, as\nwell.\n\nTest to check the no-op behaviour of \"-C \" when is\nempty, written by Junio C Hamano.\n\nHelped-by: Junio C Hamano \nHelped-by: Eric Sunchine \nSigned-off-by: Karthik Nayak \n---\n git.c | 10 ++++++----\n t/t0056-git-C.sh | 10 ++++++++++\n 2 files changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 8c7ee9c..b062e0e 100644\n--- a/git.c\n+++ b/git.c\n@@ -204,10 +204,12 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\t\tfprintf(stderr, \"No directory given for -C.\\n\" );\n \t\t\t\tusage(git_usage_string);\n \t\t\t}\n-\t\t\tif (chdir((*argv)[1]))\n-\t\t\t\tdie_errno(\"Cannot change to '%s'\", (*argv)[1]);\n-\t\t\tif (envchanged)\n-\t\t\t\t*envchanged = 1;\n+\t\t\tif (*(*argv)[1]) {\n+\t\t\t\tif (chdir((*argv)[1]))\n+\t\t\t\t\tdie_errno(\"Cannot change to '%s'\", (*argv)[1]);\n+\t\t\t\tif (envchanged)\n+\t\t\t\t\t*envchanged = 1;\n+\t\t\t}\n \t\t\t(*argv)++;\n \t\t\t(*argc)--;\n \t\t} else {\ndiff --git a/t/t0056-git-C.sh b/t/t0056-git-C.sh\nindex 99c0377..2630e75 100755\n--- a/t/t0056-git-C.sh\n+++ b/t/t0056-git-C.sh\n@@ -14,6 +14,16 @@ test_expect_success '\"git -C \" runs git from the directory ' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success '\"git -C \" with an empty is a no-op' '\n+\t(\n+\t\tmkdir -p dir1/subdir &&\n+\t\tcd dir1/subdir &&\n+\t\tgit -C \"\" rev-parse --show-prefix >actual &&\n+\t\techo subdir/ >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'Multiple -C options: \"-C dir1 -C dir2\" is equivalent to \"-C dir1/dir2\"' '\n \ttest_create_repo dir1/dir2 &&\n \techo 1 >dir1/dir2/b.txt &&\n-- \n2.3.1.167.g7f4ba4b.dirty\n"},{"id":"257220","messageId":"xmqqfv9hvawy.fsf@gitster.dls.corp.google.com","threadId":"38726","inReplyTo":"1425640688-26513-1-git-send-email-karthik.188@gmail.com","subject":"Re: [PATCH v4] git: treat \"-C \" as a no-op when is empty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-07T01:44:29Z","receivedAt":"2015-03-07T01:44:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> 'git -C \"\"' unhelpfully dies with error \"Cannot change to ''\",\n> whereas the shell treats `cd \"\"' as a no-op. Taking the shell's\n> behavior as a precedent, teach git to treat `-C \"\"' as a no-op, as\n> well.\n>\n> Test to check the no-op behaviour of \"-C \" when is\n> empty, written by Junio C Hamano.\n>\n> Helped-by: Junio C Hamano \n> Helped-by: Eric Sunchine \n> Signed-off-by: Karthik Nayak \n> ---\n\nThis iteration looks sensible, except that the Subject reads\nstrange. Will queue with minor tweaks to the log message,\nand perhaps with a fix to unreadable *(*argv)[1] that was\nmentioned elsewhere.\n\nThanks.\n\n> git.c | 10 ++++++----\n> t/t0056-git-C.sh | 10 ++++++++++\n> 2 files changed, 16 insertions(+), 4 deletions(-)\n>\n> diff --git a/git.c b/git.c\n> index 8c7ee9c..b062e0e 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -204,10 +204,12 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n> \t\t\t\tfprintf(stderr, \"No directory given for -C.\\n\" );\n> \t\t\t\tusage(git_usage_string);\n> \t\t\t}\n> -\t\t\tif (chdir((*argv)[1]))\n> -\t\t\t\tdie_errno(\"Cannot change to '%s'\", (*argv)[1]);\n> -\t\t\tif (envchanged)\n> -\t\t\t\t*envchanged = 1;\n> +\t\t\tif (*(*argv)[1]) {\n> +\t\t\t\tif (chdir((*argv)[1]))\n> +\t\t\t\t\tdie_errno(\"Cannot change to '%s'\", (*argv)[1]);\n> +\t\t\t\tif (envchanged)\n> +\t\t\t\t\t*envchanged = 1;\n> +\t\t\t}\n> \t\t\t(*argv)++;\n> \t\t\t(*argc)--;\n> \t\t} else {\n> diff --git a/t/t0056-git-C.sh b/t/t0056-git-C.sh\n> index 99c0377..2630e75 100755\n> --- a/t/t0056-git-C.sh\n> +++ b/t/t0056-git-C.sh\n> @@ -14,6 +14,16 @@ test_expect_success '\"git -C \" runs git from the directory ' '\n> \ttest_cmp expected actual\n> '\n> \n> +test_expect_success '\"git -C \" with an empty is a no-op' '\n> +\t(\n> +\t\tmkdir -p dir1/subdir &&\n> +\t\tcd dir1/subdir &&\n> +\t\tgit -C \"\" rev-parse --show-prefix >actual &&\n> +\t\techo subdir/ >expect &&\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n> +\n> test_expect_success 'Multiple -C options: \"-C dir1 -C dir2\" is equivalent to \"-C dir1/dir2\"' '\n> \ttest_create_repo dir1/dir2 &&\n> \techo 1 >dir1/dir2/b.txt &&\n"},{"id":"257222","messageId":"CAPig+cTkC1Y1sWJLpG0iUHju3GOMnvOT-nsAU51GykeV2QB+vA@mail.gmail.com","threadId":"38726","inReplyTo":"1425640688-26513-1-git-send-email-karthik.188@gmail.com","subject":"Re: [PATCH v4] git: treat \"-C \" as a no-op when is empty","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-07T02:19:57Z","receivedAt":"2015-03-07T02:19:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 6, 2015 at 6:18 AM, Karthik Nayak wrote:\n> 'git -C \"\"' unhelpfully dies with error \"Cannot change to ''\",\n> whereas the shell treats `cd \"\"' as a no-op. Taking the shell's\n> behavior as a precedent, teach git to treat `-C \"\"' as a no-op, as\n> well.\n>\n> Test to check the no-op behaviour of \"-C \" when is\n> empty, written by Junio C Hamano.\n>\n> Helped-by: Junio C Hamano \n> Helped-by: Eric Sunchine \n\ns/Sunchine/Sunshine/\n\n> Signed-off-by: Karthik Nayak \n"},{"id":"257230","messageId":"54FAD7D7.4030008@gmail.com","threadId":"38726","inReplyTo":"CAPig+cTkC1Y1sWJLpG0iUHju3GOMnvOT-nsAU51GykeV2QB+vA@mail.gmail.com","subject":"Re: [PATCH v4] git: treat \"-C \" as a no-op when is empty","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-03-07T10:49:59Z","receivedAt":"2015-03-07T10:49:59Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"\n > This iteration looks sensible, except that the Subject reads\n > strange. Will queue with minor tweaks to the log message,\n > and perhaps with a fix to unreadable *(*argv)[1] that was\n > mentioned elsewhere.\n >\n > Thanks.\n\nHey could you tell me what seems strange, so I can improve on\nit the next time.\nAlso \"*(*argv)[1]\" seems more readable to me, maybe more of a perspective?\n\nThanks\n\n\nOn 03/07/2015 07:49 AM, Eric Sunshine wrote:\n> On Fri, Mar 6, 2015 at 6:18 AM, Karthik Nayak wrote:\n>> 'git -C \"\"' unhelpfully dies with error \"Cannot change to ''\",\n>> whereas the shell treats `cd \"\"' as a no-op. Taking the shell's\n>> behavior as a precedent, teach git to treat `-C \"\"' as a no-op, as\n>> well.\n>>\n>> Test to check the no-op behaviour of \"-C \" when is\n>> empty, written by Junio C Hamano.\n>>\n>> Helped-by: Junio C Hamano \n>> Helped-by: Eric Sunchine \n>\n> s/Sunchine/Sunshine/\n>\n>> Signed-off-by: Karthik Nayak \n\nSorry Eric for the genuine mistake.\n"},{"id":"257259","messageId":"CAPig+cRDkoH-zmYhk9ag+Yiwg1h452hOpS2fx2H5xmu5KUNqiQ@mail.gmail.com","threadId":"38726","inReplyTo":"54FAD7D7.4030008@gmail.com","subject":"Re: [PATCH v4] git: treat \"-C \" as a no-op when is empty","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-08T04:38:47Z","receivedAt":"2015-03-08T04:38:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Mar 7, 2015 at 5:49 AM, karthik nayak wrote:\n>> This iteration looks sensible, except that the Subject reads\n>> strange. Will queue with minor tweaks to the log message,\n>> and perhaps with a fix to unreadable *(*argv)[1] that was\n>> mentioned elsewhere.\n>\n> Hey could you tell me what seems strange, so I can improve on\n> it the next time.\n\nJunio means that you somehow botched the Subject: line when you copied\nthe commit message I suggested into your new version of the patch.\nInstead of , you wrote .\n\n> Also \"*(*argv)[1]\" seems more readable to me, maybe more of a perspective?\n\nI also had considered suggesting (*argv)[1][0] as more readable, but\nit is primarily personal taste, and I didn't want to bike-shed the\nissue.\n"},{"id":"257265","messageId":"54FBDFCD.9030703@gmail.com","threadId":"38726","inReplyTo":"CAPig+cRDkoH-zmYhk9ag+Yiwg1h452hOpS2fx2H5xmu5KUNqiQ@mail.gmail.com","subject":"Re: [PATCH v4] git: treat \"-C \" as a no-op when is empty","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-03-08T05:36:13Z","receivedAt":"2015-03-08T05:36:13Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"\nOn 03/08/2015 10:08 AM, Eric Sunshine wrote:\n> On Sat, Mar 7, 2015 at 5:49 AM, karthik nayak wrote:\n> >> This iteration looks sensible, except that the Subject reads\n> >> strange. Will queue with minor tweaks to the log message,\n> >> and perhaps with a fix to unreadable *(*argv)[1] that was\n> >> mentioned elsewhere.\n> >\n> > Hey could you tell me what seems strange, so I can improve on\n> > it the next time.\n>\n> Junio means that you somehow botched the Subject: line when you copied\n> the commit message I suggested into your new version of the patch.\n> Instead of , you wrote .\n>\nOops! I'm too anxious i guess.\n> > Also \"*(*argv)[1]\" seems more readable to me, maybe more of a perspective?\n>\n> I also had considered suggesting (*argv)[1][0] as more readable, but\n> it is primarily personal taste, and I didn't want to bike-shed the\n> issue.\n>\nThanks.\n"},{"id":"257268","messageId":"xmqqvbict0y9.fsf@gitster.dls.corp.google.com","threadId":"38726","inReplyTo":"CAPig+cRDkoH-zmYhk9ag+Yiwg1h452hOpS2fx2H5xmu5KUNqiQ@mail.gmail.com","subject":"Re: [PATCH v4] git: treat \"-C \" as a no-op when is empty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-08T07:14:54Z","receivedAt":"2015-03-08T07:14:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Sat, Mar 7, 2015 at 5:49 AM, karthik nayak wrote:\n>\n>> Also \"*(*argv)[1]\" seems more readable to me, maybe more of a perspective?\n>\n> I also had considered suggesting (*argv)[1][0] as more readable, but\n> it is primarily personal taste, and I didn't want to bike-shed the\n> issue.\n\nI didn't mention that in earlier review rounds for the same reason,\nbut I saw Andreas Schwab also independently made the same comment,\nso that makes three of us.\n\nThat does not change the fact that this is merely a matter of\npreference; I wouldn't even call this one a \"taste\" (use of which\nimplies that there are judgement involved, as in \"good taste\" and\n\"bad taste\").\n\nBut because it is \"preference\", the only time we can discuss is when\na new code is submitted and is under review. Once it is in the\ntree, it is not really worth extra patch noise to go and change.\n\nAs everybody knows, POINTER[0] and *POINTER are equivalent. We have\nquite a few places where we say \"let's treat passing an empty string\nthe same as not passing an argument at all\" with\n\n\tif (!POINTER || !*POINTER)\n \t; /* no-op */\n\telse {\n \t/* do something with POINTER */\n fn(POINTER);\n\t}\n\nand we could say !POINTER[0] instead of !*POINTER, interchangeably.\n\nWe tend to prefer (again, I do not think this is particularly a\n\"taste\" thing) *POINTER over POINTER[0] when POINTER is just a\nsingle variable in the above pattern we often see in our code.\n\nBut when POINTER is an expression like (*argv)[1], where you unwrap\nthe operators according to their precedences, it often is easier to\nread if you do not have to flip your eyes left and right too often.\n\nYou first look at \"argv\", then notice the prefix \"*\" (you have to\nmove your eyes to the left here) and know argv points at a location\nthat holds a pointer. Then you notice the suffix [1] (now to the\nright) and know that pointer points at an array and the expression\nis talking about in its second element.\n\nNow, you want to say that second element is actually a pointer to a\nstring and want to talk about the beginning of that string. If you\nexpress it as \"*(*argv)[1]\", it forces the reader to go back to the\nleft end once more. If you write it as \"(*argv)[1][0]\", the reader\ncan keep going to the right, starting from the last thing the reader\nread and understood (which is the \"[1]\" at the right end).\n\nAt least, that is how I analyze _my_ preference---the latter feels\neasier on my eyes.\n\nBut as I said this is a mere preference thing.\n"},{"id":"257313","messageId":"54FC2134.3030806@gmail.com","threadId":"38726","inReplyTo":"xmqqvbict0y9.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4] git: treat \"-C \" as a no-op when is empty","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-03-08T10:15:16Z","receivedAt":"2015-03-08T10:15:16Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"\n\nOn 03/08/2015 12:44 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>\n>> On Sat, Mar 7, 2015 at 5:49 AM, karthik nayak wrote:\n>>\n>>> Also \"*(*argv)[1]\" seems more readable to me, maybe more of a perspective?\n>>\n>> I also had considered suggesting (*argv)[1][0] as more readable, but\n>> it is primarily personal taste, and I didn't want to bike-shed the\n>> issue.\n>\n> I didn't mention that in earlier review rounds for the same reason,\n> but I saw Andreas Schwab also independently made the same comment,\n> so that makes three of us.\n>\n> That does not change the fact that this is merely a matter of\n> preference; I wouldn't even call this one a \"taste\" (use of which\n> implies that there are judgement involved, as in \"good taste\" and\n> \"bad taste\").\n>\n> But because it is \"preference\", the only time we can discuss is when\n> a new code is submitted and is under review. Once it is in the\n> tree, it is not really worth extra patch noise to go and change.\n>\n> As everybody knows, POINTER[0] and *POINTER are equivalent. We have\n> quite a few places where we say \"let's treat passing an empty string\n> the same as not passing an argument at all\" with\n>\n> \tif (!POINTER || !*POINTER)\n> \t; /* no-op */\n> \telse {\n> \t/* do something with POINTER */\n> fn(POINTER);\n> \t}\n>\n> and we could say !POINTER[0] instead of !*POINTER, interchangeably.\n>\n> We tend to prefer (again, I do not think this is particularly a\n> \"taste\" thing) *POINTER over POINTER[0] when POINTER is just a\n> single variable in the above pattern we often see in our code.\n>\n> But when POINTER is an expression like (*argv)[1], where you unwrap\n> the operators according to their precedences, it often is easier to\n> read if you do not have to flip your eyes left and right too often.\n>\n> You first look at \"argv\", then notice the prefix \"*\" (you have to\n> move your eyes to the left here) and know argv points at a location\n> that holds a pointer. Then you notice the suffix [1] (now to the\n> right) and know that pointer points at an array and the expression\n> is talking about in its second element.\n>\n> Now, you want to say that second element is actually a pointer to a\n> string and want to talk about the beginning of that string. If you\n> express it as \"*(*argv)[1]\", it forces the reader to go back to the\n> left end once more. If you write it as \"(*argv)[1][0]\", the reader\n> can keep going to the right, starting from the last thing the reader\n> read and understood (which is the \"[1]\" at the right end).\n>\n> At least, that is how I analyze _my_ preference---the latter feels\n> easier on my eyes.\n>\n> But as I said this is a mere preference thing.\n>\n\nThe way you put it, it makes a lot of sense that most would prefer \n\"(*argv)[1][0]\" rather than \"*(*argv)[1]\".\n\nThanks for clearing that out.\nRegards\n-Karthik\n"}]}