git-am silently applying patches incorrectly

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/]

From: David Henningsson <[email protected]>
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).

---
 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(-)

index eb50ae2..2c47319 100644
@@ -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,

index a0d4fcb..a6499b6 100644
@@ -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;

index 6131da5..ffd1b41 100644
@@ -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

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

'Twas brillig, and Drew Northup at 04/03/11 16:17 did gyre and gimble:

Do you mean the line:

?

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

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/]

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.

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.

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.

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.

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

index 14951da..30857c6 100644
@@ -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",

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

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

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.

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.

'Twas brillig, and Junio C Hamano at 04/03/11 21:33 did gyre and gimble:

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/]

... 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(-)

index 14951da..4d22d16 100644
@@ -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.

'Twas brillig, and Junio C Hamano at 04/03/11 22:42 did gyre and gimble:

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/]

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.

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

index 14951da..a231c0c 100644
@@ -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

'Twas brillig, and Junio C Hamano at 06/03/11 22:15 did gyre and gimble:

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.

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

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/]

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(-)

index a231c0c..f084250 100644
@@ -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);
 		}
 

Yep. Is it safe to do this without the

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

dance?

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

From: Junio C Hamano <[email protected]>
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.

---
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(-)

index 73831aa..03fb340 100644
@@ -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

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.

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

index 03fb340..a473af4 100644
@@ -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

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.

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

Right. That is tempting.

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

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....

From: Junio C Hamano <[email protected]>
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.

---

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(-)

index 04b5958..1b253b7 100644
@@ -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

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.

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

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.

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(-)

index 14951da..04f56f8 100644
@@ -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;
 }
 

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 :)

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.

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.

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.

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

index 14951da..04f56f8 100644
@@ -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