[PATCH] revert: clarify seemingly bogus OPT_END repetition

[PATCH] revert: clarify seemingly bogus OPT_END repetition

[Johannes Schindelin]

This developer stumbled over repeated OPT_END entries and was *so
close* (almost touches his thumb with his index finger) to collapse
them into a single one. Only inspecting the file's history with
`git log -p -SOPT_END` clarified why they are there.

This confusion can be easily prevented by inserting a simple and
clear comment.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	Another patch from the rebase--helper queue.

Published-As: https://github.com/dscho/git/releases/tag/clarify-opt-end-v1
 builtin/revert.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/revert.c b/builtin/revert.c
index 56a2c36..b4da1f6 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -92,6 +92,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		{ OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"),
 		  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
 		OPT_END(),
+		/* place-holders for REPLAY_PICK's extra options, see below */
 		OPT_END(),
 		OPT_END(),
 		OPT_END(),
-- 
2.9.0.280.g32e2a70

base-commit: cf4c2cfe52be5bd973a4838f73a35d3959ce2f43

Re: [PATCH] revert: clarify seemingly bogus OPT_END repetition

[Jeff King]

On Tue, Jul 05, 2016 at 01:58:51PM +0200, Johannes Schindelin wrote:

> This developer stumbled over repeated OPT_END entries and was *so
> close* (almost touches his thumb with his index finger) to collapse
> them into a single one. Only inspecting the file's history with
> `git log -p -SOPT_END` clarified why they are there.

Wow, that's really ugly, and confused me, too.

I've been trying to move us away from this kind of manually-computed
array size, simply because it's error-prone and often not obviously
correct[1].

I wonder if parse_options_concat should simply allocate a new list
(after computing the total required size). I guess this is the only
caller, though, so perhaps it's not the end of the world. In the
meantime, your patch is certainly an improvement.

By the way, I notice that the error message when concat fails is just:

  if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))
	die(_("program error"));

Should this become:

        die("BUG: not enough room to concatenate options");

as part of your BUG cleanups elsewhere?

-Peff

[1] At least the concat interface takes ARRAY_SIZE(), so that we will
    catch an error, rather than silently causing memory corruption. A
    much more likely error is to forget OPT_END(), which will cause
    parse_options to read past the end of the array.

Re: [PATCH] revert: clarify seemingly bogus OPT_END repetition

[Jeff King]

On Tue, Jul 05, 2016 at 04:28:20PM -0400, Jeff King wrote:

> I wonder if parse_options_concat should simply allocate a new list
> (after computing the total required size). I guess this is the only
> caller, though, so perhaps it's not the end of the world. In the
> meantime, your patch is certainly an improvement.

Something like the patch below.

I admit this isn't buggy _now_, so this is potentially just churn. It
does make further patches look nicer, though (they don't have to add
apparently meaningless OPT_END() slots).

-- >8 --
Subject: [PATCH] parse_options: allocate a new array when concatenating

In exactly one callers (builtin/revert.c), we build up the
options list dynamically from multiple arrays. We do so by
manually inserting "filler" entries into one array, and then
copying the other array into the allocated space.

This is tedious and error-prone, as you have to adjust the
filler any time the second array is modified (although we do
at least check and die() when the counts do not match up).

Instead, let's just allocate a new array.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/revert.c   | 13 ++++---------
 parse-options-cb.c | 29 +++++++++++++++++------------
 parse-options.h    |  2 +-
 3 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 56a2c36..4e69380 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -76,7 +76,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
 	const char *me = action_name(opts);
 	int cmd = 0;
-	struct option options[] = {
+	struct option base_options[] = {
 		OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
 		OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'),
 		OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), 'a'),
@@ -91,13 +91,9 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 			N_("option for merge strategy"), option_parse_x),
 		{ OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"),
 		  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
-		OPT_END(),
-		OPT_END(),
-		OPT_END(),
-		OPT_END(),
-		OPT_END(),
-		OPT_END(),
+		OPT_END()
 	};
