Re: [PATCH v5 5/5] fast-export: do automatic reencoding of commit messages only if requested

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 14, 2019, 00:19 UTC
Message-ID: <CAPig+cTmgPnFud0NVcP0qcQ08t3h-QanyhSTtkx05fQVo3qV9Q@mail.gmail.com>
In-Reply-To: <20190513231726.16218-6-newren@gmail.com>

On Mon, May 13, 2019 at 7:17 PM Elijah Newren <newren@gmail.com> wrote:

Show 30 quoted lines

> Automatic re-encoding of commit messages (and dropping of the encoding
> header) hurts attempts to do reversible history rewrites (e.g. sha1sum
> <-> sha256sum transitions, some subtree rewrites), and seems
> inconsistent with the general principle followed elsewhere in
> fast-export of requiring explicit user requests to modify the output
> (e.g. --signed-tags=strip, --tag-of-filtered-object=rewrite).  Add a
> --reencode flag that the user can use to specify, and like other
> fast-export flags, default it to 'abort'.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> @@ -77,6 +78,31 @@ static int parse_opt_tag_of_filtered_mode(const struct option *opt,
> +static int parse_opt_reencode_mode(const struct option *opt,
> +                                  const char *arg, int unset)
> +{
> +       if (unset) {
> +               reencode_mode = REENCODE_ABORT;
> +               return 0;
> +       }
> +
> +       switch (git_parse_maybe_bool(arg)) {
> +       case 0:
> +               reencode_mode = REENCODE_NO;
> +               break;
> +       case 1:
> +               reencode_mode = REENCODE_YES;
> +               break;
> +       default:
> +               if (arg && !strcasecmp(arg, "abort"))

If 'arg' is NULL, git_parse_maybe_bool() will have returned 1, which is already handled above, so no need to check it here. So, dropping the "arg &&" from the conditional...

> +                       reencode_mode = REENCODE_ABORT;
> +               else
> +                       return error("Unknown reencoding mode: %s", arg);

...makes it clear that you won't ever be passing NULL to "%s", which is undefined behavior.

> +       }
> +
> +       return 0;
> +}