Re: [PATCH v4 2/2] format-patch: teach format.notes config option

[Elijah Newren <newren@gmail.com>]

Hi Denton,

I was recently digging in this area (due to the recent
rebase-applies-patches-incorrectly bug report) and found a couple bugs
from this old commit:

On Thu, May 16, 2019 at 5:27 PM Denton Liu <liu.denton@gmail.com> wrote:
>
> In git-format-patch, notes can be appended with the `--notes` option.
> However, this must be specified by the user on an
> invocation-by-invocation basis. If a user is not careful, it's possible
> that they may forget to include it and generate a patch series without
> notes.
>
> Teach git-format-patch the `format.notes` config option. Its value is a
> notes ref that will be automatically appended. The special value of
> "standard" can be used to specify the standard notes. This option is
> overridable with the `--no-notes` option in case a user wishes not to
> append notes.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  Documentation/config/format.txt    | 15 +++++++
>  Documentation/git-format-patch.txt |  3 ++
>  builtin/log.c                      | 20 ++++++++-
>  t/t4014-format-patch.sh            | 70 ++++++++++++++++++++++++++++++
>  4 files changed, 107 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config/format.txt b/Documentation/config/format.txt
> index dc77941c48..414a5a8a9d 100644
> --- a/Documentation/config/format.txt
> +++ b/Documentation/config/format.txt
> @@ -85,3 +85,18 @@ format.outputDirectory::
>  format.useAutoBase::
>         A boolean value which lets you enable the `--base=auto` option of
>         format-patch by default.
> +
> +format.notes::
> +       Provides the default value for the `--notes` option to
> +       format-patch. Accepts a boolean value, or a ref which specifies
> +       where to get notes. If false, format-patch defaults to
> +       `--no-notes`. If true, format-patch defaults to `--notes`. If
> +       set to a non-boolean value, format-patch defaults to
> +       `--notes=<ref>`, where `ref` is the non-boolean value. Defaults
> +       to false.
> ++
> +If one wishes to use the ref `ref/notes/true`, please use that literal
> +instead.
> ++
> +This configuration can be specified multiple times in order to allow
> +multiple notes refs to be included.
> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> index 2c3971390e..9ce5b8aaee 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -275,6 +275,9 @@ these explanations after `format-patch` has run but before sending,
>  keeping them as Git notes allows them to be maintained between versions
>  of the patch series (but see the discussion of the `notes.rewrite`
>  configuration options in linkgit:git-notes[1] to use this workflow).
> ++
> +The default is `--no-notes`, unless the `format.notes` configuration is
> +set.
>
>  --[no-]signature=<signature>::
>         Add a signature to each message produced. Per RFC 3676 the signature
> diff --git a/builtin/log.c b/builtin/log.c
> index e43ee12fb1..dad63cffc6 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -779,6 +779,8 @@ enum {
>
>  static int git_format_config(const char *var, const char *value, void *cb)
>  {
> +       struct rev_info *rev = cb;
> +
>         if (!strcmp(var, "format.headers")) {
>                 if (!value)
>                         die(_("format.headers without value"));
> @@ -864,6 +866,22 @@ static int git_format_config(const char *var, const char *value, void *cb)
>                         from = NULL;
>                 return 0;
>         }
> +       if (!strcmp(var, "format.notes")) {
> +               struct strbuf buf = STRBUF_INIT;
> +               int b = git_parse_maybe_bool(value);
> +               if (!b)
> +                       return 0;
> +               rev->show_notes = 1;
> +               if (b < 0) {
> +                       strbuf_addstr(&buf, value);
> +                       expand_notes_ref(&buf);
> +                       string_list_append(&rev->notes_opt.extra_notes_refs,
> +                                       strbuf_detach(&buf, NULL));
> +               } else {
> +                       rev->notes_opt.use_default_notes = 1;
> +               }
> +               return 0;
> +       }

What if someone has multiple format.notes entries in their config
file, but the last entry is "false" -- shouldn't that disable notes?
Also, what if they specify both "true" and e.g.
"refs/notes/my-cool-notes"?  In that case, should it show
refs/notes/my-cool-notes because that's obviously showing some notes
so it satisfies true as well as the specific request about which note,
or should it treat "true" the same as the-default-notes-ref and then
add the two refs together and show them both?

>
>         return git_log_config(var, value, cb);
>  }
> @@ -1617,8 +1635,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>         extra_to.strdup_strings = 1;
>         extra_cc.strdup_strings = 1;
>         init_log_defaults();
> -       git_config(git_format_config, NULL);
>         repo_init_revisions(the_repository, &rev, prefix);
> +       git_config(git_format_config, &rev);

Calling git_config() after repo_init_revisions() breaks things;
generally git_config() should always be called first.  Here,
git_format_config() can set up parameters used by
repo_init_revisions(), and by reversing the order of the two you end
up ignoring settings specified by the user (e.g. diff.context having a
value of 5).  This came up due to the bug report at
https://lore.kernel.org/git/xmqqa78d2qmk.fsf@gitster-ct.c.googlers.com/T/#mb6a09958ff10acde295b37a9136bc3791fd4a2c2
(though fixing the issue there _also_ requires fixing git_am_config()
to call git_diff_ui_config()).  To break the circular dependency here,
we'd need to store the information that git_format_config() discovers
in some data structure besides rev, and then after the
repo_init_revisions() call has finished then update rev.

I was just going to do that, but then ran into the questions above
about multiple format.notes entries in the config file, and am not as
sure about what should be done about that stuff (and I don't want to
try to translate the current behavior as-is while tweaking where the
stuff is stored, both because I'm not sure of the right behavior and
because I don't want future folks to blame the code to me when they
hit bugs in this area), so I'm firing off this email instead.

So, um...help?

Thanks,
Elijah