[BUG] git-am silently applying patches incorrectly

[Colin Guthrie <gmane@colin.guthr.ie>]

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