diff mbox series

[v9,4/6] notes.c: introduce '--separator=<paragraph-break>' option

Message ID ed930ef4f795f30792bc14d9c1939484e4976db8.1682671758.git.dyroneteng@gmail.com (mailing list archive)
State Superseded
Commit 3993a53a13c7b73a6b0548a8b3655e1f7f4524ad
Headers show
Series notes.c: introduce "--separator" option | expand

Commit Message

Teng Long April 28, 2023, 9:23 a.m. UTC
From: Teng Long <dyroneteng@gmail.com>

When adding new notes or appending to an existing notes, we will
insert a blank line between the paragraphs, like:

     $ git notes add -m foo -m bar
     $ git notes show HEAD
     foo

     bar

The default behavour sometimes is not enough, the user may want
to use a custom delimiter between paragraphs, like when
specifying '-m', '-F', '-C', '-c' options. So this commit
introduce a new '--separator' option for 'git notes add' and
'git notes append', for example when executing:

    $ git notes add -m foo -m bar --separator="-"
    $ git notes show HEAD
    foo
    -
    bar

a newline is added to the value given to --separator if it
does not end with one already. So when executing:

      $ git notes add -m foo -m bar --separator="-"
and
      $ export LF="
      "
      $ git notes add -m foo -m bar --separator="-$LF"

Both the two exections produce the same result.

The reason we use a "strbuf" array to concat but not "string_list", is
that the binary file content may contain '\0' in the middle, this will
cause the corrupt result if using a string to save.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 Documentation/git-notes.txt |  21 ++++--
 builtin/notes.c             | 108 ++++++++++++++++++++++++-------
 t/t3301-notes.sh            | 126 ++++++++++++++++++++++++++++++++++++
 3 files changed, 226 insertions(+), 29 deletions(-)

Comments

Junio C Hamano April 28, 2023, 8:44 p.m. UTC | #1
Teng Long <dyroneteng@gmail.com> writes:

> +static char *separator = "\n";

The only two ways this pointer gains a non-NULL value are with this
initialization and parsing the command line "--separator=<value>"
option with OPT_STRING().  Neither of them allocate new storage but
points an existing string that we do not "own" (and cannot free)
with the pointer.  So it probably is safer to make it a pointer to a
const string, i.e.

	static const char *separator = "\n";

