v6, 3 partsnotes.c: cleanup for "designated init"

From: Teng Long <dyroneteng@gmail.com>

Diff since v5:

1. Optimized and fixed the commit-message problems.
2. Optimized and fixed the related documentation.
3. the order of "-m" and -F should matter, but not, it was newly
introduced problem in v4, now fixed in v5.

By the way, In order to add a separator when parsing '-F and '-m', I used 'string_list' to save all messages for subsequent processing instead of processing in callback, because the value of '--separator' has not been initialized and set at this time and I have not found a similar "OPT_XXX" API for doing this. I'm not sure if it's worth adding a similar API to parse the value of an option in advance and initialize it for this scenario. If so, it may not be suitable to continue to incubate in the current patch series, but I will try to contribute it.

Thanks.

Teng Long (3):
  notes.c: cleanup 'strbuf_grow' call in 'append_edit'
  notes.c: cleanup for "designated init"
  notes.c: introduce '--separator=<paragraph-break>' option

 Documentation/git-notes.txt |  20 ++++++--
 builtin/notes.c             |  78 ++++++++++++++++++----------
 t/t3301-notes.sh            | 100 ++++++++++++++++++++++++++++++++++++
 3 files changed, 168 insertions(+), 30 deletions(-)

Range-diff against v5:
1:  9a450669 ! 1:  b029ee0b notes.c: cleanup 'strbuf_grow' call in 'append_edit'
    @@ Commit message
         needed, but actually when inserting, "strbuf_insertstr(&d.buf, 0,
         "\n");" will do the "grow" for us.
     
    -    Best guess may be that the author originally inserted "\n" manually by
    -    direct manipulation of the strbuf rather than employing a strbuf
    -    function, but then switched to strbuf_insert() before submitting the
    -    series and forgot to remove the now-unnecessary strbuf_grow().
    +    348f199b (builtin-notes: Refactor handling of -F option to allow
    +    combining -m and -F, 2010-02-13) added these to mimic the code
    +    introduced by 2347fae5 (builtin-notes: Add "append" subcommand for
    +    appending to note objects, 2010-02-13) that reads in previous note
    +    before the message.  And the resulting code with explicit sizing is
    +    carried to this day.
    +
    +    In the context of reading an existing note in, exact sizing may have
    +    made sense, but because the resulting note needs cleansing with
    +    stripspace() when appending with this option, such an exact sizing
    +    does not buy us all that much in practice.
    +
    +    It may help avoiding overallocation due to ALLOC_GROW() slop, but
    +    nobody can feed so many long messages for it to matter from the
    +    command line.
     
         Signed-off-by: Teng Long <dyroneteng@gmail.com>
         Helped-by: Eric Sunshine <sunshine@sunshineco.com>
    +    Helped-by: Junio C Hamano <gitster@pobox.com>
     
      ## builtin/notes.c ##
     @@ builtin/notes.c: static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
2:  e7bc6060 ! 2:  043db631 notes.c: cleanup for "designated init"
    @@ Commit message
         The "struct note_data d = { 0, 0, NULL, STRBUF_INIT };" style could be
         replaced with designated init format.
     
    +    Signed-off-by: Teng Long <dyroneteng@gmail.com>
    +
      ## builtin/notes.c ##
     @@ builtin/notes.c: static int add(int argc, const char **argv, const char *prefix)
      	struct notes_tree *t;
