Fix trailers atom bug and improved tests

t/t6300-for-each-ref.sh +0 -0

index a83579fbdf..495848c881 100755
@@ -776,60 +776,39 @@ test_expect_success 'set up trailers for next test' '
 '
 
 test_expect_success '%(trailers:unfold) unfolds trailers' '

t/t6300-for-each-ref.sh +0 -2

@@ -839,14 +818,7 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '
 	fatal: unknown %(trailers) argument: unsupported
 	EOF
 	test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual &&
-	test_i18ncmp expect actual
-'

From: Hariom Verma <[email protected]>

The 'contents' atom does not show any error if used with 'trailers' atom and semicolon is missing before trailers arguments.

e.g %(contents:trailersonly) works, while it shouldn't.

It is definitely not an expected behavior.

Let's fix this bug.

Mentored-by Christian Couder <[email protected]>

Mentored-by Heba Waly <[email protected]>

Signed-off-by Hariom Verma <[email protected]>

---

 ref-filter.c            | 21 ++++++++++++++++++---
 t/t6300-for-each-ref.sh |  9 +++++++++
 2 files changed, 27 insertions(+), 3 deletions(-)

ref-filter.c +18 -3

index ba85869755..dc31fbbe51 100644
@@ -332,6 +332,22 @@ static int trailers_atom_parser(const struct ref_format *format, struct used_ato
 	return 0;
 }
 