> @@ -213,65 +229,96 @@ static void write_note_data(struct note_data *d, struct object_id *oid)
>  	}
>  }
>  
> +static void insert_separator(struct strbuf *message, size_t pos)
> +{
> +	if (separator[strlen(separator) - 1] == '\n')
> +		strbuf_addstr(message, separator);
> +	else
> +		strbuf_insertf(message, pos, "%s%s", separator, "\n");
> +}
> +
> +static void concat_messages(struct note_data *d)
> +{
> +	struct strbuf msg = STRBUF_INIT;
> +
> +	size_t i;
> +	for (i = 0; i < d->msg_nr ; i++) {

Wrong placement of the blank line that separates the declaration and
the first statement.
Teng Long May 6, 2023, 9:12 a.m. UTC | #2
Junio C Hamano <gitster@pobox.com> writes:

>> +static char *separator = "\n";
>
>The only two ways this pointer gains a non-NULL value are with this
>initialization and parsing the command line "--separator=<value>"
>option with OPT_STRING().  Neither of them allocate new storage but
>points an existing string that we do not "own" (and cannot free)
>with the pointer.  So it probably is safer to make it a pointer to a
>const string, i.e.
>
>	static const char *separator = "\n";
>
>> @@ -213,65 +229,96 @@ static void write_note_data(struct note_data *d, struct object_id *oid)
>>  	}
>>  }
>>  
>> +static void insert_separator(struct strbuf *message, size_t pos)
>> +{
>> +	if (separator[strlen(separator) - 1] == '\n')
>> +		strbuf_addstr(message, separator);
>> +	else
>> +		strbuf_insertf(message, pos, "%s%s", separator, "\n");
>> +}
>> +
>> +static void concat_messages(struct note_data *d)
>> +{
>> +	struct strbuf msg = STRBUF_INIT;
>> +
>> +	size_t i;
>> +	for (i = 0; i < d->msg_nr ; i++) {
>
>Wrong placement of the blank line that separates the declaration and
>the first statement.

Thanks for your advice and detailed explanation. I have noticed
in "what's cooking in git" that this patchset is being planed to
merge into 'next', so I display the diff here for the convenience
of applying, or please let me know if a new patch is required.

diff --git a/builtin/notes.c b/builtin/notes.c
index 8905298b..6eede305 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -28,7 +28,7 @@
 #include "worktree.h"
 #include "write-or-die.h"
 
-static const char *separator = "\n";
+static char *separator = "\n";
 static const char * const git_notes_usage[] = {
        N_("git notes [--ref <notes-ref>] [list [<object>]]"),
        N_("git notes [--ref <notes-ref>] add [-f] [--allow-empty] [--separator=<paragraph-break>] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]"),
@@ -248,8 +248,8 @@ static void append_separator(struct strbuf *message)
 static void concat_messages(struct note_data *d)
 {
        struct strbuf msg = STRBUF_INIT;
-       size_t i;
 
+       size_t i;
        for (i = 0; i < d->msg_nr ; i++) {
                if (d->buf.len)
                        append_separator(&d->buf);
Teng Long May 6, 2023, 9:22 a.m. UTC | #3
>diff --git a/builtin/notes.c b/builtin/notes.c
>index 8905298b..6eede305 100644
>--- a/builtin/notes.c
>+++ b/builtin/notes.c
>@@ -28,7 +28,7 @@
> #include "worktree.h"
> #include "write-or-die.h"
> 
>-static const char *separator = "\n";
>+static char *separator = "\n";
> static const char * const git_notes_usage[] = {
>        N_("git notes [--ref <notes-ref>] [list [<object>]]"),
>        N_("git notes [--ref <notes-ref>] add [-f] [--allow-empty] [--separator=<paragraph-break>] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]"),
>@@ -248,8 +248,8 @@ static void append_separator(struct strbuf *message)
> static void concat_messages(struct note_data *d)
> {
>        struct strbuf msg = STRBUF_INIT;
>-       size_t i;
> 
>+       size_t i;
>        for (i = 0; i < d->msg_nr ; i++) {
>                if (d->buf.len)
>                        append_separator(&d->buf);

Sorry, the direction is reversed wrongly.

diff --git a/builtin/notes.c b/builtin/notes.c
index 6eede305..8905298b 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -28,7 +28,7 @@
 #include "worktree.h"
 #include "write-or-die.h"
 
-static char *separator = "\n";
+static const char *separator = "\n";
 static const char * const git_notes_usage[] = {
        N_("git notes [--ref <notes-ref>] [list [<object>]]"),
        N_("git notes [--ref <notes-ref>] add [-f] [--allow-empty] [--separator=<paragraph-break>] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]"),
@@ -248,8 +248,8 @@ static void append_separator(struct strbuf *message)
 static void concat_messages(struct note_data *d)
 {
        struct strbuf msg = STRBUF_INIT;
-
        size_t i;
+
        for (i = 0; i < d->msg_nr ; i++) {
                if (d->buf.len)
                        append_separator(&d->buf);
Kristoffer Haugsbakk May 10, 2023, 7:19 p.m. UTC | #4
I realize that this series is going to be merged to `master`.[1] I was
trying out this new change since I might have some use for it when the
next version is released.

On Fri, Apr 28, 2023, at 11:23, Teng Long wrote:
> From: Teng Long <dyroneteng@gmail.com>
>
> When adding new notes or appending to an existing notes, we will
> insert a blank line between the paragraphs, like:

The test case[2] `append: specify an empty separator` demonstrates that
`--separator=""` is the same as the default behavior, namely to add a
blank line. It has (according to the commit messages) been like this
since v5 of this patch.[3]

v4 of this patch special-cased `--separator=""` to mean “no
separator”. And this was the same behavior as the original
`--no-blankline`,[4] which eventually mutated into `--separator`.

Why was this changed to act the same as the default behavior (add a
blank line)? I can’t seem to find a note for it on the cover letter of
v5 or in the relevant replies.

It seemed that v4 of this patch (with special-cased empty argument) was
perhaps based on Eric Sunshine’s suggestion:[5]

> Taking a step back, perhaps think of this in terms of "separator". The
> default behavior is to insert "\n" as a separator between notes. If
> you add a --separator option, then users could supply their own
> separator, such as "----\n" or, in your case, "" to suppress the blank
> line.

(And then reiterated in a v4 email [6])

Was the idea perhaps to eventually (separately) add a separate option
which functions like `--each-message-is-line-not-paragraph`, like what
was mentioned in [7]?

Maybe I’ve missed something. (I probably have.)

[1] https://lore.kernel.org/git/xmqqpm785erg.fsf@gitster.g/T/#md9b20801457c3eb24dc0e793f5dfbeae2f2707fd
[2] On `next`, 74a8c73209 (Sync with 'master', 2023-05-09)
[3] https://lore.kernel.org/git/a74c96d6dd23f2f1df6d3492093f3fd27451e24c.1676551077.git.dyroneteng@gmail.com/

   Commit message on v4:

   >  * --separator='': we specify an empty separator which means will
   >  append the message directly without inserting any separator at
   >  first.

   Commit message on v5:

   > * --separator='': we specify an empty separator which has the same
   > behavour with --separator='\n' and or not specified the option.

[4] https://lore.kernel.org/git/20221013055654.39628-1-tenglong.tl@alibaba-inc.com/
[5] https://lore.kernel.org/git/CAPig+cRcezSp4Rqt1Y9bD-FT6+7b0g9qHfbGRx65AOnw2FQXKg@mail.gmail.com/
[6] https://lore.kernel.org/git/CAPig+cSF7Fp3oM4TRU1QbiSzTeKNd1qGtqU7goPc1r-p4g8mkg@mail.gmail.com/
[7] https://lore.kernel.org/git/xmqqh6yh3nk4.fsf@gitster.g/
Teng Long May 12, 2023, 4:07 a.m. UTC | #5
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:

>I realize that this series is going to be merged to `master`.[1] I was
>trying out this new change since I might have some use for it when the
>next version is released.

I'm glad there are use cases for the topic.

>The test case[2] `append: specify an empty separator` demonstrates that
>`--separator=""` is the same as the default behavior, namely to add a
>blank line. It has (according to the commit messages) been like this
>since v5 of this patch.[3]
>
>v4 of this patch special-cased `--separator=""` to mean “no
>separator”. And this was the same behavior as the original
>`--no-blankline`,[4] which eventually mutated into `--separator`.
>
>Why was this changed to act the same as the default behavior (add a
>blank line)? I can’t seem to find a note for it on the cover letter of
>v5 or in the relevant replies.
>
>It seemed that v4 of this patch (with special-cased empty argument) was
>perhaps based on Eric Sunshine’s suggestion:[5]
>
>> Taking a step back, perhaps think of this in terms of "separator". The
>> default behavior is to insert "\n" as a separator between notes. If
>> you add a --separator option, then users could supply their own
>> separator, such as "----\n" or, in your case, "" to suppress the blank
>> line.
>
>(And then reiterated in a v4 email [6])

There is indeed this change in the process, I did not express it clearly in the
patch, sorry,  allow me to explain it:

Initially, I wanted to support separator insertion with --[no-]blankline. It was
intuitive and clear in logic. If it is specified as --blankline, newline would
be append, if it is --no-blankline, newline would not be append, and --blankline
would be the default behavior.

The problem with this is that "blankline" might not be a good name because it
doesn't seem to express the meaning of the separator or paragraph-break, and
since we're going to support separators, it might be more thorough to support a
custom separator, which is a better solution I think.

Returning to the issue of -separator = "", my thought is that I want
--separator="" to behave the same way as
--separator="<any-string-without-a-trailing-\n'>" when deals a string which does
not contains a trailing newline. This will eliminate one more implicit logic and
make behavior more consistent.

>Was the idea perhaps to eventually (separately) add a separate option
>which functions like `--each-message-is-line-not-paragraph`, like what
>was mentioned in [7]?

I think --no-separator maybe a better name, means that not any separator will be
append between two paragraphs even a newline.

Thanks.
Kristoffer Haugsbakk May 12, 2023, 7:29 a.m. UTC | #6
Hi Teng Long and thanks for the explanation.

On Fri, May 12, 2023, at 06:07, Teng Long wrote:
> I think --no-separator maybe a better name, means that not any separator will be
> append between two paragraphs even a newline.

That makes sense. Using `--no-separator` seems very consistent with
other commands.
Junio C Hamano May 16, 2023, 5 p.m. UTC | #7
Teng Long <dyroneteng@gmail.com> writes:

> Returning to the issue of -separator = "", my thought is that I want
> --separator="" to behave the same way as
> --separator="<any-string-without-a-trailing-\n'>" when deals a string which does
> not contains a trailing newline. This will eliminate one more implicit logic and
> make behavior more consistent.

An obvious alternative would be to use the string given to the
"--separator" option literally, I guess, and you can add your own
newline if you want to.  But most of the time you would not want to
have an incomplete line, so appending the newline at the end by
default does make sense.

Special casing an empty value to "--separator" as "nothing" does
make sort-of sense.  Using a blank line as the inter-paragraph
separator is the default, and there is not much use in the
"--separator=''" that becomes "--separator=$'\012'" automatically.

> I think --no-separator maybe a better name, means that not any separator will be
> append between two paragraphs even a newline.

It would work as well.  Is it something we can safely add before
merging the topic to upcoming -rc1?

Thanks.
Teng Long May 17, 2023, 3:58 a.m. UTC | #8
Junio C Hamano <gitster@pobox.com> writes:

>> I think --no-separator maybe a better name, means that not any separator will be
>> append between two paragraphs even a newline.
>
>It would work as well.  Is it something we can safely add before
>merging the topic to upcoming -rc1?

I noticed rc-1 will be released at this Friday, I will post a new patch
about "--no-separator" today(UTC/GMT+08:00).

Thanks.
Junio C Hamano May 17, 2023, 3:32 p.m. UTC | #9
Teng Long <dyroneteng@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>>> I think --no-separator maybe a better name, means that not any separator will be
>>> append between two paragraphs even a newline.
>>
>>It would work as well.  Is it something we can safely add before
>>merging the topic to upcoming -rc1?
>
> I noticed rc-1 will be released at this Friday, I will post a new patch
> about "--no-separator" today(UTC/GMT+08:00).

Thanks.
Junio C Hamano June 14, 2023, 1:02 a.m. UTC | #10
This step seems to break CI asan/ubsan job

https://github.com/git/git/actions/runs/5260417146/jobs/9507299904#step:4:1826

Perhaps something like this is in order?

 builtin/notes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git c/builtin/notes.c w/builtin/notes.c
index 6eede30597..785b1922ad 100644
--- c/builtin/notes.c
+++ w/builtin/notes.c
@@ -239,7 +239,7 @@ static void write_note_data(struct note_data *d, struct object_id *oid)
 
 static void append_separator(struct strbuf *message)
 {
-	if (separator[strlen(separator) - 1] == '\n')
+	if (*separator && separator[strlen(separator) - 1] == '\n')
 		strbuf_addstr(message, separator);
 	else
 		strbuf_addf(message, "%s%s", separator, "\n");
Eric Sunshine June 14, 2023, 1:41 a.m. UTC | #11
On Tue, Jun 13, 2023 at 9:02 PM Junio C Hamano <gitster@pobox.com> wrote:
> This step seems to break CI asan/ubsan job
> https://github.com/git/git/actions/runs/5260417146/jobs/9507299904#step:4:1826
> Perhaps something like this is in order?
>
> diff --git c/builtin/notes.c w/builtin/notes.c
> @@ -239,7 +239,7 @@ static void write_note_data(struct note_data *d, struct object_id *oid)
>  static void append_separator(struct strbuf *message)
>  {
> -       if (separator[strlen(separator) - 1] == '\n')
> +       if (*separator && separator[strlen(separator) - 1] == '\n')

Is this the same issue Peff reported[1]? His proposed solution was a
bit different.

[1]: https://lore.kernel.org/git/20230519005447.GA2955320@coredump.intra.peff.net/
Junio C Hamano June 14, 2023, 2:07 a.m. UTC | #12
Eric Sunshine <sunshine@sunshineco.com> writes:

> Is this the same issue Peff reported[1]? His proposed solution was a
> bit different.
>
> [1]: https://lore.kernel.org/git/20230519005447.GA2955320@coredump.intra.peff.net/

Ah, I should have checked before merging the topic in 'next'.
Jeff King June 15, 2023, 7:13 a.m. UTC | #13
On Tue, Jun 13, 2023 at 07:07:38PM -0700, Junio C Hamano wrote:

> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> > Is this the same issue Peff reported[1]? His proposed solution was a
> > bit different.
> >
> > [1]: https://lore.kernel.org/git/20230519005447.GA2955320@coredump.intra.peff.net/
> 
> Ah, I should have checked before merging the topic in 'next'.

I only found it after it was in 'next'. :)

It originally did not cause problems, but the switch to clang started
triggering it in CI (it was this case that motivated me to send those
patches CI patches).

I was not following the topic closely, but I think there is a v10 of
this series anyway, so you may want to eject it from 'next' in the
meantime (I had assumed you were going to do so as part of the
post-release rewind, but it looks like that happened already and it got
re-merged).

-Peff
Junio C Hamano June 15, 2023, 7:15 p.m. UTC | #14
Jeff King <peff@peff.net> writes:

> I was not following the topic closely, but I think there is a v10 of
> this series anyway, so you may want to eject it from 'next' in the
> meantime (I had assumed you were going to do so as part of the
> post-release rewind, but it looks like that happened already and it got
> re-merged).

I can do the post-release rewind again ;-)

I somehow had an impression that the topic was more or less done,
but if it deserves another chance, let's give it one.

Thanks.
Teng Long June 19, 2023, 6:08 a.m. UTC | #15
Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
> 
> > I was not following the topic closely, but I think there is a v10 of
> > this series anyway, so you may want to eject it from 'next' in the
> > meantime (I had assumed you were going to do so as part of the
> > post-release rewind, but it looks like that happened already and it got
> > re-merged).
> 
> I can do the post-release rewind again ;-)
> 
> I somehow had an impression that the topic was more or less done,
> but if it deserves another chance, let's give it one.
> 
> Thanks.

I have been on vacation for the last two weeks, sorry for not
replying in time.

I think that is :

  time-1: v9 was in next, actually v9 [4/6] will break the CI, but
          [5/6] fix it so the CI passed on the whole patchset.
  time-2: then v10 was tought a new `--no-separator ` mainly
          On the release point, v10 implicitly fixed the problem
          in v9 [4/6]
  time-3: 2.41 release work begin
  time-4: Peff found that the v9 [4/6] merged in next would break
          CI and a UBSan problem.
  time-5: I wanted to make a full reroll to fix the UBSan problem,
          by the way, separate the "--no-separator" to a single
          commit, because I thought it cannot catch the last bus
          on 2.41 maybe.

I think we are now in a new cycle, maybe we could rewind and use
v11 directlly? Sorry that it may have brought on extra work.

Thanks.
Junio C Hamano June 20, 2023, 8:36 p.m. UTC | #16
Teng Long <dyroneteng@gmail.com> writes:

> I think we are now in a new cycle, maybe we could rewind and use
> v11 directlly? Sorry that it may have brought on extra work.

I think the tl/notes-separator topic is no longer in 'next' so we
can do whatever you like to it ;-).  It would be easy to just
replace the whole thing with newly reviewed version.

Thanks.
Teng Long June 21, 2023, 2:50 a.m. UTC | #17
> Teng Long <dyroneteng@gmail.com> writes:
> 
> > I think we are now in a new cycle, maybe we could rewind and use
> > v11 directlly? Sorry that it may have brought on extra work.
> 
> I think the tl/notes-separator topic is no longer in 'next' so we
> can do whatever you like to it ;-).  It would be easy to just
> replace the whole thing with newly reviewed version.

Yes, v11 fixed the UBSan problem which found by Peff and also the broken CI
under run a bisection (both are found in v9 which already be rewinded in
'next'). If there are no new comments on v11, I think maybe it's what we want at
present.

Thanks.
diff mbox series

Patch

diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index efbc10f0..59980b21 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -9,9 +9,9 @@  SYNOPSIS
 --------
 [verse]
 'git notes' [list [<object>]]
-'git notes' add [-f] [--allow-empty] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
+'git notes' add [-f] [--allow-empty] [--separator=<paragraph-break>] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
 'git notes' copy [-f] ( --stdin | <from-object> [<to-object>] )
-'git notes' append [--allow-empty] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
+'git notes' append [--allow-empty] [--separator=<paragraph-break>] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
 'git notes' edit [--allow-empty] [<object>]
 'git notes' show [<object>]
 'git notes' merge [-v | -q] [-s <strategy> ] <notes-ref>
@@ -65,7 +65,9 @@  add::
 	However, if you're using `add` interactively (using an editor
 	to supply the notes contents), then - instead of aborting -
 	the existing notes will be opened in the editor (like the `edit`
-	subcommand).
+	subcommand). If you specify multiple `-m` and `-F`, a blank
+	line will be inserted between the messages. Use the `--separator`
+	option to insert other delimiters.
 
 copy::
 	Copy the notes for the first object onto the second object (defaults to
@@ -85,8 +87,12 @@  corresponding <to-object>.  (The optional `<rest>` is ignored so that
 the command can read the input given to the `post-rewrite` hook.)
 
 append::
-	Append to the notes of an existing object (defaults to HEAD).
-	Creates a new notes object if needed.
+	Append new message(s) given by `-m` or `-F` options to an
+	existing note, or add them as a new note if one does not
+	exist, for the object (defaults to HEAD).  When appending to
+	an existing note, a blank line is added before each new
+	message as an inter-paragraph separator.  The separator can
+	be customized with the `--separator` option.
 
 edit::
 	Edit the notes for a given object (defaults to HEAD).
@@ -159,6 +165,11 @@  OPTIONS
 	Allow an empty note object to be stored. The default behavior is
 	to automatically remove empty notes.
 
+--separator <paragraph-break>::
+	Specify a string used as a custom inter-paragraph separator
+	(a newline is added at the end as needed).  Defaults to a
+	blank line.
+
 --ref <ref>::
 	Manipulate the notes tree in <ref>.  This overrides
 	`GIT_NOTES_REF` and the "core.notesRef" configuration.  The ref
diff --git a/builtin/notes.c b/builtin/notes.c
index 9d8ca795..3215bce1 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -8,6 +8,7 @@ 
  */
 
 #include "cache.h"
+#include "alloc.h"
 #include "config.h"
 #include "builtin.h"
 #include "gettext.h"
@@ -27,11 +28,12 @@ 
 #include "worktree.h"
 #include "write-or-die.h"
 
+static char *separator = "\n";
 static const char * const git_notes_usage[] = {
 	N_("git notes [--ref <notes-ref>] [list [<object>]]"),
-	N_("git notes [--ref <notes-ref>] add [-f] [--allow-empty] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]"),
+	N_("git notes [--ref <notes-ref>] add [-f] [--allow-empty] [--separator=<paragraph-break>] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]"),
 	N_("git notes [--ref <notes-ref>] copy [-f] <from-object> <to-object>"),
-	N_("git notes [--ref <notes-ref>] append [--allow-empty] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]"),
+	N_("git notes [--ref <notes-ref>] append [--allow-empty] [--separator=<paragraph-break>] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]"),
 	N_("git notes [--ref <notes-ref>] edit [--allow-empty] [<object>]"),
 	N_("git notes [--ref <notes-ref>] show [<object>]"),
 	N_("git notes [--ref <notes-ref>] merge [-v | -q] [-s <strategy>] <notes-ref>"),
@@ -99,11 +101,19 @@  static const char * const git_notes_get_ref_usage[] = {
 static const char note_template[] =
 	N_("Write/edit the notes for the following object:");
 
+struct note_msg {
+	int stripspace;
+	struct strbuf buf;
+};
+
 struct note_data {
 	int given;
 	int use_editor;
 	char *edit_path;
 	struct strbuf buf;
+	struct note_msg **messages;
+	size_t msg_nr;
+	size_t msg_alloc;
 };
 
 static void free_note_data(struct note_data *d)
@@ -113,6 +123,12 @@  static void free_note_data(struct note_data *d)
 		free(d->edit_path);
 	}
 	strbuf_release(&d->buf);
+
+	while (d->msg_nr--) {
+		strbuf_release(&d->messages[d->msg_nr]->buf);
+		free(d->messages[d->msg_nr]);
+	}
+	free(d->messages);
 }
 
 static int list_each_note(const struct object_id *object_oid,
@@ -213,65 +229,96 @@  static void write_note_data(struct note_data *d, struct object_id *oid)
 	}
 }
 
+static void insert_separator(struct strbuf *message, size_t pos)
+{
+	if (separator[strlen(separator) - 1] == '\n')
+		strbuf_addstr(message, separator);
+	else
+		strbuf_insertf(message, pos, "%s%s", separator, "\n");
+}
+
+static void concat_messages(struct note_data *d)
+{
+	struct strbuf msg = STRBUF_INIT;
+
+	size_t i;
+	for (i = 0; i < d->msg_nr ; i++) {
+		if (d->buf.len)
+			insert_separator(&d->buf, d->buf.len);
+		strbuf_add(&msg, d->messages[i]->buf.buf, d->messages[i]->buf.len);
+		strbuf_addbuf(&d->buf, &msg);
+		if (d->messages[i]->stripspace)
+			strbuf_stripspace(&d->buf, 0);
+		strbuf_reset(&msg);
+	}
+	strbuf_release(&msg);
+}
+
 static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
 {
 	struct note_data *d = opt->value;
+	struct note_msg *msg = xmalloc(sizeof(*msg));
 
 	BUG_ON_OPT_NEG(unset);
 
-	if (d->buf.len)
-		strbuf_addch(&d->buf, '\n');
-	strbuf_addstr(&d->buf, arg);
-	strbuf_stripspace(&d->buf, 0);
-
-	d->given = 1;
+	strbuf_init(&msg->buf, strlen(arg));
+	strbuf_addstr(&msg->buf, arg);
+	ALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);
+	d->messages[d->msg_nr - 1] = msg;
+	msg->stripspace = 1;
 	return 0;
 }
 
 static int parse_file_arg(const struct option *opt, const char *arg, int unset)
 {
 	struct note_data *d = opt->value;
+	struct note_msg *msg = xmalloc(sizeof(*msg));
 
 	BUG_ON_OPT_NEG(unset);
 
-	if (d->buf.len)
-		strbuf_addch(&d->buf, '\n');
+	strbuf_init(&msg->buf , 0);
 	if (!strcmp(arg, "-")) {
-		if (strbuf_read(&d->buf, 0, 1024) < 0)
+		if (strbuf_read(&msg->buf, 0, 1024) < 0)
 			die_errno(_("cannot read '%s'"), arg);
-	} else if (strbuf_read_file(&d->buf, arg, 1024) < 0)
+	} else if (strbuf_read_file(&msg->buf, arg, 1024) < 0)
 		die_errno(_("could not open or read '%s'"), arg);
-	strbuf_stripspace(&d->buf, 0);
 
-	d->given = 1;
+	ALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);
+	d->messages[d->msg_nr - 1] = msg;
+	msg->stripspace = 1;
 	return 0;
 }
 
 static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)
 {
 	struct note_data *d = opt->value;
-	char *buf;
+	struct note_msg *msg = xmalloc(sizeof(*msg));
+	char *value;
 	struct object_id object;
 	enum object_type type;
 	unsigned long len;
 
 	BUG_ON_OPT_NEG(unset);
 
-	if (d->buf.len)
-		strbuf_addch(&d->buf, '\n');
-
+	strbuf_init(&msg->buf, 0);
 	if (repo_get_oid(the_repository, arg, &object))
 		die(_("failed to resolve '%s' as a valid ref."), arg);
-	if (!(buf = repo_read_object_file(the_repository, &object, &type, &len)))
+	if (!(value = repo_read_object_file(the_repository, &object, &type, &len)))
 		die(_("failed to read object '%s'."), arg);
 	if (type != OBJ_BLOB) {
-		free(buf);
+		strbuf_release(&msg->buf);
+		free(value);
+		free(msg);
 		die(_("cannot read note data from non-blob object '%s'."), arg);
 	}
-	strbuf_add(&d->buf, buf, len);
-	free(buf);
 
-	d->given = 1;
+	strbuf_add(&msg->buf, value, len);
+	free(value);
+
+	msg->buf.len = len;
+	ALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);
+	d->messages[d->msg_nr - 1] = msg;
+	msg->stripspace = 0;
 	return 0;
 }
 
@@ -406,6 +453,7 @@  static int add(int argc, const char **argv, const char *prefix)
 	struct object_id object, new_note;
 	const struct object_id *note;
 	struct note_data d = { .buf = STRBUF_INIT };
+
 	struct option options[] = {
 		OPT_CALLBACK_F('m', "message", &d, N_("message"),
 			N_("note contents as a string"), PARSE_OPT_NONEG,
@@ -422,6 +470,8 @@  static int add(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "allow-empty", &allow_empty,
 			N_("allow storing empty note")),
 		OPT__FORCE(&force, N_("replace existing notes"), PARSE_OPT_NOCOMPLETE),
+		OPT_STRING(0, "separator", &separator, N_("separator"),
+			N_("insert <paragraph-break> between paragraphs")),
 		OPT_END()
 	};
 
@@ -433,6 +483,10 @@  static int add(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_notes_add_usage, options);
 	}
 
+	if (d.msg_nr)
+		concat_messages(&d);
+	d.given = !!d.buf.len;
+
 	object_ref = argc > 1 ? argv[1] : "HEAD";
 
 	if (repo_get_oid(the_repository, object_ref, &object))
@@ -587,6 +641,8 @@  static int append_edit(int argc, const char **argv, const char *prefix)
 			parse_reuse_arg),
 		OPT_BOOL(0, "allow-empty", &allow_empty,
 			N_("allow storing empty note")),