3:  a74c96d6 ! 3:  d5a6c747 notes.c: introduce "--separator" option
    @@ Metadata
     Author: Teng Long <dyroneteng@gmail.com>
     
      ## Commit message ##
    -    notes.c: introduce "--separator" option
    +    notes.c: introduce '--separator=<paragraph-break>' option
     
    -    When appending to a given notes object and the appended note is not
    -    empty too, we will insert a blank line at first which separates the
    -    existing note and the appended one, which as the separator.
    +    When adding new notes or appending to an existing notes, we will
    +    insert a blank line between the paragraphs, like:
     
    -    Sometimes, we want to use a specified <separator> as the separator. For
    -    example, if we specify as:
    +         $ git notes add -m foo -m bar
    +         $ git notes show HEAD | cat
    +         foo
     
    -        * --separator='------': we will insert "------\n" as the separator,
    -        because user do not provide the line break char at last, we will add
    -        the trailing '\n' compatibly.
    +         bar
     
    -        * --separator='------\n': we will insert as-is because it contains
    -        the line break at last.
    +    The default behavour sometimes is not enough, the user may want
    +    to use a custom delimiter between paragraphs, like when
    +    specifiy one or more '-m' or '-F' options. So this commit
    +    introduces a new '--separator' option for 'git-notes-add' and
    +    'git-notes-append', for example when execute:
     
    -        * not specified --separator option: will use '\n' as the separator
    -        when do appending and this is the default behavour.
    +        $ git notes add -m foo -m bar --separator="-"
    +        $ git notes show HEAD | cat
    +        foo
    +        -
    +        bar
     
    -        * --separator='': we specify an empty separator which has the same
    -        behavour with --separator='\n' and or not specified the option.
    +    We will check the option value and if the value doesn't contail
    +    a trailing '\n', will add it automatically, so execute
     
    -    In addition, if a user specifies multple "-m" with "--separator", the
    -    separator should be inserted between the messages too, so we use
    -    OPT_STRING_LIST instead of OPT_CALLBACK_F to parse "-m" option, make
    -    sure the option value of "--separator" been parsed already when we need
    -    it.
    +          $ git notes add -m foo -m bar --separator="-"
    +    and
    +          $ export LF="
    +          "
    +          $ git notes add -m foo -m bar --separator="-$LF"
    +
    +    have the same behavour.
     
         Signed-off-by: Teng Long <dyroneteng@gmail.com>
     
      ## Documentation/git-notes.txt ##
     @@ Documentation/git-notes.txt: 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] [-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] [-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>
    +@@ Documentation/git-notes.txt: 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
     @@ Documentation/git-notes.txt: 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.
    -+	Creates a new notes object if needed. If the note and the
    -+	message are not empty, "\n" will be inserted between them.
    -+	Use the `--separator` option to insert other delimiters.
    ++	Creates a new notes object if needed. 
    ++	The default delimiter is a blank line, use the `--separator`
    ++	option to insert other delimiters. More specifically, if the
    ++	note and the message are not empty, the delimiter will be
    ++	inserted between them. If you specify multiple `-m` and `-F`
    ++	options, the delimiter will be inserted between the messages
    ++	too.
      
      edit::
      	Edit the notes for a given object (defaults to HEAD).
    @@ Documentation/git-notes.txt: OPTIONS
      	Allow an empty note object to be stored. The default behavior is
      	to automatically remove empty notes.
      
    -+--separator <separator>::
    -+	The '<separator>' inserted between the note and message
    -+	by 'append', "\n" by default. A custom separator can be
    -+	provided, if it doesn't end in a "\n", one will be added
    -+	implicitly .
    ++--separator <paragraph-break>::
    ++	The '<paragraph-break>' inserted between paragraphs.
    ++	A blank line by default.
     +
      --ref <ref>::
      	Manipulate the notes tree in <ref>.  This overrides
    @@ builtin/notes.c
     +static char *separator = NULL;
      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] [-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>"),
     @@ builtin/notes.c: static void write_note_data(struct note_data *d, struct object_id *oid)
      	}
      }
    @@ builtin/notes.c: static void write_note_data(struct note_data *d, struct object_
     +	else
     +		strbuf_insertf(message, pos, "%s%s", separator, "\n");
     +}
    ++
    ++static void parse_messages(struct string_list *messages, struct note_data *d)
    ++{
    ++	size_t i;
    ++	for (i = 0; i < messages->nr; i++) {
    ++		if (d->buf.len)
    ++			insert_separator(&d->buf, d->buf.len);
    ++		strbuf_insertstr(&d->buf, d->buf.len,
    ++				 messages->items[i].string);
    ++		strbuf_stripspace(&d->buf, 0);
    ++		d->given = 1;
    ++	}
    ++}
     +
      static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
      {
    - 	struct note_data *d = opt->value;
    -@@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char *prefix)
    +-	struct note_data *d = opt->value;
    ++	struct string_list *msg = opt->value;
    + 
    + 	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;
    ++	string_list_append(msg, arg);
    + 	return 0;
    + }
    + 
    ++
    + static int parse_file_arg(const struct option *opt, const char *arg, int unset)
    + {
    +-	struct note_data *d = opt->value;
    ++	struct string_list *msg = opt->value;
    ++	struct strbuf buf = STRBUF_INIT;
    + 
    + 	BUG_ON_OPT_NEG(unset);
    + 
    +-	if (d->buf.len)
    +-		strbuf_addch(&d->buf, '\n');
    + 	if (!strcmp(arg, "-")) {
    +-		if (strbuf_read(&d->buf, 0, 1024) < 0)
    ++		if (strbuf_read(&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(&buf, arg, 1024) < 0)
    + 		die_errno(_("could not open or read '%s'"), arg);
    +-	strbuf_stripspace(&d->buf, 0);
    + 
    +-	d->given = 1;
    ++	string_list_append(msg, buf.buf);
    ++	strbuf_release(&buf);
    + 	return 0;
    + }
    + 
    +@@ builtin/notes.c: 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 string_list messages = STRING_LIST_INIT_DUP;
    + 	struct option options[] = {
    +-		OPT_CALLBACK_F('m', "message", &d, N_("message"),
    ++		OPT_CALLBACK_F('m', "message", &messages, N_("message"),
    + 			N_("note contents as a string"), PARSE_OPT_NONEG,
    + 			parse_msg_arg),
    +-		OPT_CALLBACK_F('F', "file", &d, N_("file"),
    ++		OPT_CALLBACK_F('F', "file", &messages, N_("file"),
    + 			N_("note contents in a file"), PARSE_OPT_NONEG,
    + 			parse_file_arg),
    + 		OPT_CALLBACK_F('c', "reedit-message", &d, N_("object"),
    +@@ builtin/notes.c: 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()
    + 	};
    + 
    +@@ builtin/notes.c: static int add(int argc, const char **argv, const char *prefix)
    + 		usage_with_options(git_notes_add_usage, options);
    + 	}
    + 
    ++	parse_messages(&messages, &d);
    + 	object_ref = argc > 1 ? argv[1] : "HEAD";
    + 
    + 	if (get_oid(object_ref, &object))
    +@@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char *prefix)
      	char *logmsg;
      	const char * const *usage;
    -+	size_t message_idx;
      	struct note_data d = { .buf = STRBUF_INIT };
    -+	struct string_list message = STRING_LIST_INIT_DUP;
    ++	struct string_list messages = STRING_LIST_INIT_DUP;
      	struct option options[] = {
     -		OPT_CALLBACK_F('m', "message", &d, N_("message"),
     -			N_("note contents as a string"), PARSE_OPT_NONEG,
    --			parse_msg_arg),
    -+		OPT_STRING_LIST('m', "message", &message, N_("message"),
    -+			N_("note contents as a string")),
    - 		OPT_CALLBACK_F('F', "file", &d, N_("file"),
    ++		OPT_CALLBACK_F('m', "message", &messages, N_("message"),
    ++			N_("note contents as a string"), PARSE_OPT_NONEG, 
    + 			parse_msg_arg),
    +-		OPT_CALLBACK_F('F', "file", &d, N_("file"),
    ++		OPT_CALLBACK_F('F', "file", &messages, N_("file"),
      			N_("note contents in a file"), PARSE_OPT_NONEG,
      			parse_file_arg),
    + 		OPT_CALLBACK_F('c', "reedit-message", &d, N_("object"),
     @@ builtin/notes.c: 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 <separator> as separator before appending a message")),
    ++			N_("insert <paragraph-break> between paragraphs")),
      		OPT_END()
      	};
      	int edit = !strcmp(argv[0], "edit");
    @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char
      		usage_with_options(usage, options);
      	}
      
    -+	for (message_idx = 0; message_idx < message.nr; message_idx++) {
    -+		if (d.buf.len)
    -+			insert_separator(&d.buf, d.buf.len);
    -+		strbuf_insertstr(&d.buf, d.buf.len,
    -+				 message.items[message_idx].string);
    -+		strbuf_stripspace(&d.buf, 0);
    -+		d.given = 1;
    -+	}
    ++	parse_messages(&messages, &d);
     +
      	if (d.given && edit)
      		fprintf(stderr, _("The -m/-F/-c/-C options have been deprecated "
    @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char
      		free(prev_buf);
     
      ## t/t3301-notes.sh ##
    +@@ t/t3301-notes.sh: 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
    + 
    +@@ t/t3301-notes.sh: 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 &&
     @@ t/t3301-notes.sh: test_expect_success 'listing non-existing notes fails' '
      	test_must_be_empty actual
      '
    @@ t/t3301-notes.sh: test_expect_success 'listing non-existing notes fails' '
     +	test_cmp expect actual
     +'
     +
    -+test_expect_success 'append: specify separatoro with line break' '
    ++test_expect_success 'append: specify separator with line break' '
     +	test_when_finished git notes remove HEAD &&
     +	cat >expect <<-\EOF &&
     +	notes-1
    @@ t/t3301-notes.sh: test_expect_success 'listing non-existing notes fails' '
     +	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 &&

From: Teng Long <dyroneteng@gmail.com>

The "struct note_data d = { 0, 0, NULL, STRBUF_INIT };" style could be replaced with designated init format.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 builtin/notes.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

From: Teng Long <dyroneteng@gmail.com>

Let's cleanup the unnecessary 'strbuf_grow' call in 'append_edit'. This "strbuf_grow(&d.buf, size + 1);" is prepared for insert a blank line if needed, but actually when inserting, "strbuf_insertstr(&d.buf, 0, "\n");" will do the "grow" for us.

348f199b (builtin-notes: Refactor handling of -F option to allow combining -m and -F, 2010-02-13) added these to mimic the code introduced by 2347fae5 (builtin-notes: Add "append" subcommand for appending to note objects, 2010-02-13) that reads in previous note before the message. And the resulting code with explicit sizing is carried to this day.

In the context of reading an existing note in, exact sizing may have made sense, but because the resulting note needs cleansing with stripspace() when appending with this option, such an exact sizing does not buy us all that much in practice.

It may help avoiding overallocation due to ALLOC_GROW() slop, but nobody can feed so many long messages for it to matter from the command line.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/notes.c | 2 --
 1 file changed, 2 deletions(-)

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 | cat
     foo

     bar

The default behavour sometimes is not enough, the user may want to use a custom delimiter between paragraphs, like when specifiy one or more '-m' or '-F' options. So this commit introduces a new '--separator' option for 'git-notes-add' and 'git-notes-append', for example when execute:

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

We will check the option value and if the value doesn't contail a trailing '\n', will add it automatically, so execute

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

have the same behavour.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 Documentation/git-notes.txt |  20 ++++++--
 builtin/notes.c             |  72 ++++++++++++++++++--------
 t/t3301-notes.sh            | 100 ++++++++++++++++++++++++++++++++++++
 3 files changed, 166 insertions(+), 26 deletions(-)

Teng Long <dyroneteng@gmail.com> writes:

    Introduce a new '--separator' option for 'git notes add' and
    'git notes append'.

contail?

	A newline is added to the value given to --separator if it
	does not end with one already.

	Running A and B produces the same result.

Concise and readable.  Nice.

Re-read the above three lines, pretending that it is already 6 months and you've forgotten about adding the feature yourself.

Notice something?

A reader with fresh mind would read and think along the description but

 - OK, append command is used to append to the note to an existing
   object;

 - OK, if the object does not have a note yet, we will create one;

 - The default delimiter?  Delimiter for what?  I am puzzled.

at the third step, gets puzzled. The command takes the existing note's contents, adds a delimiter and then appends the new material given by the user, but because that is not clear after reading the first two lines, the sudden appearance of "delimiter" would confuse readers.

Again, the order of presentation is somewhat backwards and that is why we need to say "More specifically" here.

	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.

or something along that line, perhaps?

Here is where you need to say about promoting incomplete line separator more than the proposed log message.

	Specify a string used as a custom inter-paragraph separator
	(a newline is added at the end as needed).  Defaults to a
	blank line.

It looks like you are very fond of "insert", but aren't we always appending with the latest control flow? In other words, is it worth carrying 'pos' around?

This is not a new problem, but if we get three 100-byte messages, we

 - add the first 100-byte message to d->buf and then run
   stripspace() over that 100-byte.

 - add separator and then the second 100-byte message to d->buf, and
   then run stripspace() over that 200-plus-byte.

 - add separator and then the third 100-byte message to d->buf, and
   then run stripspace() over that 300-plus-byte.

Shouldn't we be doing better?

Do we understand what d->given flag represents? My understanding is that it becomes true only when any of the -m/-F/-c/-C options are given to tell the command what message to use, so that we can automatically open the editor to ask for the message when nothing is given.

So, I suspect that

	d->given = !!messages->nr;

at the beginning of the function, or

	d->given = !!d->buf.len;

may be equivalent[*], instead of setting it once every iteration?

	Side note: The latter is slightly more strict, as giving a
	run of empty strings with the default separator would result
	in an empty d->buf and d->given will become false.

This helper is not parsing, but just processing after the whole thing was parsed. "parse_messages" -> "concatenate_messages" or something, perhaps?

Now d->given is set in parse_reuse_arg() and parse_reedit_arg(), because -c/-C is another source of a paragraph. Shouldn't the paragraph taken by these options go in the message list to be concatenated together with other messages, or are -c/-C incompatible with -m/-F and we are OK with two separate and distinct behaviour?

OK, this one now does not concatenate; just accumulates to the "msg" string list.

Ditto.

Yup, this one is "concatenate_messages". We do not read the existing one, we accumulated everything the user gave us in messages, and concatenate them using the separator. The result is stored in d->buf.

I wonder why separator is not a parameter into the helper function?

In short, I think handling of -m/-F got vastly better from the previous rounds in this version, but I am not sure about the only other remaining "strbuf_addch(&d->buf, '\n')" in parse_reuse_arg(). I am inclined to say that that codepath should behave the same way, but I didn't think it as deeply as you did, so...?

Thanks.

Junio C Hamano <gitster@pobox.com>:

Thanks, I will avoid this like 'git-subcommand' expression in the future.

"contain", sorry.

Will apply.

Will s/Running A and B produces the same result/have the same behavour

Will apply.

Will apply.

Actually I can't find a more suitable verb. I think it worth, but I'm not sure whether there is a better way, that is, when we add separator which use to contact messages like multiple '-m' and '-F', the 'pos' is at the end. When we add separator which use to contact the prev_notes and the new notes , the 'pos' is at the head.

After I read the comments of 'strbuf_stripspace', it does :

1. remove empty lines from beginning and end.
2. turn multiple empty lines between paragraphs into just one empty line.
3. if the input has only empty lines and spaces, no output will be produced.
4. if the last line doesn't have a newline at the end, one is added.
5. skip every commentted line if skip_comments is enabled.

I think above the five functions in 'strbuf_stripspace', we may just care about the fourth, the others just like the logic when we edit the commit message when doing 'git commit' or 'git merge', etc. My opinion is a "commit note" and a "commit message" maybe not be the same, and sometimes the user might want a blank line or something at the beginning of the notes. But after I read the tests I think they are just the same. So, let me just fix this by stripspace the message individually, thanks.

I agree, we should choose the latter and move "d->given = !!d->buf.len;" behind the place where we concat the messages.

Yes, my bad. -c/-C should be considered as well and dealing them with the same messages-contacting logic, I will fix these two place in next patch.

In my opinion, your proposal is probably similar to the current approach, but your solution may be a bit more readable. I will consider changing it in the next patch.

You really have a good eye, I didn't noticed them. There are two place remained, the first one is "parse_reuse_arg", second one is "prepare_note_data" it's made for separate the note and the commented lines (note template), I think they should both be replaced/impacted by "--separator".

Thanks.

Teng Long <dyroneteng@gmail.com> writes:

The two callers of this function prepares the string_list, and have this function consume it by concatenating its contents to d->buf. After calling this function, neither of them talks about messages, which means we are leaking the strings kept in the string_list.

I could eject the topic from today's integration run (because the topic is not ready to be merged to 'next' as-is; "-C/-c" codepaths need to be adjusted, at least), but as I took a look already, I'll queue this fix-up on top of the topic for now. Feel free to squash it in (or address the leaks in your own way) when sending in an update.

Thanks.

 builtin/notes.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Junio C Hamano <gitster@pobox.com> writes:

Thanks, will apply in next patch.

I'm working on the new patchset recently, and I found there may be a weired test code writting like this in t3301:

test_expect_success 'refusing to edit notes in refs/remotes/' '
	test_must_fail env MSG=1 GIT_NOTES_REF=refs/heads/bogus git notes edit
'

should it be like:

test_expect_success 'refusing to edit notes in refs/remotes/' '
	test_must_fail env MSG=1 GIT_NOTES_REF=refs/remotes/bogus git notes edit
'

Thanks.

From: Teng Long <dyroneteng@gmail.com>

Diff since v6:

1. apply Junio's code[1] in 3/4  
2. apply Junio's code[2] and text in 3/4
3. adjust -c/-C codepath in 3/4
4. add new test cases in 3/4
5. in v6, I used a string_list struct to temporarily save the message
then concat them with the separator. But, I found it will be a problem
when we use -C/-F to save the content of binary file(break the t3307)
because binary file maybe contains the NUL in the middle, so I use
strbuf struct instead to make sure the whole content is saved to the
notes.
6. 4/4 is a new commit, which fix the problem about the binary file
too. The binary file maybe contains multiple consecutive empty lines,
we shouldn't make them into just one empty line, which may corrupt
the file I think.

[1] https://public-inbox.org/git/xmqqk005v3ex.fsf@gitster.g/ [2] https://public-inbox.org/git/xmqqcz608cpk.fsf@gitster.g/

Thanks.

Teng Long (4):
  notes.c: cleanup 'strbuf_grow' call in 'append_edit'
  notes.c: cleanup for "designated init"
  notes.c: introduce '--separator=<paragraph-break>' option
  notes.c: don't do stripespace when parse file arg

 Documentation/git-notes.txt |  21 ++++--
 builtin/notes.c             | 106 +++++++++++++++++++++-------
 t/t3301-notes.sh            | 137 +++++++++++++++++++++++++++++++++++-
 3 files changed, 232 insertions(+), 32 deletions(-)

Range-diff against v6:
1:  b029ee0b = 1:  ea9c199e notes.c: cleanup 'strbuf_grow' call in 'append_edit'
2:  043db631 = 2:  9dc123c8 notes.c: cleanup for "designated init"
3:  d5a6c747 ! 3:  d1febf86 notes.c: introduce '--separator=<paragraph-break>' option
    @@ Commit message
     
         The default behavour sometimes is not enough, the user may want
         to use a custom delimiter between paragraphs, like when
    -    specifiy one or more '-m' or '-F' options. So this commit
    -    introduces a new '--separator' option for 'git-notes-add' and
    -    'git-notes-append', for example when execute:
    +    specifiy '-m', '-F', '-C', '-c' options. So this commit
    +    introduce a new '--separator' option for 'git notes add' and
    +    'git notes append', for example when execute:
     
             $ git notes add -m foo -m bar --separator="-"
             $ git notes show HEAD | cat
    @@ Commit message
             -
             bar
     
    -    We will check the option value and if the value doesn't contail
    -    a trailing '\n', will add it automatically, so execute
    +    A newline is added to the value given to --separator if it
    +    does not end with one already. So execute:
     
               $ git notes add -m foo -m bar --separator="-"
         and
    @@ Commit message
               "
               $ git notes add -m foo -m bar --separator="-$LF"
     
    -    have the same behavour.
    +    Running A and B produces the same result.
     
         Signed-off-by: Teng Long <dyroneteng@gmail.com>
     
    @@ Documentation/git-notes.txt: add::
     -	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. 
    ++	option to insert other delimiters.
      
      copy::
      	Copy the notes for the first object onto the second object (defaults to
    -@@ Documentation/git-notes.txt: the command can read the input given to the `post-rewrite` hook.)
    +@@ Documentation/git-notes.txt: 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).
    +-	Append to the notes of an existing object (defaults to HEAD).
     -	Creates a new notes object if needed.
    -+	Creates a new notes object if needed. 
    -+	The default delimiter is a blank line, use the `--separator`
    -+	option to insert other delimiters. More specifically, if the
    -+	note and the message are not empty, the delimiter will be
    -+	inserted between them. If you specify multiple `-m` and `-F`
    -+	options, the delimiter will be inserted between the messages
    -+	too.
    ++	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).
    @@ Documentation/git-notes.txt: OPTIONS
      	to automatically remove empty notes.
      
     +--separator <paragraph-break>::
    -+	The '<paragraph-break>' inserted between paragraphs.
    -+	A blank line by default.
    ++	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
    @@ builtin/notes.c
      	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>"),
    +@@ builtin/notes.c: struct note_data {
    + 	int use_editor;
    + 	char *edit_path;
    + 	struct strbuf buf;
    ++	struct strbuf **messages;
    ++	size_t msg_nr;
    ++	size_t msg_alloc;
    + };
    + 
    + static void free_note_data(struct note_data *d)
     @@ builtin/notes.c: static void write_note_data(struct note_data *d, struct object_id *oid)
      	}
      }
    @@ builtin/notes.c: static void write_note_data(struct note_data *d, struct object_
     +		strbuf_insertf(message, pos, "%s%s", separator, "\n");
     +}
     +
    -+static void parse_messages(struct string_list *messages, struct note_data *d)
    ++/* Consume messages and append them into d->buf, then free them */
    ++static void concat_messages(struct note_data *d)
     +{
    ++	struct strbuf msg = STRBUF_INIT;
    ++
     +	size_t i;
    -+	for (i = 0; i < messages->nr; i++) {
    ++	for (i = 0; i < d->msg_nr ; i++) {
     +		if (d->buf.len)
     +			insert_separator(&d->buf, d->buf.len);
    -+		strbuf_insertstr(&d->buf, d->buf.len,
    -+				 messages->items[i].string);
    -+		strbuf_stripspace(&d->buf, 0);
    -+		d->given = 1;
    ++		strbuf_add(&msg, d->messages[i]->buf, d->messages[i]->len);
    ++		strbuf_addbuf(&d->buf, &msg);
    ++		strbuf_reset(&msg);
    ++		strbuf_release(d->messages[i]);
    ++		free(d->messages[i]);
     +	}
    ++	strbuf_release(&msg);
    ++	free(d->messages);
     +}
     +
      static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
      {
    --	struct note_data *d = opt->value;
    -+	struct string_list *msg = opt->value;
    + 	struct note_data *d = opt->value;
    ++	struct strbuf *buf;
      
      	BUG_ON_OPT_NEG(unset);
      
    @@ builtin/notes.c: static void write_note_data(struct note_data *d, struct object_
     -		strbuf_addch(&d->buf, '\n');
     -	strbuf_addstr(&d->buf, arg);
     -	strbuf_stripspace(&d->buf, 0);
    --
    ++	buf = xmalloc(sizeof(*buf));
    ++	strbuf_init(buf, strlen(arg));
    ++	strbuf_addstr(buf, arg);
    ++	strbuf_stripspace(buf, 0);
    + 
     -	d->given = 1;
    -+	string_list_append(msg, arg);
    ++	ALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);
    ++	d->messages[d->msg_nr - 1] = buf;
      	return 0;
      }
      
    -+
      static int parse_file_arg(const struct option *opt, const char *arg, int unset)
      {
    --	struct note_data *d = opt->value;
    -+	struct string_list *msg = opt->value;
    -+	struct strbuf buf = STRBUF_INIT;
    + 	struct note_data *d = opt->value;
    ++	struct strbuf *buf;
      
      	BUG_ON_OPT_NEG(unset);
      
     -	if (d->buf.len)
     -		strbuf_addch(&d->buf, '\n');
    ++	buf = xmalloc(sizeof(*buf));
    ++	strbuf_init(buf, 0);
    ++
      	if (!strcmp(arg, "-")) {
     -		if (strbuf_read(&d->buf, 0, 1024) < 0)
    -+		if (strbuf_read(&buf, 0, 1024) < 0)
    ++		if (strbuf_read(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(&buf, arg, 1024) < 0)
    ++	} else if (strbuf_read_file(buf, arg, 1024) < 0)
      		die_errno(_("could not open or read '%s'"), arg);
     -	strbuf_stripspace(&d->buf, 0);
    ++	strbuf_stripspace(buf, 0);
    + 
    +-	d->given = 1;
    ++	// we will note stripspace the buf here, because the file maybe
    ++	// is a binary and it maybe contains multiple continuous line breaks.
    ++	ALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);
    ++	d->messages[d->msg_nr - 1] = buf;
    + 	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 strbuf *buf;
    + 	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');
    +-
    ++	buf = xmalloc(sizeof(*buf));
    + 	if (get_oid(arg, &object))
    + 		die(_("failed to resolve '%s' as a valid ref."), arg);
    +-	if (!(buf = read_object_file(&object, &type, &len)))
    ++	if (!(buf->buf = read_object_file(&object, &type, &len)))
    + 		die(_("failed to read object '%s'."), arg);
    + 	if (type != OBJ_BLOB) {
    ++		strbuf_release(buf);
    + 		free(buf);
    + 		die(_("cannot read note data from non-blob object '%s'."), arg);
    + 	}
    +-	strbuf_add(&d->buf, buf, len);
    +-	free(buf);
      
     -	d->given = 1;
    -+	string_list_append(msg, buf.buf);
    -+	strbuf_release(&buf);
    ++	// The reuse object maybe is a binary content which could
    ++	// contains '\0' in the middle, so let's set the correct
    ++	// lenth of strbuf to concat all of content.
    ++	buf->len = len;
    ++	ALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);
    ++	d->messages[d->msg_nr - 1] = buf;
      	return 0;
      }
      
    + static int parse_reedit_arg(const struct option *opt, const char *arg, int unset)
    + {
    + 	struct note_data *d = opt->value;
    ++
    + 	BUG_ON_OPT_NEG(unset);
    + 	d->use_editor = 1;
    + 	return parse_reuse_arg(opt, arg, unset);
     @@ builtin/notes.c: static int add(int argc, const char **argv, const char *prefix)
    + 	struct notes_tree *t;
      	struct object_id object, new_note;
      	const struct object_id *note;
    - 	struct note_data d = { .buf = STRBUF_INIT };
    -+	struct string_list messages = STRING_LIST_INIT_DUP;
    +-	struct note_data d = { .buf = STRBUF_INIT };
    ++	struct note_data d = { .buf = STRBUF_INIT, .messages = NULL };
    ++
      	struct option options[] = {
    --		OPT_CALLBACK_F('m', "message", &d, N_("message"),
    -+		OPT_CALLBACK_F('m', "message", &messages, N_("message"),
    + 		OPT_CALLBACK_F('m', "message", &d, N_("message"),
      			N_("note contents as a string"), PARSE_OPT_NONEG,
    - 			parse_msg_arg),
    --		OPT_CALLBACK_F('F', "file", &d, N_("file"),
    -+		OPT_CALLBACK_F('F', "file", &messages, N_("file"),
    - 			N_("note contents in a file"), PARSE_OPT_NONEG,
    - 			parse_file_arg),
    - 		OPT_CALLBACK_F('c', "reedit-message", &d, N_("object"),
     @@ builtin/notes.c: static int add(int argc, const char **argv, const char *prefix)
      		OPT_BOOL(0, "allow-empty", &allow_empty,
      			N_("allow storing empty note")),
    @@ builtin/notes.c: static int add(int argc, const char **argv, const char *prefix)
      		usage_with_options(git_notes_add_usage, options);
      	}
      
    -+	parse_messages(&messages, &d);
    ++	if (d.msg_nr)
    ++		concat_messages(&d);
    ++	d.given = !!d.buf.len;
    ++
      	object_ref = argc > 1 ? argv[1] : "HEAD";
      
      	if (get_oid(object_ref, &object))
     @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char *prefix)
    + 	const struct object_id *note;
      	char *logmsg;
      	const char * const *usage;
    - 	struct note_data d = { .buf = STRBUF_INIT };
    -+	struct string_list messages = STRING_LIST_INIT_DUP;
    +-	struct note_data d = { .buf = STRBUF_INIT };
    ++	struct note_data d = { .buf = STRBUF_INIT, .messages = NULL };
    ++
      	struct option options[] = {
    --		OPT_CALLBACK_F('m', "message", &d, N_("message"),
    --			N_("note contents as a string"), PARSE_OPT_NONEG,
    -+		OPT_CALLBACK_F('m', "message", &messages, N_("message"),
    -+			N_("note contents as a string"), PARSE_OPT_NONEG, 
    - 			parse_msg_arg),
    --		OPT_CALLBACK_F('F', "file", &d, N_("file"),
    -+		OPT_CALLBACK_F('F', "file", &messages, N_("file"),
    - 			N_("note contents in a file"), PARSE_OPT_NONEG,
    - 			parse_file_arg),
    - 		OPT_CALLBACK_F('c', "reedit-message", &d, N_("object"),
    + 		OPT_CALLBACK_F('m', "message", &d, N_("message"),
    + 			N_("note contents as a string"), PARSE_OPT_NONEG,
     @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char *prefix)
      			parse_reuse_arg),
      		OPT_BOOL(0, "allow-empty", &allow_empty,
    @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char
      		usage_with_options(usage, options);
      	}
      
    -+	parse_messages(&messages, &d);
    ++	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 "
    @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char
      		free(prev_buf);
     
      ## t/t3301-notes.sh ##
    +@@
    + 
    + test_description='Test commit notes'
    + 
    ++TEST_PASSES_SANITIZE_LEAK=true
    + . ./test-lib.sh
    + 
    + write_script fake_editor <<\EOF
     @@ t/t3301-notes.sh: test_expect_success 'do not create empty note with -m ""' '
      '
      
    @@ t/t3301-notes.sh: test_expect_success 'create note with combination of -m and -F
     +	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"' '
    @@ t/t3301-notes.sh: test_expect_success 'listing non-existing notes fails' '
      test_expect_success 'append to existing note with "git notes append"' '
      	cat >expect <<-EOF &&
      		Initial set of notes
    +@@ t/t3301-notes.sh: 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) &&
-:  -------- > 4:  b9d12f0c notes.c: don't do stripespace when parse file arg

From: Teng Long <dyroneteng@gmail.com>

Let's cleanup the unnecessary 'strbuf_grow' call in 'append_edit'. This "strbuf_grow(&d.buf, size + 1);" is prepared for insert a blank line if needed, but actually when inserting, "strbuf_insertstr(&d.buf, 0, "\n");" will do the "grow" for us.

348f199b (builtin-notes: Refactor handling of -F option to allow combining -m and -F, 2010-02-13) added these to mimic the code introduced by 2347fae5 (builtin-notes: Add "append" subcommand for appending to note objects, 2010-02-13) that reads in previous note before the message. And the resulting code with explicit sizing is carried to this day.

In the context of reading an existing note in, exact sizing may have made sense, but because the resulting note needs cleansing with stripspace() when appending with this option, such an exact sizing does not buy us all that much in practice.

It may help avoiding overallocation due to ALLOC_GROW() slop, but nobody can feed so many long messages for it to matter from the command line.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/notes.c | 2 --
 1 file changed, 2 deletions(-)

From: Teng Long <dyroneteng@gmail.com>

The "struct note_data d = { 0, 0, NULL, STRBUF_INIT };" style could be replaced with designated init format.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 builtin/notes.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Teng Long <dyroneteng@gmail.com> writes:

I think our codebase calls it "designated initializer".

    notes.c: use designated initializers for clarity
    
    The "struct note_data d = { 0, 0, NULL, STRBUF_INIT };" style could be
    replaced with designated initializer for clarity.
    
    Signed-off-by: Teng Long <dyroneteng@gmail.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

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 | cat
     foo

     bar

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

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

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

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

Running A and B produces the same result.

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

Teng Long <dyroneteng@gmail.com> writes:

"like when specifying", you mean?

"when executing"?

"a newline is ...", because this is not starting a new sentence, but continuing from the "for example, when ..." above.

Here, it is totally unclear what A and B refers to. "both ... and ... produce the same result" or something?

Excellent.

Hmph, OK. I'd think twice before allowing an array of strbufs, though. strbuf is an excellent data structure to allow editing string safely, and an array of strbufs may be very useful when these strings need to be edited simultaneously, but such a use case is very rare and I suspect this would not be it---rather, don't we take one string at a time while processing each option, perhaps running stripspace, and then after that aren't we done with the string until when we finally have these N strings to loop over and concatenate into a single string? It would be sensible to use a strbuf to produce the concatenation, but the strings to be concatenated do not have to come from strbufs. So my intuition, before reading the code but after seeing the data structure, says that "const char **messages" might be more appropriate here. It sends a strong message about the code structure and "phases" of processing, i.e. "we read each piece and store it in the array once we are done preprocessing; then in the next phase we concatenate them".

Do we need "insert" separator? The caller in "concat" certainly shouldn't---all it needs is "append".

As I suspected earlier, with the way d->messages[i] are used, they have no reason to be strbufs---they can and should be a simple string "const char *".

And this one can use an on-stack strbuf (initialized with STRBUF_INIT), and strbuf_detach() the result into d->messages[].

Likewise.

No // comments in our codebase.

Ditto.

Why explicitly initialize .messages to NULL when we are leaving other members to zero initialized implicitly by using designated initializers?

Having to insert two things in front of d.buf feels somewhat wasteful. I wonder if we can easily reorder the logic to first read the previous one, and then append new stuff to it, perhaps?

It wouldn't be a huge deal. If this weren't the only reason why we need to invent insertstr that takes "where", I wouldn't even be mentioning it.

This is a strange commit to add this. If the test and the code involved in the test were leak-sanitizer clean before this commit, then you should have been able to insert this without any other change, and we should do it that way in a separate commit. If we are fixing an existing leak that the sanitizer complains, then that is a different matter. If that is the case, it makes perfect sense to have this here, but the proposed commit log message should explain that it is what is happening.

Other than that, looks very nicely done.

Thanks.

Junio C Hamano <gitster@pobox.com> writes:

will fix, thanks.

I used string in previous patch, but I found there will be a problem when specify a binary file to '-F', that is, a binary file maybe contains NUL in the middle, then if we feed the content to a string, it will be cut off when appending to the 'string_list'. So, it seems that strbuf could solve the issue (will be reproduced as a failure in t3307).

OK, I'm willing to try to do that, "insert at a position" actually because there is a related codepath need to add the separator in the very beginning.

As I said about why I choose strbuf in the above, but I expect your further advice on that.

For the same reason as above I think.

Will fix.

My bad, will fix.

"wasteful" for?

I think it can be done in this way, but in another single commit, perhaps?

This makes me think that there is a need for a method 'insert_separator'? For example, to initialize 'separator' and then use 'strbuf_insertstr' directly? I'm not sure, but it feels like it could work.

Will fix.

Thanks.

Teng Long <dyroneteng@gmail.com> writes:

Thanks.

Just FYI, it seems that it is indeed premature to declare that this one to be leak-free. With this topic merged to 'seen', the leak checker CI job seems to choke on t3301 (in addition, it also barfs on t3307).

https://github.com/git/git/actions/runs/4557669485/jobs/8039614419#step:4:1817

Junio C Hamano <gitster@pobox.com> writes:

Yes, it contains a leak in function "parse_reuse_arg()", will fix.

Thanks.

From: Teng Long <dyroneteng@gmail.com>

The file maybe is a binary and it could contains multiple line breaks, if we do the stripespace on it's content, the notes will be different to the original content.

The binary file as the notes maybe a rare scenario, but there're some related tests about it in t3307, so let's fix the problem and add a test in t3301.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 builtin/notes.c  |  1 -
 t/t3301-notes.sh | 10 +++++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

Teng Long <dyroneteng@gmail.com> writes:

"could contain"?

"stripspace"?

If a file is "binary" then by definition there is no "line" and no "line break" in it, so while I can see what you are trying to say, this needs a bit rephrasing to make it easier to follow for future readers. Perhaps something like...

    The file given to the "-F" option could be binary, in which case
    we want to store it verbatim without corrupting its contents.

But I do not necessarily agree with the logic here, regardless of how it is phrased. Existing users are used to seeing the contents fed from "-F <file>" get cleaned up, and to them, this unconditional removal of stripspace() will be a regression. Besides, don't they expect that

	-m="$(cat file)"

be equivalent to

	-F file

for their text file?

A --binary-file=<file> option that is incompatible with -m/-F and "append" action may make sense if we really want to have a feature to help those who want to attach binary contents as a note to objects.

Junio C Hamano <gitster@pobox.com> writes:

Will fix.

Will fix, also in the commit subject.

Thanks, the phrasing is obviously better.

Yes, for text file. I agree and I actually notice it will be a regression, because I didn't figure out when user might store a binary file's content in notes, so I'd like to discuss about this issue.

Actually, -m/-F/-c/-C in 'git notes add' is quite like the options in 'git commit', rather than "--binary-file=", I think maybe '--[no-]stripspace' is a bit better. Defaults to '--stripspace' which keeps old logic (stripspace the string fed by -m/-F). If specifying '--no-stripspace', we do not stripespace.

Thanks.

Teng Long <dyroneteng@gmail.com> writes:

It is a fairly easy way to both implement and explain. The approach is certainly attractive from the "expectation management" point of view.

With "git notes add --no-stripspace -F file0 -F file1 ...", we may still be making the resulting note unusable as a binary file due to splicing the inter-paragraph separator, but the option name does not hint that we are giving this option to users to help them use binary contents at all, to prevent them from complaining. If it is truly useful is a separate matter, but I somehow suspect that there will be a lot more users who want "no stripspace on text contents" than those who want "binary contents in note", so it probably is a good way to go after all. I like that.

Before proceeding further on this front, we'd probably need a bit more tests that are specifically designed to catch behaviour change before and after this series with respect to stripspace behaviour. The current code without these patches calls stripspace() on the whole thing every time it receives and appends an additional piece of contents. The parse_msg_arg() function, for example, handles "-m <possibly multiline message>" by first appending a blank line (if we already have some contents) and then appending the new message, and runs stripspace on the whole thing. "-F <file>" is handled by parse_file_arg() the same way, i.e. append the file data and then stripspace the result. This can of course be optimized somewhat (e.g. an early part of the buffer may have already been cleansed by stripspace before appending new data, so as long as we do not introduce double-blanks and other things that are to be corrected at the boundary of appending new data, we shouldn't have to run stripspace on the earlier part in order to behave "as if we did a stripspace the whole thing over and over again"), but we should make sure we do not change the behaviour to cause regression. After doing something silly like this:

    cat >file1 <<-\EOF &&

    With a leading blank line, this is the second line of the file.
    And with a trailing blank line, this is the second from the last.

    EOF
    git notes add -m '
    Message from the command line.

    Two blank lines above.' -F file1    

we should see the leading blank line before "Message from the command line." removed, two blank lines from the "-m" option squeezed into one, only one blank line to appear before the contents taken from file1, and the trailing blank line removed.

Thanks.

From: Teng Long <dyroneteng@gmail.com>

Diff since v7:

1. Fix problems that Junio's replies in last patch, except:

   * array of strbufs -> string_list (in [1]): the message may from
   a binary file which may contain '\0' in middle, string will be
   cut off then make a unexpected result.

2. New commit [3/6] for adding test cases about stripspace behavior.
3. New commit [6/6] for supporting "--[no-]stripspace".

[1] https://public-inbox.org/git/xmqq5yakhoo9.fsf@gitster.g/

Thanks.

Teng Long (6):
  notes.c: cleanup 'strbuf_grow' call in 'append_edit'
  notes.c: use designated initializers for clarity
  t3321: add test cases about the notes stripspace behavior
  notes.c: introduce '--separator=<paragraph-break>' option
  notes.c: append separator instead of insert by pos
  notes.c: introduce "--[no-]stripspace" option

 Documentation/git-notes.txt |  42 ++-
 builtin/notes.c             | 144 +++++++---
 t/t3301-notes.sh            | 126 +++++++++
 t/t3321-notes-stripspace.sh | 521 ++++++++++++++++++++++++++++++++++++
 4 files changed, 791 insertions(+), 42 deletions(-)
 create mode 100755 t/t3321-notes-stripspace.sh

Range-diff against v7:
1:  ea9c199e ! 1:  0634434e notes.c: cleanup 'strbuf_grow' call in 'append_edit'
    @@ builtin/notes.c: static int parse_msg_arg(const struct option *opt, const char *
      		strbuf_addch(&d->buf, '\n');
      	strbuf_addstr(&d->buf, arg);
     @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char *prefix)
    - 		enum object_type type;
    - 		char *prev_buf = read_object_file(note, &type, &size);
    + 		char *prev_buf = repo_read_object_file(the_repository, note,
    + 						       &type, &size);
      
     -		strbuf_grow(&d.buf, size + 1);
      		if (d.buf.len && prev_buf && size)
2:  9dc123c8 ! 2:  4ad78405 notes.c: cleanup for "designated init"
    @@ Metadata
     Author: Teng Long <dyroneteng@gmail.com>
     
      ## Commit message ##
    -    notes.c: cleanup for "designated init"
    +    notes.c: use designated initializers for clarity
     
         The "struct note_data d = { 0, 0, NULL, STRBUF_INIT };" style could be
    -    replaced with designated init format.
    +    replaced with designated initializer for clarity.
     
         Signed-off-by: Teng Long <dyroneteng@gmail.com>
    +    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## builtin/notes.c ##
     @@ builtin/notes.c: static int add(int argc, const char **argv, const char *prefix)
-:  -------- > 3:  6dfb5bf2 t3321: add test cases about the notes stripspace behavior
3:  d1febf86 ! 4:  be86f9ca notes.c: introduce '--separator=<paragraph-break>' option
    @@ Commit message
     
         The default behavour sometimes is not enough, the user may want
         to use a custom delimiter between paragraphs, like when
    -    specifiy '-m', '-F', '-C', '-c' options. So this commit
    +    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 execute:
    +    'git notes append', for example when executing:
     
             $ git notes add -m foo -m bar --separator="-"
             $ git notes show HEAD | cat
    @@ Commit message
             -
             bar
     
    -    A newline is added to the value given to --separator if it
    -    does not end with one already. So execute:
    +    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
    @@ Commit message
               "
               $ git notes add -m foo -m bar --separator="-$LF"
     
    -    Running A and B produces the same result.
    +    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: OPTIONS
     
      ## builtin/notes.c ##
     @@
    - #include "notes-utils.h"
    +  */
    + 
    + #include "cache.h"
    ++#include "alloc.h"
    + #include "config.h"
    + #include "builtin.h"
    + #include "gettext.h"
    +@@
      #include "worktree.h"
    + #include "write-or-die.h"
      
     +static char *separator = NULL;
      static const char * const git_notes_usage[] = {
    @@ builtin/notes.c
      	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>"),
    -@@ builtin/notes.c: struct note_data {
    +@@ builtin/notes.c: 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 strbuf **messages;
    ++	struct note_msg **messages;
     +	size_t msg_nr;
     +	size_t msg_alloc;
      };
      
      static void free_note_data(struct note_data *d)
    +@@ builtin/notes.c: static void free_note_data(struct note_data *d)
    + 		free(d->edit_path);
    + 	}
    + 	strbuf_release(&d->buf);
    ++
    ++	while (d->msg_nr) {
    ++		--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,
     @@ builtin/notes.c: static void write_note_data(struct note_data *d, struct object_id *oid)
      	}
      }
    @@ builtin/notes.c: static void write_note_data(struct note_data *d, struct object_
     +		strbuf_insertf(message, pos, "%s%s", separator, "\n");
     +}
     +
    -+/* Consume messages and append them into d->buf, then free them */
     +static void concat_messages(struct note_data *d)
     +{
     +	struct strbuf msg = STRBUF_INIT;
    @@ builtin/notes.c: static void write_note_data(struct note_data *d, struct object_
     +	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, d->messages[i]->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(d->messages[i]);
    -+		free(d->messages[i]);
     +	}
     +	strbuf_release(&msg);
    -+	free(d->messages);
     +}
     +
      static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
      {
      	struct note_data *d = opt->value;
    -+	struct strbuf *buf;
    ++	struct note_msg *msg = xmalloc(sizeof(*msg));
      
      	BUG_ON_OPT_NEG(unset);
      
    @@ builtin/notes.c: static void write_note_data(struct note_data *d, struct object_
     -		strbuf_addch(&d->buf, '\n');
     -	strbuf_addstr(&d->buf, arg);
     -	strbuf_stripspace(&d->buf, 0);
    -+	buf = xmalloc(sizeof(*buf));
    -+	strbuf_init(buf, strlen(arg));
    -+	strbuf_addstr(buf, arg);
    -+	strbuf_stripspace(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] = buf;
    ++	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 strbuf *buf;
    ++	struct note_msg *msg = xmalloc(sizeof(*msg));
      
      	BUG_ON_OPT_NEG(unset);
      
     -	if (d->buf.len)
     -		strbuf_addch(&d->buf, '\n');
    -+	buf = xmalloc(sizeof(*buf));
    -+	strbuf_init(buf, 0);
    -+
    ++	strbuf_init(&msg->buf , 0);
      	if (!strcmp(arg, "-")) {
     -		if (strbuf_read(&d->buf, 0, 1024) < 0)
    -+		if (strbuf_read(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(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);
    -+	strbuf_stripspace(buf, 0);
      
     -	d->given = 1;
    -+	// we will note stripspace the buf here, because the file maybe
    -+	// is a binary and it maybe contains multiple continuous line breaks.
     +	ALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);
    -+	d->messages[d->msg_nr - 1] = buf;
    ++	d->messages[d->msg_nr - 1] = msg;
    ++	msg->stripspace = 1;
      	return 0;
      }
      
    @@ builtin/notes.c: static void write_note_data(struct note_data *d, struct object_
      {
      	struct note_data *d = opt->value;
     -	char *buf;
    -+	struct strbuf *buf;
    ++	struct note_msg *msg = xmalloc(sizeof(*msg));
    ++	char *value;
      	struct object_id object;
      	enum object_type type;
      	unsigned long len;
    @@ builtin/notes.c: static void write_note_data(struct note_data *d, struct object_
     -	if (d->buf.len)
     -		strbuf_addch(&d->buf, '\n');
     -
    -+	buf = xmalloc(sizeof(*buf));
    - 	if (get_oid(arg, &object))
    ++	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 = read_object_file(&object, &type, &len)))
    -+	if (!(buf->buf = read_object_file(&object, &type, &len)))
    +-	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) {
    -+		strbuf_release(buf);
    - 		free(buf);
    +-		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;
    -+	// The reuse object maybe is a binary content which could
    -+	// contains '\0' in the middle, so let's set the correct
    -+	// lenth of strbuf to concat all of content.
    -+	buf->len = len;
    ++	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] = buf;
    ++	d->messages[d->msg_nr - 1] = msg;
    ++	msg->stripspace = 0;
      	return 0;
      }
      
    - static int parse_reedit_arg(const struct option *opt, const char *arg, int unset)
    - {
    - 	struct note_data *d = opt->value;
    -+
    - 	BUG_ON_OPT_NEG(unset);
    - 	d->use_editor = 1;
    - 	return parse_reuse_arg(opt, arg, unset);
     @@ builtin/notes.c: static int add(int argc, const char **argv, const char *prefix)
    - 	struct notes_tree *t;
      	struct object_id object, new_note;
      	const struct object_id *note;
    --	struct note_data d = { .buf = STRBUF_INIT };
    -+	struct note_data d = { .buf = STRBUF_INIT, .messages = NULL };
    + 	struct note_data d = { .buf = STRBUF_INIT };
     +
      	struct option options[] = {
      		OPT_CALLBACK_F('m', "message", &d, N_("message"),
    @@ builtin/notes.c: static int add(int argc, const char **argv, const char *prefix)
     +
      	object_ref = argc > 1 ? argv[1] : "HEAD";
      
    - 	if (get_oid(object_ref, &object))
    -@@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char *prefix)
    - 	const struct object_id *note;
    - 	char *logmsg;
    - 	const char * const *usage;
    --	struct note_data d = { .buf = STRBUF_INIT };
    -+	struct note_data d = { .buf = STRBUF_INIT, .messages = NULL };
    -+
    - 	struct option options[] = {
    - 		OPT_CALLBACK_F('m', "message", &d, N_("message"),
    - 			N_("note contents as a string"), PARSE_OPT_NONEG,
    + 	if (repo_get_oid(the_repository, object_ref, &object))
     @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char *prefix)
      			parse_reuse_arg),
      		OPT_BOOL(0, "allow-empty", &allow_empty,
    @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char
      		fprintf(stderr, _("The -m/-F/-c/-C options have been deprecated "
      			"for the 'edit' subcommand.\n"
     @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char *prefix)
    - 		char *prev_buf = read_object_file(note, &type, &size);
    + 						       &type, &size);
      
      		if (d.buf.len && prev_buf && size)
     -			strbuf_insertstr(&d.buf, 0, "\n");
    @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char
      		free(prev_buf);
     
      ## t/t3301-notes.sh ##
    -@@
    - 
    - test_description='Test commit notes'
    - 
    -+TEST_PASSES_SANITIZE_LEAK=true
    - . ./test-lib.sh
    - 
    - write_script fake_editor <<\EOF
     @@ t/t3301-notes.sh: test_expect_success 'do not create empty note with -m ""' '
      '
      
4:  b9d12f0c < -:  -------- notes.c: don't do stripespace when parse file arg
-:  -------- > 5:  ef40e0ef notes.c: append separator instead of insert by pos
-:  -------- > 6:  f60f7432 notes.c: introduce "--[no-]stripspace" option

From: Teng Long <dyroneteng@gmail.com>

Let's cleanup the unnecessary 'strbuf_grow' call in 'append_edit'. This "strbuf_grow(&d.buf, size + 1);" is prepared for insert a blank line if needed, but actually when inserting, "strbuf_insertstr(&d.buf, 0, "\n");" will do the "grow" for us.

348f199b (builtin-notes: Refactor handling of -F option to allow combining -m and -F, 2010-02-13) added these to mimic the code introduced by 2347fae5 (builtin-notes: Add "append" subcommand for appending to note objects, 2010-02-13) that reads in previous note before the message. And the resulting code with explicit sizing is carried to this day.

In the context of reading an existing note in, exact sizing may have made sense, but because the resulting note needs cleansing with stripspace() when appending with this option, such an exact sizing does not buy us all that much in practice.

It may help avoiding overallocation due to ALLOC_GROW() slop, but nobody can feed so many long messages for it to matter from the command line.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/notes.c | 2 --
 1 file changed, 2 deletions(-)

From: Teng Long <dyroneteng@gmail.com>

The "struct note_data d = { 0, 0, NULL, STRBUF_INIT };" style could be replaced with designated initializer for clarity.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/notes.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

From: Teng Long <dyroneteng@gmail.com>

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 t/t3321-notes-stripspace.sh | 291 ++++++++++++++++++++++++++++++++++++
 1 file changed, 291 insertions(+)
 create mode 100755 t/t3321-notes-stripspace.sh

Teng Long <dyroneteng@gmail.com> writes:

Looks quite thorough.

It's 2023 now.

Inconsistent indentation confuses readers if the author meant something unexplained by the difference between the two. Stick to one style (I personally prefer the "indent to the same level as the starting 'cat' and ending 'EOF'" but it is OK to pick the other one, as long as it is consistent within a single test script).

This is a bit misleading, as there are TWO blank lines before the "first-line" (one from the here text itself, the other is from ${LF}).

I do not think it matters too much, because the point of stripspace is to remove any number of leading or trailing blank lines, and squash one or more blank lines elsewhere into one, so having two blank lines at the beginning or at the end of the file is just as good an example as having a single blank line. I am mentioning it primarily because I had to spend some time thinking about ways to make it less misleading.

Junio C Hamano <gitster@pobox.com> writes:

I'm not sure if I've covered everything, but I've tried to addi some basic test cases to give us a chance to add on top of that later.

Will fix.

I will follow as your prefer format, thanks.

Yes, a little bit, I don't found a more clealy so far. It works for now but I'm willing to optimize if there's a better way.

Thanks.

From: Teng Long <dyroneteng@gmail.com>

This commit rename "insert_separator" to "append_separator" and also remove the "postion" argument, this serves two purpose:

The first is that when specifying more than one "-m" ( like "-F", etc) to "git notes add" or "git notes append", the order of them matters, which means we need to append the each separator and message in turn, so we don't have to make the caller specify the position, the "append" operation is enough and clear.

The second is that when we execute the "git notes append" subcommand , we need to combine the "prev_note" and "current_note" to get the final result. Before, we inserted a newline character at the beginning of "current_note". Now, we will append a newline to the end of "prev_note" instead, this will give the consisitent results.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 builtin/notes.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

Teng Long <dyroneteng@gmail.com> writes:

"This commit rename" -> "Rename".

OK.

Funny placement of comma?

Is it still needed to use strbuf_insert*() variants when the only thing being done is to append at the end? Use of strbuf_add*() would let you drop message->len from the arguments.

Junio C Hamano <gitster@pobox.com> writes:

Will fix.

Thanks.

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 | cat
     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 | cat
    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             | 111 ++++++++++++++++++++++++-------
 t/t3301-notes.sh            | 126 ++++++++++++++++++++++++++++++++++++
 3 files changed, 229 insertions(+), 29 deletions(-)

Teng Long <dyroneteng@gmail.com> writes:

        while (d->msg_nr--) {
                strbuf_relesae(...);
                free(d->messages[d->msg_nr]);
        }

If you massage an unspecified separator beforehand into "\n" (e.g. initialize separator to "\n" instead of NULL), you can lose one of these three at runtime.

Interesting.  

I would have expected that where we add to d->messages[] array a new note_msg structure and set its .stripspace member, we already knew if the associated strbuf needs to be stripspace'd and instead of maintaining the .stripspace member for each note_msg, we can just have it contain only a string (not even a strbuf).

The above loop, and the design to have .stripspace per each note_msg, smell iffy to me for one thing. The .stripspace member is used to flag "after adding this element, the whole thing need to be treated with stripspace". Is it intended? What it means is that if you have two elements in d->messages[] array, one of them with and the other one without the .stripspace bit set, depending on the order of the elements, the result would be vastly different. When the later element has the stripspace bit set, everything gets cleansed, but when the earlier element has it, only the earlier element gets cleansed and the later element is added with multiple blank lines and trailing whitespaces intact. It does not sound quite right.

I wonder if the semantics intended (not implemented) by this series is to concatenate if none of the elements want stripspace and cleanse the whitespaces if any of them want stripspace, in which case an obvious implementation would be to just concatenate everything in the loop with separators, while seeing if any of them have the stripspace bit set, and then after the loop finishes, run stripspace on the whole thing just once if any of d->messages[] asked for it? If that is the case, an even better design could be to move the stripspace bit out of d->messages[] and make it d->stripspace to apply to the whole thing.

Another possiblity is to just stripspace the elements taken from the source you want to stripspace (like "-m" and "-F" but not the ones taken from parse_reuse_arg()) before you even add them to d->messages[] array. That way, the only possible source of multiple blank lines and trailing whitespaces in the concatenation of elements you want stripspace would be from the separator, which is under control by the end user. So your concatenation could just ignore running stripspace at all---if the user does (or does not) want multiple blank lines or trailing whitespaces on the separator line, that's under their control. That sounds like the simplest and cleanest design---we strip as we read from each source and make them into simple strings to be kept in d->messages[] without having to allocate and keep a strbuf per each source, and we concatenate just once, without worrying about stripspace.

The simple approach may break when any of the elements ended up to be truly empty, but then we can solve that by refraining to push an empty result into d->messages[] array, or something? I dunno.

Junio C Hamano <gitster@pobox.com> writes:

Will fix or optimize.

That's because the "-C" option, it will not do stripspace for the whole note, but support to use together with "-m" or "-F"(they will do stripspace). So, for the consistent result with backforward compatibility, we have to get the information about whether to do stripspace when concating each message.

In the previous patch, I actually followed your suggestion to only stripspace message itself but not the whole concatenate message. But actually, there's a problem with this, in fact it's also caused by "-C", "-C" doesn't stripspace, means that when mixed with "-m" and "-F", the order of the options matters.

I'm not sure if this is a design flaw or if it's intentional, but it's special about the stripspace handling of "-C" right now, which is why I've added two new test cases in [3/6], just to make sure my subsequent patches don't break them:

    * 'add notes with "-C" and "-m", "-m" will stripspace all together'

    * 'add notes with "-m" and "-C", "-C" will not stripspace all together'

I agree, but this seems to break the effect of "the order matters" because of the "-C" argument.

If we think that the "-C" particularity is a case that should be optimized or fixed(), then I strongly agree with this modification approach, and I think that compared to this particularity, on the one hand, our implementation is simple, on the other hand, users may find it easier to understand.

I think this approach is feasible, and add judgment logic when supporting subsequent --[no-] stripspaces.

The strbuf can record the "len" of the buf, to make sure the string will be cut of if the content contains '\0' in the middle, like a binary file. Maybe using a string could make that, but I‘m not sure how to solve it.

I will add some test cases about it and found it out.

Thanks.

Teng Long <dyroneteng@gmail.com> writes:

Not a huge thing and I would be very surprised if you saw a different machine code output. The original stood out like a sore thumb at me, primarily because we rarely if ever do pre-decrement or pre-increment in the void context in our own code.

OK.

Thanks.

Teng Long <dyroneteng@gmail.com> writes:

That's a pointless piping to "cat". Piping to "cat -n" might be a good way to illustrate what happens, though, i.e.

    $ git notes show HEAD | cat -n
         1  foo
         2
         3  bar

I have no strong preference either way (the other more obvious alternative is to just remove "| cat").

From: Teng Long <dyroneteng@gmail.com>

This commit introduces a new option "--[no-]stripspace" to git notes append, git notes edit, and git notes add. This option allows users to control whether the note message need to stripped out.

For the consideration of backward compatibility, let's look at the behavior about "stripspace" in "git notes" command:

1. "Edit Message" case: using the default editor to edit the note
message.

    In "edit" case, the edited message will always be stripped out, the
    implementation which can be found in the "prepare_note_data()". In
    addition, the "-c" option supports to reuse an existing blob as a
    note message, then open the editor to make a further edition on it,
    the edited message will be stripped.

    This commit doesn't change the default behavior of "edit" case by
    using an enum "notes_stripspace", only when "--no-stripspace" option
    is specified, the note message will not be stripped out. If you do
    not specify the option or you specify "--stripspace", clearly, the
    note message will be stripped out.

2. "Assign Message" case: using the "-m"/"-F"/"-C" option to specify the
note message.

    In "assign" case, when specify message by "-m" or "-F", the message
    will be stripped out by default, but when specify message by "-C",
    the message will be copied verbatim, in other word, the message will
    not be stripped out. One more thing need to note is "the order of
    the options matter", that is, if you specify "-C" before "-m" or
    "-F", the reused message by "-C" will be stripped out together,
    because everytime concat "-m" or "-F" message, the concated message
    will be stripped together. Oppositely, if you specify "-m" or "-F"
    before "-C", the reused message by "-C" will not be stripped out.

    This commit doesn't change the default behavior of "assign" case by
    extending the "stripspace" field in "struct note_msg", so we can
    distinguish the different behavior of "-m"/"-F" and "-C" options
    when we need to parse and concat the message.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 Documentation/git-notes.txt |  25 ++-
 builtin/notes.c             |  30 +++-
 t/t3321-notes-stripspace.sh | 296 ++++++++++++++++++++++++++++++++----
 3 files changed, 304 insertions(+), 47 deletions(-)

Teng Long <dyroneteng@gmail.com> writes:

Makes sense.

This sounds more like a design/implementation mistake that we may want to fix.

I doubted this either, but for compatibility, implementations of this patchset have been made, such as "note_msg" recording the "stripspace" field, to implement these two new options without breaking the old behavior.

Thanks.

Teng Long <dyroneteng@gmail.com> writes:

Thanks, that is a sensible stance to take.