git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v7 3/4] notes.c: introduce '--separator=<paragraph-break>' option

From
Junio C Hamano <gitster@pobox.com>
Date
Mar 28, 2023, 15:37 UTC
Message-ID
<xmqq5yakhoo9.fsf@gitster.g>
In-Reply-To
<d1febf86d8471c24c9a045d6962fd86f63f414d5.1680012650.git.dyroneteng@gmail.com>
Teng Long <dyroneteng@gmail.com> writes:
Show 14 quoted lines
> 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
"like when specifying", you mean?
> introduce a new '--separator' option for 'git notes add' and
> 'git notes append', for example when execute:
"when executing"?
Show 7 quoted lines
>     $ 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

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

Show 9 quoted lines
> 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.

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

Show 12 quoted lines
> @@ -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.
Excellent.
Show 8 quoted lines
> @@ -101,6 +102,9 @@ struct note_data {
>  	int use_editor;
>  	char *edit_path;
>  	struct strbuf buf;
> +	struct strbuf **messages;
> +	size_t msg_nr;
> +	size_t msg_alloc;
>  };

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

Show 13 quoted lines
> @@ -209,71 +213,110 @@ 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)
> +		strbuf_insertstr(message, pos, "\n");
> +	else if (separator[strlen(separator) - 1] == '\n')
> +		strbuf_insertstr(message, pos, separator);
> +	else
> +		strbuf_insertf(message, pos, "%s%s", separator, "\n");
> +}

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

Show 18 quoted lines
> +/* 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 < 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_addbuf(&d->buf, &msg);
> +		strbuf_reset(&msg);
> +		strbuf_release(d->messages[i]);
> +		free(d->messages[i]);
> +	}
> +	strbuf_release(&msg);
> +	free(d->messages);
> +}

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

Show 21 quoted lines
>  static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
>  {
>  	struct note_data *d = opt->value;
> +	struct strbuf *buf;
>  
>  	BUG_ON_OPT_NEG(unset);
>  
> -	if (d->buf.len)
> -		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;
> +	ALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);
> +	d->messages[d->msg_nr - 1] = buf;
>  	return 0;
>  }

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

>  static int parse_file_arg(const struct option *opt, const char *arg, int unset)
>  {
>  	struct note_data *d = opt->value;
> +	struct strbuf *buf;
Likewise.
Show 21 quoted lines
>  
>  	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)
>  			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);
> +	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.
No // comments in our codebase.
> +	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)
>  {
> ...
>  }
Ditto.
Show 6 quoted lines
> @@ -567,7 +617,8 @@ 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 };

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

Show 9 quoted lines
> @@ -618,7 +675,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>  		char *prev_buf = read_object_file(note, &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);

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.

Show 10 quoted lines
> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> index 3288aaec..716192b5 100755
> --- a/t/t3301-notes.sh
> +++ b/t/t3301-notes.sh
> @@ -5,6 +5,7 @@
>  
>  test_description='Test commit notes'
>  
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh

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.
Previous: Teng LongNext: Teng Long
Message 15 of 40 in “notes.c: introduce "--separator" option”
  1. 0/3 notes.c: introduce "--separator" optionTeng Long, Feb 23, 2023
  2. 2/3 notes.c: cleanup for "designated init"Teng Long, Feb 23, 2023
  3. 1/3 notes.c: cleanup 'strbuf_grow' call in 'append_edit'Teng Long, Feb 23, 2023
  4. 3/3 notes.c: introduce '--separator=<paragraph-break>' optionTeng Long, Feb 23, 2023
  5. Junio C HamanoFeb 23, 2023
  6. Teng LongFeb 28, 2023
  7. Junio C HamanoFeb 25, 2023
  8. Teng LongFeb 28, 2023
  9. 0/3 notes.c: introduce "--separator" optionTeng Long, Mar 27, 2023
  10. 0/4 notes.c: introduce "--separator" optionTeng Long, Mar 28, 2023
  11. 1/4 notes.c: cleanup 'strbuf_grow' call in 'append_edit'Teng Long, Mar 28, 2023
  12. 2/4 notes.c: cleanup for "designated init"Teng Long, Mar 28, 2023
  13. Junio C HamanoMar 29, 2023
  14. 3/4 notes.c: introduce '--separator=<paragraph-break>' optionTeng Long, Mar 28, 2023
  15. Junio C HamanoMar 28, 2023
  16. Teng LongMar 29, 2023
  17. Junio C HamanoMar 29, 2023
  18. Teng LongApr 13, 2023
  19. 4/4 notes.c: don't do stripespace when parse file argTeng Long, Mar 28, 2023
  20. Junio C HamanoMar 28, 2023
  21. Teng LongMar 29, 2023
  22. Junio C HamanoMar 29, 2023
  23. 0/6 notes.c: introduce "--separator" optionTeng Long, Apr 25, 2023
  24. 1/6 notes.c: cleanup 'strbuf_grow' call in 'append_edit'Teng Long, Apr 25, 2023
  25. 2/6 notes.c: use designated initializers for clarityTeng Long, Apr 25, 2023
  26. 3/6 t3321: add test cases about the notes stripspace behaviorTeng Long, Apr 25, 2023
  27. Junio C HamanoApr 25, 2023
  28. Teng LongApr 27, 2023
  29. 5/6 notes.c: append separator instead of insert by posTeng Long, Apr 25, 2023
  30. Junio C HamanoApr 25, 2023
  31. Teng LongApr 27, 2023
  32. 4/6 notes.c: introduce '--separator=<paragraph-break>' optionTeng Long, Apr 25, 2023
  33. Junio C HamanoApr 25, 2023
  34. Teng LongApr 27, 2023
  35. Junio C HamanoApr 27, 2023
  36. Junio C HamanoApr 25, 2023
  37. 6/6 notes.c: introduce "--[no-]stripspace" optionTeng Long, Apr 25, 2023
  38. Junio C HamanoApr 25, 2023
  39. Teng LongApr 28, 2023
  40. Junio C HamanoApr 28, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.