+	struct option *options = base_options;
 
 	if (opts->action == REPLAY_PICK) {
 		struct option cp_extra[] = {
@@ -108,8 +104,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 			OPT_BOOL(0, "keep-redundant-commits", &opts->keep_redundant_commits, N_("keep redundant, empty commits")),
 			OPT_END(),
 		};
-		if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))
-			die(_("program error"));
+		options = parse_options_concat(options, cp_extra);
 	}
 
 	argc = parse_options(argc, argv, NULL, options, usage_str,
diff --git a/parse-options-cb.c b/parse-options-cb.c
index 239898d..2d87520 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -117,19 +117,24 @@ int parse_opt_tertiary(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
-int parse_options_concat(struct option *dst, size_t dst_size, struct option *src)
+struct option *parse_options_concat(struct option *a, struct option *b)
 {
-	int i, j;
-
-	for (i = 0; i < dst_size; i++)
-		if (dst[i].type == OPTION_END)
-			break;
-	for (j = 0; i < dst_size; i++, j++) {
-		dst[i] = src[j];
-		if (src[j].type == OPTION_END)
-			return 0;
-	}
-	return -1;
+	struct option *ret;
+	size_t i, a_len = 0, b_len = 0;
+
+	for (i = 0; a[i].type != OPTION_END; i++)
+		a_len++;
+	for (i = 0; b[i].type != OPTION_END; i++)
+		b_len++;
+
+	ALLOC_ARRAY(ret, st_add3(a_len, b_len, 1));
+	for (i = 0; i < a_len; i++)
+		ret[i] = a[i];
+	for (i = 0; i < b_len; i++)
+		ret[a_len + i] = b[i];
+	ret[a_len + b_len] = b[b_len]; /* final OPTION_END */
+
+	return ret;
 }
 
 int parse_opt_string_list(const struct option *opt, const char *arg, int unset)
diff --git a/parse-options.h b/parse-options.h
index ea4af92..78f8384 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -215,7 +215,7 @@ extern int parse_options_step(struct parse_opt_ctx_t *ctx,
 
 extern int parse_options_end(struct parse_opt_ctx_t *ctx);
 
-extern int parse_options_concat(struct option *dst, size_t, struct option *src);
+extern struct option *parse_options_concat(struct option *a, struct option *b);
 
 /*----- some often used options -----*/
 extern int parse_opt_abbrev_cb(const struct option *, const char *, int);
-- 
2.9.0.320.gd3e6182

Re: [PATCH] revert: clarify seemingly bogus OPT_END repetition

[Jacob Keller]

On Tue, Jul 5, 2016 at 1:44 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Jul 05, 2016 at 04:28:20PM -0400, Jeff King wrote:
>
>> I wonder if parse_options_concat should simply allocate a new list
>> (after computing the total required size). I guess this is the only
>> caller, though, so perhaps it's not the end of the world. In the
>> meantime, your patch is certainly an improvement.
>
> Something like the patch below.
>
> I admit this isn't buggy _now_, so this is potentially just churn. It
> does make further patches look nicer, though (they don't have to add
> apparently meaningless OPT_END() slots).
>
> -- >8 --
> Subject: [PATCH] parse_options: allocate a new array when concatenating
>
> In exactly one callers (builtin/revert.c), we build up the
> options list dynamically from multiple arrays. We do so by
> manually inserting "filler" entries into one array, and then
> copying the other array into the allocated space.
>
> This is tedious and error-prone, as you have to adjust the
> filler any time the second array is modified (although we do
> at least check and die() when the counts do not match up).
>
> Instead, let's just allocate a new array.

This seems much preferable to me.

>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/revert.c   | 13 ++++---------
>  parse-options-cb.c | 29 +++++++++++++++++------------
>  parse-options.h    |  2 +-
>  3 files changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 56a2c36..4e69380 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -76,7 +76,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
>         const char * const * usage_str = revert_or_cherry_pick_usage(opts);
>         const char *me = action_name(opts);
>         int cmd = 0;
> -       struct option options[] = {
> +       struct option base_options[] = {
>                 OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
>                 OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'),
>                 OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), 'a'),
> @@ -91,13 +91,9 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
>                         N_("option for merge strategy"), option_parse_x),
>                 { OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"),
>                   N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
> -               OPT_END(),
> -               OPT_END(),
> -               OPT_END(),
> -               OPT_END(),
> -               OPT_END(),
> -               OPT_END(),
> +               OPT_END()
>         };
> +       struct option *options = base_options;
>
>         if (opts->action == REPLAY_PICK) {
>                 struct option cp_extra[] = {
> @@ -108,8 +104,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
>                         OPT_BOOL(0, "keep-redundant-commits", &opts->keep_redundant_commits, N_("keep redundant, empty commits")),
>                         OPT_END(),
>                 };
> -               if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))
> -                       die(_("program error"));
> +               options = parse_options_concat(options, cp_extra);
>         }
>
>         argc = parse_options(argc, argv, NULL, options, usage_str,
> diff --git a/parse-options-cb.c b/parse-options-cb.c
> index 239898d..2d87520 100644
> --- a/parse-options-cb.c
> +++ b/parse-options-cb.c
> @@ -117,19 +117,24 @@ int parse_opt_tertiary(const struct option *opt, const char *arg, int unset)
>         return 0;
>  }
>
> -int parse_options_concat(struct option *dst, size_t dst_size, struct option *src)
> +struct option *parse_options_concat(struct option *a, struct option *b)
>  {
> -       int i, j;
> -
> -       for (i = 0; i < dst_size; i++)
> -               if (dst[i].type == OPTION_END)
> -                       break;
> -       for (j = 0; i < dst_size; i++, j++) {
> -               dst[i] = src[j];
> -               if (src[j].type == OPTION_END)
> -                       return 0;
> -       }
> -       return -1;
> +       struct option *ret;
> +       size_t i, a_len = 0, b_len = 0;
> +
> +       for (i = 0; a[i].type != OPTION_END; i++)
> +               a_len++;
> +       for (i = 0; b[i].type != OPTION_END; i++)
> +               b_len++;
> +
> +       ALLOC_ARRAY(ret, st_add3(a_len, b_len, 1));
> +       for (i = 0; i < a_len; i++)
> +               ret[i] = a[i];
> +       for (i = 0; i < b_len; i++)
> +               ret[a_len + i] = b[i];
> +       ret[a_len + b_len] = b[b_len]; /* final OPTION_END */
> +
> +       return ret;
>  }
>
>  int parse_opt_string_list(const struct option *opt, const char *arg, int unset)
> diff --git a/parse-options.h b/parse-options.h
> index ea4af92..78f8384 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -215,7 +215,7 @@ extern int parse_options_step(struct parse_opt_ctx_t *ctx,
>
>  extern int parse_options_end(struct parse_opt_ctx_t *ctx);
>
> -extern int parse_options_concat(struct option *dst, size_t, struct option *src);
> +extern struct option *parse_options_concat(struct option *a, struct option *b);
>
>  /*----- some often used options -----*/
>  extern int parse_opt_abbrev_cb(const struct option *, const char *, int);
> --
> 2.9.0.320.gd3e6182
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] revert: clarify seemingly bogus OPT_END repetition

