{"thread":{"id":"47492","subject":"[PATCH] setup.c: move statement under condition","startedAt":"2017-12-23T20:12:22Z","lastAt":"2017-12-27T18:57:28Z","messageCount":7,"participants":["Vadim Petrov","Johannes Schindelin","Kevin Daudt","Ævar Arnfjörð Bjarmason","Martin Werner","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"335232","messageId":"3853941514059379@web42g.yandex.ru","threadId":"47492","inReplyTo":null,"subject":"[PATCH] setup.c: move statement under condition","fromName":"Vadim Petrov","fromEmail":"tridronet@yandex.ru","sentAt":"2017-12-23T20:02:59Z","receivedAt":"2017-12-23T20:12:22Z","isPatch":true,"sender":{"key":"tridronet@yandex.ru","avatar":null},"body":"I suppose that if the condition is fulfilled then the previously\nobtained value will not be necessary.\n\nSigned-off-by: Vadim Petrov <tridronet@protonmail.com>\n---\n setup.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/setup.c b/setup.c\nindex 8cc34186c..1ce0189fa 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -35,7 +35,6 @@ static int abspath_part_inside_repo(char *path)\n \t\treturn -1;\n \twtlen = strlen(work_tree);\n \tlen = strlen(path);\n-\toff = offset_1st_component(path);\n \n \t/* check if work tree is already the prefix */\n \tif (wtlen <= len && !strncmp(path, work_tree, wtlen)) {\n@@ -49,6 +48,8 @@ static int abspath_part_inside_repo(char *path)\n \t\t}\n \t\t/* work tree might match beginning of a symlink to work tree */\n \t\toff = wtlen;\n+\t} else {\n+\t\toff = offset_1st_component(path);\n \t}\n \tpath0 = path;\n \tpath += off;\n-- \n2.15.1.433.g936d1b989\n\n"},{"id":"335244","messageId":"alpine.DEB.2.21.1.1712232332000.406@MININT-6BKU6QN.europe.corp.microsoft.com","threadId":"47492","inReplyTo":"3853941514059379@web42g.yandex.ru","subject":"Re: [PATCH] setup.c: move statement under condition","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-12-23T22:48:18Z","receivedAt":"2017-12-23T22:48:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Vadim,\n\nthank you for your contribution!\n\nOn Sun, 24 Dec 2017, Vadim Petrov wrote:\n\n> I suppose that if the condition is fulfilled then the previously\n> obtained value will not be necessary.\n\nI have to be honest: this commit message (including the subject) left me\nquite puzzled as to the intent of this patch.\n\nMaybe something like this would have spared me that puzzlement:\n\n\tAvoid unnecessary offset_1st_component() when prefixing filenames\n\n\tIn the abspath_part_inside_repo() function that is called\n\tsomewhere deep in the call-chain when prefixing paths, we\n\tcalculate the offset of the first component, but under certain\n\tcircumstances, the result is not even used.\n\n\tThis patch changes the code to avoid that.\n\nIf you also have a background story that motivated you to work on this\npatch (for example, if you hit a huge performance bottleneck with some\ntool that fed thousands of absolute paths to Git that needed to be turned\ninto paths relative to the worktree's top-level directory), I would\ndefinitely put that into the commit message, too, if I were you.\n\n> diff --git a/setup.c b/setup.c\n> index 8cc34186c..1ce0189fa 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -35,7 +35,6 @@ static int abspath_part_inside_repo(char *path)\n>  \t\treturn -1;\n>  \twtlen = strlen(work_tree);\n>  \tlen = strlen(path);\n> -\toff = offset_1st_component(path);\n>  \n>  \t/* check if work tree is already the prefix */\n>  \tif (wtlen <= len && !strncmp(path, work_tree, wtlen)) {\n> @@ -49,6 +48,8 @@ static int abspath_part_inside_repo(char *path)\n>  \t\t}\n>  \t\t/* work tree might match beginning of a symlink to work tree */\n>  \t\toff = wtlen;\n> +\t} else {\n> +\t\toff = offset_1st_component(path);\n>  \t}\n\nUp until recently, we encouraged dropping the curly brackets from\nsingle-line statements, but apparently that changed. It is now no longer\nclear, and often left to the taste of the contributor. But not always.\nSometimes we start a beautiful thread discussion the pros and cons of\ncurly brackets in the middle of patch review, and drop altogether\nreviewing the actual patch.\n\nHowever, we still encourage to put shorter alternative code paths\n(i.e. the blocks after `if` and `else`) first, in your case:\n\n@@ -35,18 +35,19 @@ static int abspath_part_inside_repo(char *path)\n \t\treturn -1;\n \twtlen = strlen(work_tree);\n \tlen = strlen(path);\n-\toff = offset_1st_component(path);\n \n \t/* check if work tree is already the prefix */\n- \tif (wtlen <= len && !strncmp(path, work_tree, wtlen)) {\n+\tif (wtlen > len || strncmp(path, work_tree, wtlen))\n+\t\toff = offset_1st_component(path);\n+\telse {\n \t\tif (path[wtlen] == '/') {\n \t\t\tmemmove(path, path + wtlen + 1, len - wtlen);\n \t\t\treturn 0;\n\t\t} else if (path[wtlen - 1] == '/' || path[wtlen] == '\\0') {\n\t\t\t/* work tree is the root, or the whole path */\n\t\t\tmemmove(path, path + wtlen, len - wtlen + 1);\n\t\t\treturn 0;\n\t\t}\n\t\t/* work tree might match beginning of a symlink to work * tree */\n\t\toff = wtlen;\n\t}\n\nI would also encourage to generate the patch so that it includes the `off\n= wtlen` line (by passing -U11 or some such to `git format-patch`), to\nmake the review super easy.\n\nIn short: I think your patch does the right thing, and I hope that you\nfind my suggestions to improve the patch useful.\n\nCiao,\nJohannes\n"},{"id":"335247","messageId":"1483761514103335@web45o.yandex.ru","threadId":"47492","inReplyTo":"alpine.DEB.2.21.1.1712232332000.406@MININT-6BKU6QN.europe.corp.microsoft.com","subject":"Re: [PATCH] setup.c: move statement under condition","fromName":"Vadim Petrov","fromEmail":"tridronet@yandex.ru","sentAt":"2017-12-24T08:15:35Z","receivedAt":"2017-12-24T08:26:47Z","isPatch":true,"sender":{"key":"tridronet@yandex.ru","avatar":null},"body":"Thank you for your replay.\n\n> I have to be honest: this commit message (including the subject) left me\n> quite puzzled as to the intent of this patch.\n\nI still only learn English and correctly express my thoughts while somewhat difficult.\n\n> If you also have a background story that motivated you to work on this\n> patch (for example, if you hit a huge performance bottleneck with some\n> tool that fed thousands of absolute paths to Git that needed to be turned\n> into paths relative to the worktree's top-level directory), I would\n> definitely put that into the commit message, too, if I were you.\n\nI have no such reason. I just saw it and wanted to change it.\n\n> Up until recently, we encouraged dropping the curly brackets from\n> single-line statements, but apparently that changed. It is now no longer\n> clear, and often left to the taste of the contributor. But not always.\n> Sometimes we start a beautiful thread discussion the pros and cons of\n> curly brackets in the middle of patch review, and drop altogether\n> reviewing the actual patch.\n\nI was guided by the rule from the Documentation/CodingGuidelines:\n\tWhen there are multiple arms to a conditional and some of them\n\trequire braces, enclose even a single line block in braces for\n\tconsistency.\nAnd other code from setup.c:\n\tfrom function get_common_dir:\n\t\tif (!has_common) {\n\t\t\t/* several commands */\n\t\t} else {\n\t\t\tfree(candidate->work_tree);\n\t\t}\n\tfrom function get_common_dir_noenv:\n\t\tif (file_exists(path.buf)) {\n\t\t\t/* several commands */\n\t\t} else {\n\t\t\tstrbuf_addstr(sb, gitdir);\n\t\t}\n\n> In short: I think your patch does the right thing, and I hope that you\n> find my suggestions to improve the patch useful.\n\nI fixed the patch according to your suggestions.\n\n\nSigned-off-by: Vadim Petrov <tridronet@yandex.ru>\n---\n setup.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 8cc34186c..1a414c256 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -27,26 +27,26 @@ static int abspath_part_inside_repo(char *path)\n {\n \tsize_t len;\n \tsize_t wtlen;\n \tchar *path0;\n \tint off;\n \tconst char *work_tree = get_git_work_tree();\n \n \tif (!work_tree)\n \t\treturn -1;\n \twtlen = strlen(work_tree);\n \tlen = strlen(path);\n-\toff = offset_1st_component(path);\n \n-\t/* check if work tree is already the prefix */\n-\tif (wtlen <= len && !strncmp(path, work_tree, wtlen)) {\n+\tif (wtlen > len || strncmp(path, work_tree, wtlen))\n+\t\toff = offset_1st_component(path);\n+\telse { /* check if work tree is already the prefix */\n \t\tif (path[wtlen] == '/') {\n \t\t\tmemmove(path, path + wtlen + 1, len - wtlen);\n \t\t\treturn 0;\n \t\t} else if (path[wtlen - 1] == '/' || path[wtlen] == '\\0') {\n \t\t\t/* work tree is the root, or the whole path */\n \t\t\tmemmove(path, path + wtlen, len - wtlen + 1);\n \t\t\treturn 0;\n \t\t}\n \t\t/* work tree might match beginning of a symlink to work tree */\n \t\toff = wtlen;\n \t}\n-- \n2.15.1.433.g936d1b989\n"},{"id":"335263","messageId":"20171224171118.GD27280@alpha.vpn.ikke.info","threadId":"47492","inReplyTo":"1483761514103335@web45o.yandex.ru","subject":"Re: [PATCH] setup.c: move statement under condition","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2017-12-24T17:11:18Z","receivedAt":"2017-12-24T17:12:22Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Sun, Dec 24, 2017 at 12:15:35PM +0400, Vadim Petrov wrote:\n> Thank you for your replay.\n> \n> > I have to be honest: this commit message (including the subject) left me\n> > quite puzzled as to the intent of this patch.\n> \n> I still only learn English and correctly express my thoughts while somewhat difficult.\n> \n> > If you also have a background story that motivated you to work on this\n> > patch (for example, if you hit a huge performance bottleneck with some\n> > tool that fed thousands of absolute paths to Git that needed to be turned\n> > into paths relative to the worktree's top-level directory), I would\n> > definitely put that into the commit message, too, if I were you.\n> \n> I have no such reason. I just saw it and wanted to change it.\n\nA commit message contains the reason why this is a good change to make.\nIt lets others know what problems it's trying to solve or what usecase\nit tries to satisfy.\n\nThe commit message basically needs to convince others why the change is\nnecessary / desired, now, and in the future. \n\nThis will help others to follow your thought process and it gives you\nthe possiblity to communicate trade-offs you made, all which cannot\ninferred from the patch.\n\nFor simple changes, the motivation can be simple too.\n\nTo make it concrete: You are talking about a condition. What condition?\nAnd you say that the previously obtained value will not be necessary.\nWhat do you do with that value then? Why does this change improve the\nsituation? \n\nThese are things you can state in your commit message.\n\nHope this helps, Kevin\n\n> > Up until recently, we encouraged dropping the curly brackets from\n> > single-line statements, but apparently that changed. It is now no longer\n> > clear, and often left to the taste of the contributor. But not always.\n> > Sometimes we start a beautiful thread discussion the pros and cons of\n> > curly brackets in the middle of patch review, and drop altogether\n> > reviewing the actual patch.\n> \n> I was guided by the rule from the Documentation/CodingGuidelines:\n> \tWhen there are multiple arms to a conditional and some of them\n> \trequire braces, enclose even a single line block in braces for\n> \tconsistency.\n> And other code from setup.c:\n> \tfrom function get_common_dir:\n> \t\tif (!has_common) {\n> \t\t\t/* several commands */\n> \t\t} else {\n> \t\t\tfree(candidate->work_tree);\n> \t\t}\n> \tfrom function get_common_dir_noenv:\n> \t\tif (file_exists(path.buf)) {\n> \t\t\t/* several commands */\n> \t\t} else {\n> \t\t\tstrbuf_addstr(sb, gitdir);\n> \t\t}\n> \n> > In short: I think your patch does the right thing, and I hope that you\n> > find my suggestions to improve the patch useful.\n> \n> I fixed the patch according to your suggestions.\n> \n> \n> Signed-off-by: Vadim Petrov <tridronet@yandex.ru>\n> ---\n>  setup.c | 6 +++---\n>  1 file changed, 3 insertions(+), 3 deletions(-)\n> \n> diff --git a/setup.c b/setup.c\n> index 8cc34186c..1a414c256 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -27,26 +27,26 @@ static int abspath_part_inside_repo(char *path)\n>  {\n>  \tsize_t len;\n>  \tsize_t wtlen;\n>  \tchar *path0;\n>  \tint off;\n>  \tconst char *work_tree = get_git_work_tree();\n>  \n>  \tif (!work_tree)\n>  \t\treturn -1;\n>  \twtlen = strlen(work_tree);\n>  \tlen = strlen(path);\n> -\toff = offset_1st_component(path);\n>  \n> -\t/* check if work tree is already the prefix */\n> -\tif (wtlen <= len && !strncmp(path, work_tree, wtlen)) {\n> +\tif (wtlen > len || strncmp(path, work_tree, wtlen))\n> +\t\toff = offset_1st_component(path);\n> +\telse { /* check if work tree is already the prefix */\n>  \t\tif (path[wtlen] == '/') {\n>  \t\t\tmemmove(path, path + wtlen + 1, len - wtlen);\n>  \t\t\treturn 0;\n>  \t\t} else if (path[wtlen - 1] == '/' || path[wtlen] == '\\0') {\n>  \t\t\t/* work tree is the root, or the whole path */\n>  \t\t\tmemmove(path, path + wtlen, len - wtlen + 1);\n>  \t\t\treturn 0;\n>  \t\t}\n>  \t\t/* work tree might match beginning of a symlink to work tree */\n>  \t\toff = wtlen;\n>  \t}\n> -- \n> 2.15.1.433.g936d1b989\n"},{"id":"335264","messageId":"87zi67rkab.fsf@evledraar.gmail.com","threadId":"47492","inReplyTo":"20171224171118.GD27280@alpha.vpn.ikke.info","subject":"Re: [PATCH] setup.c: move statement under condition","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-24T19:35:40Z","receivedAt":"2017-12-24T19:38:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Dec 24 2017, Kevin Daudt jotted:\n\n> On Sun, Dec 24, 2017 at 12:15:35PM +0400, Vadim Petrov wrote:\n>> Thank you for your replay.\n>>\n>> > I have to be honest: this commit message (including the subject) left me\n>> > quite puzzled as to the intent of this patch.\n>>\n>> I still only learn English and correctly express my thoughts while somewhat difficult.\n>>\n>> > If you also have a background story that motivated you to work on this\n>> > patch (for example, if you hit a huge performance bottleneck with some\n>> > tool that fed thousands of absolute paths to Git that needed to be turned\n>> > into paths relative to the worktree's top-level directory), I would\n>> > definitely put that into the commit message, too, if I were you.\n>>\n>> I have no such reason. I just saw it and wanted to change it.\n>\n> A commit message contains the reason why this is a good change to make.\n> It lets others know what problems it's trying to solve or what usecase\n> it tries to satisfy.\n>\n> The commit message basically needs to convince others why the change is\n> necessary / desired, now, and in the future.\n>\n> This will help others to follow your thought process and it gives you\n> the possiblity to communicate trade-offs you made, all which cannot\n> inferred from the patch.\n>\n> For simple changes, the motivation can be simple too.\n\n...and a good example would be 6127ff63cf which introduced this logic\nVadim is trying to change, i.e. does this still retain the fix for\nwhatever issue that was trying to address?\n\nIt's also good to CC the people who've directly worked on the code\nyou're changing in the past, I've CC'd Martin.\n\n> To make it concrete: You are talking about a condition. What condition?\n> And you say that the previously obtained value will not be necessary.\n> What do you do with that value then? Why does this change improve the\n> situation?\n>\n> These are things you can state in your commit message.\n>\n> Hope this helps, Kevin\n>\n>> > Up until recently, we encouraged dropping the curly brackets from\n>> > single-line statements, but apparently that changed. It is now no longer\n>> > clear, and often left to the taste of the contributor. But not always.\n>> > Sometimes we start a beautiful thread discussion the pros and cons of\n>> > curly brackets in the middle of patch review, and drop altogether\n>> > reviewing the actual patch.\n>>\n>> I was guided by the rule from the Documentation/CodingGuidelines:\n>> \tWhen there are multiple arms to a conditional and some of them\n>> \trequire braces, enclose even a single line block in braces for\n>> \tconsistency.\n>> And other code from setup.c:\n>> \tfrom function get_common_dir:\n>> \t\tif (!has_common) {\n>> \t\t\t/* several commands */\n>> \t\t} else {\n>> \t\t\tfree(candidate->work_tree);\n>> \t\t}\n>> \tfrom function get_common_dir_noenv:\n>> \t\tif (file_exists(path.buf)) {\n>> \t\t\t/* several commands */\n>> \t\t} else {\n>> \t\t\tstrbuf_addstr(sb, gitdir);\n>> \t\t}\n>>\n>> > In short: I think your patch does the right thing, and I hope that you\n>> > find my suggestions to improve the patch useful.\n>>\n>> I fixed the patch according to your suggestions.\n>>\n>>\n>> Signed-off-by: Vadim Petrov <tridronet@yandex.ru>\n>> ---\n>>  setup.c | 6 +++---\n>>  1 file changed, 3 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/setup.c b/setup.c\n>> index 8cc34186c..1a414c256 100644\n>> --- a/setup.c\n>> +++ b/setup.c\n>> @@ -27,26 +27,26 @@ static int abspath_part_inside_repo(char *path)\n>>  {\n>>  \tsize_t len;\n>>  \tsize_t wtlen;\n>>  \tchar *path0;\n>>  \tint off;\n>>  \tconst char *work_tree = get_git_work_tree();\n>>\n>>  \tif (!work_tree)\n>>  \t\treturn -1;\n>>  \twtlen = strlen(work_tree);\n>>  \tlen = strlen(path);\n>> -\toff = offset_1st_component(path);\n>>\n>> -\t/* check if work tree is already the prefix */\n>> -\tif (wtlen <= len && !strncmp(path, work_tree, wtlen)) {\n>> +\tif (wtlen > len || strncmp(path, work_tree, wtlen))\n>> +\t\toff = offset_1st_component(path);\n>> +\telse { /* check if work tree is already the prefix */\n>>  \t\tif (path[wtlen] == '/') {\n>>  \t\t\tmemmove(path, path + wtlen + 1, len - wtlen);\n>>  \t\t\treturn 0;\n>>  \t\t} else if (path[wtlen - 1] == '/' || path[wtlen] == '\\0') {\n>>  \t\t\t/* work tree is the root, or the whole path */\n>>  \t\t\tmemmove(path, path + wtlen, len - wtlen + 1);\n>>  \t\t\treturn 0;\n>>  \t\t}\n>>  \t\t/* work tree might match beginning of a symlink to work tree */\n>>  \t\toff = wtlen;\n>>  \t}\n>> --\n>> 2.15.1.433.g936d1b989\n"},{"id":"335297","messageId":"CABW-mG0R8aFmHc+ka_8ZtQ=iz8wnp9D6G64V25=2SS6rVYc=7A@mail.gmail.com","threadId":"47492","inReplyTo":"87zi67rkab.fsf@evledraar.gmail.com","subject":"Re: [PATCH] setup.c: move statement under condition","fromName":"Martin Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2017-12-25T17:52:44Z","receivedAt":"2017-12-25T17:53:12Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"On Sun, Dec 24, 2017 at 8:35 PM, Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n> On Sun, Dec 24 2017, Kevin Daudt jotted:\n>\n>> On Sun, Dec 24, 2017 at 12:15:35PM +0400, Vadim Petrov wrote:\n>>> Thank you for your replay.\n>>>\n>>> > I have to be honest: this commit message (including the subject) left me\n>>> > quite puzzled as to the intent of this patch.\n>>>\n>>> I still only learn English and correctly express my thoughts while somewhat difficult.\n>>>\n>>> > If you also have a background story that motivated you to work on this\n>>> > patch (for example, if you hit a huge performance bottleneck with some\n>>> > tool that fed thousands of absolute paths to Git that needed to be turned\n>>> > into paths relative to the worktree's top-level directory), I would\n>>> > definitely put that into the commit message, too, if I were you.\n>>>\n>>> I have no such reason. I just saw it and wanted to change it.\n>>\n>> A commit message contains the reason why this is a good change to make.\n>> It lets others know what problems it's trying to solve or what usecase\n>> it tries to satisfy.\n>>\n>> The commit message basically needs to convince others why the change is\n>> necessary / desired, now, and in the future.\n>>\n>> This will help others to follow your thought process and it gives you\n>> the possiblity to communicate trade-offs you made, all which cannot\n>> inferred from the patch.\n>>\n>> For simple changes, the motivation can be simple too.\n>\n> ...and a good example would be 6127ff63cf which introduced this logic\n> Vadim is trying to change, i.e. does this still retain the fix for\n> whatever issue that was trying to address?\n>\n> It's also good to CC the people who've directly worked on the code\n> you're changing in the past, I've CC'd Martin.\n>\n>> To make it concrete: You are talking about a condition. What condition?\n>> And you say that the previously obtained value will not be necessary.\n>> What do you do with that value then? Why does this change improve the\n>> situation?\n>>\n>> These are things you can state in your commit message.\n>>\n>> Hope this helps, Kevin\n>>\n>>> > Up until recently, we encouraged dropping the curly brackets from\n>>> > single-line statements, but apparently that changed. It is now no longer\n>>> > clear, and often left to the taste of the contributor. But not always.\n>>> > Sometimes we start a beautiful thread discussion the pros and cons of\n>>> > curly brackets in the middle of patch review, and drop altogether\n>>> > reviewing the actual patch.\n>>>\n>>> I was guided by the rule from the Documentation/CodingGuidelines:\n>>>      When there are multiple arms to a conditional and some of them\n>>>      require braces, enclose even a single line block in braces for\n>>>      consistency.\n>>> And other code from setup.c:\n>>>      from function get_common_dir:\n>>>              if (!has_common) {\n>>>                      /* several commands */\n>>>              } else {\n>>>                      free(candidate->work_tree);\n>>>              }\n>>>      from function get_common_dir_noenv:\n>>>              if (file_exists(path.buf)) {\n>>>                      /* several commands */\n>>>              } else {\n>>>                      strbuf_addstr(sb, gitdir);\n>>>              }\n>>>\n>>> > In short: I think your patch does the right thing, and I hope that you\n>>> > find my suggestions to improve the patch useful.\n>>>\n>>> I fixed the patch according to your suggestions.\n>>>\n>>>\n>>> Signed-off-by: Vadim Petrov <tridronet@yandex.ru>\n>>> ---\n>>>  setup.c | 6 +++---\n>>>  1 file changed, 3 insertions(+), 3 deletions(-)\n>>>\n>>> diff --git a/setup.c b/setup.c\n>>> index 8cc34186c..1a414c256 100644\n>>> --- a/setup.c\n>>> +++ b/setup.c\n>>> @@ -27,26 +27,26 @@ static int abspath_part_inside_repo(char *path)\n>>>  {\n>>>      size_t len;\n>>>      size_t wtlen;\n>>>      char *path0;\n>>>      int off;\n>>>      const char *work_tree = get_git_work_tree();\n>>>\n>>>      if (!work_tree)\n>>>              return -1;\n>>>      wtlen = strlen(work_tree);\n>>>      len = strlen(path);\n>>> -    off = offset_1st_component(path);\n>>>\n>>> -    /* check if work tree is already the prefix */\n>>> -    if (wtlen <= len && !strncmp(path, work_tree, wtlen)) {\n>>> +    if (wtlen > len || strncmp(path, work_tree, wtlen))\n>>> +            off = offset_1st_component(path);\n>>> +    else { /* check if work tree is already the prefix */\n>>>              if (path[wtlen] == '/') {\n>>>                      memmove(path, path + wtlen + 1, len - wtlen);\n>>>                      return 0;\n>>>              } else if (path[wtlen - 1] == '/' || path[wtlen] == '\\0') {\n>>>                      /* work tree is the root, or the whole path */\n>>>                      memmove(path, path + wtlen, len - wtlen + 1);\n>>>                      return 0;\n>>>              }\n>>>              /* work tree might match beginning of a symlink to work tree */\n>>>              off = wtlen;\n>>>      }\n\nAs far as I can tell this retains existing functionality.\n\nIs this intended as just a style change or a speculative performance\nchange?\n\nSo the general concept is:\n\nx = fa();\n\nif (...) {\n  if (...) {\n    return 0;\n  }\n  x = fb();\n}\n\nbeing rewritten as\n\nif (...) {\n  if (...) {\n    return 0;\n  }\n  x = fb();\n} else {\n  x = fa();\n}\n\nor, in the last iteration\n\nif (!...) {\n  x = fa();\n} else {\n  if (...) {\n    return 0;\n  }\n  x = fb();\n}\n\nwhich (at least conceptually) avoids setting x unnecessarily when we do\nthe early return.\n\nI think the last iteration might suffer a bit from the condition\ninversion, since the comment feels a bit odd placed there at the\n\"} else {\" line, and if it were to be placed at the top, it would have\nto be negated \"check if work tree is NOT already the prefix\". Therefore\nI think the original or the first iteration might be a tad better from a\nreadability perspective.\n\n\n(Going down this path, We could potentially also remove the 'off'\nvariable completely, increment the 'path' pointer directly, and set\nthe 'path0' pointer before, not sure if that's a good idea though...)\n\n--\nMartin Erik Werner <martinerikwerner@gmail.com>\n"},{"id":"335382","messageId":"xmqqvagsm227.fsf@gitster.mtv.corp.google.com","threadId":"47492","inReplyTo":"alpine.DEB.2.21.1.1712232332000.406@MININT-6BKU6QN.europe.corp.microsoft.com","subject":"Re: [PATCH] setup.c: move statement under condition","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-27T18:57:20Z","receivedAt":"2017-12-27T18:57:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> I suppose that if the condition is fulfilled then the previously\n>> obtained value will not be necessary.\n>\n> I have to be honest: this commit message (including the subject) left me\n> quite puzzled as to the intent of this patch.\n>\n> Maybe something like this would have spared me that puzzlement:\n>\n> \tAvoid unnecessary offset_1st_component() when prefixing filenames\n>\n> \tIn the abspath_part_inside_repo() function that is called\n> \tsomewhere deep in the call-chain when prefixing paths, we\n> \tcalculate the offset of the first component, but under certain\n> \tcircumstances, the result is not even used.\n>\n> \tThis patch changes the code to avoid that.\n\nSensible.\n\n>> diff --git a/setup.c b/setup.c\n>> index 8cc34186c..1ce0189fa 100644\n>> --- a/setup.c\n>> +++ b/setup.c\n>> @@ -35,7 +35,6 @@ static int abspath_part_inside_repo(char *path)\n>>  \t\treturn -1;\n>>  \twtlen = strlen(work_tree);\n>>  \tlen = strlen(path);\n>> -\toff = offset_1st_component(path);\n>>  \n>>  \t/* check if work tree is already the prefix */\n>>  \tif (wtlen <= len && !strncmp(path, work_tree, wtlen)) {\n>> @@ -49,6 +48,8 @@ static int abspath_part_inside_repo(char *path)\n>>  \t\t}\n>>  \t\t/* work tree might match beginning of a symlink to work tree */\n>>  \t\toff = wtlen;\n>> +\t} else {\n>> +\t\toff = offset_1st_component(path);\n>>  \t}\n>\n> Up until recently, we encouraged dropping the curly brackets from\n> single-line statements, but apparently that changed. \n\nThat is not quite correct.  We do encourage \n\n\tif (...)\n\t\tsingle statement;\n\nwith or without\n\n\telse\n\t\tanother single statement;\n\nIOW, when both sides do not need curlies, we save vertical space.\nOn the other hand, when one side needs curlies, we tend to add to\nboth to make it easier to spot the correspondence, i.e.\n\n\tif (...) {\n\t\tcompond statement;\n\t\tcompond statement;\n\t} else {\n\t\tsingle statement;\n\t}\n\nor the other way around.\n\n\n\tif (...) {\n\t\tsingle statement;\n\t} else {\n\t\tcompond statement;\n\t\tcompond statement;\n\t}\n\n> However, we still encourage to put shorter alternative code paths\n> (i.e. the blocks after `if` and `else`) first, in your case:\n>\n> @@ -35,18 +35,19 @@ static int abspath_part_inside_repo(char *path)\n>  \t\treturn -1;\n>  \twtlen = strlen(work_tree);\n>  \tlen = strlen(path);\n> -\toff = offset_1st_component(path);\n>  \n>  \t/* check if work tree is already the prefix */\n> - \tif (wtlen <= len && !strncmp(path, work_tree, wtlen)) {\n> +\tif (wtlen > len || strncmp(path, work_tree, wtlen))\n> +\t\toff = offset_1st_component(path);\n> +\telse {\n>  \t\tif (path[wtlen] == '/') {\n>  \t\t\tmemmove(path, path + wtlen + 1, len - wtlen);\n>  \t\t\treturn 0;\n> \t\t} else if (path[wtlen - 1] == '/' || path[wtlen] == '\\0') {\n> \t\t\t/* work tree is the root, or the whole path */\n> \t\t\tmemmove(path, path + wtlen, len - wtlen + 1);\n> \t\t\treturn 0;\n> \t\t}\n> \t\t/* work tree might match beginning of a symlink to work * tree */\n> \t\toff = wtlen;\n> \t}\n\nThis also may allow you to further dedent the if/else chain and make\nthe result easier to follow.  I dunno.\n\nThanks for a review.\n"}]}