{"thread":{"id":"37328","subject":"[PATCH] unpack-tree.c: remove dead code","startedAt":"2014-08-11T19:44:28Z","lastAt":"2014-08-13T20:02:27Z","messageCount":15,"participants":["Stefan Beller","Junio C Hamano","Jonathan Nieder","Ronnie Sahlberg","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"247593","messageId":"1407786268-15249-1-git-send-email-stefanbeller@gmail.com","threadId":"37328","inReplyTo":null,"subject":"[PATCH] unpack-tree.c: remove dead code","fromName":"Stefan Beller","fromEmail":"stefanbeller@gmail.com","sentAt":"2014-08-11T19:44:28Z","receivedAt":"2014-08-11T19:44:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In line 1763 of unpack-tree.c we have a condition on the current tree\n\tif (current) {\n\t\t...\nWithin this block of code we can assume current to be non NULL, hence\nthe code after the statement in line 1796:\n\tif (current)\n\t\treturn ...\n\ncannot be reached.\n\nThe proposed patch here changes the order of the current tree and the\nnewtree part. I'm not sure if that's the right way to handle it.\n\nAll referenced lines have been introduced in the same commit\n076b0adc (2006-07-30, read-tree: move merge functions to the library),\nwhich was just moving the code around.\nThe outer condition on the current tree (now in line 1763) was introduced\nin c859600954df4c292e, June 2005, [PATCH] read-tree: save more user hassles during fast-forward.\nThe inner condition on the current tree was introduced in\nee6566e8d70da682ac4926d, Sept. 2005, [PATCH] Rewrite read-tree\n\nThis issue was found by coverity, Id:290002\n\nSigned-off-by: Stefan Beller \n---\n unpack-trees.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex c6aa8fb..e6d37ff 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1793,11 +1793,10 @@ int twoway_merge(const struct cache_entry * const *src,\n \t\t\t/* all other failures */\n \t\t\tif (oldtree)\n \t\t\t\treturn o->gently ? -1 : reject_merge(oldtree, o);\n-\t\t\tif (current)\n-\t\t\t\treturn o->gently ? -1 : reject_merge(current, o);\n \t\t\tif (newtree)\n \t\t\t\treturn o->gently ? -1 : reject_merge(newtree, o);\n-\t\t\treturn -1;\n+\t\t\t/* current is definitely exists here */\n+\t\t\treturn o->gently ? -1 : reject_merge(current, o);\n \t\t}\n \t}\n \telse if (newtree) {\n-- \n2.1.0.rc2\n"},{"id":"247616","messageId":"xmqqha1h60fy.fsf@gitster.dls.corp.google.com","threadId":"37328","inReplyTo":"1407786268-15249-1-git-send-email-stefanbeller@gmail.com","subject":"Re: [PATCH] unpack-tree.c: remove dead code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-12T18:13:21Z","receivedAt":"2014-08-12T18:13:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> In line 1763 of unpack-tree.c we have a condition on the current tree\n> \tif (current) {\n> \t\t...\n> Within this block of code we can assume current to be non NULL, hence\n> the code after the statement in line 1796:\n> \tif (current)\n> \t\treturn ...\n>\n> cannot be reached.\n>\n> The proposed patch here changes the order of the current tree and the\n> newtree part. I'm not sure if that's the right way to handle it.\n\nIf the existing code decides to reject the merge and falls into that\ncode path, src[0] aka current is not NULL at that point as you\nnoticed, and we would call reject_merge(current, o); we would keep\ndoing so after this \"remove dead code\" patch is applied.\n\nIf you remove the dead code, which are the inner check for current,\nreject_merge() call with newtree and the final fallback of returning\n-1, then it would be a faithful \"remove dead code without changing\nanything else\" update.\n\nHaving said that, I think current/newtree/oldtree are used in the\ncall to reject_merge() *only* for their path aka ce->name, and they\nall point at the same name (there is no rename funkies here); hence\n\"all other failures\" code path should just rely on current always\nbeing present and become something like this instead:\n\n\t\t/* 20 or 21 */\n ...\n\t} else if (o->gently) {\n \treturn -1;\n\t} else {\n \treturn reject_merge(current, o);\n\t}\n\nThanks.\n"},{"id":"247623","messageId":"1407878107-22850-1-git-send-email-stefanbeller@gmail.com","threadId":"37328","inReplyTo":"xmqqha1h60fy.fsf@gitster.dls.corp.google.com","subject":"[PATCH] unpack-tree.c: remove dead code","fromName":"Stefan Beller","fromEmail":"stefanbeller@gmail.com","sentAt":"2014-08-12T21:15:07Z","receivedAt":"2014-08-12T21:15:07Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In line 1763 of unpack-tree.c we have a condition on the current tree\n\tif (current) {\n\t\t...\nWithin this block of code we can assume current to be non NULL, hence\nthe code after the statement in line 1796:\n\tif (current)\n\t\treturn ...\n\ncannot be reached.\n\ncurrent/newtree/oldtree are used in the\ncall to reject_merge() *only* for their path aka ce->name, and they\nall point at the same name (there is no rename funkies here); hence\n\"all other failures\" code path should just rely on current always\nbeing present.\n\nAll referenced lines have been introduced in the same commit\n076b0adc (2006-07-30, read-tree: move merge functions to the library),\nwhich was just moving the code around.\nThe outer condition on the current tree (now in line 1763) was introduced\nin c859600954df4c292e, June 2005, [PATCH] read-tree: save more user hassles during fast-forward.\nThe inner condition on the current tree was introduced in\nee6566e8d70da682ac4926d, Sept. 2005, [PATCH] Rewrite read-tree\n\nThis issue was found by coverity, Id:290002\n\nSigned-off-by: Stefan Beller \nHelped-by: Junio C Hamano \n---\n unpack-trees.c | 12 ++++--------\n 1 file changed, 4 insertions(+), 8 deletions(-)\n\nDid I understand you right, when changing to this one?\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex c6aa8fb..42ee84e 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1789,15 +1789,11 @@ int twoway_merge(const struct cache_entry * const *src,\n \t\t\t/* 20 or 21 */\n \t\t\treturn merged_entry(newtree, current, o);\n \t\t}\n+\t\telse if (o->gently) {\n+\t\t\treturn -1 ;\n+\t\t}\n \t\telse {\n-\t\t\t/* all other failures */\n-\t\t\tif (oldtree)\n-\t\t\t\treturn o->gently ? -1 : reject_merge(oldtree, o);\n-\t\t\tif (current)\n-\t\t\t\treturn o->gently ? -1 : reject_merge(current, o);\n-\t\t\tif (newtree)\n-\t\t\t\treturn o->gently ? -1 : reject_merge(newtree, o);\n-\t\t\treturn -1;\n+\t\t\treject_merge(current, o);\n \t\t}\n \t}\n \telse if (newtree) {\n-- \n2.1.0.rc2\n"},{"id":"247631","messageId":"xmqqmwb94a8l.fsf@gitster.dls.corp.google.com","threadId":"37328","inReplyTo":"1407878107-22850-1-git-send-email-stefanbeller@gmail.com","subject":"Re: [PATCH] unpack-tree.c: remove dead code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-12T22:24:42Z","receivedAt":"2014-08-12T22:24:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> In line 1763 of unpack-tree.c we have a condition on the current tree\n> \tif (current) {\n> \t\t...\n> Within this block of code we can assume current to be non NULL, hence\n> the code after the statement in line 1796:\n> \tif (current)\n> \t\treturn ...\n>\n> cannot be reached.\n>\n> current/newtree/oldtree are used in the\n> call to reject_merge() *only* for their path aka ce->name, and they\n> all point at the same name (there is no rename funkies here); hence\n> \"all other failures\" code path should just rely on current always\n> being present.\n>\n> All referenced lines have been introduced in the same commit\n> 076b0adc (2006-07-30, read-tree: move merge functions to the library),\n> which was just moving the code around.\n> The outer condition on the current tree (now in line 1763) was introduced\n> in c859600954df4c292e, June 2005, [PATCH] read-tree: save more user hassles during fast-forward.\n> The inner condition on the current tree was introduced in\n> ee6566e8d70da682ac4926d, Sept. 2005, [PATCH] Rewrite read-tree\n>\n> This issue was found by coverity, Id:290002\n>\n> Signed-off-by: Stefan Beller \n> Helped-by: Junio C Hamano \n> ---\n> unpack-trees.c | 12 ++++--------\n> 1 file changed, 4 insertions(+), 8 deletions(-)\n>\n> Did I understand you right, when changing to this one?\n\nSomething like that. I've already pushed out the original with a\ntentative \"SQUASH???\" on top for today's integration; I'll try to\nremember replacing them with this version.\n\nThanks.\n\n>\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index c6aa8fb..42ee84e 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -1789,15 +1789,11 @@ int twoway_merge(const struct cache_entry * const *src,\n> \t\t\t/* 20 or 21 */\n> \t\t\treturn merged_entry(newtree, current, o);\n> \t\t}\n> +\t\telse if (o->gently) {\n> +\t\t\treturn -1 ;\n> +\t\t}\n> \t\telse {\n> -\t\t\t/* all other failures */\n> -\t\t\tif (oldtree)\n> -\t\t\t\treturn o->gently ? -1 : reject_merge(oldtree, o);\n> -\t\t\tif (current)\n> -\t\t\t\treturn o->gently ? -1 : reject_merge(current, o);\n> -\t\t\tif (newtree)\n> -\t\t\t\treturn o->gently ? -1 : reject_merge(newtree, o);\n> -\t\t\treturn -1;\n> +\t\t\treject_merge(current, o);\n> \t\t}\n> \t}\n> \telse if (newtree) {\n"},{"id":"247634","messageId":"20140812235731.GD24621@google.com","threadId":"37328","inReplyTo":"1407878107-22850-1-git-send-email-stefanbeller@gmail.com","subject":"[PATCH 0/3] Re: [PATCH] unpack-tree.c: remove dead code","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-08-12T23:57:31Z","receivedAt":"2014-08-12T23:57:31Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> In line 1763 of unpack-tree.c we have a condition on the current tree\n[...]\n\nThe description is describing why the patch is *correct* (i.e., not\ngoing to introduce a bug), while what the reader wants to know is why\nthe change is *desirable*.\n\nIs this about making the code more readable, or robust, or suppressing\na static analysis error, or something else? What did the user or\nreader want to do that they couldn't do before and now can after this\npatch?\n\n[...]\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -1789,15 +1789,11 @@ int twoway_merge(const struct cache_entry * const *src,\n> \t\t\t/* 20 or 21 */\n> \t\t\treturn merged_entry(newtree, current, o);\n> \t\t}\n> +\t\telse if (o->gently) {\n> +\t\t\treturn -1 ;\n> +\t\t}\n\n(not about this patch) Elsewhere git uses the 'cuddled else':\n\n\t\tif (foo) {\n\t\t\t...\n\t\t} else if (bar) {\n\t\t\t...\n\t\t} else {\n\t\t\t...\n\t\t}\n\nThat stylefix would be a topic for a different patch, though.\n\n> \t\telse {\n> -\t\t\t/* all other failures */\n> -\t\t\tif (oldtree)\n> -\t\t\t\treturn o->gently ? -1 : reject_merge(oldtree, o);\n> -\t\t\tif (current)\n> -\t\t\t\treturn o->gently ? -1 : reject_merge(current, o);\n> -\t\t\tif (newtree)\n> -\t\t\t\treturn o->gently ? -1 : reject_merge(newtree, o);\n> -\t\t\treturn -1;\n\nDoes the static analysis tool support comments like\n\n\t\t\tif (oldtree)\n\t\t\t\t...\n\t\t\tif (current)\n\t\t\t\t...\n\t\t\t...\n\n\t\t\t/* not reached */\n\t\t\treturn -1;\n\n? That might be the simplest minimally invasive fix for what coverity\npointed out.\n\nNow that we're looking there, though, it's worth understanding why we\ndo the 'if oldtree exists, use it, else fall back to, etc' thing. Was\nthis meant as futureproofing in case commands like 'git checkout' want\nto do rename detection some day?\n\nEverywhere else in the file that reject_merge is used, it is as\n\n\treturn o->gently ? -1 : reject_merge(..., o);\n\nThe one exception is\n\n\t!current &&\n\toldtree &&\n\tnewtree &&\n\toldtree != newtree &&\n\t!initial_checkout\n\n(#17), which seems like a bug (it should have the same check). Would\nit make sense to inline the o->gently check into reject_merge so callers\ndon't have to care?\n\nIn that spirit, I suspect the simplest fix would be\n\n\t\telse\n\t\t\treturn o->gently ? -1 : reject_merge(current, o);\n\nand then all calls could be replaced in a followup patch.\n\nSensible?\n\nThanks,\n\nJonathan Nieder (2):\n unpack-trees: use 'cuddled' style for if-else cascade\n checkout -m: attempt merge when deletion of path was staged\n\nStefan Beller (1):\n unpack-trees: simplify 'all other failures' case\n\n unpack-trees.c | 31 ++++++++++---------------------\n 1 file changed, 10 insertions(+), 21 deletions(-)\n"},{"id":"247635","messageId":"20140812235931.GE24621@google.com","threadId":"37328","inReplyTo":"20140812235731.GD24621@google.com","subject":"[PATCH 1/3] unpack-trees: simplify 'all other failures' case","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-08-12T23:59:31Z","receivedAt":"2014-08-12T23:59:31Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Stefan Beller \n\nIn the 'if (current)' block of twoway_merge, we handle the boring\nerrors by checking if the entry from the old tree, current index, and\nnew tree are present, to get a pathname for the error message from one\nof them:\n\n\tif (oldtree)\n\t\treturn o->gently ? -1 : reject_merge(oldtree, o);\n\tif (current)\n\t\treturn o->gently ? -1 : reject_merge(current, o);\n\tif (newtree)\n\t\treturn o->gently ? -1 : reject_merge(newtree, o);\n\treturn -1;\n\nSince this is guarded by 'if (current)', the second test is guaranteed\nto succeed. Moreover, any of the three entries, if present, would\nhave the same path because there is no rename detection in this code\npath. Even if some day in the future the entries' paths differ, the\n'current' path used in the index and worktree would presumably be the\nmost recognizable for the end user.\n\nSimplify by just using 'current'.\n\nNoticed by coverity, Id:290002\n\nSigned-off-by: Stefan Beller \nImproved-by: Junio C Hamano \nSigned-off-by: Jonathan Nieder \n---\n unpack-trees.c | 12 ++----------\n 1 file changed, 2 insertions(+), 10 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex ad3e9a0..f4a9aa9 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1791,16 +1791,8 @@ int twoway_merge(const struct cache_entry * const *src,\n \t\t\t/* 20 or 21 */\n \t\t\treturn merged_entry(newtree, current, o);\n \t\t}\n-\t\telse {\n-\t\t\t/* all other failures */\n-\t\t\tif (oldtree)\n-\t\t\t\treturn o->gently ? -1 : reject_merge(oldtree, o);\n-\t\t\tif (current)\n-\t\t\t\treturn o->gently ? -1 : reject_merge(current, o);\n-\t\t\tif (newtree)\n-\t\t\t\treturn o->gently ? -1 : reject_merge(newtree, o);\n-\t\t\treturn -1;\n-\t\t}\n+\t\telse\n+\t\t\treturn o->gently ? -1 : reject_merge(current, o);\n \t}\n \telse if (newtree) {\n \t\tif (oldtree && !o->initial_checkout) {\n-- \n"},{"id":"247636","messageId":"20140813000045.GF24621@google.com","threadId":"37328","inReplyTo":"20140812235731.GD24621@google.com","subject":"[PATCH 2/3] unpack-trees: use 'cuddled' style for if-else cascade","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-08-13T00:00:45Z","receivedAt":"2014-08-13T00:00:45Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Match the predominant style in git by following K&R style for if/else\ncascades. Documentation/CodingStyle from linux.git explains:\n\n Note that the closing brace is empty on a line of its own, _except_ in\n the cases where it is followed by a continuation of the same statement,\n ie a \"while\" in a do-statement or an \"else\" in an if-statement, like\n this:\n\n\tif (x == y) {\n\t\t..\n\t} else if (x > y) {\n\t\t...\n\t} else {\n\t\t....\n\t}\n\n Rationale: K&R.\n\n Also, note that this brace-placement also minimizes the number of empty\n (or almost empty) lines, without any loss of readability.\n\nSigned-off-by: Jonathan Nieder \n---\n unpack-trees.c | 12 ++++--------\n 1 file changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex f4a9aa9..187b15b 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1771,8 +1771,7 @@ int twoway_merge(const struct cache_entry * const *src,\n \t\t\t\t\treturn merged_entry(newtree, current, o);\n \t\t\t}\n \t\t\treturn o->gently ? -1 : reject_merge(current, o);\n-\t\t}\n-\t\telse if ((!oldtree && !newtree) || /* 4 and 5 */\n+\t\t} else if ((!oldtree && !newtree) || /* 4 and 5 */\n \t\t\t (!oldtree && newtree &&\n \t\t\t same(current, newtree)) || /* 6 and 7 */\n \t\t\t (oldtree && newtree &&\n@@ -1781,17 +1780,14 @@ int twoway_merge(const struct cache_entry * const *src,\n \t\t\t !same(oldtree, newtree) && /* 18 and 19 */\n \t\t\t same(current, newtree))) {\n \t\t\treturn keep_entry(current, o);\n-\t\t}\n-\t\telse if (oldtree && !newtree && same(current, oldtree)) {\n+\t\t} else if (oldtree && !newtree && same(current, oldtree)) {\n \t\t\t/* 10 or 11 */\n \t\t\treturn deleted_entry(oldtree, current, o);\n-\t\t}\n-\t\telse if (oldtree && newtree &&\n+\t\t} else if (oldtree && newtree &&\n \t\t\t same(current, oldtree) && !same(current, newtree)) {\n \t\t\t/* 20 or 21 */\n \t\t\treturn merged_entry(newtree, current, o);\n-\t\t}\n-\t\telse\n+\t\t} else\n \t\t\treturn o->gently ? -1 : reject_merge(current, o);\n \t}\n \telse if (newtree) {\n-- \n"},{"id":"247637","messageId":"20140813000317.GG24621@google.com","threadId":"37328","inReplyTo":"20140812235731.GD24621@google.com","subject":"[PATCH 3/3] checkout -m: attempt merge when deletion of path was staged","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-08-13T00:03:18Z","receivedAt":"2014-08-13T00:03:18Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"twoway_merge() is missing an o->gently check in the case where a file\nthat needs to be modified is missing from the index but present in the\nold and new trees. As a result, in this case 'git checkout -m' errors\nout instead of trying to perform a merge.\n\nFix it by checking o->gently. While at it, inline the o->gently check\ninto reject_merge to prevent future call sites from making the same\nmistake.\n\nNoticed by code inspection. The motivating case hasn't been tested.\n\nSigned-off-by: Jonathan Nieder \n---\nThis is the most iffy of the three patches, mostly because I was too\nlazy to write a test. I believe it's safe as-is nonetheless.\n\nThanks for reading.\n\n unpack-trees.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 187b15b..6c45af7 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1178,7 +1178,8 @@ return_failed:\n static int reject_merge(const struct cache_entry *ce,\n \t\t\tstruct unpack_trees_options *o)\n {\n-\treturn add_rejected_path(o, ERROR_WOULD_OVERWRITE, ce->name);\n+\treturn o->gently ? -1 :\n+\t\tadd_rejected_path(o, ERROR_WOULD_OVERWRITE, ce->name);\n }\n \n static int same(const struct cache_entry *a, const struct cache_entry *b)\n@@ -1633,7 +1634,7 @@ int threeway_merge(const struct cache_entry * const *stages,\n \t/* #14, #14ALT, #2ALT */\n \tif (remote && !df_conflict_head && head_match && !remote_match) {\n \t\tif (index && !same(index, remote) && !same(index, head))\n-\t\t\treturn o->gently ? -1 : reject_merge(index, o);\n+\t\t\treturn reject_merge(index, o);\n \t\treturn merged_entry(remote, index, o);\n \t}\n \t/*\n@@ -1641,7 +1642,7 @@ int threeway_merge(const struct cache_entry * const *stages,\n \t * make sure that it matches head.\n \t */\n \tif (index && !same(index, head))\n-\t\treturn o->gently ? -1 : reject_merge(index, o);\n+\t\treturn reject_merge(index, o);\n \n \tif (head) {\n \t\t/* #5ALT, #15 */\n@@ -1770,7 +1771,7 @@ int twoway_merge(const struct cache_entry * const *src,\n \t\t\t\telse\n \t\t\t\t\treturn merged_entry(newtree, current, o);\n \t\t\t}\n-\t\t\treturn o->gently ? -1 : reject_merge(current, o);\n+\t\t\treturn reject_merge(current, o);\n \t\t} else if ((!oldtree && !newtree) || /* 4 and 5 */\n \t\t\t (!oldtree && newtree &&\n \t\t\t same(current, newtree)) || /* 6 and 7 */\n@@ -1788,7 +1789,7 @@ int twoway_merge(const struct cache_entry * const *src,\n \t\t\t/* 20 or 21 */\n \t\t\treturn merged_entry(newtree, current, o);\n \t\t} else\n-\t\t\treturn o->gently ? -1 : reject_merge(current, o);\n+\t\t\treturn reject_merge(current, o);\n \t}\n \telse if (newtree) {\n \t\tif (oldtree && !o->initial_checkout) {\n-- \n"},{"id":"247638","messageId":"CAPc5daV-p25_W9+3_HsOZ-nsab86JNVH0mABQJjX=HX6Kvjenw@mail.gmail.com","threadId":"37328","inReplyTo":"20140813000317.GG24621@google.com","subject":"Re: [PATCH 3/3] checkout -m: attempt merge when deletion of path was staged","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-13T00:38:04Z","receivedAt":"2014-08-13T00:38:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Tue, Aug 12, 2014 at 5:03 PM, Jonathan Nieder wrote:\n> twoway_merge() is missing an o->gently check in the case where a file\n> that needs to be modified is missing from the index but present in the\n> old and new trees. As a result, in this case 'git checkout -m' errors\n> out instead of trying to perform a merge.\n>\n> Fix it by checking o->gently. While at it, inline the o->gently check\n> into reject_merge to prevent future call sites from making the same\n> mistake.\n>\n> Noticed by code inspection. The motivating case hasn't been tested.\n\nThat sounds sloppy X-<. I may comment more after figuring out\nwhat _other_ reject_merge() caller that does not appear in the\npatch would change its behaviour with this patch.\n\n side note: of course, if this were two patches, one that adds the\n same o->gently ? -1 : reject thing to places where they forget to\n do so, and the other that moves the gently thing to reject helper,\n then we can read all the necessary information to judge the change\n in the patch ;-)\n\nThanks.\n\n>\n> Signed-off-by: Jonathan Nieder \n> ---\n> This is the most iffy of the three patches, mostly because I was too\n> lazy to write a test. I believe it's safe as-is nonetheless.\n>\n> Thanks for reading.\n>\n> unpack-trees.c | 11 ++++++-----\n> 1 file changed, 6 insertions(+), 5 deletions(-)\n>\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 187b15b..6c45af7 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -1178,7 +1178,8 @@ return_failed:\n> static int reject_merge(const struct cache_entry *ce,\n> struct unpack_trees_options *o)\n> {\n> - return add_rejected_path(o, ERROR_WOULD_OVERWRITE, ce->name);\n> + return o->gently ? -1 :\n> + add_rejected_path(o, ERROR_WOULD_OVERWRITE, ce->name);\n> }\n>\n> static int same(const struct cache_entry *a, const struct cache_entry *b)\n> @@ -1633,7 +1634,7 @@ int threeway_merge(const struct cache_entry * const *stages,\n> /* #14, #14ALT, #2ALT */\n> if (remote && !df_conflict_head && head_match && !remote_match) {\n> if (index && !same(index, remote) && !same(index, head))\n> - return o->gently ? -1 : reject_merge(index, o);\n> + return reject_merge(index, o);\n> return merged_entry(remote, index, o);\n> }\n> /*\n> @@ -1641,7 +1642,7 @@ int threeway_merge(const struct cache_entry * const *stages,\n> * make sure that it matches head.\n> */\n> if (index && !same(index, head))\n> - return o->gently ? -1 : reject_merge(index, o);\n> + return reject_merge(index, o);\n>\n> if (head) {\n> /* #5ALT, #15 */\n> @@ -1770,7 +1771,7 @@ int twoway_merge(const struct cache_entry * const *src,\n> else\n> return merged_entry(newtree, current, o);\n> }\n> - return o->gently ? -1 : reject_merge(current, o);\n> + return reject_merge(current, o);\n> } else if ((!oldtree && !newtree) || /* 4 and 5 */\n> (!oldtree && newtree &&\n> same(current, newtree)) || /* 6 and 7 */\n> @@ -1788,7 +1789,7 @@ int twoway_merge(const struct cache_entry * const *src,\n> /* 20 or 21 */\n> return merged_entry(newtree, current, o);\n> } else\n> - return o->gently ? -1 : reject_merge(current, o);\n> + return reject_merge(current, o);\n> }\n> else if (newtree) {\n> if (oldtree && !o->initial_checkout) {\n> --\n"},{"id":"247642","messageId":"53EB0896.7060106@gmail.com","threadId":"37328","inReplyTo":"20140812235731.GD24621@google.com","subject":"Re: [PATCH 0/3] Re: [PATCH] unpack-tree.c: remove dead code","fromName":"Stefan Beller","fromEmail":"stefanbeller@gmail.com","sentAt":"2014-08-13T06:41:26Z","receivedAt":"2014-08-13T06:41:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 13.08.2014 01:57, Jonathan Nieder wrote:\n> Stefan Beller wrote:\n> \n>> In line 1763 of unpack-tree.c we have a condition on the current tree\n> [...]\n> \n> The description is describing why the patch is *correct* (i.e., not\n> going to introduce a bug), while what the reader wants to know is why\n> the change is *desirable*.\n\nIndeed. Thanks for the reminder!\n\n> \n> Is this about making the code more readable, or robust, or suppressing\n> a static analysis error, or something else? What did the user or\n> reader want to do that they couldn't do before and now can after this\n> patch?\n\nIn my opinion it's making the code easier to read as there are less\nlines of code with less conditionals.\nThe supression of a static code analysis warning is rather a desired\nside effect, but not the main reason for the patch.\n\n\n> \n> [...]\n>> --- a/unpack-trees.c\n>> +++ b/unpack-trees.c\n>> @@ -1789,15 +1789,11 @@ int twoway_merge(const struct cache_entry * const *src,\n>> \t\t\t/* 20 or 21 */\n>> \t\t\treturn merged_entry(newtree, current, o);\n>> \t\t}\n>> +\t\telse if (o->gently) {\n>> +\t\t\treturn -1 ;\n>> +\t\t}\n> \n> (not about this patch) Elsewhere git uses the 'cuddled else':\n\nYes, I intentionally used this style, as the surrounding code was\nusing this style. You already added the reformatting follow up patch,\nthanks!\n\n> \n> \t\tif (foo) {\n> \t\t\t...\n> \t\t} else if (bar) {\n> \t\t\t...\n> \t\t} else {\n> \t\t\t...\n> \t\t}\n> \n> That stylefix would be a topic for a different patch, though.\n> \n>> \t\telse {\n>> -\t\t\t/* all other failures */\n>> -\t\t\tif (oldtree)\n>> -\t\t\t\treturn o->gently ? -1 : reject_merge(oldtree, o);\n>> -\t\t\tif (current)\n>> -\t\t\t\treturn o->gently ? -1 : reject_merge(current, o);\n>> -\t\t\tif (newtree)\n>> -\t\t\t\treturn o->gently ? -1 : reject_merge(newtree, o);\n>> -\t\t\treturn -1;\n> \n> Does the static analysis tool support comments like\n> \n> \t\t\tif (oldtree)\n> \t\t\t\t...\n> \t\t\tif (current)\n> \t\t\t\t...\n> \t\t\t...\n> \n> \t\t\t/* not reached */\n> \t\t\treturn -1;\n> \n> ? That might be the simplest minimally invasive fix for what coverity\n> pointed out.\n\nI was looking for things like that, but either the\nextensive documentation is well hidden or there is only short\ntutorial-like documentation, which doesn't cover this case.\n\n\n> \n> Now that we're looking there, though, it's worth understanding why we\n> do the 'if oldtree exists, use it, else fall back to, etc' thing. Was\n> this meant as futureproofing in case commands like 'git checkout' want\n> to do rename detection some day?\n> \n> Everywhere else in the file that reject_merge is used, it is as\n> \n> \treturn o->gently ? -1 : reject_merge(..., o);\n> \n> The one exception is\n> \n> \t!current &&\n> \toldtree &&\n> \tnewtree &&\n> \toldtree != newtree &&\n> \t!initial_checkout\n> \n> (#17), which seems like a bug (it should have the same check). Would\n> it make sense to inline the o->gently check into reject_merge so callers\n> don't have to care?\n> \n> In that spirit, I suspect the simplest fix would be\n> \n> \t\telse\n> \t\t\treturn o->gently ? -1 : reject_merge(current, o);\n> \n> and then all calls could be replaced in a followup patch.\n> \n> Sensible?\n\nI need to read more code to follow.\n\nThanks for picking up my inital patch and improving. :)\nStefan\n\n> \n> Thanks,\n> \n> Jonathan Nieder (2):\n> unpack-trees: use 'cuddled' style for if-else cascade\n> checkout -m: attempt merge when deletion of path was staged\n> \n> Stefan Beller (1):\n> unpack-trees: simplify 'all other failures' case\n> \n> unpack-trees.c | 31 ++++++++++---------------------\n> 1 file changed, 10 insertions(+), 21 deletions(-)\n> \n"},{"id":"247668","messageId":"CAL=YDWnpiUOiCTrDfcXFUUJc+uP77kfNNoN3VZ+7g2i0fvXu2A@mail.gmail.com","threadId":"37328","inReplyTo":"20140813000045.GF24621@google.com","subject":"Re: [PATCH 2/3] unpack-trees: use 'cuddled' style for if-else cascade","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-08-13T14:52:13Z","receivedAt":"2014-08-13T14:52:13Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Tue, Aug 12, 2014 at 5:00 PM, Jonathan Nieder wrote:\n> Match the predominant style in git by following K&R style for if/else\n> cascades. Documentation/CodingStyle from linux.git explains:\n>\n> Note that the closing brace is empty on a line of its own, _except_ in\n> the cases where it is followed by a continuation of the same statement,\n> ie a \"while\" in a do-statement or an \"else\" in an if-statement, like\n> this:\n>\n> if (x == y) {\n> ..\n> } else if (x > y) {\n> ...\n> } else {\n> ....\n> }\n>\n> Rationale: K&R.\n>\n> Also, note that this brace-placement also minimizes the number of empty\n> (or almost empty) lines, without any loss of readability.\n>\n> Signed-off-by: Jonathan Nieder \n\nReviewed-by: Ronnie Sahlberg \n\n> ---\n> unpack-trees.c | 12 ++++--------\n> 1 file changed, 4 insertions(+), 8 deletions(-)\n>\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index f4a9aa9..187b15b 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -1771,8 +1771,7 @@ int twoway_merge(const struct cache_entry * const *src,\n> return merged_entry(newtree, current, o);\n> }\n> return o->gently ? -1 : reject_merge(current, o);\n> - }\n> - else if ((!oldtree && !newtree) || /* 4 and 5 */\n> + } else if ((!oldtree && !newtree) || /* 4 and 5 */\n> (!oldtree && newtree &&\n> same(current, newtree)) || /* 6 and 7 */\n> (oldtree && newtree &&\n> @@ -1781,17 +1780,14 @@ int twoway_merge(const struct cache_entry * const *src,\n> !same(oldtree, newtree) && /* 18 and 19 */\n> same(current, newtree))) {\n> return keep_entry(current, o);\n> - }\n> - else if (oldtree && !newtree && same(current, oldtree)) {\n> + } else if (oldtree && !newtree && same(current, oldtree)) {\n> /* 10 or 11 */\n> return deleted_entry(oldtree, current, o);\n> - }\n> - else if (oldtree && newtree &&\n> + } else if (oldtree && newtree &&\n> same(current, oldtree) && !same(current, newtree)) {\n> /* 20 or 21 */\n> return merged_entry(newtree, current, o);\n> - }\n> - else\n> + } else\n> return o->gently ? -1 : reject_merge(current, o);\n> }\n> else if (newtree) {\n> --\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"247678","messageId":"xmqqzjf82sc7.fsf@gitster.dls.corp.google.com","threadId":"37328","inReplyTo":"20140813000317.GG24621@google.com","subject":"Re: [PATCH 3/3] checkout -m: attempt merge when deletion of path was staged","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-13T17:48:56Z","receivedAt":"2014-08-13T17:48:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> twoway_merge() is missing an o->gently check in the case where a file\n> that needs to be modified is missing from the index but present in the\n> old and new trees. As a result, in this case 'git checkout -m' errors\n> out instead of trying to perform a merge.\n\nI see two hunks in threeway_merge(), so two existing callers there\nwill not change their behaviour. Two hunks in twoway_merge() means\nthat among three existing callers in that function, this one at the\nend (not shown in your patch) changes its behaviour:\n\n\telse if (newtree) {\n\t\tif (oldtree && !o->initial_checkout) {\n\t\t\t/*\n\t\t\t * deletion of the path was staged;\n\t\t\t */\n\t\t\tif (same(oldtree, newtree))\n\t\t\t\treturn 1;\n\t\t\treturn reject_merge(oldtree, o);\n\t\t}\n\t\treturn merged_entry(newtree, current, o);\n\t}\n\treturn deleted_entry(oldtree, current, o);\n\n> This is the most iffy of the three patches, mostly because I was too\n> lazy to write a test.\n\nYou would trigger this codepath by jumping from an old revision to a\nnew revision after \"git rm $path\" any path that has been modified\nbetween the two. The only behaviour difference is that it will stop\nissuing an error message---the \"checkout -m\" will successfully switch\nbetween the revs and leave the index in a \"we modified, they removed\"\nconflicting state with or without your patch.\n"},{"id":"247679","messageId":"xmqqvbpw2p2x.fsf@gitster.dls.corp.google.com","threadId":"37328","inReplyTo":"xmqqzjf82sc7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/3] checkout -m: attempt merge when deletion of path was staged","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-13T18:59:18Z","receivedAt":"2014-08-13T18:59:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jonathan Nieder writes:\n>\n>> twoway_merge() is missing an o->gently check in the case where a file\n>> that needs to be modified is missing from the index but present in the\n>> old and new trees. As a result, in this case 'git checkout -m' errors\n>> out instead of trying to perform a merge.\n>\n> I see two hunks in threeway_merge(), so two existing callers there\n> will not change their behaviour. Two hunks in twoway_merge() means\n> that among three existing callers in that function, this one at the\n> end (not shown in your patch) changes its behaviour:\n>\n> \telse if (newtree) {\n> \t\tif (oldtree && !o->initial_checkout) {\n> \t\t\t/*\n> \t\t\t * deletion of the path was staged;\n> \t\t\t */\n> \t\t\tif (same(oldtree, newtree))\n> \t\t\t\treturn 1;\n> \t\t\treturn reject_merge(oldtree, o);\n> \t\t}\n> \t\treturn merged_entry(newtree, current, o);\n> \t}\n> \treturn deleted_entry(oldtree, current, o);\n>\n>> This is the most iffy of the three patches, mostly because I was too\n>> lazy to write a test.\n>\n> You would trigger this codepath by jumping from an old revision to a\n> new revision after \"git rm $path\" any path that has been modified\n> between the two. The only behaviour difference is that it will stop\n> issuing an error message---the \"checkout -m\" will successfully switch\n> between the revs and leave the index in a \"we modified, they removed\"\n> conflicting state with or without your patch.\n\nIOW, something like this perhaps?\n\ndiff --git a/t/t7201-co.sh b/t/t7201-co.sh\nindex 0c9ec0a..cedbb6a 100755\n--- a/t/t7201-co.sh\n+++ b/t/t7201-co.sh\n@@ -223,6 +223,23 @@ test_expect_success 'checkout --merge --conflict=diff3 ' '\n \ttest_cmp two expect\n '\n \n+test_expect_success 'switch to another branch while carrying a deletion' '\n+\n+\tgit checkout -f master && git reset --hard && git clean -f &&\n+\tgit rm two &&\n+\n+\ttest_must_fail git checkout simple 2>errs &&\n+\ttest_i18ngrep overwritten errs &&\n+\n+\tgit checkout --merge simple 2>errs &&\n+\t! test_i18ngrep overwritten errs &&\n+\tgit ls-files -u &&\n+\ttest_must_fail git cat-file -t :0:two &&\n+\ttest \"$(git cat-file -t :1:two)\" = blob &&\n+\ttest \"$(git cat-file -t :2:two)\" = blob &&\n+\ttest_must_fail git cat-file -t :3:two\n+'\n+\n test_expect_success 'checkout to detach HEAD (with advice declined)' '\n \n \tgit config advice.detachedHead false &&\n"},{"id":"247682","messageId":"53EBBCC3.4060309@kdbg.org","threadId":"37328","inReplyTo":"xmqqvbpw2p2x.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/3] checkout -m: attempt merge when deletion of path was staged","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-08-13T19:30:11Z","receivedAt":"2014-08-13T19:30:11Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 13.08.2014 20:59, schrieb Junio C Hamano:\n> diff --git a/t/t7201-co.sh b/t/t7201-co.sh\n> index 0c9ec0a..cedbb6a 100755\n> --- a/t/t7201-co.sh\n> +++ b/t/t7201-co.sh\n> @@ -223,6 +223,23 @@ test_expect_success 'checkout --merge --conflict=diff3 ' '\n> \ttest_cmp two expect\n> '\n> \n> +test_expect_success 'switch to another branch while carrying a deletion' '\n> +\n> +\tgit checkout -f master && git reset --hard && git clean -f &&\n> +\tgit rm two &&\n> +\n> +\ttest_must_fail git checkout simple 2>errs &&\n> +\ttest_i18ngrep overwritten errs &&\n> +\n> +\tgit checkout --merge simple 2>errs &&\n> +\t! test_i18ngrep overwritten errs &&\n\nThis must be written as\n\n\ttest_i18ngrep ! overwritten errs &&\n\n> +\tgit ls-files -u &&\n> +\ttest_must_fail git cat-file -t :0:two &&\n> +\ttest \"$(git cat-file -t :1:two)\" = blob &&\n> +\ttest \"$(git cat-file -t :2:two)\" = blob &&\n> +\ttest_must_fail git cat-file -t :3:two\n> +'\n> +\n> test_expect_success 'checkout to detach HEAD (with advice declined)' '\n> \n> \tgit config advice.detachedHead false &&\n> \n\nI see a few wrong usages in the current code base. Here's a fix.\n\n--- >8 ---\nSubject: [PATCH] tests: fix negated test_i18ngrep calls\n\nThe helper function test_i18ngrep pretends that it found the expected\nresults when it is running under GETTEXT_POISON. For this reason, it must\nnot be used negated like so\n\n ! test_i18ngrep foo bar\n\nbecause the test case would fail under GETTEXT_POISON. The function offers\na special syntax to test that a pattern is *not* found:\n\n test_i18ngrep ! foo bar\n\nConvert incorrect uses to this syntax.\n\nSigned-off-by: Johannes Sixt \n---\n t/t4018-diff-funcname.sh | 8 ++++----\n t/t9800-git-p4-basic.sh | 2 +-\n t/t9807-git-p4-submit.sh | 2 +-\n 3 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh\nindex 34591c2..1dbaa38 100755\n--- a/t/t4018-diff-funcname.sh\n+++ b/t/t4018-diff-funcname.sh\n@@ -52,15 +52,15 @@ do\n \t\techo \"*.java diff=$p\" >.gitattributes &&\n \t\ttest_expect_code 1 git diff --no-index \\\n \t\t\tA.java B.java 2>msg &&\n-\t\t! test_i18ngrep fatal msg &&\n-\t\t! test_i18ngrep error msg\n+\t\ttest_i18ngrep ! fatal msg &&\n+\t\ttest_i18ngrep ! error msg\n \t'\n \ttest_expect_success \"builtin $p wordRegex pattern compiles\" '\n \t\techo \"*.java diff=$p\" >.gitattributes &&\n \t\ttest_expect_code 1 git diff --no-index --word-diff \\\n \t\t\tA.java B.java 2>msg &&\n-\t\t! test_i18ngrep fatal msg &&\n-\t\t! test_i18ngrep error msg\n+\t\ttest_i18ngrep ! fatal msg &&\n+\t\ttest_i18ngrep ! error msg\n \t'\n done\n \ndiff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh\nindex 665607c..5b56212 100755\n--- a/t/t9800-git-p4-basic.sh\n+++ b/t/t9800-git-p4-basic.sh\n@@ -145,7 +145,7 @@ test_expect_success 'exit when p4 fails to produce marshaled output' '\n \t\ttest_expect_code 1 git p4 clone --dest=\"$git\" //depot >errs 2>&1\n \t) &&\n \tcat errs &&\n-\t! test_i18ngrep Traceback errs\n+\ttest_i18ngrep ! Traceback errs\n '\n \n # Hide a file from p4d, make sure we catch its complaint. This won't fail in\ndiff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh\nindex 7fab2ed..1f74a88 100755\n--- a/t/t9807-git-p4-submit.sh\n+++ b/t/t9807-git-p4-submit.sh\n@@ -404,7 +404,7 @@ test_expect_success 'submit --prepare-p4-only' '\n \t\tgit p4 submit --prepare-p4-only >out &&\n \t\ttest_i18ngrep \"prepared for submission\" out &&\n \t\ttest_i18ngrep \"must be deleted\" out &&\n-\t\t! test_i18ngrep \"everything below this line is just the diff\" out\n+\t\ttest_i18ngrep ! \"everything below this line is just the diff\" out\n \t) &&\n \t(\n \t\tcd \"$cli\" &&\n-- \n2.0.0.12.gbcf935e\n"},{"id":"247684","messageId":"xmqqr40k2m5o.fsf@gitster.dls.corp.google.com","threadId":"37328","inReplyTo":"53EBBCC3.4060309@kdbg.org","subject":"Re: [PATCH 3/3] checkout -m: attempt merge when deletion of path was staged","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-13T20:02:27Z","receivedAt":"2014-08-13T20:02:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 13.08.2014 20:59, schrieb Junio C Hamano:\n>> diff --git a/t/t7201-co.sh b/t/t7201-co.sh\n>> index 0c9ec0a..cedbb6a 100755\n>> --- a/t/t7201-co.sh\n>> +++ b/t/t7201-co.sh\n>> @@ -223,6 +223,23 @@ test_expect_success 'checkout --merge --conflict=diff3 ' '\n>> \ttest_cmp two expect\n>> '\n>> \n>> +test_expect_success 'switch to another branch while carrying a deletion' '\n>> +\n>> +\tgit checkout -f master && git reset --hard && git clean -f &&\n>> +\tgit rm two &&\n>> +\n>> +\ttest_must_fail git checkout simple 2>errs &&\n>> +\ttest_i18ngrep overwritten errs &&\n>> +\n>> +\tgit checkout --merge simple 2>errs &&\n>> +\t! test_i18ngrep overwritten errs &&\n>\n> This must be written as\n>\n> \ttest_i18ngrep ! overwritten errs &&\n\nOops. Thanks for spotting.\n\n> I see a few wrong usages in the current code base. Here's a fix.\n\nWill apply; thanks.\n\n> --- >8 ---\n> Subject: [PATCH] tests: fix negated test_i18ngrep calls\n>\n> The helper function test_i18ngrep pretends that it found the expected\n> results when it is running under GETTEXT_POISON. For this reason, it must\n> not be used negated like so\n>\n> ! test_i18ngrep foo bar\n>\n> because the test case would fail under GETTEXT_POISON. The function offers\n> a special syntax to test that a pattern is *not* found:\n>\n> test_i18ngrep ! foo bar\n>\n> Convert incorrect uses to this syntax.\n>\n> Signed-off-by: Johannes Sixt \n> ---\n> t/t4018-diff-funcname.sh | 8 ++++----\n> t/t9800-git-p4-basic.sh | 2 +-\n> t/t9807-git-p4-submit.sh | 2 +-\n> 3 files changed, 6 insertions(+), 6 deletions(-)\n>\n> diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh\n> index 34591c2..1dbaa38 100755\n> --- a/t/t4018-diff-funcname.sh\n> +++ b/t/t4018-diff-funcname.sh\n> @@ -52,15 +52,15 @@ do\n> \t\techo \"*.java diff=$p\" >.gitattributes &&\n> \t\ttest_expect_code 1 git diff --no-index \\\n> \t\t\tA.java B.java 2>msg &&\n> -\t\t! test_i18ngrep fatal msg &&\n> -\t\t! test_i18ngrep error msg\n> +\t\ttest_i18ngrep ! fatal msg &&\n> +\t\ttest_i18ngrep ! error msg\n> \t'\n> \ttest_expect_success \"builtin $p wordRegex pattern compiles\" '\n> \t\techo \"*.java diff=$p\" >.gitattributes &&\n> \t\ttest_expect_code 1 git diff --no-index --word-diff \\\n> \t\t\tA.java B.java 2>msg &&\n> -\t\t! test_i18ngrep fatal msg &&\n> -\t\t! test_i18ngrep error msg\n> +\t\ttest_i18ngrep ! fatal msg &&\n> +\t\ttest_i18ngrep ! error msg\n> \t'\n> done\n> \n> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh\n> index 665607c..5b56212 100755\n> --- a/t/t9800-git-p4-basic.sh\n> +++ b/t/t9800-git-p4-basic.sh\n> @@ -145,7 +145,7 @@ test_expect_success 'exit when p4 fails to produce marshaled output' '\n> \t\ttest_expect_code 1 git p4 clone --dest=\"$git\" //depot >errs 2>&1\n> \t) &&\n> \tcat errs &&\n> -\t! test_i18ngrep Traceback errs\n> +\ttest_i18ngrep ! Traceback errs\n> '\n> \n> # Hide a file from p4d, make sure we catch its complaint. This won't fail in\n> diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh\n> index 7fab2ed..1f74a88 100755\n> --- a/t/t9807-git-p4-submit.sh\n> +++ b/t/t9807-git-p4-submit.sh\n> @@ -404,7 +404,7 @@ test_expect_success 'submit --prepare-p4-only' '\n> \t\tgit p4 submit --prepare-p4-only >out &&\n> \t\ttest_i18ngrep \"prepared for submission\" out &&\n> \t\ttest_i18ngrep \"must be deleted\" out &&\n> -\t\t! test_i18ngrep \"everything below this line is just the diff\" out\n> +\t\ttest_i18ngrep ! \"everything below this line is just the diff\" out\n> \t) &&\n> \t(\n> \t\tcd \"$cli\" &&\n"}]}