On 2024-10-23 22:14:24+0200, Bence Ferdinandy <bence@xxxxxxxxxxxxxx> 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 --------------
--
Danh