+		OPT_STRING(0, "separator", &separator, N_("separator"),
+			N_("insert <paragraph-break> between paragraphs")),
 		OPT_END()
 	};
 	int edit = !strcmp(argv[0], "edit");
@@ -600,6 +656,10 @@  static int append_edit(int argc, const char **argv, const char *prefix)
 		usage_with_options(usage, options);
 	}
 
+	if (d.msg_nr)
+		concat_messages(&d);
+	d.given = !!d.buf.len;
+
 	if (d.given && edit)
 		fprintf(stderr, _("The -m/-F/-c/-C options have been deprecated "
 			"for the 'edit' subcommand.\n"
@@ -623,7 +683,7 @@  static int append_edit(int argc, const char **argv, const char *prefix)
 						       &type, &size);
 
 		if (d.buf.len && prev_buf && size)
-			strbuf_insertstr(&d.buf, 0, "\n");
+			insert_separator(&d.buf, 0);
 		if (prev_buf && size)
 			strbuf_insert(&d.buf, 0, prev_buf, size);
 		free(prev_buf);
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 3288aaec..dbadcf13 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -362,6 +362,7 @@  test_expect_success 'do not create empty note with -m ""' '
 '
 
 test_expect_success 'create note with combination of -m and -F' '
+	test_when_finished git notes remove HEAD &&
 	cat >expect-combine_m_and_F <<-EOF &&
 		foo
 
@@ -380,6 +381,25 @@  test_expect_success 'create note with combination of -m and -F' '
 	test_cmp expect-combine_m_and_F actual
 '
 
+test_expect_success 'create note with combination of -m and -F and --separator' '
+	cat >expect-combine_m_and_F <<-\EOF &&
+	foo
+	-------
+	xyzzy
+	-------
+	bar
+	-------
+	zyxxy
+	-------
+	baz
+	EOF
+	echo "xyzzy" >note_a &&
+	echo "zyxxy" >note_b &&
+	git notes add -m "foo" -F note_a -m "bar" -F note_b -m "baz" --separator "-------" &&
+	git notes show >actual &&
+	test_cmp expect-combine_m_and_F actual
+'
+
 test_expect_success 'remove note with "git notes remove"' '
 	git notes remove HEAD^ &&
 	git notes remove &&
@@ -521,6 +541,85 @@  test_expect_success 'listing non-existing notes fails' '
 	test_must_be_empty actual
 '
 
+test_expect_success 'append: specify an empty separator' '
+	test_when_finished git notes remove HEAD &&
+	cat >expect <<-\EOF &&
+	notes-1
+
+	notes-2
+	EOF
+
+	git notes add -m "notes-1" &&
+	git notes append --separator="" -m "notes-2" &&
+	git notes show >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'append: specify separator with line break' '
+	test_when_finished git notes remove HEAD &&
+	cat >expect <<-\EOF &&
+	notes-1
+	-------
+	notes-2
+	EOF
+
+	git notes add -m "notes-1" &&
+	git notes append --separator="-------$LF" -m "notes-2" &&
+	git notes show >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'append: specify separator without line break' '
+	test_when_finished git notes remove HEAD &&
+	cat >expect <<-\EOF &&
+	notes-1
+	-------
+	notes-2
+	EOF
+
+	git notes add -m "notes-1" &&
+	git notes append --separator="-------" -m "notes-2" &&
+	git notes show >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'append: specify separator with multiple messages' '
+	test_when_finished git notes remove HEAD &&
+	cat >expect <<-\EOF &&
+	notes-1
+	-------
+	notes-2
+	-------
+	notes-3
+	EOF
+
+	git notes add -m "notes-1" &&
+	git notes append --separator="-------" -m "notes-2" -m "notes-3" &&
+	git notes show >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'append note with combination of -m and -F and --separator' '
+	test_when_finished git notes remove HEAD &&
+	cat >expect-combine_m_and_F <<-\EOF &&
+	m-notes-1
+	-------
+	f-notes-1
+	-------
+	m-notes-2
+	-------
+	f-notes-2
+	-------
+	m-notes-3
+	EOF
+
+	echo "f-notes-1" >note_a &&
+	echo "f-notes-2" >note_b &&
+	git notes append -m "m-notes-1" -F note_a -m "m-notes-2" -F note_b -m "m-notes-3" --separator "-------" &&
+	git notes show >actual &&
+	test_cmp expect-combine_m_and_F actual
+'
+
 test_expect_success 'append to existing note with "git notes append"' '
 	cat >expect <<-EOF &&
 		Initial set of notes
@@ -818,6 +917,33 @@  test_expect_success 'create note from blob with "git notes add -C" reuses blob i
 	test_cmp blob actual
 '
 
+test_expect_success 'create note from blob with "-C", also specify "-m", "-F" and "--separator"' '
+	# 8th will be reuseed in following tests, so rollback when the test is done
+	test_when_finished "git notes remove && git notes add -C $(cat blob)" &&
+	commit=$(git rev-parse HEAD) &&
+	cat >expect <<-EOF &&
+		commit $commit
+		Author: A U Thor <author@example.com>
+		Date:   Thu Apr 7 15:20:13 2005 -0700
+
+		${indent}8th
+
+		Notes:
+		${indent}This is a blob object
+		${indent}-------
+		${indent}This is created by -m
+		${indent}-------
+		${indent}This is created by -F
+	EOF
+
+	git notes remove &&
+	echo "This is a blob object" | git hash-object -w --stdin >blob &&
+	echo "This is created by -F" >note_a &&
+	git notes add -C $(cat blob) -m "This is created by -m" -F note_a --separator="-------" &&
+	git log -1 >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'create note from other note with "git notes add -c"' '
 	test_commit 9th &&
 	commit=$(git rev-parse HEAD) &&