[Johannes Schindelin]

Hi Peff & Jacob,

On Tue, 5 Jul 2016, Jacob Keller wrote:

> On Tue, Jul 5, 2016 at 1:44 PM, Jeff King <peff@peff.net> wrote:
> > On Tue, Jul 05, 2016 at 04:28:20PM -0400, Jeff King wrote:
> >
> > Something like the patch below.
> >
> > I admit this isn't buggy _now_, so this is potentially just churn. It
> > does make further patches look nicer, though (they don't have to add
> > apparently meaningless OPT_END() slots).
> >
> > -- >8 --
> > Subject: [PATCH] parse_options: allocate a new array when concatenating
> >
> > In exactly one callers (builtin/revert.c), we build up the
> > options list dynamically from multiple arrays. We do so by
> > manually inserting "filler" entries into one array, and then
> > copying the other array into the allocated space.
> >
> > This is tedious and error-prone, as you have to adjust the
> > filler any time the second array is modified (although we do
> > at least check and die() when the counts do not match up).
> >
> > Instead, let's just allocate a new array.
> 
> This seems much preferable to me.

Yes, this is better than my patch.

BTW Jacob, would you terribly mind cutting the quoted parts properly (I
cut 112 lines)? It may not seem like much, but I seem to spend more and
more of my email time budget on skimming unaddressed remainders of quoted
mails, and I would much rather spend that time on something productive.

