Re: Parsing trailers

[Jeff King <peff@peff.net>]

On Wed, Jan 02, 2019 at 11:43:55PM -0800, William Chargin wrote:

> > IMHO this is a bug in --parse. It was always meant to give sane,
> > normalized output
> 
> Okay; this is good to hear. In that case, what would you think about
> changing `interpret-trailers` as a whole to always emit colons? (Note
> that the command is already lossy: even with no flags, it replaces each
> separator character with the first character of `trailer.separators`.)
> This has the advantage that we avoid adding a configuration option of
> dubious value—it’s not clear to me why a user would actually _want_ to
> change the separator to anything other than a colon. The patch should be
> quite simple, too (only tested lightly on my machine):
> 
> diff --git a/trailer.c b/trailer.c
> index 0796f326b3..722040e48c 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -156,7 +156,7 @@ static void print_tok_val(FILE *outfile, const
> char *tok, const char *val)
>         if (strchr(separators, c))
>                 fprintf(outfile, "%s%s\n", tok, val);
>         else
> -               fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
> +               fprintf(outfile, "%s: %s\n", tok, val);
>  }
> 
> Is this veering too much from “bug fix” toward “backward-incompatible
> behavior change” for your comfort?

I think that gets weird in non-parse modes. For example, if I'm not
trying to parse, but rather to _add_ a new trailer (because I'm writing
out a commit message to feed to git-commit-tree), then I'd presumably
want the normal configured separators.

I dunno. I don't think I've ever seen a trailer with a non-colon
separator, nor have I ever used interpret-trailers for anything except
parsing. But it obviously was designed with more flexibility in mind,
and I suspect the patch above has a high chance of breaking something
somewhere.  Tying it to --parse seems a lot safer, since that was
introduced exactly for this case.

-Peff