{"thread":{"id":"26655","subject":"[BUG] git-am silently applying patches incorrectly","startedAt":"2011-03-04T13:40:19Z","lastAt":"2011-03-10T09:24:10Z","messageCount":34,"participants":["Colin Guthrie","Drew Northup","Junio C Hamano","Linus Torvalds","Alexander Miseler","Jonathan Nieder","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"162766","messageId":"4D70EBC3.3010400@colin.guthr.ie","threadId":"26655","inReplyTo":null,"subject":"[BUG] git-am silently applying patches incorrectly","fromName":"Colin Guthrie","fromEmail":"gmane@colin.guthr.ie","sentAt":"2011-03-04T13:40:19Z","receivedAt":"2011-03-04T13:40:19Z","isPatch":false,"sender":{"key":"gmane@colin.guthr.ie","avatar":null},"body":"Hi,\n\nWe recently found a bug in git-am 1.7.4.1 while working on PulseAudio.\n\nIt seems that it mis-applied a patch and did so silently without\ngenerating any warnings. It is reproducible and has been confirmed on\ndifferent distros.\n\nI make reference to the bug here:\nhttp://thread.gmane.org/gmane.comp.audio.pulseaudio.general/8840/focus=8857\n\nIn order to reproduce:\n\ngit clone http://git.0pointer.de/repos/pulseaudio.git\ngit co -b misapply 0ce3017b7407ab1c4094f7ce271bb68319a7eba7\ngit am 0002-alsa-mixer-add-required-any-and-required-for-enum-op.patch\n\n(I've attached the patch here for convenience).\n\nAs you can see, a call to the function check_required is incorrectly\nlocated after application. It is now recursive and uncalled. It should\nbe called in element_probe() function as per the original patch.\n\nThis is quite a serious problem. In our case it turned out to compile\nfine. So after reviewing the patch, I applied it and received no errors\nand thus thought no more about it. If this kind of thing can sneak by\nundetected, then it could introduce flaws quite easily :s\n\n\n\nFor reference, applying the patch manually with patch works fine and\ndoes not result in an error:\n\n$ cat 0002-alsa-mixer-add-required-any-and-required-for-enum-op.patch |\npatch -p1\npatching file src/modules/alsa/alsa-mixer.c\nHunk #1 succeeded at 1121 (offset 103 lines).\nHunk #2 succeeded at 1325 (offset 103 lines).\nHunk #3 succeeded at 1356 (offset 103 lines).\nHunk #4 succeeded at 1613 (offset 103 lines).\nHunk #5 succeeded at 1640 (offset 103 lines).\nHunk #6 succeeded at 1913 (offset 103 lines).\nHunk #7 succeeded at 1997 (offset 105 lines).\nHunk #8 succeeded at 2242 (offset 106 lines).\nHunk #9 succeeded at 2261 (offset 106 lines).\nHunk #10 succeeded at 2312 (offset 106 lines).\npatching file src/modules/alsa/alsa-mixer.h\nHunk #1 succeeded at 112 (offset 1 line).\nHunk #2 succeeded at 133 (offset 1 line).\nHunk #3 succeeded at 169 (offset 1 line).\npatching file src/modules/alsa/mixer/paths/analog-output.conf.common\n\n\nAll the best\n\nCol\n\n\n\n-- \n\nColin Guthrie\ngmane(at)colin.guthr.ie\nhttp://colin.guthr.ie/\n\nDay Job:\n Tribalogic Limited [http://www.tribalogic.net/]\nOpen Source:\n Mageia Contributor [http://www.mageia.org/]\n PulseAudio Hacker [http://www.pulseaudio.org/]\n Trac Hacker [http://trac.edgewall.org/]\n\n\n\n>From ae83e51c82a747332494bf10c245281e49343fe3 Mon Sep 17 00:00:00 2001\nFrom: David Henningsson \nDate: Mon, 20 Dec 2010 12:29:27 +0100\nSubject: [PATCH 2/6] alsa-mixer: add required-any and required-* for enum options\n\nNow you can add required-any to elements in a path and the path\nwill be valid as long as at least one of the elements are present.\nAlso you can have required, required-any and required-absent in\nelement options, causing a path to be unsupported if an option is\n(not) present (simplified example: to skip line in path if\n\"Capture source\" doesn't have a \"Line In\" option).\n\nSigned-off-by: David Henningsson \n---\n src/modules/alsa/alsa-mixer.c | 90 +++++++++++++++++---\n src/modules/alsa/alsa-mixer.h | 8 ++\n .../alsa/mixer/paths/analog-output.conf.common | 5 +\n 3 files changed, 91 insertions(+), 12 deletions(-)\n\ndiff --git a/src/modules/alsa/alsa-mixer.c b/src/modules/alsa/alsa-mixer.c\nindex eb50ae2..2c47319 100644\n--- a/src/modules/alsa/alsa-mixer.c\n+++ b/src/modules/alsa/alsa-mixer.c\n@@ -1018,6 +1018,38 @@ static int check_required(pa_alsa_element *e, snd_mixer_elem_t *me) {\n if (e->required_absent == PA_ALSA_REQUIRED_ANY && (has_switch || has_volume || has_enumeration))\n return -1;\n \n+ if (e->required_any != PA_ALSA_REQUIRED_IGNORE) {\n+ switch (e->required_any) {\n+ case PA_ALSA_REQUIRED_VOLUME:\n+ e->path->req_any_present |= (e->volume_use != PA_ALSA_VOLUME_IGNORE);\n+ break;\n+ case PA_ALSA_REQUIRED_SWITCH:\n+ e->path->req_any_present |= (e->switch_use != PA_ALSA_SWITCH_IGNORE);\n+ break;\n+ case PA_ALSA_REQUIRED_ENUMERATION:\n+ e->path->req_any_present |= (e->enumeration_use != PA_ALSA_ENUMERATION_IGNORE);\n+ break;\n+ case PA_ALSA_REQUIRED_ANY:\n+ e->path->req_any_present |=\n+ (e->volume_use != PA_ALSA_VOLUME_IGNORE) ||\n+ (e->switch_use != PA_ALSA_SWITCH_IGNORE) ||\n+ (e->enumeration_use != PA_ALSA_ENUMERATION_IGNORE);\n+ break;\n+ }\n+ }\n+\n+ if (e->enumeration_use == PA_ALSA_ENUMERATION_SELECT) {\n+ pa_alsa_option *o;\n+ PA_LLIST_FOREACH(o, e->options) {\n+ e->path->req_any_present |= (o->required_any != PA_ALSA_REQUIRED_IGNORE) &&\n+ (o->alsa_idx >= 0);\n+ if (o->required != PA_ALSA_REQUIRED_IGNORE && o->alsa_idx < 0)\n+ return -1;\n+ if (o->required_absent != PA_ALSA_REQUIRED_IGNORE && o->alsa_idx >= 0)\n+ return -1;\n+ }\n+ }\n+\n return 0;\n }\n \n@@ -1190,9 +1222,6 @@ static int element_probe(pa_alsa_element *e, snd_mixer_t *m) {\n \n }\n \n- if (check_required(e, me) < 0)\n- return -1;\n-\n if (e->switch_use == PA_ALSA_SWITCH_SELECT) {\n pa_alsa_option *o;\n \n@@ -1224,6 +1253,9 @@ static int element_probe(pa_alsa_element *e, snd_mixer_t *m) {\n }\n }\n \n+ if (check_required(e, me) < 0)\n+ return -1;\n+\n return 0;\n }\n \n@@ -1478,20 +1510,23 @@ static int element_parse_required(\n \n pa_alsa_path *p = userdata;\n pa_alsa_element *e;\n+ pa_alsa_option *o;\n pa_alsa_required_t req;\n \n pa_assert(p);\n \n- if (!(e = element_get(p, section, TRUE))) {\n+ e = element_get(p, section, TRUE);\n+ o = option_get(p, section);\n+ if (!e && !o) {\n pa_log(\"[%s:%u] Required makes no sense in '%s'\", filename, line, section);\n return -1;\n }\n \n if (pa_streq(rvalue, \"ignore\"))\n req = PA_ALSA_REQUIRED_IGNORE;\n- else if (pa_streq(rvalue, \"switch\"))\n+ else if (pa_streq(rvalue, \"switch\") && e)\n req = PA_ALSA_REQUIRED_SWITCH;\n- else if (pa_streq(rvalue, \"volume\"))\n+ else if (pa_streq(rvalue, \"volume\") && e)\n req = PA_ALSA_REQUIRED_VOLUME;\n else if (pa_streq(rvalue, \"enumeration\"))\n req = PA_ALSA_REQUIRED_ENUMERATION;\n@@ -1502,10 +1537,28 @@ static int element_parse_required(\n return -1;\n }\n \n- if (pa_streq(lvalue, \"required-absent\"))\n- e->required_absent = req;\n- else\n- e->required = req;\n+ if (pa_streq(lvalue, \"required-absent\")) {\n+ if (e)\n+ e->required_absent = req;\n+ if (o)\n+ o->required_absent = req;\n+ }\n+ else if (pa_streq(lvalue, \"required-any\")) {\n+ if (e) {\n+ e->required_any = req;\n+ e->path->has_req_any = TRUE;\n+ }\n+ if (o) {\n+ o->required_any = req;\n+ o->element->path->has_req_any = TRUE;\n+ }\n+ }\n+ else {\n+ if (e)\n+ e->required = req;\n+ if (o)\n+ o->required = req;\n+ }\n \n return 0;\n }\n@@ -1757,7 +1810,10 @@ static int element_verify(pa_alsa_element *e) {\n \n pa_assert(e);\n \n+// pa_log_debug(\"Element %s, path %s: r=%d, r-any=%d, r-abs=%d\", e->alsa_name, e->path->name, e->required, e->required_any, e->required_absent);\n if ((e->required != PA_ALSA_REQUIRED_IGNORE && e->required == e->required_absent) ||\n+ (e->required_any != PA_ALSA_REQUIRED_IGNORE && e->required_any == e->required_absent) ||\n+ (e->required_absent == PA_ALSA_REQUIRED_ANY && e->required_any != PA_ALSA_REQUIRED_IGNORE) ||\n (e->required_absent == PA_ALSA_REQUIRED_ANY && e->required != PA_ALSA_REQUIRED_IGNORE)) {\n pa_log(\"Element %s cannot be required and absent at the same time.\", e->alsa_name);\n return -1;\n@@ -1836,6 +1892,7 @@ pa_alsa_path* pa_alsa_path_new(const char *fname, pa_alsa_direction_t direction)\n { \"override-map.2\", element_parse_override_map, NULL, NULL },\n /* ... later on we might add override-map.3 and so on here ... */\n { \"required\", element_parse_required, NULL, NULL },\n+ { \"required-any\", element_parse_required, NULL, NULL },\n { \"required-absent\", element_parse_required, NULL, NULL },\n { \"direction\", element_parse_direction, NULL, NULL },\n { \"direction-try-other\", element_parse_direction_try_other, NULL, NULL },\n@@ -2079,11 +2136,13 @@ int pa_alsa_path_probe(pa_alsa_path *p, snd_mixer_t *m, pa_bool_t ignore_dB) {\n min_dB[t] += e->min_dB;\n max_dB[t] += e->max_dB;\n }\n- } else\n+ } else {\n /* Hmm, there's another element before us\n * which cannot do dB volumes, so we we need\n * to 'neutralize' this slider */\n e->volume_use = PA_ALSA_VOLUME_ZERO;\n+ pa_log_info(\"Zeroing volume of '%s' on path '%s'\", e->alsa_name, p->name);\n+ }\n }\n } else if (p->has_volume)\n /* We can't use this volume, so let's ignore it */\n@@ -2096,6 +2155,12 @@ int pa_alsa_path_probe(pa_alsa_path *p, snd_mixer_t *m, pa_bool_t ignore_dB) {\n p->has_mute = TRUE;\n }\n \n+ if (p->has_req_any && !p->req_any_present) {\n+ p->supported = FALSE;\n+ pa_log_debug(\"Skipping path '%s', none of required-any elements preset.\", p->name);\n+ return -1;\n+ }\n+\n path_drop_unsupported(p);\n path_make_options_unique(p);\n path_create_settings(p);\n@@ -2141,13 +2206,14 @@ void pa_alsa_element_dump(pa_alsa_element *e) {\n pa_alsa_option *o;\n pa_assert(e);\n \n- pa_log_debug(\"Element %s, direction=%i, switch=%i, volume=%i, enumeration=%i, required=%i, required_absent=%i, mask=0x%llx, n_channels=%u, override_map=%s\",\n+ pa_log_debug(\"Element %s, direction=%i, switch=%i, volume=%i, enumeration=%i, required=%i, required_any=%i, required_absent=%i, mask=0x%llx, n_channels=%u, override_map=%s\",\n e->alsa_name,\n e->direction,\n e->switch_use,\n e->volume_use,\n e->enumeration_use,\n e->required,\n+ e->required_any,\n e->required_absent,\n (long long unsigned) e->merged_mask,\n e->n_channels,\ndiff --git a/src/modules/alsa/alsa-mixer.h b/src/modules/alsa/alsa-mixer.h\nindex a0d4fcb..a6499b6 100644\n--- a/src/modules/alsa/alsa-mixer.h\n+++ b/src/modules/alsa/alsa-mixer.h\n@@ -111,6 +111,10 @@ struct pa_alsa_option {\n char *name;\n char *description;\n unsigned priority;\n+\n+ pa_alsa_required_t required;\n+ pa_alsa_required_t required_any;\n+ pa_alsa_required_t required_absent;\n };\n \n /* And element wraps one specific ALSA element. A series of elements *\n@@ -128,6 +132,7 @@ struct pa_alsa_element {\n pa_alsa_enumeration_use_t enumeration_use;\n \n pa_alsa_required_t required;\n+ pa_alsa_required_t required_any;\n pa_alsa_required_t required_absent;\n \n pa_bool_t override_map:1;\n@@ -163,6 +168,9 @@ struct pa_alsa_path {\n pa_bool_t has_mute:1;\n pa_bool_t has_volume:1;\n pa_bool_t has_dB:1;\n+ /* These two are used during probing only */\n+ pa_bool_t has_req_any:1;\n+ pa_bool_t req_any_present:1;\n \n long min_volume, max_volume;\n double min_dB, max_dB;\ndiff --git a/src/modules/alsa/mixer/paths/analog-output.conf.common b/src/modules/alsa/mixer/paths/analog-output.conf.common\nindex 6131da5..ffd1b41 100644\n--- a/src/modules/alsa/mixer/paths/analog-output.conf.common\n+++ b/src/modules/alsa/mixer/paths/analog-output.conf.common\n@@ -63,10 +63,15 @@\n ; # by the option name, resp. on/off if the element is a switch.\n ; name = ... # Logical name to use in the path identifier\n ; priority = ... # Priority if this is made into a device port\n+; required = ignore | enumeration | any # In this element, this option must exist or the path will be invalid. (\"any\" is an alias for \"enumeration\".)\n+; required-any = ignore | enumeration | any # In this element, either this or another option must exist (or an element)\n+; required-absent = ignore | enumeration | any # In this element, this option must not exist or the path will be invalid\n ;\n ; [Element ...] # For each element that we shall control\n ; required = ignore | switch | volume | enumeration | any # If set, require this element to be of this kind and available,\n ; # otherwise don't consider this path valid for the card\n+; required-any = ignore | switch | volume | enumeration | any # If set, at least one of the elements with required-any in this\n+; # path must be present, otherwise this path is invalid for the card\n ; required-absent = ignore | switch | volume # If set, require this element to not be of this kind and not\n ; # available, otherwise don't consider this path valid for the card\n ;\n-- \n1.7.1\n\n\n"},{"id":"162772","messageId":"1299255471.22002.15.camel@drew-northup.unet.maine.edu","threadId":"26655","inReplyTo":"4D70EBC3.3010400@colin.guthr.ie","subject":"Re: [BUG] git-am silently applying patches incorrectly","fromName":"Drew Northup","fromEmail":"drew.northup@maine.edu","sentAt":"2011-03-04T16:17:51Z","receivedAt":"2011-03-04T16:17:51Z","isPatch":false,"sender":{"key":"drew.northup@maine.edu","avatar":"https://avatars.githubusercontent.com/u/18331571?v=4"},"body":"\nOn Fri, 2011-03-04 at 13:40 +0000, Colin Guthrie wrote:\n> Hi,\n> \n> We recently found a bug in git-am 1.7.4.1 while working on PulseAudio.\n> \n> It seems that it mis-applied a patch and did so silently without\n> generating any warnings. It is reproducible and has been confirmed on\n> different distros.\n> \n> I make reference to the bug here:\n> http://thread.gmane.org/gmane.comp.audio.pulseaudio.general/8840/focus=8857\n> \n> In order to reproduce:\n> \n> git clone http://git.0pointer.de/repos/pulseaudio.git\n> git co -b misapply 0ce3017b7407ab1c4094f7ce271bb68319a7eba7\n> git am 0002-alsa-mixer-add-required-any-and-required-for-enum-op.patch\n> \n> (I've attached the patch here for convenience).\n\n> For reference, applying the patch manually with patch works fine and\n> does not result in an error:\n> \n> $ cat 0002-alsa-mixer-add-required-any-and-required-for-enum-op.patch |\n> patch -p1\n> patching file src/modules/alsa/alsa-mixer.c\n> Hunk #1 succeeded at 1121 (offset 103 lines).\n> Hunk #2 succeeded at 1325 (offset 103 lines).\n> Hunk #3 succeeded at 1356 (offset 103 lines).\n> Hunk #4 succeeded at 1613 (offset 103 lines).\n> Hunk #5 succeeded at 1640 (offset 103 lines).\n> Hunk #6 succeeded at 1913 (offset 103 lines).\n> Hunk #7 succeeded at 1997 (offset 105 lines).\n> Hunk #8 succeeded at 2242 (offset 106 lines).\n> Hunk #9 succeeded at 2261 (offset 106 lines).\n> Hunk #10 succeeded at 2312 (offset 106 lines).\n> patching file src/modules/alsa/alsa-mixer.h\n> Hunk #1 succeeded at 112 (offset 1 line).\n> Hunk #2 succeeded at 133 (offset 1 line).\n> Hunk #3 succeeded at 169 (offset 1 line).\n> patching file src/modules/alsa/mixer/paths/analog-output.conf.common\n\nDid you try removing the first line from the patch mbox file?\nIt seems to work just fine if you do that. \n\nThat first line is \"removed\" from the output of \"git format-patch\" when\nyou correctly import the mbox file into your mail client's drafts folder\nas described in the documentation. Then you send the mail created by\nimporting that draft.\nIf you just send the output of \"git format-patch\" untouched as an\nattachment you can expect problems.\n\n-- \n-Drew Northup\n________________________________________________\n\"As opposed to vegetable or mineral error?\"\n-John Pescatore, SANS NewsBites Vol. 12 Num. 59\n"},{"id":"162774","messageId":"4D711639.4070706@colin.guthr.ie","threadId":"26655","inReplyTo":"1299255471.22002.15.camel@drew-northup.unet.maine.edu","subject":"Re: [BUG] git-am silently applying patches incorrectly","fromName":"Colin Guthrie","fromEmail":"gmane@colin.guthr.ie","sentAt":"2011-03-04T16:41:29Z","receivedAt":"2011-03-04T16:41:29Z","isPatch":false,"sender":{"key":"gmane@colin.guthr.ie","avatar":null},"body":"'Twas brillig, and Drew Northup at 04/03/11 16:17 did gyre and gimble:\n> \n> On Fri, 2011-03-04 at 13:40 +0000, Colin Guthrie wrote:\n>> Hi,\n>>\n>> We recently found a bug in git-am 1.7.4.1 while working on PulseAudio.\n>>\n>> It seems that it mis-applied a patch and did so silently without\n>> generating any warnings. It is reproducible and has been confirmed on\n>> different distros.\n>>\n>> I make reference to the bug here:\n>> http://thread.gmane.org/gmane.comp.audio.pulseaudio.general/8840/focus=8857\n>>\n>> In order to reproduce:\n>>\n>> git clone http://git.0pointer.de/repos/pulseaudio.git\n>> git co -b misapply 0ce3017b7407ab1c4094f7ce271bb68319a7eba7\n>> git am 0002-alsa-mixer-add-required-any-and-required-for-enum-op.patch\n>>\n>> (I've attached the patch here for convenience).\n> \n>> For reference, applying the patch manually with patch works fine and\n>> does not result in an error:\n>>\n>> $ cat 0002-alsa-mixer-add-required-any-and-required-for-enum-op.patch |\n>> patch -p1\n>> patching file src/modules/alsa/alsa-mixer.c\n>> Hunk #1 succeeded at 1121 (offset 103 lines).\n>> Hunk #2 succeeded at 1325 (offset 103 lines).\n>> Hunk #3 succeeded at 1356 (offset 103 lines).\n>> Hunk #4 succeeded at 1613 (offset 103 lines).\n>> Hunk #5 succeeded at 1640 (offset 103 lines).\n>> Hunk #6 succeeded at 1913 (offset 103 lines).\n>> Hunk #7 succeeded at 1997 (offset 105 lines).\n>> Hunk #8 succeeded at 2242 (offset 106 lines).\n>> Hunk #9 succeeded at 2261 (offset 106 lines).\n>> Hunk #10 succeeded at 2312 (offset 106 lines).\n>> patching file src/modules/alsa/alsa-mixer.h\n>> Hunk #1 succeeded at 112 (offset 1 line).\n>> Hunk #2 succeeded at 133 (offset 1 line).\n>> Hunk #3 succeeded at 169 (offset 1 line).\n>> patching file src/modules/alsa/mixer/paths/analog-output.conf.common\n> \n> Did you try removing the first line from the patch mbox file?\n> It seems to work just fine if you do that. \n\nDo you mean the line:\n\n>From ae83e51c82a747332494bf10c245281e49343fe3 Mon Sep 17 00:00:00 2001\n\n?\n\nIf so, I removed that line and it still failed to apply correctly with\ngit am.\n\n> If you just send the output of \"git format-patch\" untouched as an\n> attachment you can expect problems.\n\nWow! I've never heard of this before... So you're saying it's actually\ninvalid to do a git format-patch and then a git am on the files it\ngenerates?\n\nIf that's the case, then I need to rethink a whole lot of things,\nincluding the way several distros deal with patch management in their\npackage VCSs.... I'm quite shocked by this! Can you point me to\nsomewhere in the docs that discusses this?\n\n\nI'd like to point out that \"patch\" is able to apply the exact same patch\nfine as noted above. To me this seems like a very serious bug in the way\nthat git-am deals with the application of the patch, but perhaps I'm\nmissing something.....\n\n\nCol\n\n\n\n-- \n\nColin Guthrie\ngmane(at)colin.guthr.ie\nhttp://colin.guthr.ie/\n\nDay Job:\n Tribalogic Limited [http://www.tribalogic.net/]\nOpen Source:\n Mageia Contributor [http://www.mageia.org/]\n PulseAudio Hacker [http://www.pulseaudio.org/]\n Trac Hacker [http://trac.edgewall.org/]\n"},{"id":"162777","messageId":"7vvczy7q4c.fsf@alter.siamese.dyndns.org","threadId":"26655","inReplyTo":"4D711639.4070706@colin.guthr.ie","subject":"Re: [BUG] git-am silently applying patches incorrectly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-04T17:27:31Z","receivedAt":"2011-03-04T17:27:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Colin Guthrie writes:\n\n>> If you just send the output of \"git format-patch\" untouched as an\n>> attachment you can expect problems.\n>\n> Wow! I've never heard of this before... So you're saying it's actually\n> invalid to do a git format-patch and then a git am on the files it\n> generates?\n\nI don't think you understand what Drew is saying.\n\nThe output from format-patch mimics mbox format already; it specifically\nwas designed so that \"format-patch --stdout | am\" pipeline would work\nwithout your doing anything funky.\n\nIf you include the output from format-patch in your MUA, however, the\nmessage your MUA will send out would look like:\n\n * From: ... you ...\n * Subject: Hi, I am sending a patch (the message typed to your MUA)\n * Date: ... date ...\n\n % From \n % From: ... author name output by format patch\n % Subject: [PATCH] ... first paragraph from commit log message ...\n\n . The second paragraph and what follows...\n . ---\n . patch\n\nIn the above illustration, the lines marked with \"*\" are what your MUA\nwould add as the header, and the ones marked with '%' are the headers\nformat-patch placed to make its output look like mbox. You are supposed\nto move the \"Subject: \" line marked with '%' to the Subject input field of\nyour MUA and drop all other lines marked with '%'.\n\nDrew is talking about the problem it causes to the recipient if you did\nnot do so, and left '%' lines in your MUA.\n"},{"id":"162780","messageId":"7vr5am7p30.fsf@alter.siamese.dyndns.org","threadId":"26655","inReplyTo":"4D70EBC3.3010400@colin.guthr.ie","subject":"Re: [BUG] git-am silently applying patches incorrectly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-04T17:49:55Z","receivedAt":"2011-03-04T17:49:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Colin Guthrie writes:\n\n> It seems that it mis-applied a patch and did so silently without\n> generating any warnings. It is reproducible and has been confirmed on\n> different distros.\n\nThe patch text instructs to move the check you have at around ll.1190-1199\nto around ll.1224-1230. Here are the relevant parts.\n\n@@ -1190,9 +1222,6 @@ static int element_probe(pa_alsa_element *e, snd_mixer_t *m) {\n \n }\n \n- if (check_required(e, me) < 0)\n- return -1;\n-\n if (e->switch_use == PA_ALSA_SWITCH_SELECT) {\n pa_alsa_option *o;\n \n@@ -1224,6 +1253,9 @@ static int element_probe(pa_alsa_element *e, snd_mixer_t *m) {\n }\n }\n \n+ if (check_required(e, me) < 0)\n+ return -1;\n+\n return 0;\n }\n\nThanks for a report.\n \nWe find the match for the first hunk (there is only a single callsite) and\ncorrectly remove it, but there are many places that match the preimage of\nthe second hunk (two blocks closed, blank line and then return 0 from the\nfunction). We chose to add it to at line 1156, instead of patch's choice\nof line 1359, presumably because we thought that is closer to the place\nthe patch tells us to (i.e. ll.1224-1230).\n\nI haven't looked at the offset logic in git-apply for a long time since\nLinus wrote its original version (I don't think the logic has changed very\nmuch since then), but I thought we are taking accumulated offsets into\naccount when we decide where the patch target should roughly correspond\nto. When we attempt to apply the second hunk, we have already found that\nthe line the patch says should be at l.1190 is actually at l.1296 (iow,\nthere are about 100 lines of new material above that the patch didn't\nexpect), so instead of trying to find the lines that matches the preimage\nof the second hunk at around l.1224, we _should_ be trying to find that at\naround l.1224+100---perhaps we are not doing that.\n"},{"id":"162784","messageId":"7vei6m7muw.fsf@alter.siamese.dyndns.org","threadId":"26655","inReplyTo":"7vr5am7p30.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] git-am silently applying patches incorrectly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-04T18:37:59Z","receivedAt":"2011-03-04T18:37:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> @@ -1224,6 +1253,9 @@ static int element_probe(pa_alsa_element *e, snd_mixer_t *m) {\n> }\n> }\n> \n> + if (check_required(e, me) < 0)\n> + return -1;\n> +\n> return 0;\n> }\n>\n> I haven't looked at the offset logic in git-apply for a long time since\n> Linus wrote its original version (I don't think the logic has changed very\n> much since then), but I thought we are taking accumulated offsets into\n> account when we decide where the patch target should roughly correspond\n> to. When we attempt to apply the second hunk, we have already found that\n> the line the patch says should be at l.1190 is actually at l.1296 (iow,\n> there are about 100 lines of new material above that the patch didn't\n> expect), so instead of trying to find the lines that matches the preimage\n> of the second hunk at around l.1224, we _should_ be trying to find that at\n> around l.1224+100---perhaps we are not doing that.\n\nI don't necessarily think mucking with the first location we try to apply\nusing the offset we found by applying the previous hunk is actually a good\nthing. With so many offset lines and multiple places that a hunk can\napply to make the patch application unreliable, that change would be\nrobbing Peter to pay Paul. Depending on the nature of the change between\nthe version the patch is based on and the version the patch is being\napplied with offsets, such a heuristics will sometimes err on the wrong\nside.\n\nLooking at it closer, however, I noticed that the false hit (i.e. \"two\nblocks closed, a blank line, return 0 and the end of function\") in this\nparticular case only appears because we applied the previous hunk. In the\nversion of the file in 0ce3017b, there is only one such place and there\nshould be no ambiguity in the patch application.\n\nThe problem we are seeing is caused only because we look at the result of\napplication of the previous hunks in the patch and incrementally try to\napply the remaining hunks. So clearly \"git apply\" can and should be fixed\nfor this case by teaching find_pos() not to report a match on a line that\nwas touched by application of the previous hunk.\n"},{"id":"162787","messageId":"7v39n27llq.fsf@alter.siamese.dyndns.org","threadId":"26655","inReplyTo":"7vei6m7muw.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] git-am silently applying patches incorrectly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-04T19:05:05Z","receivedAt":"2011-03-04T19:05:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Looking at it closer, however, I noticed that the false hit (i.e. \"two\n> blocks closed, a blank line, return 0 and the end of function\") in this\n> particular case only appears because we applied the previous hunk. In the\n> version of the file in 0ce3017b, there is only one such place and there\n> should be no ambiguity in the patch application.\n>\n> The problem we are seeing is caused only because we look at the result of\n> application of the previous hunks in the patch and incrementally try to\n> apply the remaining hunks. So clearly \"git apply\" can and should be fixed\n> for this case by teaching find_pos() not to report a match on a line that\n> was touched by application of the previous hunk.\n\nAnd here is a quick and dirty fix to do something like that. It assumes\nthat the hunks for a single file being patched are already sorted in the\nascending order (which should be the case), and may regress cases where we\nused to find a match even when the version you are patching has moved\nfunctions around in the file by failing to notice a match. And it does\nget the same result as your GNU patch test.\n\n-- >8 --\nSubject: [PATCH] apply: do not look behind beyond what we already patched\n\nWhen looking for a place to apply a hunk, we used to check lines that\nmatch the preimage of it, starting from the line that the patch wants to\napply the hunk at, with increasing offsets in both ways until we find a\nmatch.\n\nColin Guthrie found an interesting case where this misapplied a patch that\nwanted to touch a preimage that consists of \"two block closed '}',\na blank line, 'return 0;', the function closed '}'\". The\ntarget version of the file originally had only one such location, but the hunk\nimmediately before that created another such preimage, and find_pos() happily\nreported that the preimage matched what the hunk wanted to modify. Oops.\n\nBy recording where the last hunk is applied, and limiting the search done\nby find_pos() to the rest of the file, we can reduce such an accident.\nIdeally, we should not simply limit the upward search like this patch\ndoes, but skip the regions that were touched by previous hunks, but this\napproach was much simpler ;-)\n\nI also considered to teach apply_one_fragment() to take the offset we have\nfound while applying the previous hunk into account when looking for a\nmatch with find_pos(), but dismissed that approach, because it would\nsometimes work better but sometimes worse, depending on the difference\nbetween the version the patch was created against and the version the\npatch is being applied.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/apply.c | 6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 14951da..30857c6 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -211,6 +211,7 @@ struct line {\n */\n struct image {\n \tchar *buf;\n+\tsize_t last_match;\n \tsize_t len;\n \tsize_t nr;\n \tsize_t alloc;\n@@ -2323,11 +2324,11 @@ static int find_pos(struct image *img,\n \t\t\treturn try_lno;\n \n \tagain:\n-\t\tif (backwards_lno == 0 && forwards_lno == img->nr)\n+\t\tif (backwards_lno <= img->last_match && forwards_lno == img->nr)\n \t\t\tbreak;\n \n \t\tif (i & 1) {\n-\t\t\tif (backwards_lno == 0) {\n+\t\t\tif (backwards_lno <= img->last_match) {\n \t\t\t\ti++;\n \t\t\t\tgoto again;\n \t\t\t}\n@@ -2648,6 +2649,7 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,\n \t\t\t\t\" to apply fragment at %d\\n\",\n \t\t\t\tleading, trailing, applied_pos+1);\n \t\tupdate_image(img, applied_pos, &preimage, &postimage);\n+\t\timg->last_match = applied_pos;\n \t} else {\n \t\tif (apply_verbosely)\n \t\t\terror(\"while searching for:\\n%.*s\",\n"},{"id":"162788","messageId":"AANLkTim=jpJmBZmtAVX2V8Ui44AwpTbevJtSR2Xk=wLX@mail.gmail.com","threadId":"26655","inReplyTo":"7v39n27llq.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] git-am silently applying patches incorrectly","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2011-03-04T19:18:01Z","receivedAt":"2011-03-04T19:18:01Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Fri, Mar 4, 2011 at 11:05 AM, Junio C Hamano wrote:\n>\n> And here is a quick and dirty fix to do something like that.  It assumes\n> that the hunks for a single file being patched are already sorted in the\n> ascending order (which should be the case), and may regress cases where we\n> used to find a match even when the version you are patching has moved\n> functions around in the file by failing to notice a match.  And it does\n> get the same result as your GNU patch test.\n\nAck. Looks correct. In fact, shouldn't we make that \"last_match\" be\nthe _end_ of the last place we applied the patch at, rather than the\nbeginning?\n\nIOW, maybe something like \"img->last_match = applied_pos +\npostimage.nr;\" or whatever.\n\nI dunno.\n\n Linus\n"},{"id":"162789","messageId":"7vy64u65ta.fsf@alter.siamese.dyndns.org","threadId":"26655","inReplyTo":"AANLkTim=jpJmBZmtAVX2V8Ui44AwpTbevJtSR2Xk=wLX@mail.gmail.com","subject":"Re: [BUG] git-am silently applying patches incorrectly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-04T19:31:29Z","receivedAt":"2011-03-04T19:31:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> IOW, maybe something like \"img->last_match = applied_pos +\n> postimage.nr;\" or whatever.\n>\n> I dunno.\n\nI was lazy and didn't want to worry about the consequences of excluding\nthe end of the context lines in the previous hunk, especially when the\npatch was generated with small number of context lines.\n\nBut it would probably be a good idea to cut the search at the tail end of\nthe previous hunk.\n"},{"id":"162792","messageId":"loom.20110304T210337-216@post.gmane.org","threadId":"26655","inReplyTo":"7vy64u65ta.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] git-am silently applying patches incorrectly","fromName":"Alexander Miseler","fromEmail":"alexander@miseler.de","sentAt":"2011-03-04T20:14:01Z","receivedAt":"2011-03-04T20:14:01Z","isPatch":false,"sender":{"key":"alexander@miseler.de","avatar":null},"body":"It's a bit intimidating for a newbie to chime in on a discussion between the \ncreator and the maintainer, but:\nIMHO the biggest problem here isn't the incorrectly, but rather the silently. \nReducing the chance of guessing incorrectly is good, but git-am still has to \nguess sometimes and it should warn/inform the user when it does that.\n"},{"id":"162794","messageId":"7vtyfi606a.fsf@alter.siamese.dyndns.org","threadId":"26655","inReplyTo":"loom.20110304T210337-216@post.gmane.org","subject":"Re: [BUG] git-am silently applying patches incorrectly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-04T21:33:17Z","receivedAt":"2011-03-04T21:33:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alexander Miseler writes:\n\n> It's a bit intimidating for a newbie to chime in on a discussion between the \n> creator and the maintainer, but:\n> IMHO the biggest problem here isn't the incorrectly, but rather the silently. \n> Reducing the chance of guessing incorrectly is good, but git-am still has to \n> guess sometimes and it should warn/inform the user when it does that.\n\n(Please don't cull Cc line).\n\nNo need to feel intimidated. \n\nThe patch under discussion was merely \"first things first--let's fix the\nobviously wrong case that can be fixed without regression\".\n\nTry implementing that warning logic, and using it in real-life projects.\nYou don't actually have to _code_ it, but merely imagining how it would\nwork and perform would be sufficient for you to realize that it would be\nquite expensive (you need to find all the possible mismatches, essentially\nscanning the whole file), and worse yet, it would be annoyingly noisy with\nmany false positives, because in many real-life projects, end of function\ntends to match the problematic pattern that triggered this discussion\nquite often even without patches that introduce more of the pattern.\n\nUnless you can reduce the false hits to manageable levels, such a warning\nis not very useful (it would be useful as a lame excuse \"we warned, but\nyou took the suspicious result\", but that does not help the users).\n\nIn short, Linus and I both know what you are talking about, and we may\nrevisit that issue later, but the thing is that it would not be very\npleasant, and not something that can be done in one sitting during a\nsingle discussion thread on the list.\n"},{"id":"162796","messageId":"1299275390.24965.17.camel@drew-northup.unet.maine.edu","threadId":"26655","inReplyTo":"7v39n27llq.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] git-am silently applying patches incorrectly","fromName":"Drew Northup","fromEmail":"drew.northup@maine.edu","sentAt":"2011-03-04T21:49:50Z","receivedAt":"2011-03-04T21:49:50Z","isPatch":false,"sender":{"key":"drew.northup@maine.edu","avatar":"https://avatars.githubusercontent.com/u/18331571?v=4"},"body":"\nOn Fri, 2011-03-04 at 11:05 -0800, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > Looking at it closer, however, I noticed that the false hit (i.e. \"two\n> > blocks closed, a blank line, return 0 and the end of function\") in this\n> > particular case only appears because we applied the previous hunk. In the\n> > version of the file in 0ce3017b, there is only one such place and there\n> > should be no ambiguity in the patch application.\n> >\n> > The problem we are seeing is caused only because we look at the result of\n> > application of the previous hunks in the patch and incrementally try to\n> > apply the remaining hunks. So clearly \"git apply\" can and should be fixed\n> > for this case by teaching find_pos() not to report a match on a line that\n> > was touched by application of the previous hunk.\n> \n> And here is a quick and dirty fix to do something like that. It assumes\n> that the hunks for a single file being patched are already sorted in the\n> ascending order (which should be the case), and may regress cases where we\n> used to find a match even when the version you are patching has moved\n> functions around in the file by failing to notice a match. And it does\n> get the same result as your GNU patch test.\n> \n\nIt checks out here applied against master. I don't know how I got it to\nwork the first time without this patch--but I'm pretty sure I don't want\nto know at this point.\n\n-- \n-Drew Northup\n________________________________________________\n\"As opposed to vegetable or mineral error?\"\n-John Pescatore, SANS NewsBites Vol. 12 Num. 59\n"},{"id":"162797","messageId":"4D7165A3.5080308@colin.guthr.ie","threadId":"26655","inReplyTo":"7vtyfi606a.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] git-am silently applying patches incorrectly","fromName":"Colin Guthrie","fromEmail":"gmane@colin.guthr.ie","sentAt":"2011-03-04T22:20:19Z","receivedAt":"2011-03-04T22:20:19Z","isPatch":false,"sender":{"key":"gmane@colin.guthr.ie","avatar":null},"body":"'Twas brillig, and Junio C Hamano at 04/03/11 21:33 did gyre and gimble:\n> In short, Linus and I both know what you are talking about, and we may\n> revisit that issue later, but the thing is that it would not be very\n> pleasant, and not something that can be done in one sitting during a\n> single discussion thread on the list.\n\nAs a simple option to avoid that, how about just printing out (by\ndefault) the line offsets if hunks don't apply 100% cleanly? This would\nat least alert you to the fact that some fixups were needed.\n\nJust a thought...\n\n\n-- \n\nColin Guthrie\ngmane(at)colin.guthr.ie\nhttp://colin.guthr.ie/\n\nDay Job:\n Tribalogic Limited [http://www.tribalogic.net/]\nOpen Source:\n Mageia Contributor [http://www.mageia.org/]\n PulseAudio Hacker [http://www.pulseaudio.org/]\n Trac Hacker [http://trac.edgewall.org/]\n"},{"id":"162798","messageId":"7vpqq65xcx.fsf@alter.siamese.dyndns.org","threadId":"26655","inReplyTo":"4D7165A3.5080308@colin.guthr.ie","subject":"Re: [BUG] git-am silently applying patches incorrectly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-04T22:34:06Z","receivedAt":"2011-03-04T22:34:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Colin Guthrie writes:\n\n> 'Twas brillig, and Junio C Hamano at 04/03/11 21:33 did gyre and gimble:\n>> In short, Linus and I both know what you are talking about, and we may\n>> revisit that issue later, but the thing is that it would not be very\n>> pleasant, and not something that can be done in one sitting during a\n>> single discussion thread on the list.\n>\n> As a simple option to avoid that, how about just printing out (by\n> default) the line offsets if hunks don't apply 100% cleanly? This would\n> at least alert you to the fact that some fixups were needed.\n\nYeah, that is what GNU patch does, and would be a small improvement that\nmight be in a right direction.\n\nWhile we are at it, here is an alternate patch that does not lose the\nability to cope with the case where the target version has functions moved\naround without introducing new ambiguous patch application sites. Instead\nof keeping the \"last position was here -- we won't look beyond that\", we\nmark the lines that were brought into the target by the patch application\nso far, and reject preimage matches against these lines.\n\nA full solution for detecting a potential ambiguity and warning it would\nbe based on this version instead. It would involve letting find_pos() not\nstop at the first hit near the intended target, and marking the hunk that\ncan apply at more than one location.\n\nA yet even more reliable alternative solution _might_ be to first scan the\noriginal without applying any hunks just to find the possible sites to be\npatched, warn ambiguities and decide & commit to these patch application\nsites, and then apply the hunks. If we did so, we wouldn't need this\npatch nor the previous one.\n\nBut that would be a larger change, and would require a good test vector,\nperhaps a large quilt series, to make sure it does not introduce\nregression.\n\n\n builtin/apply.c | 7 ++++++-\n 1 files changed, 6 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 14951da..04f56f8 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -204,6 +204,7 @@ struct line {\n \tunsigned hash : 24;\n \tunsigned flag : 8;\n #define LINE_COMMON 1\n+#define LINE_PATCHED\t2\n };\n \n /*\n@@ -2085,7 +2086,8 @@ static int match_fragment(struct image *img,\n \n \t/* Quick hash check */\n \tfor (i = 0; i < preimage_limit; i++)\n-\t\tif (preimage->line[i].hash != img->line[try_lno + i].hash)\n+\t\tif ((img->line[try_lno + i].flag & LINE_PATCHED) ||\n+\t\t (preimage->line[i].hash != img->line[try_lno + i].hash))\n \t\t\treturn 0;\n \n \tif (preimage_limit == preimage->nr) {\n@@ -2428,6 +2430,9 @@ static void update_image(struct image *img,\n \tmemcpy(img->line + applied_pos,\n \t postimage->line,\n \t postimage->nr * sizeof(*img->line));\n+\tfor (i = 0; i < postimage->nr; i++)\n+\t\timg->line[applied_pos + i].flag |= LINE_PATCHED;\n+\n \timg->nr = nr;\n }\n \n"},{"id":"162799","messageId":"7vlj0u5wyw.fsf@alter.siamese.dyndns.org","threadId":"26655","inReplyTo":"4D7165A3.5080308@colin.guthr.ie","subject":"Re: [BUG] git-am silently applying patches incorrectly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-04T22:42:31Z","receivedAt":"2011-03-04T22:42:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Colin Guthrie writes:\n\n> 'Twas brillig, and Junio C Hamano at 04/03/11 21:33 did gyre and gimble:\n>> In short, Linus and I both know what you are talking about, and we may\n>> revisit that issue later, but the thing is that it would not be very\n>> pleasant, and not something that can be done in one sitting during a\n>> single discussion thread on the list.\n>\n> As a simple option to avoid that, how about just printing out (by\n> default) the line offsets if hunks don't apply 100% cleanly? This would\n> at least alert you to the fact that some fixups were needed.\n>\n> Just a thought...\n\n... and a patch to do so would look like this. \"git apply -v\" and (GNU)\n\"patch -p1\" seems to report exactly the same numbers for the problematic\npatch and the initial state that started this discussion.\n\n builtin/apply.c | 15 ++++++++++++++-\n 1 files changed, 14 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 14951da..4d22d16 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -2638,6 +2643,14 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,\n \t\t\t\tapply = 0;\n \t\t}\n \n+\t\tif (apply_verbosely && applied_pos != pos) {\n+\t\t\tint offset = applied_pos - pos;\n+\t\t\tif (offset < 0)\n+\t\t\t\toffset = 0 - offset;\n+\t\t\tfprintf(stderr, \"Applied at %d (offset %d line(s)).\\n\",\n+\t\t\t\tapplied_pos + 1, offset);\n+\t\t}\n+\n \t\t/*\n \t\t * Warn if it was necessary to reduce the number\n \t\t * of context lines.\n"},{"id":"162800","messageId":"7vhbbi5w87.fsf@alter.siamese.dyndns.org","threadId":"26655","inReplyTo":"AANLkTim=jpJmBZmtAVX2V8Ui44AwpTbevJtSR2Xk=wLX@mail.gmail.com","subject":"Re: [BUG] git-am silently applying patches incorrectly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-04T22:58:32Z","receivedAt":"2011-03-04T22:58:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sorry to bother you with another review request; I slightly prefer this\none better. The exact same issue, different approach.\n\n-- >8 --\nSubject: [PATCH] apply: do not patch lines that were already patched\n\nWhen looking for a place to apply a hunk, we used to check lines that\nmatch the preimage of it, starting from the line that the patch wants to\napply the hunk at, looking forward and backward with increasing offsets\nuntil we find a match.\n\nColin Guthrie found an interesting case where this misapplied a patch that\nwanted to touch a preimage that consists of\n\n }\n }\n\n return 0;\n }\n\nwhich is a rather unfortunately common pattern.\n\nThe target version of the file originally had only one such location, but\nthe hunk immediately before that created another instance of such block of\nlines, and find_pos() happily reported that the preimage of the hunk\nmatched what it wanted to modify.\n\nOops.\n\nBy marking the lines application of earlier hunks touched and preventing\nmatch_fragment() from considering them as a match with preimage of other\nhunks, we can reduce such an accident.\n\nI also considered to teach apply_one_fragment() to take the offset we have\nfound while applying the previous hunk into account when looking for a\nmatch with find_pos(), but dismissed that approach, because it would\nsometimes work better but sometimes worse, depending on the difference\nbetween the version the patch was created against and the version the\npatch is being applied.\n\nThis does _not_ prevent misapplication of patches to a file that has many\nsimilar looking blocks of lines and a preimage cannot identify which one\nof them should be applied. For that, we would need to scan beyond the\nfirst match in find_pos(), and issue a warning (or error out). That will\nbe a separate topic.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/apply.c | 7 ++++++-\n 1 files changed, 6 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 14951da..04f56f8 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -204,6 +204,7 @@ struct line {\n \tunsigned hash : 24;\n \tunsigned flag : 8;\n #define LINE_COMMON 1\n+#define LINE_PATCHED\t2\n };\n \n /*\n@@ -2085,7 +2086,8 @@ static int match_fragment(struct image *img,\n \n \t/* Quick hash check */\n \tfor (i = 0; i < preimage_limit; i++)\n-\t\tif (preimage->line[i].hash != img->line[try_lno + i].hash)\n+\t\tif ((img->line[try_lno + i].flag & LINE_PATCHED) ||\n+\t\t (preimage->line[i].hash != img->line[try_lno + i].hash))\n \t\t\treturn 0;\n \n \tif (preimage_limit == preimage->nr) {\n@@ -2428,6 +2430,9 @@ static void update_image(struct image *img,\n \tmemcpy(img->line + applied_pos,\n \t postimage->line,\n \t postimage->nr * sizeof(*img->line));\n+\tfor (i = 0; i < postimage->nr; i++)\n+\t\timg->line[applied_pos + i].flag |= LINE_PATCHED;\n+\n \timg->nr = nr;\n }\n \n-- \n1.7.4.1.287.g1ecf2\n"},{"id":"162801","messageId":"4D717116.3050305@miseler.de","threadId":"26655","inReplyTo":"7vtyfi606a.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] git-am silently applying patches incorrectly","fromName":"Alexander Miseler","fromEmail":"alexander@miseler.de","sentAt":"2011-03-04T23:09:10Z","receivedAt":"2011-03-04T23:09:10Z","isPatch":false,"sender":{"key":"alexander@miseler.de","avatar":null},"body":"On 04.03.2011 22:33, Junio C Hamano wrote:\n> (Please don't cull Cc line).\n\nSorry. I used the nice gmane web interface and hoped that it keeps the CC intact, which it apparently doesn't. I guess i \nwill go old school now and use the mailing list via actual emails :)\n\n\n> Try implementing that warning logic, and using it in real-life projects.\n> You don't actually have to _code_ it, but merely imagining how it would\n> work and perform would be sufficient for you to realize that it would be\n> quite expensive (you need to find all the possible mismatches, essentially\n> scanning the whole file), and worse yet, it would be annoyingly noisy with\n> many false positives, because in many real-life projects, end of function\n> tends to match the problematic pattern that triggered this discussion\n> quite often even without patches that introduce more of the pattern.\n>\n> Unless you can reduce the false hits to manageable levels, such a warning\n> is not very useful (it would be useful as a lame excuse \"we warned, but\n> you took the suspicious result\", but that does not help the users).\n>\n> In short, Linus and I both know what you are talking about, and we may\n> revisit that issue later, but the thing is that it would not be very\n> pleasant, and not something that can be done in one sitting during a\n> single discussion thread on the list.\n\nUnderstood. On a side note: if this problem is tackled it might be sensible to add a heuristic to git format-patch that \nincreases the context size for hunks that are likely to be ambiguous. \"Likely to be ambiguous\" is of course a problem in \nitself but even a less than perfect detection might be helpful and it would suffer less from some of the aforementioned \nproblems, like noisiness/false hits, which would just increase the patch size instead of harassing the user.\n"},{"id":"162803","messageId":"7vd3m65t4g.fsf@alter.siamese.dyndns.org","threadId":"26655","inReplyTo":"4D717116.3050305@miseler.de","subject":"Re: [BUG] git-am silently applying patches incorrectly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-05T00:05:35Z","receivedAt":"2011-03-05T00:05:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alexander Miseler writes:\n\n> .... On a side note: if this problem is tackled it might be\n> sensible to add a heuristic to git format-patch that increases the\n> context size for hunks that are likely to be ambiguous.\n\nIn general that approach would not help. Imagine a case where the\nproblematic patch from David Henningsson were only about moving the\ncalling site of check_required() within the element_probe() function.\nIOW, imagine that the hunk starting at line 1018 to modify\ncheck_required() itself weren't involved in his change.\n\nBefore or after such a change, there would be only one location that\ncloses two blocks followed by a blank line and return 0 at the end of the\nfunction, which is the necessary context of the \"moved after\" hunk of the\npatch, so when producing the patch, there is no way for format-patch to\nnotice that it is likely to become ambiguous.\n\nBut if the change to check_required() were applied as a separate patch\nbefore the change to move the call site was applied, at the application\ntime, the patch becomes ambiguous.\n"},{"id":"162817","messageId":"4D7223A9.6080105@colin.guthr.ie","threadId":"26655","inReplyTo":"7vlj0u5wyw.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] git-am silently applying patches incorrectly","fromName":"Colin Guthrie","fromEmail":"gmane@colin.guthr.ie","sentAt":"2011-03-05T11:51:05Z","receivedAt":"2011-03-05T11:51:05Z","isPatch":false,"sender":{"key":"gmane@colin.guthr.ie","avatar":null},"body":"'Twas brillig, and Junio C Hamano at 04/03/11 22:42 did gyre and gimble:\n> Colin Guthrie writes:\n> \n>> 'Twas brillig, and Junio C Hamano at 04/03/11 21:33 did gyre and gimble:\n>>> In short, Linus and I both know what you are talking about, and we may\n>>> revisit that issue later, but the thing is that it would not be very\n>>> pleasant, and not something that can be done in one sitting during a\n>>> single discussion thread on the list.\n>>\n>> As a simple option to avoid that, how about just printing out (by\n>> default) the line offsets if hunks don't apply 100% cleanly? This would\n>> at least alert you to the fact that some fixups were needed.\n>>\n>> Just a thought...\n> \n> ... and a patch to do so would look like this. \"git apply -v\" and (GNU)\n> \"patch -p1\" seems to report exactly the same numbers for the problematic\n> patch and the initial state that started this discussion.\n> \n> builtin/apply.c | 15 ++++++++++++++-\n> 1 files changed, 14 insertions(+), 1 deletions(-)\n> \n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index 14951da..4d22d16 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -2638,6 +2643,14 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,\n> \t\t\t\tapply = 0;\n> \t\t}\n> \n> +\t\tif (apply_verbosely && applied_pos != pos) {\n> +\t\t\tint offset = applied_pos - pos;\n> +\t\t\tif (offset < 0)\n> +\t\t\t\toffset = 0 - offset;\n> +\t\t\tfprintf(stderr, \"Applied at %d (offset %d line(s)).\\n\",\n> +\t\t\t\tapplied_pos + 1, offset);\n> +\t\t}\n> +\n> \t\t/*\n> \t\t * Warn if it was necessary to reduce the number\n> \t\t * of context lines.\n\nPersonally I wouldn't bother making offset absolute... (equiv of\nabs(offset)) as knowing it applied earlier or later could be useful...\nthe direction is lost here and I don't really see why that's nicer for\nthe user. But maybe that's just my opinion?\n\nCol\n\nPS Many thanks for working on this :)\n\n\n-- \n\nColin Guthrie\ngmane(at)colin.guthr.ie\nhttp://colin.guthr.ie/\n\nDay Job:\n Tribalogic Limited [http://www.tribalogic.net/]\nOpen Source:\n Mageia Contributor [http://www.mageia.org/]\n PulseAudio Hacker [http://www.pulseaudio.org/]\n Trac Hacker [http://trac.edgewall.org/]\n"},{"id":"162871","messageId":"7vsjuz520w.fsf@alter.siamese.dyndns.org","threadId":"26655","inReplyTo":"4D7223A9.6080105@colin.guthr.ie","subject":"Re: [BUG] git-am silently applying patches incorrectly","fromName":"Junio C Hamano","fromEmail":"junio@pobox.com","sentAt":"2011-03-06T22:15:27Z","receivedAt":"2011-03-06T22:15:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> In any case, here is an update to match what GNU patch seems to do more\n> closely.\n> ...\n\nThis is an unrelated tangent, but in a separate thread there was a\ndiscussion on \"%d noun(s)\" in a recent weatherbaloon patch, and use of\nngettext(3) was suggested to solve this portably to languages other than\nGermanic and Romanic family.\n\nSo here is my exercise for preparing the new code for upcoming i18n.\nDoes it look sane?\n\nDo we want a new wrapper similar to _() that would easily make this into a\nnoop under NO_GETTEXT in the proposed i18n infrastructure?\n\n builtin/apply.c | 3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex a231c0c..f084250 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -2644,7 +2644,10 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,\n \t\t\tif (apply_in_reverse)\n \t\t\t\toffset = 0 - offset;\n \t\t\tfprintf(stderr,\n+\t\t\t\tngettext(\n+\t\t\t\t\"Hunk #%d succeeded at %d (offset %d line).\\n\",\n \t\t\t\t\"Hunk #%d succeeded at %d (offset %d lines).\\n\",\n+\t\t\t\t(offset < 0 ? (0 - offset) : offset)),\n \t\t\t\tnth_fragment, applied_pos + 1, offset);\n \t\t}\n \n"},{"id":"162870","messageId":"7vlj0r520k.fsf@alter.siamese.dyndns.org","threadId":"26655","inReplyTo":"4D7223A9.6080105@colin.guthr.ie","subject":"Re: [BUG] git-am silently applying patches incorrectly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-06T22:15:39Z","receivedAt":"2011-03-06T22:15:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Colin Guthrie writes:\n\n> 'Twas brillig, and Junio C Hamano at 04/03/11 22:42 did gyre and gimble:\n>> ... and a patch to do so would look like this. \"git apply -v\" and (GNU)\n>> \"patch -p1\" seems to report exactly the same numbers for the problematic\n>> patch and the initial state that started this discussion.\n>> ... \n>\n> Personally I wouldn't bother making offset absolute... (equiv of\n> abs(offset)) as knowing it applied earlier or later could be useful...\n> the direction is lost here and I don't really see why that's nicer for\n> the user. But maybe that's just my opinion?\n\nI don't have a strong opinion on this either way; I would just imitate\nwhat GNU patch would do, which would probably be to show the offset as-is,\nexcept that it flips the sign if it is being run in reverse with -R\noption.\n\nA bigger question I would actually care _more_ about is if this should be\non by default without -v. We usually do not allow fuzz by default for\nsafety, and we do warn loudly when -C reduces the context and we actually\nneed to use it to match the preimage.\n\nIn any case, here is an update to match what GNU patch seems to do more\nclosely.\n\n-- >8 --\nSubject: [PATCH] apply -v: show offset count when patch did not apply exactly\n\nWhen the line number the patch intended to touch does not match\nthe line in the version being patched, GNU patch reports that\nit applied the hunk at a different line number, with how big an\noffset.\n\nTeach \"git apply\" to do the same under --verbose option.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/apply.c | 16 ++++++++++++++--\n 1 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 14951da..a231c0c 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -2432,7 +2432,8 @@ static void update_image(struct image *img,\n }\n \n static int apply_one_fragment(struct image *img, struct fragment *frag,\n-\t\t\t int inaccurate_eof, unsigned ws_rule)\n+\t\t\t int inaccurate_eof, unsigned ws_rule,\n+\t\t\t int nth_fragment)\n {\n \tint match_beginning, match_end;\n \tconst char *patch = frag->patch;\n@@ -2638,6 +2639,15 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,\n \t\t\t\tapply = 0;\n \t\t}\n \n+\t\tif (apply_verbosely && applied_pos != pos) {\n+\t\t\tint offset = applied_pos - pos;\n+\t\t\tif (apply_in_reverse)\n+\t\t\t\toffset = 0 - offset;\n+\t\t\tfprintf(stderr,\n+\t\t\t\t\"Hunk #%d succeeded at %d (offset %d lines).\\n\",\n+\t\t\t\tnth_fragment, applied_pos + 1, offset);\n+\t\t}\n+\n \t\t/*\n \t\t * Warn if it was necessary to reduce the number\n \t\t * of context lines.\n@@ -2785,12 +2795,14 @@ static int apply_fragments(struct image *img, struct patch *patch)\n \tconst char *name = patch->old_name ? patch->old_name : patch->new_name;\n \tunsigned ws_rule = patch->ws_rule;\n \tunsigned inaccurate_eof = patch->inaccurate_eof;\n+\tint nth = 0;\n \n \tif (patch->is_binary)\n \t\treturn apply_binary(img, patch);\n \n \twhile (frag) {\n-\t\tif (apply_one_fragment(img, frag, inaccurate_eof, ws_rule)) {\n+\t\tnth++;\n+\t\tif (apply_one_fragment(img, frag, inaccurate_eof, ws_rule, nth)) {\n \t\t\terror(\"patch failed: %s:%ld\", name, frag->oldpos);\n \t\t\tif (!apply_with_reject)\n \t\t\t\treturn -1;\n-- \n1.7.4.1.299.ga459d\n"},{"id":"162872","messageId":"7vhbbf50vu.fsf@alter.siamese.dyndns.org","threadId":"26655","inReplyTo":"7vsjuz520w.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] git-am silently applying patches incorrectly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-06T22:40:05Z","receivedAt":"2011-03-06T22:40:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> ...\n> So here is my exercise for preparing the new code for upcoming i18n.\n> Does it look sane?\n>\n> Do we want a new wrapper similar to _() that would easily make this into a\n> noop under NO_GETTEXT in the proposed i18n infrastructure?\n>\n> builtin/apply.c | 3 +++\n> 1 files changed, 3 insertions(+), 0 deletions(-)\n>\n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index a231c0c..f084250 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -2644,7 +2644,10 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,\n> \t\t\tif (apply_in_reverse)\n> \t\t\t\toffset = 0 - offset;\n> \t\t\tfprintf(stderr,\n> +\t\t\t\tngettext(\n> +\t\t\t\t\"Hunk #%d succeeded at %d (offset %d line).\\n\",\n> \t\t\t\t\"Hunk #%d succeeded at %d (offset %d lines).\\n\",\n> +\t\t\t\t(offset < 0 ? (0 - offset) : offset)),\n> \t\t\t\tnth_fragment, applied_pos + 1, offset);\n> \t\t}\n> \n\nIf we were to do i18n, we would probably need to include something like\nthe following in the early fast-tracked part of the series, perhaps as\npart of the e6bb27e (i18n: add no-op _() and N_() wrappers, 2011-02-22)\n\n\n\n gettext.h | 6 ++++++\n 1 files changed, 6 insertions(+), 0 deletions(-)\n\ndiff --git a/gettext.h b/gettext.h\nindex 6949d73..1510c5d 100644\n--- a/gettext.h\n+++ b/gettext.h\n@@ -23,4 +23,10 @@ static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)\n /* Mark msgid for translation but do not translate it. */\n #define N_(msgid) (msgid)\n \n+static inline const char *ngettext(const char *msgid, const char *plu, unsigned long n)\n+{\n+\t/* fallback ngettext() without using libintl */\n+\treturn (n == 1) ? msgid : plu;\n+}\n+\n #endif\n"},{"id":"162874","messageId":"20110306225641.GB24327@elie","threadId":"26655","inReplyTo":"7vhbbf50vu.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] git-am silently applying patches incorrectly","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-06T22:56:41Z","receivedAt":"2011-03-06T22:56:41Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> If we were to do i18n, we would probably need to include something like\n> the following in the early fast-tracked part of the series, perhaps as\n> part of the e6bb27e (i18n: add no-op _() and N_() wrappers, 2011-02-22)\n\nYep. Is it safe to do this without the\n\n#define ngettext git_ngettext\nstatic inline const char *git_ngettext(...)\n\ndance?\n"},{"id":"162898","messageId":"4D74A753.2020200@colin.guthr.ie","threadId":"26655","inReplyTo":"7vlj0r520k.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] git-am silently applying patches incorrectly","fromName":"Colin Guthrie","fromEmail":"gmane@colin.guthr.ie","sentAt":"2011-03-07T09:37:23Z","receivedAt":"2011-03-07T09:37:23Z","isPatch":false,"sender":{"key":"gmane@colin.guthr.ie","avatar":null},"body":"'Twas brillig, and Junio C Hamano at 06/03/11 22:15 did gyre and gimble:\n>> > Personally I wouldn't bother making offset absolute... (equiv of\n>> > abs(offset)) as knowing it applied earlier or later could be useful...\n>> > the direction is lost here and I don't really see why that's nicer for\n>> > the user. But maybe that's just my opinion?\n> I don't have a strong opinion on this either way; I would just imitate\n> what GNU patch would do, which would probably be to show the offset as-is,\n> except that it flips the sign if it is being run in reverse with -R\n> option.\n\nYeah I think that's quite sensible. I think converging with the way GNU\npatch does it except when there is really good reason not to makes a lot\nof sense, if nothing more than general familiarity and expectations.\n\n> A bigger question I would actually care _more_ about is if this should be\n> on by default without -v. We usually do not allow fuzz by default for\n> safety, and we do warn loudly when -C reduces the context and we actually\n> need to use it to match the preimage.\n\nUsers used to using patch may simply think there are no offset\nadjustments when using git am and live in blissful ignorance. For that\nreason I'd say it should be on by default. But then again, I've been\nrecently jaded by a mis-applied patch... if I'm honest, I would probably\nsay that 99 times in a 100, I couldn't really care less (or really read)\nthe offset adjustments.... so I can't really comment very subjectively\nhere :s\n\n> In any case, here is an update to match what GNU patch seems to do more\n> closely.\n\nLooks good to me!\n\nThanks again for looking into this issue :) Hopefully the primary fix\ncan be pushed soon and the nice usability improvements that have spawned\nfrom it can head the same direction too :)\n\nCol\n\n-- \n\nColin Guthrie\ngmane(at)colin.guthr.ie\nhttp://colin.guthr.ie/\n\nDay Job:\n Tribalogic Limited [http://www.tribalogic.net/]\nOpen Source:\n Mageia Contributor [http://www.mageia.org/]\n PulseAudio Hacker [http://www.pulseaudio.org/]\n Trac Hacker [http://trac.edgewall.org/]\n"},{"id":"163051","messageId":"20110309103104.GA30980@elie","threadId":"26655","inReplyTo":"AANLkTikctSrfqKCdeYUyvUmAZjr=i7kaFhPeB-LfwgUz@mail.gmail.com","subject":"[RFC/PATCH 0/2] i18n: add ngettext stub","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-09T10:31:04Z","receivedAt":"2011-03-09T10:31:04Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> On Mar 6, 2011 3:07 PM, \"Jonathan Nieder\" wrote:\n\n>> Yep. Is it safe to do this without the\n>>\n>> #define ngettext git_ngettext\n>> static inline const char *git_ngettext(...)\n>>\n>> dance?\n>\n> Heh you tell me.\n\nYegh. My general feeling was that we should make sure gettext.h works\neven if was included elsewhere, just in case some system\nheader decides to start including it. That makes life slightly less\npleasant, since libintl.h does\n\n #if defined __OPTIMIZE__ && !defined __cplusplus\n[...]\n # define ngettext(msgid1, msgid2, n) dngettext (NULL, msgid1, msgid2, n)\n[...]\n #endif\t/* Optimizing. */\n\nHow about this?\n\nJonathan Nieder (1):\n i18n: avoid conflict with ngettext from libintl\n\nJunio C Hamano (1):\n i18n: add no-op ngettext() fallback\n\n gettext.h | 13 +++++++++++++\n 1 files changed, 13 insertions(+), 0 deletions(-)\n\n-- \n1.7.4.1\n"},{"id":"163053","messageId":"20110309104623.GB30980@elie","threadId":"26655","inReplyTo":"20110309103104.GA30980@elie","subject":"[PATCH 1/2] i18n: add stub ngettext implementation","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-09T10:46:23Z","receivedAt":"2011-03-09T10:46:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Junio C Hamano \nDate: Sun, 6 Mar 2011 14:40:05 -0800\n\nThe ngettext function translates a string representing some pharse\nwith an alternative plural form and uses the 'count' argument to\nchoose which form to return. Use of ngettext solves the \"%d noun(s)\"\nproblem in a way that is portable to languages outside the Germanic\nand Romance families.\n\nIn English, the semantics of ngettext(sing, plur, count) are roughly\nequivalent to\n\n\tcount == 1 ? _(sing) : _(plur)\n\nwhile in other languages there can be more variants (count == 0; more\nrandom-looking rules based on the historical pronunciation of the\nnumber). Behind the scenes, the singular form is used to look up a\nfamily of translations and the plural form is ignored unless no\ntranslation is available.\n\nAdd a simple wrapper with the English semantics so C code can start\nusing it to mark phrases with a count for translation.\n\nSigned-off-by: Jonathan Nieder \n---\nThis is based against a version of gettext.h with the fixup\n(s/# GETTEXT POISON #/ GETTEXT POISON /) from\nhttp://thread.gmane.org/gmane.comp.version-control.git/167661/focus=167878\nApplying that substitution to the patch should be enough for it to\napply to a gettext.h without that change.\n\n gettext.h | 8 ++++++++\n 1 files changed, 8 insertions(+), 0 deletions(-)\n\ndiff --git a/gettext.h b/gettext.h\nindex 73831aa..03fb340 100644\n--- a/gettext.h\n+++ b/gettext.h\n@@ -26,6 +26,14 @@ static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)\n \treturn use_gettext_poison() ? \" GETTEXT POISON \" : msgid;\n }\n \n+static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)\n+const char *ngettext(const char *msgid, const char *plu, unsigned long n)\n+{\n+\tif (use_gettext_poison())\n+\t\treturn \" GETTEXT POISON \";\n+\treturn n == 1 ? msgid : plu;\n+}\n+\n /* Mark msgid for translation but do not translate it. */\n #define N_(msgid) (msgid)\n \n-- \n1.7.4.1\n"},{"id":"163054","messageId":"20110309105236.GC30980@elie","threadId":"26655","inReplyTo":"20110309103104.GA30980@elie","subject":"[PATCH 2/2] i18n: avoid conflict with ngettext from libintl","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-09T10:52:36Z","receivedAt":"2011-03-09T10:52:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Although git itself would not be using \"#include \" anywhere\nin a NO_GETTEXT build, git's gettext.h is meant to tolerate prior\ndeclarations from libintl, to prepare for a scenario in which some\nsystem or compat/ header decides to start including libintl. GNU\nlibintl.h defines ngettext as a macro when __OPTIMIZE__ is defined, so\ntake care to \"#undef ngettext\" if it was defined for us.\n\nTo avoid having to worry about a conflicting ngettext symbol when\nlibintl is part of libc, also rename the no-op ngettext stub to\ngit_ngettext and make ngettext a macro referring to it. This is\nprobably never necessary (because git's ngettext is declared \"static\ninline\") but it buys peace of mind.\n\nThis change does not protect against conflicts due to a header\nincluded _after_ git's i18n support (e.g., pthread.h) being the first\nto pull in libintl. We can deal with that separately if it happens.\n\nSigned-off-by: Jonathan Nieder \n---\n gettext.h | 7 ++++++-\n 1 files changed, 6 insertions(+), 1 deletions(-)\n\ndiff --git a/gettext.h b/gettext.h\nindex 03fb340..a473af4 100644\n--- a/gettext.h\n+++ b/gettext.h\n@@ -13,6 +13,10 @@\n #error \"namespace conflict: '_' is pre-defined?\"\n #endif\n \n+#ifdef ngettext\n+#undef ngettext\n+#endif\n+\n #define FORMAT_PRESERVING(n) __attribute__((format_arg(n)))\n \n #ifdef GETTEXT_POISON\n@@ -26,8 +30,9 @@ static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)\n \treturn use_gettext_poison() ? \" GETTEXT POISON \" : msgid;\n }\n \n+#define ngettext git_ngettext\n static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)\n-const char *ngettext(const char *msgid, const char *plu, unsigned long n)\n+const char *git_ngettext(const char *msgid, const char *plu, unsigned long n)\n {\n \tif (use_gettext_poison())\n \t\treturn \" GETTEXT POISON \";\n-- \n1.7.4.1\n"},{"id":"163079","messageId":"7vfwqw9g9b.fsf@alter.siamese.dyndns.org","threadId":"26655","inReplyTo":"20110309105236.GC30980@elie","subject":"Re: [PATCH 2/2] i18n: avoid conflict with ngettext from libintl","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-09T20:43:28Z","receivedAt":"2011-03-09T20:43:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> To avoid having to worry about a conflicting ngettext symbol when\n> libintl is part of libc, also rename the no-op ngettext stub to\n> git_ngettext and make ngettext a macro referring to it. This is\n> probably never necessary (because git's ngettext is declared \"static\n> inline\") but it buys peace of mind.\n\nHmph. An obviously safer alternative would be to use git_ngettext() in\nour source all over the place, and it would by even more peace of mind but\nthat is even longer.\n\n> This change does not protect against conflicts due to a header\n> included _after_ git's i18n support (e.g., pthread.h) being the first\n> to pull in libintl. We can deal with that separately if it happens.\n\nAlso the same problem exists already for the _() macro.\n"},{"id":"163080","messageId":"20110309205155.GC22292@elie","threadId":"26655","inReplyTo":"7vfwqw9g9b.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] i18n: avoid conflict with ngettext from libintl","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-09T20:51:55Z","receivedAt":"2011-03-09T20:51:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> Hmph. An obviously safer alternative would be to use git_ngettext() in\n> our source all over the place, and it would by even more peace of mind but\n> that is even longer.\n\nRight. That is tempting.\n\nÆvar, is there some usual or obvious abbreviated form for ngettext we\ncould use to avoid this fuss altogether?\n\n> Also the same problem exists already for the _() macro.\n\nThe usual convention is that the _() macro is private to each\napplication. libintl provides a gettext function or macro, and\nvarious programs do\n\n\t#define _(msg) gettext(msg)\n\nin some private header (that does not pollute the public namespace)\nfor notational convenience.\n"},{"id":"163082","messageId":"7v7hc89fp7.fsf@alter.siamese.dyndns.org","threadId":"26655","inReplyTo":"20110309205155.GC22292@elie","subject":"Re: [PATCH 2/2] i18n: avoid conflict with ngettext from libintl","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-09T20:55:32Z","receivedAt":"2011-03-09T20:55:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> The usual convention is that the _() macro is private to each\n> application. libintl provides a gettext function or macro, and\n> various programs do\n>\n> \t#define _(msg) gettext(msg)\n>\n> in some private header (that does not pollute the public namespace)\n> for notational convenience.\n\nYeah, I am aware of that. Is there a similar convention for [dn]gettext?\nPerhaps not....\n"},{"id":"163109","messageId":"20110310031734.GA24781@elie","threadId":"26655","inReplyTo":"7v7hc89fp7.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2] i18n: add stub Q_() wrapper for ngettext","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-10T03:17:58Z","receivedAt":"2011-03-10T03:17:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Junio C Hamano \nDate: Sun, 6 Mar 2011 14:40:05 -0800\nSubject: i18n: add stub Q_() wrapper for ngettext\n\nThe Q_ function translates a string representing some pharse with an\nalternative plural form and uses the 'count' argument to choose which\nform to return. Use of Q_ solves the \"%d noun(s)\" problem in a way\nthat is portable to languages outside the Germanic and Romance\nfamilies.\n\nIn English, the semantics of Q_(sing, plur, count) are roughly\nequivalent to\n\n\tcount == 1 ? _(sing) : _(plur)\n\nwhile in other languages there can be more variants (count == 0; more\nrandom-looking rules based on the historical pronunciation of the\nnumber). Behind the scenes, the singular form is used to look up a\nfamily of translations and the plural form is ignored unless no\ntranslation is available.\n\nDefine such a Q_ in gettext.h with the English semantics so C code can\nstart using it to mark phrases with a count for translation.\n\nThe name \"Q_\" is taken from subversion and stands for \"quantity\".\nMany projects just use ngettext directly without a wrapper analogous\nto _; we should not do so because git's gettext.h is meant not to\nconflict with system headers that might include libintl.h.\n\nSigned-off-by: Jonathan Nieder \n---\nJunio C Hamano wrote:\n\n> Yeah, I am aware of that. Is there a similar convention for [dn]gettext?\n> Perhaps not....\n\nsubversion uses Q_. Though it does not seem to be standard --- e.g.,\nglib uses:\n\n _() for gettext\n Q_() for a variant on gettext that allows a string of the form\n \"context|message\" as its argument;\n C_() for a nicer version of Q_() that takes the context and message\n as distinct arguments;\n N_() to mark a string for translation without translating it;\n NC_() to mark a string with context for translation without\n translating it.\n\nI suppose Q_ is as good a name as any.\n\nHopefully veterans from glib would not be used to glib's alternative\nmeaning of Q_, preferring to use C_ for messages with flags.\n\n gettext.h | 12 ++++++++++--\n 1 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/gettext.h b/gettext.h\nindex 04b5958..1b253b7 100644\n--- a/gettext.h\n+++ b/gettext.h\n@@ -9,8 +9,8 @@\n #ifndef GETTEXT_H\n #define GETTEXT_H\n \n-#ifdef _\n-#error \"namespace conflict: '_' is pre-defined?\"\n+#if defined(_) || defined(Q_)\n+#error \"namespace conflict: '_' or 'Q_' is pre-defined?\"\n #endif\n \n #define FORMAT_PRESERVING(n) __attribute__((format_arg(n)))\n@@ -26,6 +26,14 @@ static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)\n \treturn use_gettext_poison() ? \"# GETTEXT POISON #\" : msgid;\n }\n \n+static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)\n+const char *Q_(const char *msgid, const char *plu, unsigned long n)\n+{\n+\tif (use_gettext_poison())\n+\t\treturn \"# GETTEXT POISON #\";\n+\treturn n == 1 ? msgid : plu;\n+}\n+\n /* Mark msgid for translation but do not translate it. */\n #define N_(msgid) (msgid)\n \n-- \n1.7.4.1\n"},{"id":"163117","messageId":"7vd3lz76eq.fsf@alter.siamese.dyndns.org","threadId":"26655","inReplyTo":"20110310031734.GA24781@elie","subject":"Re: [PATCH v2] i18n: add stub Q_() wrapper for ngettext","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-10T07:59:09Z","receivedAt":"2011-03-10T07:59:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> I suppose Q_ is as good a name as any.\n\nOk, let's run with this for now, as we hopefully don't have too many\nplaces that might want to use ngettext(), and they should wait until the\nearly parts of the series settles and in-flight topics are adjusted to the\nbarebones infrastructure.\n\nThanks.\n"},{"id":"163122","messageId":"AANLkTimkVrbosOZeCDxcgRvzRDqYiiz72M0_zQ4_NSvi@mail.gmail.com","threadId":"26655","inReplyTo":"20110309205155.GC22292@elie","subject":"Re: [PATCH 2/2] i18n: avoid conflict with ngettext from libintl","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2011-03-10T09:21:18Z","receivedAt":"2011-03-10T09:21:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Wed, Mar 9, 2011 at 21:51, Jonathan Nieder wrote:\n> Junio C Hamano wrote:\n>\n>> Hmph.  An obviously safer alternative would be to use git_ngettext() in\n>> our source all over the place, and it would by even more peace of mind but\n>> that is even longer.\n>\n> Right.  That is tempting.\n>\n> Ævar, is there some usual or obvious abbreviated form for ngettext we\n> could use to avoid this fuss altogether?\n\nI was looking yesterday but I couldn't find a common. I think your\nQ_() is fine.\n\nI see GNU Make uses S_() internally, netcat uses PL_(). There seems to\nbe no common convention for it like for gettext().\n\nPersonally I'd have chosen n_(), although confusing with our existing\nN_() we could add dn_(), dcn_() etc for dngettext() and dcngettext()\nlater.\n\nBut I don't think it matters. Let's use your patch as-is.\n"},{"id":"163123","messageId":"AANLkTinfsiJHbkpy4BW8yFYzc7qdUZ16iT8W85hMe4Nh@mail.gmail.com","threadId":"26655","inReplyTo":"7vd3lz76eq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] i18n: add stub Q_() wrapper for ngettext","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2011-03-10T09:24:10Z","receivedAt":"2011-03-10T09:24:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Thu, Mar 10, 2011 at 08:59, Junio C Hamano wrote:\n> Jonathan Nieder writes:\n>\n>> I suppose Q_ is as good a name as any.\n>\n> Ok, let's run with this for now, as we hopefully don't have too many\n> places that might want to use ngettext(), and they should wait until the\n> early parts of the series settles and in-flight topics are adjusted to the\n> barebones infrastructure.\n\nYeah, it looks good to me. You can add my Acked-by to it if you'd like.\n"}]}