[v8,5/6] notes.c: append separator instead of insert by pos

Commit Message

Teng Long April 25, 2023, 1:34 p.m. UTC

From: Teng Long <[e-mail]>

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 <[e-mail]>
---
 builtin/notes.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

Comments

Junio C Hamano April 25, 2023, 5:47 p.m. UTC | #1

Teng Long <[e-mail]> writes:

> From: Teng Long <[e-mail]>
>
> This commit rename "insert_separator" to "append_separator" and also

"This commit rename" -> "Rename".

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

OK.

> 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

Funny placement of comma?

> +static void append_separator(struct strbuf *message)
>  {
>  	if (!separator)
> -		strbuf_insertstr(message, pos, "\n");
> +		strbuf_insertstr(message, message->len, "\n");
>  	else if (separator[strlen(separator) - 1] == '\n')
> -		strbuf_insertstr(message, pos, separator);
> +		strbuf_insertstr(message, message->len, separator);
>  	else
> -		strbuf_insertf(message, pos, "%s%s", separator, "\n");
> +		strbuf_insertf(message, message->len, "%s%s", separator, "\n");
>  }

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.

Teng Long April 27, 2023, 7:51 a.m. UTC | #2

Junio C Hamano <[e-mail]> writes:

>> From: Teng Long <[e-mail]>
>>
>> This commit rename "insert_separator" to "append_separator" and also
>
>"This commit rename" -> "Rename".
>
>...
>
>> 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
>
>Funny placement of comma?
>
>> +static void append_separator(struct strbuf *message)
>>  {
>>  	if (!separator)
>> -		strbuf_insertstr(message, pos, "\n");
>> +		strbuf_insertstr(message, message->len, "\n");
>>  	else if (separator[strlen(separator) - 1] == '\n')
>> -		strbuf_insertstr(message, pos, separator);
>> +		strbuf_insertstr(message, message->len, separator);
>>  	else
>> -		strbuf_insertf(message, pos, "%s%s", separator, "\n");
>> +		strbuf_insertf(message, message->len, "%s%s", separator, "\n");
>>  }
>
>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.

Will fix.

Thanks.

Patch

diff --git a/builtin/notes.c b/builtin/notes.c
index 6ae8b57b..84bc09ed 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -230,14 +230,14 @@  static void write_note_data(struct note_data *d, struct object_id *oid)
 	}
 }
 
-static void insert_separator(struct strbuf *message, size_t pos)
+static void append_separator(struct strbuf *message)
 {
 	if (!separator)
-		strbuf_insertstr(message, pos, "\n");
+		strbuf_insertstr(message, message->len, "\n");
 	else if (separator[strlen(separator) - 1] == '\n')
-		strbuf_insertstr(message, pos, separator);
+		strbuf_insertstr(message, message->len, separator);
 	else
-		strbuf_insertf(message, pos, "%s%s", separator, "\n");
+		strbuf_insertf(message, message->len, "%s%s", separator, "\n");
 }
 
 static void concat_messages(struct note_data *d)
@@ -247,7 +247,7 @@  static void concat_messages(struct note_data *d)
 	size_t i;
 	for (i = 0; i < d->msg_nr ; i++) {
 		if (d->buf.len)
-			insert_separator(&d->buf, d->buf.len);
+			append_separator(&d->buf);
 		strbuf_add(&msg, d->messages[i]->buf.buf, d->messages[i]->buf.len);
 		strbuf_addbuf(&d->buf, &msg);
 		if (d->messages[i]->stripspace)
@@ -682,14 +682,17 @@  static int append_edit(int argc, const char **argv, const char *prefix)
 		/* Append buf to previous note contents */
 		unsigned long size;
 		enum object_type type;
-		char *prev_buf = repo_read_object_file(the_repository, note,
-						       &type, &size);
+		struct strbuf buf = STRBUF_INIT;
+		char *prev_buf = repo_read_object_file(the_repository, note, &type, &size);
 
-		if (d.buf.len && prev_buf && size)
-			insert_separator(&d.buf, 0);
 		if (prev_buf && size)
-			strbuf_insert(&d.buf, 0, prev_buf, size);
+			strbuf_add(&buf, prev_buf, size);
+		if (d.buf.len && prev_buf && size)
+			append_separator(&buf);
+		strbuf_insert(&d.buf, 0, buf.buf, buf.len);
+
 		free(prev_buf);
+		strbuf_release(&buf);
 	}
 
 	if (d.buf.len || allow_empty) {