Thanks,
Johannes

Re: [PATCH] revert: clarify seemingly bogus OPT_END repetition

[Michael J Gruber]

Johannes Schindelin venit, vidit, dixit 06.07.2016 09:01:
> Hi Peff & Jacob,
> 
> On Tue, 5 Jul 2016, Jacob Keller wrote:
> 
>> On Tue, Jul 5, 2016 at 1:44 PM, Jeff King <peff@peff.net> wrote:
>>> On Tue, Jul 05, 2016 at 04:28:20PM -0400, Jeff King wrote:
>>>
>>> Something like the patch below.
>>>
>>> I admit this isn't buggy _now_, so this is potentially just churn. It
>>> does make further patches look nicer, though (they don't have to add
>>> apparently meaningless OPT_END() slots).
>>>
>>> -- >8 --
>>> Subject: [PATCH] parse_options: allocate a new array when concatenating
>>>
>>> In exactly one callers (builtin/revert.c), we build up the
>>> options list dynamically from multiple arrays. We do so by
>>> manually inserting "filler" entries into one array, and then
>>> copying the other array into the allocated space.
>>>
>>> This is tedious and error-prone, as you have to adjust the
>>> filler any time the second array is modified (although we do
>>> at least check and die() when the counts do not match up).
>>>
>>> Instead, let's just allocate a new array.
>>
>> This seems much preferable to me.
> 
> Yes, this is better than my patch.
> 
> BTW Jacob, would you terribly mind cutting the quoted parts properly (I
> cut 112 lines)? It may not seem like much, but I seem to spend more and
> more of my email time budget on skimming unaddressed remainders of quoted
> mails, and I would much rather spend that time on something productive.

OTOH, I often have to look up the original message because people cut
too much, or because they take one sentence out of context.

It is not unheard of that a MUA can collapse and expand properly quoted
parts on request...

Michael

Over-/underquoting, was Re: [PATCH] revert: clarify seemingly bogus OPT_END repetition

[Johannes Schindelin]

Hi Michael,

On Wed, 6 Jul 2016, Michael J Gruber wrote:

> Johannes Schindelin venit, vidit, dixit 06.07.2016 09:01:
> 
> > BTW Jacob, would you terribly mind cutting the quoted parts properly (I
> > cut 112 lines)? It may not seem like much, but I seem to spend more and
> > more of my email time budget on skimming unaddressed remainders of quoted
> > mails, and I would much rather spend that time on something productive.
> 
> OTOH, I often have to look up the original message because people cut
> too much, or because they take one sentence out of context.

And others quote too much. In your case, the reference to Peff's patch was
not at all what you referred to.

> It is not unheard of that a MUA can collapse and expand properly quoted
> parts on request...

Sure. Show me some kick-ass, scriptable mail client and I will have a
look.

Ciao,
Johannes

Re: Over-/underquoting, was Re: [PATCH] revert: clarify seemingly bogus OPT_END repetition

[Jacob Keller]

On Wed, Jul 6, 2016 at 6:16 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>> Johannes Schindelin venit, vidit, dixit 06.07.2016 09:01:
>> OTOH, I often have to look up the original message because people cut
>> too much, or because they take one sentence out of context.
>
> And others quote too much. In your case, the reference to Peff's patch was
> not at all what you referred to.
>

I obviously left the whole message, and I usually try to cut what I am
replying to but I forgot and was in a bit of a hurry that day. I think
most people are capable of checking archives if they are worried about
someone quoting out of context.

>> It is not unheard of that a MUA can collapse and expand properly quoted
>> parts on request...
>
> Sure. Show me some kick-ass, scriptable mail client and I will have a
> look.
>

Yep, I haven't found a mail client that really works for me either :(

> Ciao,
> Johannes

Regards,
Jake

Re: Over-/underquoting, was Re: [PATCH] revert: clarify seemingly bogus OPT_END repetition

[Michael J Gruber]

2016-07-06 22:15 GMT+02:00 Jacob Keller <jacob.keller@gmail.com>:
> On Wed, Jul 6, 2016 at 6:16 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
...
>>> It is not unheard of that a MUA can collapse and expand properly quoted
>>> parts on request...
>>
>> Sure. Show me some kick-ass, scriptable mail client and I will have a
>> look.
>>
>
> Yep, I haven't found a mail client that really works for me either :(

