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

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

[Denton Liu]

Hi Junio,

I've changed format.notes so that it can accept a boolean as well.
Hopefully, my last email has addressed the remainder of your comments.

Changes since v3:

* Made format.notes accept a boolean instead of "standard" to get
  default notes

Changes since v2:

* Fixed if-else code style
* Fixed typoed errors in 2/2 log message

Changes since v1:

* Made format.notes accept a notes ref instead of a boolean


Denton Liu (2):
  git-format-patch.txt: document --no-notes option
  format-patch: teach format.notes config option

 Documentation/config/format.txt    | 15 +++++++
 Documentation/git-format-patch.txt |  7 ++-
 builtin/log.c                      | 20 ++++++++-
 t/t4014-format-patch.sh            | 70 ++++++++++++++++++++++++++++++
 4 files changed, 110 insertions(+), 2 deletions(-)

Range-diff against v3:
1:  4c3535f25b = 1:  4c3535f25b git-format-patch.txt: document --no-notes option
2:  df864c4adf ! 2:  7cb770e190 format-patch: teach format.notes config option
    @@ -25,14 +25,16 @@
      	format-patch by default.
     +
     +format.notes::
    -+	A ref which specifies where to get the notes (see
    -+	linkgit:git-notes[1]) that are appended for the commit after the
    -+	three-dash line.
    ++	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 the special value of "standard" is specified, then the standard notes
    -+ref is used (i.e. the notes ref used by `git notes` when no `--ref`
    -+argument is specified). If one wishes to use the ref
    -+`ref/notes/standard`, please use that literal instead.
    ++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.
    @@ -69,15 +71,17 @@
      	}
     +	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 (!strcmp(value, "standard")) {
    -+			rev->notes_opt.use_default_notes = 1;
    -+		} else {
    ++		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;
     +	}
    @@ -117,7 +121,7 @@
     +	git format-patch -1 --stdout --no-notes --notes >out &&
     +	grep "notes config message" out &&
     +
    -+	test_config format.notes standard &&
    ++	test_config format.notes true &&
     +	git format-patch -1 --stdout >out &&
     +	grep "notes config message" out &&
     +	git format-patch -1 --stdout --notes >out &&
-- 
2.21.0.1049.geb646f7864

[PATCH v4 1/2] git-format-patch.txt: document --no-notes option

[Denton Liu]

Internally, git-format-patch uses the `handle_revision_opt` parser. The
parser handles the `--no-notes` option to negate an earlier `--notes`
option, but it isn't documented. Document this option so that users are
aware of it.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-format-patch.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 1af85d404f..2c3971390e 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -22,7 +22,8 @@ SYNOPSIS
 		   [--rfc] [--subject-prefix=Subject-Prefix]
 		   [(--reroll-count|-v) <n>]
 		   [--to=<email>] [--cc=<email>]
-		   [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
+		   [--[no-]cover-letter] [--quiet]
+		   [--no-notes | --notes[=<ref>]]
 		   [--interdiff=<previous>]
 		   [--range-diff=<previous> [--creation-factor=<percent>]]
 		   [--progress]
@@ -263,6 +264,7 @@ material (this may change in the future).
 	for details.
 
 --notes[=<ref>]::
+--no-notes::
 	Append the notes (see linkgit:git-notes[1]) for the commit
 	after the three-dash line.
 +
-- 
2.21.0.1049.geb646f7864

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

[Denton Liu]

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;
+	}
 
 	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);
 	rev.commit_format = CMIT_FMT_EMAIL;
 	rev.expand_tabs_in_log_default = 0;
 	rev.verbose_header = 1;
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index b6e2fdbc44..4d5719fe2c 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -738,6 +738,76 @@ test_expect_success 'format-patch --notes --signoff' '
 	sed "1,/^---$/d" out | grep "test message"
 '
 
