Re: [RFC PATCH] notes: add prepend command

From: Bence Ferdinandy <bence@ferdinandy.com>
Date: Oct 26, 2024, 22:34 UTC
Message-ID: <D563GBE4H09H.2JENKJVUOLMD6@ferdinandy.com>
In-Reply-To: <ZxotMcKv5rEIMZ8q@danh.dev>

On Thu Oct 24, 2024 at 13:19, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:

Show 34 quoted lines

> 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);
>
> Without prejudice about whether we should take this command.
> (I think we shouldn't, just like we shouldn't top-posting).

Again, I do not feel very strongly, about this patch, since it's not that hard to do with a script, but I don't think the analogy with top-posting is appropriate. It's usually not a discussion going on in the comments, and prepending might happen for any reason. The ordering of content in a note may not even be temporal in nature (although to be fair, I have personally never used it for anything else than versioning patches).

The specific use-case came up in patch versioning (pointed out by Kristoffer), where in a longer series with many iterations, seeing the "v1024: no change" at the top would save reviewers from having to scroll an indefinite amount in the particular patch just to find that they actually don't need to look at that one, since it hasn't changed since the previous iteration they saw. In this sense having the newest at the top rather than the bottom would be more natural. I'd think probably even new reviewers jumping in during the middle might not be very interested in the beginning.

Show 44 quoted lines

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

Thanks, I do find this a bit more easier to read indeed.

Show 33 quoted lines

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

Thanks! I didn't look at tests (and documentation) before it was clear if the idea got a green light or not, but I guess if it does, this would cover tests.

Best, Bence

-- 
bence.ferdinandy.com