Re: [RFC PATCH] notes: add prepend command

From

Đoàn Trần Công Danh <congdanhqx@gmail.com>

Date

Oct 24, 2024, 11:19 UTC

Message-ID

<ZxotMcKv5rEIMZ8q@danh.dev>

In-Reply-To

<20241023201430.986389-1-bence@ferdinandy.com>

On 2024-10-23 22:14:24+0200, Bence Ferdinandy <bence@ferdinandy.com> wrote:

> -static int append_edit(int argc, const char **argv, const char *prefix)
> +
> +static int append_prepend_edit(int argc, const char **argv, const char *prefix, int prepend)
>  {
>  	int allow_empty = 0;
>  	const char *object_ref;
> @@ -716,11 +718,18 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>  
>  		if (!prev_buf)
>  			die(_("unable to read %s"), oid_to_hex(note));
> -		if (size)
> -			strbuf_add(&buf, prev_buf, size);
> -		if (d.buf.len && size)
> -			append_separator(&buf);
> -		strbuf_insert(&d.buf, 0, buf.buf, buf.len);
> +		if (prepend) {
> +			if (d.buf.len && size)
> +				append_separator(&buf);
> +			if (size)
> +				strbuf_add(&buf, prev_buf, size);
> +		} else {
> +			if (size)
> +				strbuf_add(&buf, prev_buf, size);
> +			if (d.buf.len && size)
> +				append_separator(&buf);
> +		}
> +		strbuf_insert(&d.buf, prepend ? d.buf.len : 0, buf.buf, buf.len);
>  
>  		free(prev_buf);
>  		strbuf_release(&buf);

I think this diff should be written like this for easier reasoning:

----- 8< -----------------

@@ -711,19 +713,27 @@ 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;
-		struct strbuf buf = STRBUF_INIT;
 		char *prev_buf = repo_read_object_file(the_repository, note, &type, &size);
 
 		if (!prev_buf)
 			die(_("unable to read %s"), oid_to_hex(note));
-		if (size)
+		if (!size) {
+			// no existing notes, use whatever we have here
+		} else if (prepend) {
+			if (d.buf.len)
+				append_separator(&d.buf);
+			strbuf_add(&d.buf, prev_buf, size);
+		} else {
+			struct strbuf buf = STRBUF_INIT;
 			strbuf_add(&buf, prev_buf, size);
-		if (d.buf.len && size)
-			append_separator(&buf);
-		strbuf_insert(&d.buf, 0, buf.buf, buf.len);
+			if (d.buf.len)
+				append_separator(&buf);
+			strbuf_addbuf(&buf, &d.buf);
+			strbuf_swap(&buf, &d.buf);
+			strbuf_release(&buf);
+		}
 
 		free(prev_buf);
-		strbuf_release(&buf);
 	}
 
 	if (d.buf.len || allow_empty) {
-------------- 8< --------------------

Even if we don't take this subcommand, I think we should re-write the
append part, so:
- we can see the append logic better,
- we can avoid the `strbuf_insert` which will require memmove/memcpy.

Well, the second point is micro-optimisation, so take it with a grain
of salt.


Also tests.
-------------- 8< -----------------------
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 99137fb235731..5a7ad40fde6a8 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -558,6 +558,20 @@ test_expect_success 'listing non-existing notes fails' '
 	test_must_be_empty actual
 '
 
+test_expect_success 'append: specify a separator with an empty arg' '
+	test_when_finished git notes remove HEAD &&
+	cat >expect <<-\EOF &&
+	notes-2
+
+	notes-1
+	EOF
+
+	git notes add -m "notes-1" &&
+	git notes prepend --separator="" -m "notes-2" &&
+	git notes show >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'append: specify a separator with an empty arg' '
 	test_when_finished git notes remove HEAD &&
 	cat >expect <<-\EOF &&
----------- >8 --------------

-- 
Danh