+static int check_format_field(const char *arg, const char *field, const char **option)
+{
+	const char *opt;
+	if (skip_prefix(arg, field, &opt)) {
+		if (*opt == '\0') {
+			*option = NULL;
+			return 1;
+		}
+		else if (*opt == ':') {
+			*option = ++opt;
+			return 1;
+		}
+	}
+	return 0;
+}
+
 static int contents_atom_parser(const struct ref_format *format, struct used_atom *atom,
 				const char *arg, struct strbuf *err)
 {

@@ -345,9 +361,8 @@ static int contents_atom_parser(const struct ref_format *format, struct used_ato
 		atom->u.contents.option = C_SIG;
 	else if (!strcmp(arg, "subject"))
 		atom->u.contents.option = C_SUB;
-	else if (skip_prefix(arg, "trailers", &arg)) {
-		skip_prefix(arg, ":", &arg);
-		if (trailers_atom_parser(format, atom, *arg ? arg : NULL, err))
+	else if (check_format_field(arg, "trailers", &arg)) {
+		if (trailers_atom_parser(format, atom, arg, err))
 			return -1;
 	} else if (skip_prefix(arg, "lines=", &arg)) {
 		atom->u.contents.option = C_LINES;

Quoted 7 lines · Hariom Verma via GitGitGadget

From: Hariom Verma <[email protected]>

The 'contents' atom does not show any error if used with 'trailers' atom and semicolon is missing before trailers arguments.

e.g %(contents:trailersonly) works, while it shouldn't.

Quoted 8 lines

It is definitely not an expected behavior.

Let's fix this bug.

Mentored-by Christian Couder <[email protected]>

Mentored-by Heba Waly <[email protected]>

Signed-off-by Hariom Verma <[email protected]>

---

Nice spotting. 7a5edbdb (ref-filter.c: parse trailers arguments with %(contents) atom, 2017-10-01) talks about being deliberate about the case where skip_prefix(":") does not find a colon after the "trailers" token, but from the message it is clear that it expected that the case happens only when "trailers" is at the end of the string.

The new helper that is overly verbose and may be overkill.

Shouldn't this be clear enough, equivalent and sufficient?

	else if (skip_prefix(arg, "trailers", &arg) &&
		 (!*arg || *arg == ':'))) {
		if (trailers_atom_parser(...);

Quoted 7 lines

ref-filter.c +7 -0

+static int check_format_field(const char *arg, const char *field, const char **option)
+{
+	const char *opt;
+	if (skip_prefix(arg, field, &opt)) {
+		if (*opt == '\0') {
+			*option = NULL;
+			return 1;

ref-filter.c static int trailers_atom_parser(const struct ref_format *format, struct used_ato · line 349

Quoted 8 lines

ref-filter.c +8 -0

+		}
+		else if (*opt == ':') {
+			*option = ++opt;
+			return 1;
+		}
+	}
+	return 0;
+}

Quoted 7 lines · Junio C Hamano

ref-filter.c static int trailers_atom_parser(const struct ref_format *format, struct used_ato · line 346

Quoted 6 lines · Hariom Verma via GitGitGadget

ref-filter.c +6 -0

+static int check_format_field(const char *arg, const char *field, const char **option)
+{
+            else if (*opt == ':') {
+                    *option = ++opt;
+                    return 1;
+            }

And the helper does not have such a breakage. It looks good.

Quoted 10 lines · Junio C Hamano

Quoted 7 lines · Junio C Hamano

ref-filter.c static int trailers_atom_parser(const struct ref_format *format, struct used_ato · line 346

Quoted 6 lines · Hariom Verma via GitGitGadget

ref-filter.c +6 -0

+static int check_format_field(const char *arg, const char *field, const char **option)
+{
+            else if (*opt == ':') {
+                    *option = ++opt;
+                    return 1;
+            }

And the helper does not have such a breakage. It looks good.

One minor comment (not worth a re-roll): I personally found:

    *option = ++opt;

more confusing than:

    *option = opt + 1;

The ++opt places a higher cognitive load on the reader. As a reviewer, I had to go back and carefully reread the function to see if the side-effect of ++opt had some impact which I didn't notice on the first readthrough. The simpler opt + 1 does not have a side-effect, thus is easier to reason about (and doesn't require me to re-study the function when I encounter it).

Quoted 20 lines · Eric Sunshine

Quoted 10 lines · Junio C Hamano

Quoted 7 lines · Junio C Hamano

ref-filter.c static int trailers_atom_parser(const struct ref_format *format, struct used_ato · line 346

Quoted 6 lines · Hariom Verma via GitGitGadget

ref-filter.c +6 -0

+static int check_format_field(const char *arg, const char *field, const char **option)
+{
+            else if (*opt == ':') {
+                    *option = ++opt;
+                    return 1;
+            }

And the helper does not have such a breakage. It looks good.

One minor comment (not worth a re-roll): I personally found:

    *option = ++opt;

more confusing than:

    *option = opt + 1;

Quoted 6 lines

The ++opt places a higher cognitive load on the reader. As a reviewer, I had to go back and carefully reread the function to see if the side-effect of ++opt had some impact which I didn't notice on the first readthrough. The simpler opt + 1 does not have a side-effect, thus is easier to reason about (and doesn't require me to re-study the function when I encounter it).

That makes the two of us ... thanks.

Quoted 14 lines · Hariom Verma via GitGitGadget

t/t6300-for-each-ref.sh +1 -8

@@ -839,14 +818,7 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '
      fatal: unknown %(trailers) argument: unsupported
      EOF
      test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual &&
-     test_i18ncmp expect actual
-'
-
-test_expect_success '%(contents:trailers) rejects unknown trailers arguments' '
-     # error message cannot be checked under i18n
-     cat >expect <<-EOF &&
-     fatal: unknown %(trailers) argument: unsupported
-     EOF
+     test_i18ncmp expect actual &&
      test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual &&

t/t6300-for-each-ref.sh test_expect_success '%(trailers) rejects unknown trailers arguments' ' · line 824

Quoted 2 lines

t/t6300-for-each-ref.sh +0 -0

      test_i18ncmp expect actual
 '

Doesn't this highlight a small bug, where an end-user request for an unknown %(contents:trailers:unsupported) is flagged as an error about %(trailers)? Is it OK because we expect that users who use the longer %(contents:trailers) to know that it is a synonym for %(trailers) and the latter is the official way to write it?

The 'contents' atom does not show any error if used with 'trailers' atom and semicolon is missing before trailers arguments.

e.g %(contents:trailersonly) works, while it shouldn't.

It is definitely not an expected behavior.

Let's fix this bug.

Signed-off-by Hariom Verma <[email protected]>

---

ref-filter.c +5 -0

@@ -332,6 +332,22 @@ static int trailers_atom_parser(const struct ref_format *format, struct used_ato
+static int check_format_field(const char *arg, const char *field, const char **option)
+{
+       const char *opt;
+       if (skip_prefix(arg, field, &opt)) {
+               if (*opt == '\0') {

t/t6300-for-each-ref.sh +6 -0

+       cat >expect <<-EOF &&
+       fatal: unrecognized %(contents) argument: trailersonly
+       EOF
+       test_must_fail git for-each-ref --format="%(contents:trailersonly)" 2>actual &&
+       test_i18ncmp expect actual
+'

...an alternative would have been something like:

    else if (!strcmp(arg, "trailers")) {
        if (trailers_atom_parser(format, atom, NULL, err))
            return -1;
    } else if (skip_prefix(arg, "trailers:", &arg)) {
        if (trailers_atom_parser(format, atom, arg, err))
            return -1;
    }

which is quite simple to reason about (though has the cost of a tiny bit of duplication).

t/t6300-for-each-ref.sh test_expect_success '%(trailers) rejects unknown trailers arguments' ' · line 823

Quoted 3 lines

t/t6300-for-each-ref.sh +1 -0

@@ -823,6 +823,15 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '
+test_expect_success 'if arguments, %(contents:trailers) shows error if semicolon is missing' '

s/semicolon/colon/

t/t6300-for-each-ref.sh test_expect_success '%(trailers) rejects unknown trailers arguments' ' · line 827

Quoted 2 lines

t/t6300-for-each-ref.sh +1 -0

+       # error message cannot be checked under i18n

What is this comment about? I realize that you copied it from other nearby tests, but I find that it muddies rather than clarifies.

Quoted 6 lines

t/t6300-for-each-ref.sh +6 -0

+       cat >expect <<-EOF &&
+       fatal: unrecognized %(contents) argument: trailersonly
+       EOF
+       test_must_fail git for-each-ref --format="%(contents:trailersonly)" 2>actual &&
+       test_i18ncmp expect actual
+'

The 'contents' atom does not show any error if used with 'trailers' atom and colon is missing before trailers arguments.

e.g %(contents:trailersonly) works, while it shouldn't.

It is definitely not an expected behavior.

Let's fix this bug.

Acked-by Eric Sunshine <[email protected]>

Quoted 5 lines · Hariom Verma via GitGitGadget

The 'contents' atom does not show any error if used with 'trailers' atom and colon is missing before trailers arguments.

e.g %(contents:trailersonly) works, while it shouldn't.

Quoted 5 lines

It is definitely not an expected behavior.

Let's fix this bug.

Acked-by Eric Sunshine <[email protected]>

I didn't "ack" this patch. If you think some sort of attribution with my name is warranted, then a "Helped-by:" would be more appropriate.

Quoted 8 lines

Signed-off-by Hariom Verma <[email protected]>

---

ref-filter.c +1 -3

@@ -345,9 +345,11 @@ static int contents_atom_parser(const struct ref_format *format, struct used_ato
-       else if (skip_prefix(arg, "trailers", &arg)) {
-               skip_prefix(arg, ":", &arg);
-               if (trailers_atom_parser(format, atom, *arg ? arg : NULL, err))
+       else if (!strcmp(arg, "trailers")) {

ref-filter.c static int contents_atom_parser(const struct ref_format *format, struct used_ato · line 353

Quoted 5 lines

ref-filter.c +4 -0

+               if (trailers_atom_parser(format, atom, NULL, err))
+                       return -1;
+       } else if (skip_prefix(arg, "trailers:", &arg)) {
+               if (trailers_atom_parser(format, atom, arg, err))
                        return -1;

This looks better and easier to reason about (but I may be biased in thinking so).

t/t6300-for-each-ref.sh test_expect_success '%(trailers) rejects unknown trailers arguments' ' · line 823

Quoted 3 lines

t/t6300-for-each-ref.sh +1 -0

@@ -823,6 +823,14 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '
+test_expect_success 'if arguments, %(contents:trailers) shows error if semicolon is missing' '

This still needs a s/semicolon/colon/ (mentioned in my previous review).

Quoted 5 lines · Hariom Verma via GitGitGadget

The 'contents' atom does not show any error if used with 'trailers' atom and colon is missing before trailers arguments.

e.g %(contents:trailersonly) works, while it shouldn't.

Quoted 5 lines

It is definitely not an expected behavior.

Let's fix this bug.

Acked-by Eric Sunshine <[email protected]>

I didn't "ack" this patch. If you think some sort of attribution with my name is warranted, then a "Helped-by:" would be more appropriate.

ref-filter.c static int contents_atom_parser(const struct ref_format *format, struct used_ato · line 350

Quoted 13 lines

Signed-off-by Hariom Verma <[email protected]>

---

ref-filter.c +5 -3

@@ -345,9 +345,11 @@ static int contents_atom_parser(const struct ref_format *format, struct used_ato
-       else if (skip_prefix(arg, "trailers", &arg)) {
-               skip_prefix(arg, ":", &arg);
-               if (trailers_atom_parser(format, atom, *arg ? arg : NULL, err))
+       else if (!strcmp(arg, "trailers")) {
+               if (trailers_atom_parser(format, atom, NULL, err))
+                       return -1;
+       } else if (skip_prefix(arg, "trailers:", &arg)) {
+               if (trailers_atom_parser(format, atom, arg, err))
                        return -1;

This looks better and easier to reason about (but I may be biased in thinking so).

t/t6300-for-each-ref.sh test_expect_success '%(trailers) rejects unknown trailers arguments' ' · line 823

Quoted 3 lines

t/t6300-for-each-ref.sh +1 -0

@@ -823,6 +823,14 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '
+test_expect_success 'if arguments, %(contents:trailers) shows error if semicolon is missing' '

This still needs a s/semicolon/colon/ (mentioned in my previous review).

Currently, there exists a bug in 'contents' atom. It does not show any error if used with modifier 'trailers' and semicolon is missing before trailers arguments. This small patch series is focused on fixing that bug and also unified 'trailers' and 'contents:trailers' tests. Thus, removed duplicate code from t6300 and made tests more compact.

Quoted 5 lines · Hariom Verma via GitGitGadget

Currently, there exists a bug in 'contents' atom. It does not show any error if used with modifier 'trailers' and semicolon is missing before trailers arguments. This small patch series is focused on fixing that bug and also unified 'trailers' and 'contents:trailers' tests. Thus, removed duplicate code from t6300 and made tests more compact.

I think we should focus on completing the first two patches and send them to 'next' down to 'master', before extending the scope of the topic by piling more patches that do not have to be part of the topic. Let's take the other two separately from the first two.

Quoted 10 lines · Eric Sunshine

...an alternative would have been something like:

    else if (!strcmp(arg, "trailers")) {
        if (trailers_atom_parser(format, atom, NULL, err))
            return -1;
    } else if (skip_prefix(arg, "trailers:", &arg)) {
        if (trailers_atom_parser(format, atom, arg, err))
            return -1;
    }

Quoted 2 lines

which is quite simple to reason about (though has the cost of a tiny bit of duplication).

Yeah, that looks quite simple and straight-forward.

Quoted 11 lines · Junio C Hamano

Quoted 10 lines · Eric Sunshine

...an alternative would have been something like:

  else if (!strcmp(arg, "trailers")) {
    if (trailers_atom_parser(format, atom, NULL, err))
      return -1;
  } else if (skip_prefix(arg, "trailers:", &arg)) {
    if (trailers_atom_parser(format, atom, arg, err))
      return -1;
  }

Quoted 4 lines

Quoted 2 lines

which is quite simple to reason about (though has the cost of a tiny bit of duplication).

Yeah, that looks quite simple and straight-forward.

Recently, I sent a patch series "Improvements to ref-filter"[1]. A patch in this patch series introduced "sanitize" modifier to "subject" atom. i.e "%(subject:sanitize)".

What if in the future we also want "%(contents:subject:sanitize)" to work? We can use this helper any number of times, whenever there is a need.

Sorry, I missed saying this earlier. But I don't prefer duplicating the code here.

Quoted 21 lines · Hariom verma

Quoted 11 lines · Junio C Hamano

Quoted 10 lines · Eric Sunshine

...an alternative would have been something like:

  else if (!strcmp(arg, "trailers")) {
    if (trailers_atom_parser(format, atom, NULL, err))
      return -1;
  } else if (skip_prefix(arg, "trailers:", &arg)) {
    if (trailers_atom_parser(format, atom, arg, err))
      return -1;
  }

Quoted 4 lines

Quoted 2 lines

which is quite simple to reason about (though has the cost of a tiny bit of duplication).

Yeah, that looks quite simple and straight-forward.

Recently, I sent a patch series "Improvements to ref-filter"[1]. A patch in this patch series introduced "sanitize" modifier to "subject" atom. i.e "%(subject:sanitize)".

Quoted 5 lines

What if in the future we also want "%(contents:subject:sanitize)" to work? We can use this helper any number of times, whenever there is a need.

Sorry, I missed saying this earlier. But I don't prefer duplicating the code here.

Pushing back on a reviewer suggestion is fine. Explaining the reason for your position -- as you do here -- helps reviewers understand why you feel the way you do. My review suggestion about making it easier to reason about the code while avoiding a brand new function, at the cost of a minor amount of duplication, was made in the context of this one-off case in which the function increased cognitive load and was used just once (not knowing that you envisioned future callers). If you expect the new function to be re-used by upcoming changes, then that may be a good reason to keep it. Stating so in the commit message will help reviewers see beyond the immediate patch or patch series.

Aside from a couple minor style violations[1,2], I don't particularly oppose the helper function, though I have a quibble with the name check_format_field(), which I don't find helpful, and which (at least for me) increases the cognitive load. The increased cognitive load, I think, comes not only from the function name not spelling out what the function actually does, but also because the function is dual-purpose:
it's both checking that the argument matches a particular token ("trailers", in this case) and extracting the sub-argument. Perhaps naming it match_and_extract_subarg() or something similar would help, though that's a mouthful.

But the observation about the function being dual-purpose (thus potentially confusing) brings up other questions. For instance, is it too special-purpose? If you foresee more callers in the future with multiple-token arguments such as %(content:subject:sanitize), should

the function provide more assistance by splitting out each of the sub-arguments rather than stopping at the first? Taking that even further, a generalized helper for "splitting" arguments like that might be useful at the top-level of contents_atom_parser() too, rather than only for specific arguments, such as "trailers". Of course, this may all be way too ambitious for this little bug fix series or even for whatever upcoming changes you're planning, thus not worth pursuing.

As for the helper's implementation, I might have written it like this:

    static int check_format_field(...)
    {
        const char *opt
        if (!strcmp(arg, field))
            *option = NULL;
        else if (skip_prefix(arg, field, opt) && *opt == ':')
            *option = opt + 1;
        else
            return 0;
        return 1;
    }

which is more compact and closer to what I suggested earlier for avoiding the helper function in the first place. But, of course, programming is quite subjective, and you may find your implementation easier to reason about. Plus, your version has the benefit of being slightly more optimal since it avoids an extra string scan, although that probably is mostly immaterial considering that contents_atom_parser() itself contains a long chain of potentially sub-optimal strcmp() and skip_prefix() calls.

Quoted 13 lines · Eric Sunshine

Quoted 9 lines · Hariom verma

Recently, I sent a patch series "Improvements to ref-filter"[1]. A patch in this patch series introduced "sanitize" modifier to "subject" atom. i.e "%(subject:sanitize)".

What if in the future we also want "%(contents:subject:sanitize)" to work? We can use this helper any number of times, whenever there is a need.

Sorry, I missed saying this earlier. But I don't prefer duplicating the code here.

Quoted 10 lines

Pushing back on a reviewer suggestion is fine. Explaining the reason for your position -- as you do here -- helps reviewers understand why you feel the way you do. My review suggestion about making it easier to reason about the code while avoiding a brand new function, at the cost of a minor amount of duplication, was made in the context of this one-off case in which the function increased cognitive load and was used just once (not knowing that you envisioned future callers). If you expect the new function to be re-used by upcoming changes, then that may be a good reason to keep it. Stating so in the commit message will help reviewers see beyond the immediate patch or patch series.

Yeah. I should have mentioned this in the commit message.

Quoted 10 lines

Aside from a couple minor style violations[1,2], I don't particularly oppose the helper function, though I have a quibble with the name check_format_field(), which I don't find helpful, and which (at least for me) increases the cognitive load. The increased cognitive load, I think, comes not only from the function name not spelling out what the function actually does, but also because the function is dual-purpose:
it's both checking that the argument matches a particular token ("trailers", in this case) and extracting the sub-argument. Perhaps naming it match_and_extract_subarg() or something similar would help, though that's a mouthful.

I will fix those violations.
Also, "match_and_extract_subarg()" looks good to me.

Quoted 4 lines

But the observation about the function being dual-purpose (thus potentially confusing) brings up other questions. For instance, is it too special-purpose? If you foresee more callers in the future with multiple-token arguments such as %(content:subject:sanitize), should

Quoted 8 lines

the function provide more assistance by splitting out each of the sub-arguments rather than stopping at the first? Taking that even further, a generalized helper for "splitting" arguments like that might be useful at the top-level of contents_atom_parser() too, rather than only for specific arguments, such as "trailers". Of course, this may all be way too ambitious for this little bug fix series or even for whatever upcoming changes you're planning, thus not worth pursuing.

Splitting sub-arguments is done at "<atomname>_atom_parser()". If you mean pre-splitting every argument...
something like: ['contents', 'subject', 'sanitize'] for %(content:subject:sanitize) in contents_atom_parser() ? I'm not able to see how it can be useful.

Sorry, If I got your concerned wrong.

Quoted 14 lines

As for the helper's implementation, I might have written it like this:

    static int check_format_field(...)
    {
        const char *opt
        if (!strcmp(arg, field))
            *option = NULL;
        else if (skip_prefix(arg, field, opt) && *opt == ':')
            *option = opt + 1;
        else
            return 0;
        return 1;
    }

Quoted 8 lines

which is more compact and closer to what I suggested earlier for avoiding the helper function in the first place. But, of course, programming is quite subjective, and you may find your implementation easier to reason about. Plus, your version has the benefit of being slightly more optimal since it avoids an extra string scan, although that probably is mostly immaterial considering that contents_atom_parser() itself contains a long chain of potentially sub-optimal strcmp() and skip_prefix() calls.

"programming is quite subjective"
Yeah, I couldn't agree more.

The change you suggested looks good too. But I'm little inclined to my keeping my changes. I'm curious, what others have to say on this.

I think we could also get rid of the "match_and_" part of the suggestion, in the same way as skip_prefix() is not called match_and_skip_prefix(). Readers can just expect that if there is no match the function will return 0.

So maybe "extract_field_option()".

Hi,

Quoted 13 lines · Hariom verma

Quoted 10 lines · Eric Sunshine

Aside from a couple minor style violations[1,2], I don't particularly oppose the helper function, though I have a quibble with the name check_format_field(), which I don't find helpful, and which (at least for me) increases the cognitive load. The increased cognitive load, I think, comes not only from the function name not spelling out what the function actually does, but also because the function is dual-purpose:
it's both checking that the argument matches a particular token ("trailers", in this case) and extracting the sub-argument. Perhaps naming it match_and_extract_subarg() or something similar would help, though that's a mouthful.

Quoted 2 lines

I will fix those violations.
Also, "match_and_extract_subarg()" looks good to me.

I am not sure about the "subarg" part of the name. In the for-each-ref doc, names inside %(...) are called "field names", and parts after ":" are called "options". So it might be better to have "field_option" instead of "subarg" in the name.

I think we could also get rid of the "match_and_" part of the suggestion, in the same way as skip_prefix() is not called match_and_skip_prefix(). Readers can just expect that if there is no match the function will return 0.

So maybe "extract_field_option()".

Quoted 13 lines

Quoted 4 lines

But the observation about the function being dual-purpose (thus potentially confusing) brings up other questions. For instance, is it too special-purpose? If you foresee more callers in the future with multiple-token arguments such as %(content:subject:sanitize), should

Quoted 8 lines

the function provide more assistance by splitting out each of the sub-arguments rather than stopping at the first? Taking that even further, a generalized helper for "splitting" arguments like that might be useful at the top-level of contents_atom_parser() too, rather than only for specific arguments, such as "trailers". Of course, this may all be way too ambitious for this little bug fix series or even for whatever upcoming changes you're planning, thus not worth pursuing.

Quoted 5 lines

Splitting sub-arguments is done at "<atomname>_atom_parser()". If you mean pre-splitting every argument...
something like: ['contents', 'subject', 'sanitize'] for %(content:subject:sanitize) in contents_atom_parser() ? I'm not able to see how it can be useful.

Yeah, it seems to me that such a splitting would require a complete rewrite of the current code, so I am not sure it's an interesting way forward for now. And anyway adding extract_field_option() goes in the right direction of abstracting the parsing and making the code simpler, more efficient and likely more correct.

Quoted 25 lines

Sorry, If I got your concerned wrong.

Quoted 22 lines

As for the helper's implementation, I might have written it like this:

    static int check_format_field(...)
    {
        const char *opt
        if (!strcmp(arg, field))
            *option = NULL;
        else if (skip_prefix(arg, field, opt) && *opt == ':')
            *option = opt + 1;
        else
            return 0;
        return 1;
    }

which is more compact and closer to what I suggested earlier for avoiding the helper function in the first place. But, of course, programming is quite subjective, and you may find your implementation easier to reason about. Plus, your version has the benefit of being slightly more optimal since it avoids an extra string scan, although that probably is mostly immaterial considering that contents_atom_parser() itself contains a long chain of potentially sub-optimal strcmp() and skip_prefix() calls.

Quoted 5 lines

"programming is quite subjective"
Yeah, I couldn't agree more.

The change you suggested looks good too. But I'm little inclined to my keeping my changes. I'm curious, what others have to say on this.

I also prefer a slightly more optimal one even if it's a bit less compact.