+test_expect_success 'format-patch notes output control' '
+	git notes add -m "notes config message" HEAD &&
+	test_when_finished git notes remove HEAD &&
+
+	git format-patch -1 --stdout >out &&
+	! grep "notes config message" out &&
+	git format-patch -1 --stdout --notes >out &&
+	grep "notes config message" out &&
+	git format-patch -1 --stdout --no-notes >out &&
+	! grep "notes config message" out &&
+	git format-patch -1 --stdout --notes --no-notes >out &&
+	! grep "notes config message" out &&
+	git format-patch -1 --stdout --no-notes --notes >out &&
+	grep "notes config message" out &&
+
+	test_config format.notes true &&
+	git format-patch -1 --stdout >out &&
+	grep "notes config message" out &&
+	git format-patch -1 --stdout --notes >out &&
+	grep "notes config message" out &&
+	git format-patch -1 --stdout --no-notes >out &&
+	! grep "notes config message" out &&
+	git format-patch -1 --stdout --notes --no-notes >out &&
+	! grep "notes config message" out &&
+	git format-patch -1 --stdout --no-notes --notes >out &&
+	grep "notes config message" out
+'
+
+test_expect_success 'format-patch with multiple notes refs' '
+	git notes --ref note1 add -m "this is note 1" HEAD &&
+	test_when_finished git notes --ref note1 remove HEAD &&
+	git notes --ref note2 add -m "this is note 2" HEAD &&
+	test_when_finished git notes --ref note2 remove HEAD &&
+
+	git format-patch -1 --stdout >out &&
+	! grep "this is note 1" out &&
+	! grep "this is note 2" out &&
+	git format-patch -1 --stdout --notes=note1 >out &&
+	grep "this is note 1" out &&
+	! grep "this is note 2" out &&
+	git format-patch -1 --stdout --notes=note2 >out &&
+	! grep "this is note 1" out &&
+	grep "this is note 2" out &&
+	git format-patch -1 --stdout --notes=note1 --notes=note2 >out &&
+	grep "this is note 1" out &&
+	grep "this is note 2" out &&
+
+	test_config format.notes note1 &&
+	git format-patch -1 --stdout >out &&
+	grep "this is note 1" out &&
+	! grep "this is note 2" out &&
+	git format-patch -1 --stdout --no-notes >out &&
+	! grep "this is note 1" out &&
+	! grep "this is note 2" out &&
+	git format-patch -1 --stdout --notes=note2 >out &&
+	grep "this is note 1" out &&
+	grep "this is note 2" out &&
+	git format-patch -1 --stdout --no-notes --notes=note2 >out &&
+	! grep "this is note 1" out &&
+	grep "this is note 2" out &&
+
+	git config --add format.notes note2 &&
+	git format-patch -1 --stdout >out &&
+	grep "this is note 1" out &&
+	grep "this is note 2" out &&
+	git format-patch -1 --stdout --no-notes >out &&
+	! grep "this is note 1" out &&
+	! grep "this is note 2" out
+'
+
 echo "fatal: --name-only does not make sense" > expect.name-only
 echo "fatal: --name-status does not make sense" > expect.name-status
 echo "fatal: --check does not make sense" > expect.check
-- 
2.21.0.1049.geb646f7864

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

[Elijah Newren]

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

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

[Denton Liu]

Hi Elijah,

On Sat, Dec 07, 2019 at 11:48:59PM -0800, Elijah Newren wrote:
> > @@ -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?

I think I'll just copy the existing logic of --notes, --notes=<ref> and
--no-notes from revision.c to `format.notes = true`, 
`format.notes = <ref>` and `format.notes = false` respectively. IOW,
with `format.notes = true`, we'll unconditionally use the default notes,
with `format.notes = <ref>`, we'll append <ref> to the reflist and with
`format.notes = false`, we'll clear and unset the notes refs.

It seems like that logic has been around for almost a decade and I don't
think anyone's complained about it so I think it should be safe to
duplicate.

> 
> >
> >         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 see, I'll move the git_config() back up while I'm at it.

> 
> 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.

Sounds good, I don't want my bugs blamed on anyone else either ;)

I'll try to get a patchset out soon and hopefully you'll be able to base
your work off of that.

Thanks,

Denton

> 
> So, um...help?
> 
> Thanks,
> Elijah