Every other day I want to get rid of Thunderbird... always on the
verge of going (back) to mutt, though I do need some calendar
integration and such.

Until the day when the scriptable mua we wish for comes upon us, may I
recommend the "QuoteCollapse" extension to Thunderbird that transforms
those huge quotes into neat short headlines with a twist(y).

Michael

Re: Over-/underquoting, was Re: [PATCH] revert: clarify seemingly bogus OPT_END repetition

[Eric Wong]

Michael J Gruber <git@drmicha.warpmail.net> wrote:
> 2016-07-06 22:15 GMT+02:00 Jacob Keller <jacob.keller@gmail.com>:
> > On Wed, Jul 6, 2016 at 6:16 AM, Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:
> ...
> >>> It is not unheard of that a MUA can collapse and expand properly quoted
> >>> parts on request...
> >>
> >> Sure. Show me some kick-ass, scriptable mail client and I will have a
> >> look.

I like mutt since it lets me pipe stuff to arbitrary commands,
"T" works in the pager for toggling quoted text.

> > Yep, I haven't found a mail client that really works for me either :(
> 
> Every other day I want to get rid of Thunderbird... always on the
> verge of going (back) to mutt, though I do need some calendar
> integration and such.

It's possible to use both with IMAP (offlineimap + local dovecot works
well on slow connections).

I rely on at+atd to email myself and also maintain a text file over IMAP.

> Until the day when the scriptable mua we wish for comes upon us, may I
> recommend the "QuoteCollapse" extension to Thunderbird that transforms
> those huge quotes into neat short headlines with a twist(y).

The "T" (toggle-quoted) command in mutt hides quoted entirely, which
makes it a bit confusing, at times; but toggling again unhides it.

Re: [PATCH] revert: clarify seemingly bogus OPT_END repetition

[Johannes Schindelin]

Hi Peff,

On Tue, 5 Jul 2016, Jeff King wrote:

> +	ALLOC_ARRAY(ret, st_add3(a_len, b_len, 1));
> +	for (i = 0; i < a_len; i++)
> +		ret[i] = a[i];
> +	for (i = 0; i < b_len; i++)
> +		ret[a_len + i] = b[i];
> +	ret[a_len + b_len] = b[b_len]; /* final OPTION_END */

Not really a big deal, but I'd have written "i <= b_len" in the for loop
and avoided the final line. However, your version is clearer.

Ciao,
Dscho

Re: [PATCH] revert: clarify seemingly bogus OPT_END repetition

[Johannes Schindelin]

Hi Peff,

On Tue, 5 Jul 2016, Jeff King wrote:

> By the way, I notice that the error message when concat fails is just:
> 
>   if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))
> 	die(_("program error"));
> 
> Should this become:
> 
>         die("BUG: not enough room to concatenate options");
> 
> as part of your BUG cleanups elsewhere?

Not really. I do not have time for a crusade through all of the die()
statements to determine whether they're really bug reports. You refer to
my clean-up of merge-recursive.c, right? That was a necessary prerequisite
for the patch to stop die()ing in case of errors.

Ciao,
Dscho

Re: [PATCH] revert: clarify seemingly bogus OPT_END repetition

[Jeff King]

On Wed, Jul 06, 2016 at 09:03:44AM +0200, Johannes Schindelin wrote:

> > By the way, I notice that the error message when concat fails is just:
> > 
> >   if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))
> > 	die(_("program error"));
> > 
> > Should this become:
> > 
> >         die("BUG: not enough room to concatenate options");
> > 
> > as part of your BUG cleanups elsewhere?
> 
> Not really. I do not have time for a crusade through all of the die()
> statements to determine whether they're really bug reports. You refer to
> my clean-up of merge-recursive.c, right? That was a necessary prerequisite
> for the patch to stop die()ing in case of errors.

Yeah. I didn't look at it all that closely, but it looked like you hit
places outside of merge-recursive, too. I agree it is not worth
including unless you are crusading, though (and anyway, my patch gets
rid of the message entirely).

-Peff