[BUG] git-am silently applying patches incorrectly

[BUG] git-am silently applying patches incorrectly

[Colin Guthrie]

[-- Attachment #1: Type: text/plain, Size: 2306 bytes --]

Hi,

We recently found a bug in git-am 1.7.4.1 while working on PulseAudio.

It seems that it mis-applied a patch and did so silently without
generating any warnings. It is reproducible and has been confirmed on
different distros.

I make reference to the bug here:
http://thread.gmane.org/gmane.comp.audio.pulseaudio.general/8840/focus=8857

In order to reproduce:

git clone http://git.0pointer.de/repos/pulseaudio.git
git co -b misapply 0ce3017b7407ab1c4094f7ce271bb68319a7eba7
git am 0002-alsa-mixer-add-required-any-and-required-for-enum-op.patch

(I've attached the patch here for convenience).

As you can see, a call to the function check_required is incorrectly
located after application. It is now recursive and uncalled. It should
be called in element_probe() function as per the original patch.

This is quite a serious problem. In our case it turned out to compile
fine. So after reviewing the patch, I applied it and received no errors
and thus thought no more about it. If this kind of thing can sneak by
undetected, then it could introduce flaws quite easily :s



For reference, applying the patch manually with patch works fine and
does not result in an error:

$ cat 0002-alsa-mixer-add-required-any-and-required-for-enum-op.patch |
patch -p1
patching file src/modules/alsa/alsa-mixer.c
Hunk #1 succeeded at 1121 (offset 103 lines).
Hunk #2 succeeded at 1325 (offset 103 lines).
Hunk #3 succeeded at 1356 (offset 103 lines).
Hunk #4 succeeded at 1613 (offset 103 lines).
Hunk #5 succeeded at 1640 (offset 103 lines).
Hunk #6 succeeded at 1913 (offset 103 lines).
Hunk #7 succeeded at 1997 (offset 105 lines).
Hunk #8 succeeded at 2242 (offset 106 lines).
Hunk #9 succeeded at 2261 (offset 106 lines).
Hunk #10 succeeded at 2312 (offset 106 lines).
patching file src/modules/alsa/alsa-mixer.h
Hunk #1 succeeded at 112 (offset 1 line).
Hunk #2 succeeded at 133 (offset 1 line).
Hunk #3 succeeded at 169 (offset 1 line).
patching file src/modules/alsa/mixer/paths/analog-output.conf.common


All the best

Col



-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
  Tribalogic Limited [http://www.tribalogic.net/]
Open Source:
  Mageia Contributor [http://www.mageia.org/]
  PulseAudio Hacker [http://www.pulseaudio.org/]
  Trac Hacker [http://trac.edgewall.org/]


[-- Attachment #2: 0002-alsa-mixer-add-required-any-and-required-for-enum-op.patch --]
[-- Type: text/x-patch, Size: 11154 bytes --]

>From ae83e51c82a747332494bf10c245281e49343fe3 Mon Sep 17 00:00:00 2001
From: David Henningsson <david.henningsson@canonical.com>
Date: Mon, 20 Dec 2010 12:29:27 +0100
Subject: [PATCH 2/6] alsa-mixer: add required-any and required-* for enum options

Now you can add required-any to elements in a path and the path
will be valid as long as at least one of the elements are present.
Also you can have required, required-any and required-absent in
element options, causing a path to be unsupported if an option is
(not) present (simplified example: to skip line in path if
"Capture source" doesn't have a "Line In" option).

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 src/modules/alsa/alsa-mixer.c                      |   90 +++++++++++++++++---
 src/modules/alsa/alsa-mixer.h                      |    8 ++
 .../alsa/mixer/paths/analog-output.conf.common     |    5 +
 3 files changed, 91 insertions(+), 12 deletions(-)

diff --git a/src/modules/alsa/alsa-mixer.c b/src/modules/alsa/alsa-mixer.c
index eb50ae2..2c47319 100644
--- a/src/modules/alsa/alsa-mixer.c
+++ b/src/modules/alsa/alsa-mixer.c
@@ -1018,6 +1018,38 @@ static int check_required(pa_alsa_element *e, snd_mixer_elem_t *me) {
     if (e->required_absent == PA_ALSA_REQUIRED_ANY && (has_switch || has_volume || has_enumeration))
         return -1;
 
+    if (e->required_any != PA_ALSA_REQUIRED_IGNORE) {
+        switch (e->required_any) {
+        case PA_ALSA_REQUIRED_VOLUME:
+            e->path->req_any_present |= (e->volume_use != PA_ALSA_VOLUME_IGNORE);
+            break;
+        case PA_ALSA_REQUIRED_SWITCH:
+            e->path->req_any_present |= (e->switch_use != PA_ALSA_SWITCH_IGNORE);
+            break;
+        case PA_ALSA_REQUIRED_ENUMERATION:
+            e->path->req_any_present |= (e->enumeration_use != PA_ALSA_ENUMERATION_IGNORE);
+            break;
+        case PA_ALSA_REQUIRED_ANY:
+            e->path->req_any_present |=
+                (e->volume_use != PA_ALSA_VOLUME_IGNORE) ||
+                (e->switch_use != PA_ALSA_SWITCH_IGNORE) ||
+                (e->enumeration_use != PA_ALSA_ENUMERATION_IGNORE);
+            break;
+        }
+    }
+
+    if (e->enumeration_use == PA_ALSA_ENUMERATION_SELECT) {
+        pa_alsa_option *o;
+        PA_LLIST_FOREACH(o, e->options) {
+            e->path->req_any_present |= (o->required_any != PA_ALSA_REQUIRED_IGNORE) &&
+                (o->alsa_idx >= 0);
+            if (o->required != PA_ALSA_REQUIRED_IGNORE && o->alsa_idx < 0)
+                return -1;
+            if (o->required_absent != PA_ALSA_REQUIRED_IGNORE && o->alsa_idx >= 0)
+                return -1;
+        }
+    }
+
     return 0;
 }
 
@@ -1190,9 +1222,6 @@ static int element_probe(pa_alsa_element *e, snd_mixer_t *m) {
 
     }
 
-    if (check_required(e, me) < 0)
-        return -1;
-
     if (e->switch_use == PA_ALSA_SWITCH_SELECT) {
         pa_alsa_option *o;
 
@@ -1224,6 +1253,9 @@ static int element_probe(pa_alsa_element *e, snd_mixer_t *m) {
         }
     }
 
+    if (check_required(e, me) < 0)
+        return -1;
+
     return 0;
 }
 
@@ -1478,20 +1510,23 @@ static int element_parse_required(
 
     pa_alsa_path *p = userdata;
     pa_alsa_element *e;
+    pa_alsa_option *o;
     pa_alsa_required_t req;
 
     pa_assert(p);
 
-    if (!(e = element_get(p, section, TRUE))) {
+    e = element_get(p, section, TRUE);
+    o = option_get(p, section);
+    if (!e && !o) {
         pa_log("[%s:%u] Required makes no sense in '%s'", filename, line, section);
         return -1;
     }
 
     if (pa_streq(rvalue, "ignore"))
         req = PA_ALSA_REQUIRED_IGNORE;
-    else if (pa_streq(rvalue, "switch"))
+    else if (pa_streq(rvalue, "switch") && e)
         req = PA_ALSA_REQUIRED_SWITCH;
-    else if (pa_streq(rvalue, "volume"))
+    else if (pa_streq(rvalue, "volume") && e)
         req = PA_ALSA_REQUIRED_VOLUME;
     else if (pa_streq(rvalue, "enumeration"))
         req = PA_ALSA_REQUIRED_ENUMERATION;
@@ -1502,10 +1537,28 @@ static int element_parse_required(
         return -1;
     }
 
-    if (pa_streq(lvalue, "required-absent"))
-        e->required_absent = req;
-    else
-        e->required = req;
+    if (pa_streq(lvalue, "required-absent")) {
+        if (e)
+            e->required_absent = req;
+        if (o)
+            o->required_absent = req;
+    }
+    else if (pa_streq(lvalue, "required-any")) {
+        if (e) {
+            e->required_any = req;
+            e->path->has_req_any = TRUE;
+        }
+        if (o) {
+            o->required_any = req;
+            o->element->path->has_req_any = TRUE;
+        }
+    }
+    else {
+        if (e)
+            e->required = req;
+        if (o)
+            o->required = req;
+    }
 
     return 0;
 }
@@ -1757,7 +1810,10 @@ static int element_verify(pa_alsa_element *e) {
 
     pa_assert(e);
 
+//    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);
     if ((e->required != PA_ALSA_REQUIRED_IGNORE && e->required == e->required_absent) ||
+        (e->required_any != PA_ALSA_REQUIRED_IGNORE && e->required_any == e->required_absent) ||
+        (e->required_absent == PA_ALSA_REQUIRED_ANY && e->required_any != PA_ALSA_REQUIRED_IGNORE) ||
         (e->required_absent == PA_ALSA_REQUIRED_ANY && e->required != PA_ALSA_REQUIRED_IGNORE)) {
         pa_log("Element %s cannot be required and absent at the same time.", e->alsa_name);
         return -1;
@@ -1836,6 +1892,7 @@ pa_alsa_path* pa_alsa_path_new(const char *fname, pa_alsa_direction_t direction)
         { "override-map.2",      element_parse_override_map,        NULL, NULL },
         /* ... later on we might add override-map.3 and so on here ... */
         { "required",            element_parse_required,            NULL, NULL },
+        { "required-any",        element_parse_required,            NULL, NULL },
         { "required-absent",     element_parse_required,            NULL, NULL },
         { "direction",           element_parse_direction,           NULL, NULL },
         { "direction-try-other", element_parse_direction_try_other, NULL, NULL },
@@ -2079,11 +2136,13 @@ int pa_alsa_path_probe(pa_alsa_path *p, snd_mixer_t *m, pa_bool_t ignore_dB) {
                                 min_dB[t] += e->min_dB;
                                 max_dB[t] += e->max_dB;
                             }
-                    } else
+                    } else {
                         /* Hmm, there's another element before us
                          * which cannot do dB volumes, so we we need
                          * to 'neutralize' this slider */
                         e->volume_use = PA_ALSA_VOLUME_ZERO;
+                        pa_log_info("Zeroing volume of '%s' on path '%s'", e->alsa_name, p->name);
+                    }
                 }
             } else if (p->has_volume)
                 /* We can't use this volume, so let's ignore it */
@@ -2096,6 +2155,12 @@ int pa_alsa_path_probe(pa_alsa_path *p, snd_mixer_t *m, pa_bool_t ignore_dB) {
             p->has_mute = TRUE;
     }
 
+    if (p->has_req_any && !p->req_any_present) {
+        p->supported = FALSE;
+        pa_log_debug("Skipping path '%s', none of required-any elements preset.", p->name);
+        return -1;
+    }
+
     path_drop_unsupported(p);
     path_make_options_unique(p);
     path_create_settings(p);
@@ -2141,13 +2206,14 @@ void pa_alsa_element_dump(pa_alsa_element *e) {
     pa_alsa_option *o;
     pa_assert(e);
 
-    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",
+    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",
                  e->alsa_name,
                  e->direction,
                  e->switch_use,
                  e->volume_use,
                  e->enumeration_use,
                  e->required,
+                 e->required_any,
                  e->required_absent,
                  (long long unsigned) e->merged_mask,
                  e->n_channels,
diff --git a/src/modules/alsa/alsa-mixer.h b/src/modules/alsa/alsa-mixer.h
index a0d4fcb..a6499b6 100644
--- a/src/modules/alsa/alsa-mixer.h
+++ b/src/modules/alsa/alsa-mixer.h
@@ -111,6 +111,10 @@ struct pa_alsa_option {
     char *name;
     char *description;
     unsigned priority;
+
+    pa_alsa_required_t required;
+    pa_alsa_required_t required_any;
+    pa_alsa_required_t required_absent;
 };
 
 /* And element wraps one specific ALSA element. A series of elements *
@@ -128,6 +132,7 @@ struct pa_alsa_element {
     pa_alsa_enumeration_use_t enumeration_use;
 
     pa_alsa_required_t required;
+    pa_alsa_required_t required_any;
     pa_alsa_required_t required_absent;
 
     pa_bool_t override_map:1;
@@ -163,6 +168,9 @@ struct pa_alsa_path {
     pa_bool_t has_mute:1;
     pa_bool_t has_volume:1;
     pa_bool_t has_dB:1;
+    /* These two are used during probing only */
+    pa_bool_t has_req_any:1;
+    pa_bool_t req_any_present:1;
 
     long min_volume, max_volume;
     double min_dB, max_dB;
diff --git a/src/modules/alsa/mixer/paths/analog-output.conf.common b/src/modules/alsa/mixer/paths/analog-output.conf.common
index 6131da5..ffd1b41 100644
--- a/src/modules/alsa/mixer/paths/analog-output.conf.common
+++ b/src/modules/alsa/mixer/paths/analog-output.conf.common
@@ -63,10 +63,15 @@
 ;                                        # by the option name, resp. on/off if the element is a switch.
 ; name = ...                             # Logical name to use in the path identifier
 ; priority = ...                         # Priority if this is made into a device port
+; required = ignore | enumeration | any            # In this element, this option must exist or the path will be invalid. ("any" is an alias for "enumeration".)
+; required-any = ignore | enumeration | any        # In this element, either this or another option must exist (or an element)
+; required-absent = ignore | enumeration | any     # In this element, this option must not exist or the path will be invalid
 ;
 ; [Element ...]                          # For each element that we shall control
 ; required = ignore | switch | volume | enumeration | any     # If set, require this element to be of this kind and available,
 ;                                                             # otherwise don't consider this path valid for the card
+; required-any = ignore | switch | volume | enumeration | any # If set, at least one of the elements with required-any in this
+;                                                             # path must be present, otherwise this path is invalid for the card
 ; required-absent = ignore | switch | volume                  # If set, require this element to not be of this kind and not
 ;                                                             # available, otherwise don't consider this path valid for the card
 ;
-- 
1.7.1

Re: [BUG] git-am silently applying patches incorrectly

[Drew Northup]

On Fri, 2011-03-04 at 13:40 +0000, Colin Guthrie wrote:
> Hi,
> 
> We recently found a bug in git-am 1.7.4.1 while working on PulseAudio.
> 
> It seems that it mis-applied a patch and did so silently without
> generating any warnings. It is reproducible and has been confirmed on
> different distros.
> 
> I make reference to the bug here:
> http://thread.gmane.org/gmane.comp.audio.pulseaudio.general/8840/focus=8857
> 
> In order to reproduce:
> 
> git clone http://git.0pointer.de/repos/pulseaudio.git
> git co -b misapply 0ce3017b7407ab1c4094f7ce271bb68319a7eba7
> git am 0002-alsa-mixer-add-required-any-and-required-for-enum-op.patch
> 
> (I've attached the patch here for convenience).

> For reference, applying the patch manually with patch works fine and
> does not result in an error:
> 
> $ cat 0002-alsa-mixer-add-required-any-and-required-for-enum-op.patch |
> patch -p1
> patching file src/modules/alsa/alsa-mixer.c
> Hunk #1 succeeded at 1121 (offset 103 lines).
> Hunk #2 succeeded at 1325 (offset 103 lines).
> Hunk #3 succeeded at 1356 (offset 103 lines).
> Hunk #4 succeeded at 1613 (offset 103 lines).
> Hunk #5 succeeded at 1640 (offset 103 lines).
> Hunk #6 succeeded at 1913 (offset 103 lines).
> Hunk #7 succeeded at 1997 (offset 105 lines).
> Hunk #8 succeeded at 2242 (offset 106 lines).
> Hunk #9 succeeded at 2261 (offset 106 lines).
> Hunk #10 succeeded at 2312 (offset 106 lines).
> patching file src/modules/alsa/alsa-mixer.h
> Hunk #1 succeeded at 112 (offset 1 line).
> Hunk #2 succeeded at 133 (offset 1 line).
> Hunk #3 succeeded at 169 (offset 1 line).
> patching file src/modules/alsa/mixer/paths/analog-output.conf.common

Did you try removing the first line from the patch mbox file?
It seems to work just fine if you do that. 

That first line is "removed" from the output of "git format-patch" when
you correctly import the mbox file into your mail client's drafts folder
as described in the documentation. Then you send the mail created by
importing that draft.
If you just send the output of "git format-patch" untouched as an
attachment you can expect problems.

-- 
-Drew Northup
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59

Re: [BUG] git-am silently applying patches incorrectly

[Colin Guthrie]

'Twas brillig, and Drew Northup at 04/03/11 16:17 did gyre and gimble:
> 
> On Fri, 2011-03-04 at 13:40 +0000, Colin Guthrie wrote:
>> Hi,
>>
>> We recently found a bug in git-am 1.7.4.1 while working on PulseAudio.
>>
>> It seems that it mis-applied a patch and did so silently without
>> generating any warnings. It is reproducible and has been confirmed on
>> different distros.
>>
>> I make reference to the bug here:
>> http://thread.gmane.org/gmane.comp.audio.pulseaudio.general/8840/focus=8857
>>
>> In order to reproduce:
>>
>> git clone http://git.0pointer.de/repos/pulseaudio.git
>> git co -b misapply 0ce3017b7407ab1c4094f7ce271bb68319a7eba7
>> git am 0002-alsa-mixer-add-required-any-and-required-for-enum-op.patch
>>
>> (I've attached the patch here for convenience).
> 
>> For reference, applying the patch manually with patch works fine and
>> does not result in an error:
>>
>> $ cat 0002-alsa-mixer-add-required-any-and-required-for-enum-op.patch |
>> patch -p1
>> patching file src/modules/alsa/alsa-mixer.c
>> Hunk #1 succeeded at 1121 (offset 103 lines).
>> Hunk #2 succeeded at 1325 (offset 103 lines).
>> Hunk #3 succeeded at 1356 (offset 103 lines).
>> Hunk #4 succeeded at 1613 (offset 103 lines).
>> Hunk #5 succeeded at 1640 (offset 103 lines).
>> Hunk #6 succeeded at 1913 (offset 103 lines).
>> Hunk #7 succeeded at 1997 (offset 105 lines).
>> Hunk #8 succeeded at 2242 (offset 106 lines).
>> Hunk #9 succeeded at 2261 (offset 106 lines).
>> Hunk #10 succeeded at 2312 (offset 106 lines).
>> patching file src/modules/alsa/alsa-mixer.h
>> Hunk #1 succeeded at 112 (offset 1 line).
>> Hunk #2 succeeded at 133 (offset 1 line).
>> Hunk #3 succeeded at 169 (offset 1 line).
>> patching file src/modules/alsa/mixer/paths/analog-output.conf.common
> 
> Did you try removing the first line from the patch mbox file?
> It seems to work just fine if you do that. 

Do you mean the line:

>From ae83e51c82a747332494bf10c245281e49343fe3 Mon Sep 17 00:00:00 2001

?

If so, I removed that line and it still failed to apply correctly with
git am.

> If you just send the output of "git format-patch" untouched as an
> attachment you can expect problems.

Wow! I've never heard of this before... So you're saying it's actually
invalid to do a git format-patch and then a git am on the files it
generates?

If that's the case, then I need to rethink a whole lot of things,
including the way several distros deal with patch management in their
package VCSs.... I'm quite shocked by this! Can you point me to
somewhere in the docs that discusses this?


I'd like to point out that "patch" is able to apply the exact same patch
fine as noted above. To me this seems like a very serious bug in the way
that git-am deals with the application of the patch, but perhaps I'm
missing something.....


Col



-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
  Tribalogic Limited [http://www.tribalogic.net/]
Open Source:
  Mageia Contributor [http://www.mageia.org/]
  PulseAudio Hacker [http://www.pulseaudio.org/]
  Trac Hacker [http://trac.edgewall.org/]

Re: [BUG] git-am silently applying patches incorrectly

[Junio C Hamano]

Colin Guthrie <gmane@colin.guthr.ie> writes:

>> If you just send the output of "git format-patch" untouched as an
>> attachment you can expect problems.
>
> Wow! I've never heard of this before... So you're saying it's actually
> invalid to do a git format-patch and then a git am on the files it
> generates?

I don't think you understand what Drew is saying.

The output from format-patch mimics mbox format already; it specifically
was designed so that "format-patch --stdout | am" pipeline would work
without your doing anything funky.

If you include the output from format-patch in your MUA, however, the
message your MUA will send out would look like:

 * From: ... you ...
 * Subject: Hi, I am sending a patch (the message typed to your MUA)
 * Date: ... date ...

 % From <object name> <date looking format-patch signature string>
 % From: ... author name output by format patch
 % Subject: [PATCH] ... first paragraph from commit log message ...

 . The second paragraph and what follows...
 . ---
 . patch

In the above illustration, the lines marked with "*" are what your MUA
would add as the header, and the ones marked with '%' are the headers
format-patch placed to make its output look like mbox.  You are supposed
to move the "Subject: " line marked with '%' to the Subject input field of
your MUA and drop all other lines marked with '%'.

Drew is talking about the problem it causes to the recipient if you did
not do so, and left '%' lines in your MUA.

Re: [BUG] git-am silently applying patches incorrectly

[Junio C Hamano]

Colin Guthrie <gmane@colin.guthr.ie> writes:

> It seems that it mis-applied a patch and did so silently without
> generating any warnings. It is reproducible and has been confirmed on
> different distros.

The patch text instructs to move the check you have at around ll.1190-1199
to around ll.1224-1230.  Here are the relevant parts.

@@ -1190,9 +1222,6 @@ static int element_probe(pa_alsa_element *e, snd_mixer_t *m) {
 
     }
 
-    if (check_required(e, me) < 0)
-        return -1;
-
     if (e->switch_use == PA_ALSA_SWITCH_SELECT) {
         pa_alsa_option *o;
 
@@ -1224,6 +1253,9 @@ static int element_probe(pa_alsa_element *e, snd_mixer_t *m) {
         }
     }
 
+    if (check_required(e, me) < 0)
+        return -1;
+
     return 0;
 }

Thanks for a report.
 
We find the match for the first hunk (there is only a single callsite) and
correctly remove it, but there are many places that match the preimage of
the second hunk (two blocks closed, blank line and then return 0 from the
function).  We chose to add it to at line 1156, instead of patch's choice
of line 1359, presumably because we thought that is closer to the place
the patch tells us to (i.e. ll.1224-1230).

I haven't looked at the offset logic in git-apply for a long time since
Linus wrote its original version (I don't think the logic has changed very
much since then), but I thought we are taking accumulated offsets into
account when we decide where the patch target should roughly correspond
to.  When we attempt to apply the second hunk, we have already found that
the line the patch says should be at l.1190 is actually at l.1296 (iow,
there are about 100 lines of new material above that the patch didn't
expect), so instead of trying to find the lines that matches the preimage
of the second hunk at around l.1224, we _should_ be trying to find that at
around l.1224+100---perhaps we are not doing that.

Re: [BUG] git-am silently applying patches incorrectly

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> @@ -1224,6 +1253,9 @@ static int element_probe(pa_alsa_element *e, snd_mixer_t *m) {
>          }
>      }
>  
> +    if (check_required(e, me) < 0)
> +        return -1;
> +
>      return 0;
>  }
>
> I haven't looked at the offset logic in git-apply for a long time since
> Linus wrote its original version (I don't think the logic has changed very
> much since then), but I thought we are taking accumulated offsets into
> account when we decide where the patch target should roughly correspond
> to.  When we attempt to apply the second hunk, we have already found that
> the line the patch says should be at l.1190 is actually at l.1296 (iow,
> there are about 100 lines of new material above that the patch didn't
> expect), so instead of trying to find the lines that matches the preimage
> of the second hunk at around l.1224, we _should_ be trying to find that at
> around l.1224+100---perhaps we are not doing that.

I don't necessarily think mucking with the first location we try to apply
using the offset we found by applying the previous hunk is actually a good
thing.  With so many offset lines and multiple places that a hunk can
apply to make the patch application unreliable, that change would be
robbing Peter to pay Paul.  Depending on the nature of the change between
the version the patch is based on and the version the patch is being
applied with offsets, such a heuristics will sometimes err on the wrong
side.

Looking at it closer, however, I noticed that the false hit (i.e. "two
blocks closed, a blank line, return 0 and the end of function") in this
particular case only appears because we applied the previous hunk.  In the
version of the file in 0ce3017b, there is only one such place and there
should be no ambiguity in the patch application.

The problem we are seeing is caused only because we look at the result of
application of the previous hunks in the patch and incrementally try to
apply the remaining hunks.  So clearly "git apply" can and should be fixed
for this case by teaching find_pos() not to report a match on a line that
was touched by application of the previous hunk.

Re: [BUG] git-am silently applying patches incorrectly

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Looking at it closer, however, I noticed that the false hit (i.e. "two
> blocks closed, a blank line, return 0 and the end of function") in this
> particular case only appears because we applied the previous hunk.  In the
> version of the file in 0ce3017b, there is only one such place and there
> should be no ambiguity in the patch application.
>
> The problem we are seeing is caused only because we look at the result of
> application of the previous hunks in the patch and incrementally try to
> apply the remaining hunks.  So clearly "git apply" can and should be fixed
> for this case by teaching find_pos() not to report a match on a line that
> was touched by application of the previous hunk.

And here is a quick and dirty fix to do something like that.  It assumes
that the hunks for a single file being patched are already sorted in the
ascending order (which should be the case), and may regress cases where we
used to find a match even when the version you are patching has moved
functions around in the file by failing to notice a match.  And it does
get the same result as your GNU patch test.

-- >8 --
Subject: [PATCH] apply: do not look behind beyond what we already patched

When looking for a place to apply a hunk, we used to check lines that
match the preimage of it, starting from the line that the patch wants to
apply the hunk at, with increasing offsets in both ways until we find a
match.

Colin Guthrie found an interesting case where this misapplied a patch that
wanted to touch a preimage that consists of "two block closed '<indent>}<LF>',
a blank line, '<indent>return 0;<LF>', the function closed '}<LF>'".  The
target version of the file originally had only one such location, but the hunk
immediately before that created another such preimage, and find_pos() happily
reported that the preimage matched what the hunk wanted to modify.  Oops.

By recording where the last hunk is applied, and limiting the search done
by find_pos() to the rest of the file, we can reduce such an accident.
Ideally, we should not simply limit the upward search like this patch
does, but skip the regions that were touched by previous hunks, but this
approach was much simpler ;-)

I also considered to teach apply_one_fragment() to take the offset we have
found while applying the previous hunk into account when looking for a
match with find_pos(), but dismissed that approach, because it would
sometimes work better but sometimes worse, depending on the difference
between the version the patch was created against and the version the
patch is being applied.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/apply.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 14951da..30857c6 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -211,6 +211,7 @@ struct line {
  */
 struct image {
 	char *buf;
+	size_t last_match;
 	size_t len;
 	size_t nr;
 	size_t alloc;
@@ -2323,11 +2324,11 @@ static int find_pos(struct image *img,
 			return try_lno;
 
 	again:
-		if (backwards_lno == 0 && forwards_lno == img->nr)
+		if (backwards_lno <= img->last_match && forwards_lno == img->nr)
 			break;
 
 		if (i & 1) {
-			if (backwards_lno == 0) {
+			if (backwards_lno <= img->last_match) {
 				i++;
 				goto again;
 			}
@@ -2648,6 +2649,7 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,
 				" to apply fragment at %d\n",
 				leading, trailing, applied_pos+1);
 		update_image(img, applied_pos, &preimage, &postimage);
+		img->last_match = applied_pos;
 	} else {
 		if (apply_verbosely)
 			error("while searching for:\n%.*s",

Re: [BUG] git-am silently applying patches incorrectly

[Linus Torvalds]

On Fri, Mar 4, 2011 at 11:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> And here is a quick and dirty fix to do something like that.  It assumes
> that the hunks for a single file being patched are already sorted in the
> ascending order (which should be the case), and may regress cases where we
> used to find a match even when the version you are patching has moved
> functions around in the file by failing to notice a match.  And it does
> get the same result as your GNU patch test.

Ack. Looks correct. In fact, shouldn't we make that "last_match" be
the _end_ of the last place we applied the patch at, rather than the
beginning?

IOW, maybe something like "img->last_match = applied_pos +
postimage.nr;" or whatever.

I dunno.

                     Linus

Re: [BUG] git-am silently applying patches incorrectly

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> IOW, maybe something like "img->last_match = applied_pos +
> postimage.nr;" or whatever.
>
> I dunno.

I was lazy and didn't want to worry about the consequences of excluding
the end of the context lines in the previous hunk, especially when the
patch was generated with small number of context lines.

But it would probably be a good idea to cut the search at the tail end of
the previous hunk.

Re: [BUG] git-am silently applying patches incorrectly

[Alexander Miseler]

It's a bit intimidating for a newbie to chime in on a discussion between the 
creator and the maintainer, but:
IMHO the biggest problem here isn't the incorrectly, but rather the silently. 
Reducing the chance of guessing incorrectly is good, but git-am still has to 
guess sometimes and it should warn/inform the user when it does that.

Re: [BUG] git-am silently applying patches incorrectly

[Junio C Hamano]

Alexander Miseler <alexander@miseler.de> writes:

> It's a bit intimidating for a newbie to chime in on a discussion between the 
> creator and the maintainer, but:
> IMHO the biggest problem here isn't the incorrectly, but rather the silently. 
> Reducing the chance of guessing incorrectly is good, but git-am still has to 
> guess sometimes and it should warn/inform the user when it does that.

(Please don't cull Cc line).

No need to feel intimidated.  

The patch under discussion was merely "first things first--let's fix the
obviously wrong case that can be fixed without regression".

Try implementing that warning logic, and using it in real-life projects.
You don't actually have to _code_ it, but merely imagining how it would
work and perform would be sufficient for you to realize that it would be
quite expensive (you need to find all the possible mismatches, essentially
scanning the whole file), and worse yet, it would be annoyingly noisy with
many false positives, because in many real-life projects, end of function
tends to match the problematic pattern that triggered this discussion
quite often even without patches that introduce more of the pattern.

Unless you can reduce the false hits to manageable levels, such a warning
is not very useful (it would be useful as a lame excuse "we warned, but
you took the suspicious result", but that does not help the users).

In short, Linus and I both know what you are talking about, and we may
revisit that issue later, but the thing is that it would not be very
pleasant, and not something that can be done in one sitting during a
single discussion thread on the list.

Re: [BUG] git-am silently applying patches incorrectly

[Drew Northup]

On Fri, 2011-03-04 at 11:05 -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Looking at it closer, however, I noticed that the false hit (i.e. "two
> > blocks closed, a blank line, return 0 and the end of function") in this
> > particular case only appears because we applied the previous hunk.  In the
> > version of the file in 0ce3017b, there is only one such place and there
> > should be no ambiguity in the patch application.
> >
> > The problem we are seeing is caused only because we look at the result of
> > application of the previous hunks in the patch and incrementally try to
> > apply the remaining hunks.  So clearly "git apply" can and should be fixed
> > for this case by teaching find_pos() not to report a match on a line that
> > was touched by application of the previous hunk.
> 
> And here is a quick and dirty fix to do something like that.  It assumes
> that the hunks for a single file being patched are already sorted in the
> ascending order (which should be the case), and may regress cases where we
> used to find a match even when the version you are patching has moved
> functions around in the file by failing to notice a match.  And it does
> get the same result as your GNU patch test.
> 

It checks out here applied against master. I don't know how I got it to
work the first time without this patch--but I'm pretty sure I don't want
to know at this point.

-- 
-Drew Northup
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59

Re: [BUG] git-am silently applying patches incorrectly

[Colin Guthrie]

'Twas brillig, and Junio C Hamano at 04/03/11 21:33 did gyre and gimble:
> In short, Linus and I both know what you are talking about, and we may
> revisit that issue later, but the thing is that it would not be very
> pleasant, and not something that can be done in one sitting during a
> single discussion thread on the list.

As a simple option to avoid that, how about just printing out (by
default) the line offsets if hunks don't apply 100% cleanly? This would
at least alert you to the fact that some fixups were needed.

Just a thought...


-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
  Tribalogic Limited [http://www.tribalogic.net/]
Open Source:
  Mageia Contributor [http://www.mageia.org/]
  PulseAudio Hacker [http://www.pulseaudio.org/]
  Trac Hacker [http://trac.edgewall.org/]

Re: [BUG] git-am silently applying patches incorrectly

[Junio C Hamano]

Colin Guthrie <gmane@colin.guthr.ie> writes:

> 'Twas brillig, and Junio C Hamano at 04/03/11 21:33 did gyre and gimble:
>> In short, Linus and I both know what you are talking about, and we may
>> revisit that issue later, but the thing is that it would not be very
>> pleasant, and not something that can be done in one sitting during a
>> single discussion thread on the list.
>
> As a simple option to avoid that, how about just printing out (by
> default) the line offsets if hunks don't apply 100% cleanly? This would
> at least alert you to the fact that some fixups were needed.

Yeah, that is what GNU patch does, and would be a small improvement that
might be in a right direction.

While we are at it, here is an alternate patch that does not lose the
ability to cope with the case where the target version has functions moved
around without introducing new ambiguous patch application sites.  Instead
of keeping the "last position was here -- we won't look beyond that", we
mark the lines that were brought into the target by the patch application
so far, and reject preimage matches against these lines.

A full solution for detecting a potential ambiguity and warning it would
be based on this version instead.  It would involve letting find_pos() not
stop at the first hit near the intended target, and marking the hunk that
can apply at more than one location.

A yet even more reliable alternative solution _might_ be to first scan the
original without applying any hunks just to find the possible sites to be
patched, warn ambiguities and decide & commit to these patch application
sites, and then apply the hunks.  If we did so, we wouldn't need this
patch nor the previous one.

But that would be a larger change, and would require a good test vector,
perhaps a large quilt series, to make sure it does not introduce
regression.


 builtin/apply.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 14951da..04f56f8 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -204,6 +204,7 @@ struct line {
 	unsigned hash : 24;
 	unsigned flag : 8;
 #define LINE_COMMON     1
+#define LINE_PATCHED	2
 };
 
 /*
@@ -2085,7 +2086,8 @@ static int match_fragment(struct image *img,
 
 	/* Quick hash check */
 	for (i = 0; i < preimage_limit; i++)
-		if (preimage->line[i].hash != img->line[try_lno + i].hash)
+		if ((img->line[try_lno + i].flag & LINE_PATCHED) ||
+		    (preimage->line[i].hash != img->line[try_lno + i].hash))
 			return 0;
 
 	if (preimage_limit == preimage->nr) {
@@ -2428,6 +2430,9 @@ static void update_image(struct image *img,
 	memcpy(img->line + applied_pos,
 	       postimage->line,
 	       postimage->nr * sizeof(*img->line));
+	for (i = 0; i < postimage->nr; i++)
+		img->line[applied_pos + i].flag |= LINE_PATCHED;
+
 	img->nr = nr;
 }
 

Re: [BUG] git-am silently applying patches incorrectly

[Junio C Hamano]

Colin Guthrie <gmane@colin.guthr.ie> writes:

> 'Twas brillig, and Junio C Hamano at 04/03/11 21:33 did gyre and gimble:
>> In short, Linus and I both know what you are talking about, and we may
>> revisit that issue later, but the thing is that it would not be very
>> pleasant, and not something that can be done in one sitting during a
>> single discussion thread on the list.
>
> As a simple option to avoid that, how about just printing out (by
> default) the line offsets if hunks don't apply 100% cleanly? This would
> at least alert you to the fact that some fixups were needed.
>
> Just a thought...

... and a patch to do so would look like this.  "git apply -v" and (GNU)
"patch -p1" seems to report exactly the same numbers for the problematic
patch and the initial state that started this discussion.

 builtin/apply.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 14951da..4d22d16 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -2638,6 +2643,14 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,
 				apply = 0;
 		}
 
+		if (apply_verbosely && applied_pos != pos) {
+			int offset = applied_pos - pos;
+			if (offset < 0)
+				offset = 0 - offset;
+			fprintf(stderr, "Applied at %d (offset %d line(s)).\n",
+				applied_pos + 1, offset);
+		}
+
 		/*
 		 * Warn if it was necessary to reduce the number
 		 * of context lines.

Re: [BUG] git-am silently applying patches incorrectly

[Junio C Hamano]

Sorry to bother you with another review request; I slightly prefer this
one better.  The exact same issue, different approach.

-- >8 --
Subject: [PATCH] apply: do not patch lines that were already patched

When looking for a place to apply a hunk, we used to check lines that
match the preimage of it, starting from the line that the patch wants to
apply the hunk at, looking forward and backward with increasing offsets
until we find a match.

Colin Guthrie found an interesting case where this misapplied a patch that
wanted to touch a preimage that consists of

                        }
                }

                return 0;
        }

which is a rather unfortunately common pattern.

The target version of the file originally had only one such location, but
the hunk immediately before that created another instance of such block of
lines, and find_pos() happily reported that the preimage of the hunk
matched what it wanted to modify.

Oops.

By marking the lines application of earlier hunks touched and preventing
match_fragment() from considering them as a match with preimage of other
hunks, we can reduce such an accident.

I also considered to teach apply_one_fragment() to take the offset we have
found while applying the previous hunk into account when looking for a
match with find_pos(), but dismissed that approach, because it would
sometimes work better but sometimes worse, depending on the difference
between the version the patch was created against and the version the
patch is being applied.

This does _not_ prevent misapplication of patches to a file that has many
similar looking blocks of lines and a preimage cannot identify which one
of them should be applied.  For that, we would need to scan beyond the
first match in find_pos(), and issue a warning (or error out).  That will
be a separate topic.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/apply.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 14951da..04f56f8 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -204,6 +204,7 @@ struct line {
 	unsigned hash : 24;
 	unsigned flag : 8;
 #define LINE_COMMON     1
+#define LINE_PATCHED	2
 };
 
 /*
@@ -2085,7 +2086,8 @@ static int match_fragment(struct image *img,
 
 	/* Quick hash check */
 	for (i = 0; i < preimage_limit; i++)
-		if (preimage->line[i].hash != img->line[try_lno + i].hash)
+		if ((img->line[try_lno + i].flag & LINE_PATCHED) ||
+		    (preimage->line[i].hash != img->line[try_lno + i].hash))
 			return 0;
 
 	if (preimage_limit == preimage->nr) {
@@ -2428,6 +2430,9 @@ static void update_image(struct image *img,
 	memcpy(img->line + applied_pos,
 	       postimage->line,
 	       postimage->nr * sizeof(*img->line));
+	for (i = 0; i < postimage->nr; i++)
+		img->line[applied_pos + i].flag |= LINE_PATCHED;
+
 	img->nr = nr;
 }
 
-- 
1.7.4.1.287.g1ecf2

Re: [BUG] git-am silently applying patches incorrectly

[Alexander Miseler]

On 04.03.2011 22:33, Junio C Hamano wrote:
> (Please don't cull Cc line).

Sorry. I used the nice gmane web interface and hoped that it keeps the CC intact, which it apparently doesn't. I guess i 
will go old school now and use the mailing list via actual emails :)


> Try implementing that warning logic, and using it in real-life projects.
> You don't actually have to _code_ it, but merely imagining how it would
> work and perform would be sufficient for you to realize that it would be
> quite expensive (you need to find all the possible mismatches, essentially
> scanning the whole file), and worse yet, it would be annoyingly noisy with
> many false positives, because in many real-life projects, end of function
> tends to match the problematic pattern that triggered this discussion
> quite often even without patches that introduce more of the pattern.
>
> Unless you can reduce the false hits to manageable levels, such a warning
> is not very useful (it would be useful as a lame excuse "we warned, but
> you took the suspicious result", but that does not help the users).
>
> In short, Linus and I both know what you are talking about, and we may
> revisit that issue later, but the thing is that it would not be very
> pleasant, and not something that can be done in one sitting during a
> single discussion thread on the list.

Understood. On a side note: if this problem is tackled it might be sensible to add a heuristic to git format-patch that 
increases the context size for hunks that are likely to be ambiguous. "Likely to be ambiguous" is of course a problem in 
itself but even a less than perfect detection might be helpful and it would suffer less from some of the aforementioned 
problems, like noisiness/false hits, which would just increase the patch size instead of harassing the user.

Re: [BUG] git-am silently applying patches incorrectly

[Junio C Hamano]

Alexander Miseler <alexander@miseler.de> writes:

> .... On a side note: if this problem is tackled it might be
> sensible to add a heuristic to git format-patch that increases the
> context size for hunks that are likely to be ambiguous.

In general that approach would not help.  Imagine a case where the
problematic patch from David Henningsson were only about moving the
calling site of check_required() within the element_probe() function.
IOW, imagine that the hunk starting at line 1018 to modify
check_required() itself weren't involved in his change.

Before or after such a change, there would be only one location that
closes two blocks followed by a blank line and return 0 at the end of the
function, which is the necessary context of the "moved after" hunk of the
patch, so when producing the patch, there is no way for format-patch to
notice that it is likely to become ambiguous.

But if the change to check_required() were applied as a separate patch
before the change to move the call site was applied, at the application
time, the patch becomes ambiguous.

Re: [BUG] git-am silently applying patches incorrectly

[Colin Guthrie]

'Twas brillig, and Junio C Hamano at 04/03/11 22:42 did gyre and gimble:
> Colin Guthrie <gmane@colin.guthr.ie> writes:
> 
>> 'Twas brillig, and Junio C Hamano at 04/03/11 21:33 did gyre and gimble:
>>> In short, Linus and I both know what you are talking about, and we may
>>> revisit that issue later, but the thing is that it would not be very
>>> pleasant, and not something that can be done in one sitting during a
>>> single discussion thread on the list.
>>
>> As a simple option to avoid that, how about just printing out (by
>> default) the line offsets if hunks don't apply 100% cleanly? This would
>> at least alert you to the fact that some fixups were needed.
>>
>> Just a thought...
> 
> ... and a patch to do so would look like this.  "git apply -v" and (GNU)
> "patch -p1" seems to report exactly the same numbers for the problematic
> patch and the initial state that started this discussion.
> 
>  builtin/apply.c |   15 ++++++++++++++-
>  1 files changed, 14 insertions(+), 1 deletions(-)
> 
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 14951da..4d22d16 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -2638,6 +2643,14 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,
>  				apply = 0;
>  		}
>  
> +		if (apply_verbosely && applied_pos != pos) {
> +			int offset = applied_pos - pos;
> +			if (offset < 0)
> +				offset = 0 - offset;
> +			fprintf(stderr, "Applied at %d (offset %d line(s)).\n",
> +				applied_pos + 1, offset);
> +		}
> +
>  		/*
>  		 * Warn if it was necessary to reduce the number
>  		 * of context lines.

Personally I wouldn't bother making offset absolute... (equiv of
abs(offset)) as knowing it applied earlier or later could be useful...
the direction is lost here and I don't really see why that's nicer for
the user. But maybe that's just my opinion?

Col

PS Many thanks for working on this :)


-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
  Tribalogic Limited [http://www.tribalogic.net/]
Open Source:
  Mageia Contributor [http://www.mageia.org/]
  PulseAudio Hacker [http://www.pulseaudio.org/]
  Trac Hacker [http://trac.edgewall.org/]

Re: [BUG] git-am silently applying patches incorrectly

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> In any case, here is an update to match what GNU patch seems to do more
> closely.
> ...

This is an unrelated tangent, but in a separate thread there was a
discussion on "%d noun(s)" in a recent weatherbaloon patch, and use of
ngettext(3) was suggested to solve this portably to languages other than
Germanic and Romanic family.

So here is my exercise for preparing the new code for upcoming i18n.
Does it look sane?

Do we want a new wrapper similar to _() that would easily make this into a
noop under NO_GETTEXT in the proposed i18n infrastructure?

 builtin/apply.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index a231c0c..f084250 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -2644,7 +2644,10 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,
 			if (apply_in_reverse)
 				offset = 0 - offset;
 			fprintf(stderr,
+				ngettext(
+				"Hunk #%d succeeded at %d (offset %d line).\n",
 				"Hunk #%d succeeded at %d (offset %d lines).\n",
+				(offset < 0 ? (0 - offset) : offset)),
 				nth_fragment, applied_pos + 1, offset);
 		}
 

Re: [BUG] git-am silently applying patches incorrectly

[Junio C Hamano]

Colin Guthrie <gmane@colin.guthr.ie> writes:

> 'Twas brillig, and Junio C Hamano at 04/03/11 22:42 did gyre and gimble:
>> ... and a patch to do so would look like this.  "git apply -v" and (GNU)
>> "patch -p1" seems to report exactly the same numbers for the problematic
>> patch and the initial state that started this discussion.
>> ... 
>
> Personally I wouldn't bother making offset absolute... (equiv of
> abs(offset)) as knowing it applied earlier or later could be useful...
> the direction is lost here and I don't really see why that's nicer for
> the user. But maybe that's just my opinion?

I don't have a strong opinion on this either way; I would just imitate
what GNU patch would do, which would probably be to show the offset as-is,
except that it flips the sign if it is being run in reverse with -R
option.

A bigger question I would actually care _more_ about is if this should be
on by default without -v.  We usually do not allow fuzz by default for
safety, and we do warn loudly when -C reduces the context and we actually
need to use it to match the preimage.

In any case, here is an update to match what GNU patch seems to do more
closely.

-- >8 --
Subject: [PATCH] apply -v: show offset count when patch did not apply exactly

When the line number the patch intended to touch does not match
the line in the version being patched, GNU patch reports that
it applied the hunk at a different line number, with how big an
offset.

Teach "git apply" to do the same under --verbose option.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/apply.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 14951da..a231c0c 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -2432,7 +2432,8 @@ static void update_image(struct image *img,
 }
 
 static int apply_one_fragment(struct image *img, struct fragment *frag,
-			      int inaccurate_eof, unsigned ws_rule)
+			      int inaccurate_eof, unsigned ws_rule,
+			      int nth_fragment)
 {
 	int match_beginning, match_end;
 	const char *patch = frag->patch;
@@ -2638,6 +2639,15 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,
 				apply = 0;
 		}
 
+		if (apply_verbosely && applied_pos != pos) {
+			int offset = applied_pos - pos;
+			if (apply_in_reverse)
+				offset = 0 - offset;
+			fprintf(stderr,
+				"Hunk #%d succeeded at %d (offset %d lines).\n",
+				nth_fragment, applied_pos + 1, offset);
+		}
+
 		/*
 		 * Warn if it was necessary to reduce the number
 		 * of context lines.
@@ -2785,12 +2795,14 @@ static int apply_fragments(struct image *img, struct patch *patch)
 	const char *name = patch->old_name ? patch->old_name : patch->new_name;
 	unsigned ws_rule = patch->ws_rule;
 	unsigned inaccurate_eof = patch->inaccurate_eof;
+	int nth = 0;
 
 	if (patch->is_binary)
 		return apply_binary(img, patch);
 
 	while (frag) {
-		if (apply_one_fragment(img, frag, inaccurate_eof, ws_rule)) {
+		nth++;
+		if (apply_one_fragment(img, frag, inaccurate_eof, ws_rule, nth)) {
 			error("patch failed: %s:%ld", name, frag->oldpos);
 			if (!apply_with_reject)
 				return -1;
-- 
1.7.4.1.299.ga459d

Re: [BUG] git-am silently applying patches incorrectly

[Junio C Hamano]

Junio C Hamano <junio@pobox.com> writes:

> ...
> So here is my exercise for preparing the new code for upcoming i18n.
> Does it look sane?
>
> Do we want a new wrapper similar to _() that would easily make this into a
> noop under NO_GETTEXT in the proposed i18n infrastructure?
>
>  builtin/apply.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index a231c0c..f084250 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -2644,7 +2644,10 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,
>  			if (apply_in_reverse)
>  				offset = 0 - offset;
>  			fprintf(stderr,
> +				ngettext(
> +				"Hunk #%d succeeded at %d (offset %d line).\n",
>  				"Hunk #%d succeeded at %d (offset %d lines).\n",
> +				(offset < 0 ? (0 - offset) : offset)),
>  				nth_fragment, applied_pos + 1, offset);
>  		}
>  

If we were to do i18n, we would probably need to include something like
the following in the early fast-tracked part of the series, perhaps as
part of the e6bb27e (i18n: add no-op _() and N_() wrappers, 2011-02-22)



 gettext.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/gettext.h b/gettext.h
index 6949d73..1510c5d 100644
--- a/gettext.h
+++ b/gettext.h
@@ -23,4 +23,10 @@ static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)
 /* Mark msgid for translation but do not translate it. */
 #define N_(msgid) (msgid)
 
+static inline const char *ngettext(const char *msgid, const char *plu, unsigned long n)
+{
+	/* fallback ngettext() without using libintl */
+	return (n == 1) ? msgid : plu;
+}
+
 #endif

Re: [BUG] git-am silently applying patches incorrectly

[Jonathan Nieder]

Junio C Hamano wrote:

> If we were to do i18n, we would probably need to include something like
> the following in the early fast-tracked part of the series, perhaps as
> part of the e6bb27e (i18n: add no-op _() and N_() wrappers, 2011-02-22)

Yep.  Is it safe to do this without the

#define ngettext git_ngettext
static inline const char *git_ngettext(...)

dance?

Re: [BUG] git-am silently applying patches incorrectly

[Colin Guthrie]

'Twas brillig, and Junio C Hamano at 06/03/11 22:15 did gyre and gimble:
>> > Personally I wouldn't bother making offset absolute... (equiv of
>> > abs(offset)) as knowing it applied earlier or later could be useful...
>> > the direction is lost here and I don't really see why that's nicer for
>> > the user. But maybe that's just my opinion?
> I don't have a strong opinion on this either way; I would just imitate
> what GNU patch would do, which would probably be to show the offset as-is,
> except that it flips the sign if it is being run in reverse with -R
> option.

Yeah I think that's quite sensible. I think converging with the way GNU
patch does it except when there is really good reason not to makes a lot
of sense, if nothing more than general familiarity and expectations.

> A bigger question I would actually care _more_ about is if this should be
> on by default without -v.  We usually do not allow fuzz by default for
> safety, and we do warn loudly when -C reduces the context and we actually
> need to use it to match the preimage.

Users used to using patch may simply think there are no offset
adjustments when using git am and live in blissful ignorance. For that
reason I'd say it should be on by default. But then again, I've been
recently jaded by a mis-applied patch... if I'm honest, I would probably
say that 99 times in a 100, I couldn't really care less (or really read)
the offset adjustments.... so I can't really comment very subjectively
here :s

> In any case, here is an update to match what GNU patch seems to do more
> closely.

Looks good to me!

Thanks again for looking into this issue :) Hopefully the primary fix
can be pushed soon and the nice usability improvements that have spawned
from it can head the same direction too :)

Col

-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
  Tribalogic Limited [http://www.tribalogic.net/]
Open Source:
  Mageia Contributor [http://www.mageia.org/]
  PulseAudio Hacker [http://www.pulseaudio.org/]
  Trac Hacker [http://trac.edgewall.org/]

[RFC/PATCH 0/2] i18n: add ngettext stub

[Jonathan Nieder]

Junio C Hamano wrote:
> On Mar 6, 2011 3:07 PM, "Jonathan Nieder" <jrnieder@gmail.com> wrote:

>> Yep.  Is it safe to do this without the
>>
>> #define ngettext git_ngettext
>> static inline const char *git_ngettext(...)
>>
>> dance?
>
> Heh you tell me.

Yegh.  My general feeling was that we should make sure gettext.h works
even if <libintl.h> was included elsewhere, just in case some system
header decides to start including it.  That makes life slightly less
pleasant, since libintl.h does

 #if defined __OPTIMIZE__ && !defined __cplusplus
[...]
 # define ngettext(msgid1, msgid2, n) dngettext (NULL, msgid1, msgid2, n)
[...]
 #endif	/* Optimizing.  */

How about this?

Jonathan Nieder (1):
  i18n: avoid conflict with ngettext from libintl

Junio C Hamano (1):
  i18n: add no-op ngettext() fallback

 gettext.h |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

-- 
1.7.4.1

[PATCH 1/2] i18n: add stub ngettext implementation

[Jonathan Nieder]

From: Junio C Hamano <gitster@pobox.com>
Date: Sun, 6 Mar 2011 14:40:05 -0800

The ngettext function translates a string representing some pharse
with an alternative plural form and uses the 'count' argument to
choose which form to return.  Use of ngettext solves the "%d noun(s)"
problem in a way that is portable to languages outside the Germanic
and Romance families.

In English, the semantics of ngettext(sing, plur, count) are roughly
equivalent to

	count == 1 ? _(sing) : _(plur)

while in other languages there can be more variants (count == 0; more
random-looking rules based on the historical pronunciation of the
number).  Behind the scenes, the singular form is used to look up a
family of translations and the plural form is ignored unless no
translation is available.

Add a simple wrapper with the English semantics so C code can start
using it to mark phrases with a count for translation.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
This is based against a version of gettext.h with the fixup
(s/# GETTEXT POISON #/ GETTEXT POISON /) from
http://thread.gmane.org/gmane.comp.version-control.git/167661/focus=167878
Applying that substitution to the patch should be enough for it to
apply to a gettext.h without that change.

 gettext.h |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/gettext.h b/gettext.h
index 73831aa..03fb340 100644
--- a/gettext.h
+++ b/gettext.h
@@ -26,6 +26,14 @@ static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)
 	return use_gettext_poison() ? " GETTEXT POISON " : msgid;
 }
 
+static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)
+const char *ngettext(const char *msgid, const char *plu, unsigned long n)
+{
+	if (use_gettext_poison())
+		return " GETTEXT POISON ";
+	return n == 1 ? msgid : plu;
+}
+
 /* Mark msgid for translation but do not translate it. */
 #define N_(msgid) (msgid)
 
-- 
1.7.4.1

[PATCH 2/2] i18n: avoid conflict with ngettext from libintl

[Jonathan Nieder]

Although git itself would not be using "#include <libintl.h>" anywhere
in a NO_GETTEXT build, git's gettext.h is meant to tolerate prior
declarations from libintl, to prepare for a scenario in which some
system or compat/ header decides to start including libintl.  GNU
libintl.h defines ngettext as a macro when __OPTIMIZE__ is defined, so
take care to "#undef ngettext" if it was defined for us.

To avoid having to worry about a conflicting ngettext symbol when
libintl is part of libc, also rename the no-op ngettext stub to
git_ngettext and make ngettext a macro referring to it.  This is
probably never necessary (because git's ngettext is declared "static
inline") but it buys peace of mind.

This change does not protect against conflicts due to a header
included _after_ git's i18n support (e.g., pthread.h) being the first
to pull in libintl.  We can deal with that separately if it happens.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 gettext.h |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/gettext.h b/gettext.h
index 03fb340..a473af4 100644
--- a/gettext.h
+++ b/gettext.h
@@ -13,6 +13,10 @@
 #error "namespace conflict: '_' is pre-defined?"
 #endif
 
+#ifdef ngettext
+#undef ngettext
+#endif
+
 #define FORMAT_PRESERVING(n) __attribute__((format_arg(n)))
 
 #ifdef GETTEXT_POISON
@@ -26,8 +30,9 @@ static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)
 	return use_gettext_poison() ? " GETTEXT POISON " : msgid;
 }
 
+#define ngettext git_ngettext
 static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)
-const char *ngettext(const char *msgid, const char *plu, unsigned long n)
+const char *git_ngettext(const char *msgid, const char *plu, unsigned long n)
 {
 	if (use_gettext_poison())
 		return " GETTEXT POISON ";
-- 
1.7.4.1

Re: [PATCH 2/2] i18n: avoid conflict with ngettext from libintl

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> To avoid having to worry about a conflicting ngettext symbol when
> libintl is part of libc, also rename the no-op ngettext stub to
> git_ngettext and make ngettext a macro referring to it.  This is
> probably never necessary (because git's ngettext is declared "static
> inline") but it buys peace of mind.

Hmph.  An obviously safer alternative would be to use git_ngettext() in
our source all over the place, and it would by even more peace of mind but
that is even longer.

> This change does not protect against conflicts due to a header
> included _after_ git's i18n support (e.g., pthread.h) being the first
> to pull in libintl.  We can deal with that separately if it happens.

Also the same problem exists already for the _() macro.

Re: [PATCH 2/2] i18n: avoid conflict with ngettext from libintl

[Jonathan Nieder]

Junio C Hamano wrote:

> Hmph.  An obviously safer alternative would be to use git_ngettext() in
> our source all over the place, and it would by even more peace of mind but
> that is even longer.

Right.  That is tempting.

Ævar, is there some usual or obvious abbreviated form for ngettext we
could use to avoid this fuss altogether?

> Also the same problem exists already for the _() macro.

The usual convention is that the _() macro is private to each
application.  libintl provides a gettext function or macro, and
various programs do

	#define _(msg) gettext(msg)

in some private header (that does not pollute the public namespace)
for notational convenience.

Re: [PATCH 2/2] i18n: avoid conflict with ngettext from libintl

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> The usual convention is that the _() macro is private to each
> application.  libintl provides a gettext function or macro, and
> various programs do
>
> 	#define _(msg) gettext(msg)
>
> in some private header (that does not pollute the public namespace)
> for notational convenience.

Yeah, I am aware of that.  Is there a similar convention for [dn]gettext?
Perhaps not....

[PATCH v2] i18n: add stub Q_() wrapper for ngettext

[Jonathan Nieder]

From: Junio C Hamano <gitster@pobox.com>
Date: Sun, 6 Mar 2011 14:40:05 -0800
Subject: i18n: add stub Q_() wrapper for ngettext

The Q_ function translates a string representing some pharse with an
alternative plural form and uses the 'count' argument to choose which
form to return.  Use of Q_ solves the "%d noun(s)" problem in a way
that is portable to languages outside the Germanic and Romance
families.

In English, the semantics of Q_(sing, plur, count) are roughly
equivalent to

	count == 1 ? _(sing) : _(plur)

while in other languages there can be more variants (count == 0; more
random-looking rules based on the historical pronunciation of the
number).  Behind the scenes, the singular form is used to look up a
family of translations and the plural form is ignored unless no
translation is available.

Define such a Q_ in gettext.h with the English semantics so C code can
start using it to mark phrases with a count for translation.

The name "Q_" is taken from subversion and stands for "quantity".
Many projects just use ngettext directly without a wrapper analogous
to _; we should not do so because git's gettext.h is meant not to
conflict with system headers that might include libintl.h.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Junio C Hamano wrote:

> Yeah, I am aware of that.  Is there a similar convention for [dn]gettext?
> Perhaps not....

subversion uses Q_.  Though it does not seem to be standard --- e.g.,
glib uses:

 _() for gettext
 Q_() for a variant on gettext that allows a string of the form
  "context|message" as its argument;
 C_() for a nicer version of Q_() that takes the context and message
  as distinct arguments;
 N_() to mark a string for translation without translating it;
 NC_() to mark a string with context for translation without
  translating it.

I suppose Q_ is as good a name as any.

Hopefully veterans from glib would not be used to glib's alternative
meaning of Q_, preferring to use C_ for messages with flags.

 gettext.h |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/gettext.h b/gettext.h
index 04b5958..1b253b7 100644
--- a/gettext.h
+++ b/gettext.h
@@ -9,8 +9,8 @@
 #ifndef GETTEXT_H
 #define GETTEXT_H
 
-#ifdef _
-#error "namespace conflict: '_' is pre-defined?"
+#if defined(_) || defined(Q_)
+#error "namespace conflict: '_' or 'Q_' is pre-defined?"
 #endif
 
 #define FORMAT_PRESERVING(n) __attribute__((format_arg(n)))
@@ -26,6 +26,14 @@ static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)
 	return use_gettext_poison() ? "# GETTEXT POISON #" : msgid;
 }
 
+static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)
+const char *Q_(const char *msgid, const char *plu, unsigned long n)
+{
+	if (use_gettext_poison())
+		return "# GETTEXT POISON #";
+	return n == 1 ? msgid : plu;
+}
+
 /* Mark msgid for translation but do not translate it. */
 #define N_(msgid) (msgid)
 
-- 
1.7.4.1

Re: [PATCH v2] i18n: add stub Q_() wrapper for ngettext

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> I suppose Q_ is as good a name as any.

Ok, let's run with this for now, as we hopefully don't have too many
places that might want to use ngettext(), and they should wait until the
early parts of the series settles and in-flight topics are adjusted to the
barebones infrastructure.

Thanks.

Re: [PATCH 2/2] i18n: avoid conflict with ngettext from libintl

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 9, 2011 at 21:51, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Junio C Hamano wrote:
>
>> Hmph.  An obviously safer alternative would be to use git_ngettext() in
>> our source all over the place, and it would by even more peace of mind but
>> that is even longer.
>
> Right.  That is tempting.
>
> Ævar, is there some usual or obvious abbreviated form for ngettext we
> could use to avoid this fuss altogether?

I was looking yesterday but I couldn't find a common. I think your
Q_() is fine.

I see GNU Make uses S_() internally, netcat uses PL_(). There seems to
be no common convention for it like for gettext().

Personally I'd have chosen n_(), although confusing with our existing
N_() we could add dn_(), dcn_() etc for dngettext() and dcngettext()
later.

But I don't think it matters. Let's use your patch as-is.

Re: [PATCH v2] i18n: add stub Q_() wrapper for ngettext

[Ævar Arnfjörð Bjarmason]

On Thu, Mar 10, 2011 at 08:59, Junio C Hamano <gitster@pobox.com> wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> I suppose Q_ is as good a name as any.
>
> Ok, let's run with this for now, as we hopefully don't have too many
> places that might want to use ngettext(), and they should wait until the
> early parts of the series settles and in-flight topics are adjusted to the
> barebones infrastructure.

Yeah, it looks good to me. You can add my Acked-by to it if you'd like.