RFC patchnotes: add prepend command

> When a note is detailing commit history, it makes sense to keep the
> latest change on top, but unlike adding things at the bottom with
> "git notes append" this can only be done manually. Add a
>
>     git notes prepend
>
> command, which works exactly like the append command, except that it
> inserts the text before the current contents of the note instead of
> after.

> On Wed, Oct 23, 2024 at 10:14:24PM +0200, Bence Ferdinandy wrote:
>> When a note is detailing commit history, it makes sense to keep the
>> latest change on top, but unlike adding things at the bottom with
>> "git notes append" this can only be done manually. Add a
>>
>>     git notes prepend
>>
>> command, which works exactly like the append command, except that it
>> inserts the text before the current contents of the note instead of
>> after.
>
> Hmmm. I am not sure that I see the widespread need for such a tool. If
> this is specific to your use-case, I think a custom script and
> `$GIT_EDITOR` would do the trick.

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

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

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