| Message ID | f7edbd0e508243ab55c13721a21b78bf50278a21.1673490953.git.dyroneteng@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | notes.c: introduce "--separator" optio | expand |
On Thu, Jan 12 2023, Teng Long wrote: > From: Teng Long <dyroneteng@gmail.com> > > When appending to a given notes object and the appended note is not > empty too, we will insert a blank line at first which separates the > existing note and the appended one, which as the separator. > > Sometimes, we want to use a specified <text> as the separator. For > example, if we specify as: > > * --separator='------': we will insert "------\n" as the separator, > because user do not provide the line break char at last, we will add > the trailing '\n' compatibly. > > * --separator='------\n': we will insert as-is because it contains > the line break at last. > > * --separator='': we specify an empty separator which means will > append the message directly without inserting any separator at > first. > > * not specified --separator option: will use '\n' as the separator > when do appending and this is the default behavour. > > Signed-off-by: Teng Long <dyroneteng@gmail.com> > --- > Documentation/git-notes.txt | 18 +++++++++-- > builtin/notes.c | 49 +++++++++++++++++++++++++++--- > t/t3301-notes.sh | 59 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 120 insertions(+), 6 deletions(-) > > diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt > index efbc10f0f5..227fa88317 100644 > --- a/Documentation/git-notes.txt > +++ b/Documentation/git-notes.txt > @@ -11,7 +11,7 @@ SYNOPSIS > 'git notes' [list [<object>]] > 'git notes' add [-f] [--allow-empty] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>] > 'git notes' copy [-f] ( --stdin | <from-object> [<to-object>] ) > -'git notes' append [--allow-empty] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>] > +'git notes' append [--allow-empty] [--separator] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>] > 'git notes' edit [--allow-empty] [<object>] > 'git notes' show [<object>] > 'git notes' merge [-v | -q] [-s <strategy> ] <notes-ref> > @@ -86,7 +86,11 @@ the command can read the input given to the `post-rewrite` hook.) > > append:: > Append to the notes of an existing object (defaults to HEAD). > - Creates a new notes object if needed. > + Creates a new notes object if needed. If the note of the given > + object and the note to be appended are not empty, a blank line > + will be inserted between them as the separator ("blank line" is > + the default behavior, `--separator` option supports to specify > + a customized one). I think this should change to: [...]will be inserted between them. Use the `--separator` option to insert other delimiters. I.e. part of that's fixes for odd grammar, but mainly just offloading the explanation to the --separator discussion below. > > edit:: > Edit the notes for a given object (defaults to HEAD). > @@ -159,6 +163,16 @@ OPTIONS > Allow an empty note object to be stored. The default behavior is > to automatically remove empty notes. > > +--separator <text>:: > + Specify the <text> to be inserted between existing note and appended > + message, the <text> acts as a separator. Maybe let's use '<string>' or '<separator>' here instead? e.g.: Specifies the <string> ... Maybe "<text>" just looks odd to me. More generally, let's say something like: When invoking "git notes append", specify the... I.e. this is only for "append", but nothing here says so. > + If <text> is empty (`--separator=''`), will append the message to > + existing note directly without insert any separator. > + If <text> is nonempty, will use as-is. One thing to notice is if > + the <text> lacks newline charactor, will add the newline automatically. > + If not specify this option, a blank line will be inserted as the > + separator. We're spending a lot of text here on a pretty simple concept if I understand it correctly, I.e. just (pseudocode): int sep_extra_nl = 0; const char *sep = opt_sep ? opt_sep : "\n"; if (!strstr(sep, '\n')) sep_extra_nl = 1; [...] Except that was written after I read your explanation, but looking at the code it's incorrect, it's whether the "*last*" character contains a newline or not. So all in all, I think we should just say: --separator <separator>: The '<separator>' inserted between the note and message by 'append', "\n" by default. A custom separator can be provided, if it doesn't end in a "\n" one will be added implicitly . > + > --ref <ref>:: > Manipulate the notes tree in <ref>. This overrides > `GIT_NOTES_REF` and the "core.notesRef" configuration. The ref > diff --git a/builtin/notes.c b/builtin/notes.c > index f2efb3736c..6746ad3232 100644 > --- a/builtin/notes.c > +++ b/builtin/notes.c > @@ -24,6 +24,8 @@ > #include "notes-utils.h" > #include "worktree.h" > > +static char *separator = "\n"; > + > static const char * const git_notes_usage[] = { > N_("git notes [--ref <notes-ref>] [list [<object>]]"), > N_("git notes [--ref <notes-ref>] add [-f] [--allow-empty] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]"), > @@ -209,7 +211,7 @@ static void write_note_data(struct note_data *d, struct object_id *oid) > } > } > > -static int parse_msg_arg(const struct option *opt, const char *arg, int unset) > +static int parse_msg_arg_add(const struct option *opt, const char *arg, int unset) > { > struct note_data *d = opt->value; > > @@ -225,6 +227,43 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset) > return 0; > } > > +static void insert_separator(struct strbuf *message) > +{ > + const char *insert; > + > + if (!separator) > + separator = "\n"; > + if (*separator == '\0') Style: Don't compare to 0, NULL, '\0' etc. Just use !*separator. > + /* separator is empty; use as-is (no blank line) */ > + return; > + else if (separator[strlen(separator) - 1] == '\n') > + /* user supplied newline; use as-is */ > + insert = separator; > + else > + /* separator lacks newline; add it ourselves */ > + insert = xstrfmt("%s%s", separator,"\n"); We're leaking memor here, and making it hard to fix that by conflating a const "insert" with this allocated version. I haven't read the whole context, but this seems really complex per the doc feedback above. Why can't we just keep track of if we're using the default value or not? I.e. just have the "--separator" option default to NULL, if it's not set y ou don't need to do this "\n" check, and just use the default, otherwise append etc. > + strbuf_insertstr(message, 0, insert); Maybe you were trying to get around using a more complex strbuf_splice() here, but let's just avoid teh xstrfmt() and splice() that "\n" in, if needed? > +} > + > +static int parse_msg_arg_append(const struct option *opt, const char *arg, int unset) > +{ > + struct note_data *d = opt->value; > + struct strbuf append = STRBUF_INIT; > + > + BUG_ON_OPT_NEG(unset); > + > + strbuf_addstr(&append, arg); > + if (d->buf.len){ > + insert_separator(&append); > + } Drop the {} here. > + strbuf_addbuf(&d->buf, &append); > + strbuf_stripspace(&d->buf, 0); > + > + d->given = 1; > + strbuf_release(&append); Why do we need this other variable, canet'w just append to d.buf directly? Do we mean to strbuf_stripspace() here over the whole buffer, or just what we're appending?
On Thu, Jan 12, 2023 at 5:07 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > On Thu, Jan 12 2023, Teng Long wrote: > > When appending to a given notes object and the appended note is not > > empty too, we will insert a blank line at first which separates the > > existing note and the appended one, which as the separator. > > [...] > > Signed-off-by: Teng Long <dyroneteng@gmail.com> > > --- > > diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt > > @@ -159,6 +163,16 @@ OPTIONS > > +--separator <text>:: > > + Specify the <text> to be inserted between existing note and appended > > + message, the <text> acts as a separator. > > Maybe let's use '<string>' or '<separator>' here instead? e.g.: > Specifies the <string> ... > Maybe "<text>" just looks odd to me. > > More generally, let's say something like: > When invoking "git notes append", specify the... > I.e. this is only for "append", but nothing here says so. Agreed on these points. > > + If <text> is empty (`--separator=''`), will append the message to > > + existing note directly without insert any separator. > > + If <text> is nonempty, will use as-is. One thing to notice is if > > + the <text> lacks newline charactor, will add the newline automatically. > > + If not specify this option, a blank line will be inserted as the > > + separator. > > We're spending a lot of text here on a pretty simple concept if I > understand it correctly, I.e. just (pseudocode): > > int sep_extra_nl = 0; > const char *sep = opt_sep ? opt_sep : "\n"; > if (!strstr(sep, '\n')) > sep_extra_nl = 1; > [...] > > Except that was written after I read your explanation, but looking at > the code it's incorrect, it's whether the "*last*" character contains a > newline or not. > > So all in all, I think we should just say: > > --separator <separator>: > The '<separator>' inserted between the note and message > by 'append', "\n" by default. A custom separator can be > provided, if it doesn't end in a "\n" one will be added > implicitly . Unfortunately, this misses the point. The original reason Teng Long started on this patch series was to be able to _suppress_ the blank line added unconditionally between notes. In the original submission, this was done via a --no-blankline option, but that met with resistance from some reviewers as being potentially confusing and too specialized. (The commit message of this patch should probably do a better job of explaining that one purpose of this change is to support the case of no-separator.) A generalized --separator= option was suggested[1] as a possibly more palatable alternative, with which an empty string (meaning "no separator") would cover the case for which the original --no-blankline was meant to handle. So, at the very least, the documentation needs to call out the empty string as being a special case for which automatic appending of "\n" does not occur. > > diff --git a/builtin/notes.c b/builtin/notes.c > > +static void insert_separator(struct strbuf *message) > > +{ > > + const char *insert; > > + > > + if (!separator) > > + separator = "\n"; > > + if (*separator == '\0') > > Style: Don't compare to 0, NULL, '\0' etc. Just use !*separator. My fault[2]. Your suggestion is indeed more appropriate in this codebase. > > + /* separator is empty; use as-is (no blank line) */ > > + return; > > + else if (separator[strlen(separator) - 1] == '\n') > > + /* user supplied newline; use as-is */ > > + insert = separator; > > + else > > + /* separator lacks newline; add it ourselves */ > > + insert = xstrfmt("%s%s", separator,"\n"); > > We're leaking memor here, and making it hard to fix that by conflating a > const "insert" with this allocated version. > > I haven't read the whole context, but this seems really complex per the > doc feedback above. Why can't we just keep track of if we're using the > default value or not? I.e. just have the "--separator" option default to > NULL, if it's not set y ou don't need to do this "\n" check, and just > use the default, otherwise append etc. That wouldn't work for the reason given above. The idea outlined in [2] is that an empty separator is treated specially as meaning "nothing-between-notes, not even a blank line". > > + strbuf_insertstr(message, 0, insert); > > Maybe you were trying to get around using a more complex strbuf_splice() > here, but let's just avoid teh xstrfmt() and splice() that "\n" in, if > needed? The code example I gave in [2] was meant to illustrate the suggested behavior as clearly as possible, not necessarily to be copied verbatim. Being able to do this without leaking memory should certainly be possible. > Do we mean to strbuf_stripspace() here over the whole buffer, or just > what we're appending? That's a very good question. The note being appended might indeed have leading whitespace gunk which ought to be removed before the append operation. (Plus, it's a reasonable assumption that the existing note text has already been "stripspaced".) [1]: https://lore.kernel.org/git/CAPig+cRcezSp4Rqt1Y9bD-FT6+7b0g9qHfbGRx65AOnw2FQXKg@mail.gmail.com/ [2]: https://lore.kernel.org/git/CAPig+cTFBVAL2gd3LqQEzS--cXqJXR+1OVerii-D6JqFvJwXqQ@mail.gmail.com/
On Wed, Jan 11, 2023 at 9:48 PM Teng Long <dyroneteng@gmail.com> wrote: > When appending to a given notes object and the appended note is not > empty too, we will insert a blank line at first which separates the > existing note and the appended one, which as the separator. > [...] > Signed-off-by: Teng Long <dyroneteng@gmail.com> > --- > diff --git a/builtin/notes.c b/builtin/notes.c > @@ -24,6 +24,8 @@ > +static char *separator = "\n"; If you are initializing `separator` to "\n"... > @@ -225,6 +227,43 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset) > +static void insert_separator(struct strbuf *message) > +{ > + if (!separator) > + separator = "\n"; ... then these lines are not needed (and are effectively dead-code). > + if (*separator == '\0') > + /* separator is empty; use as-is (no blank line) */ > + return; > + else if (separator[strlen(separator) - 1] == '\n') > + /* user supplied newline; use as-is */ > + insert = separator; > + else > + /* separator lacks newline; add it ourselves */ > + insert = xstrfmt("%s%s", separator,"\n"); > + strbuf_insertstr(message, 0, insert); > +} > git format-patch a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1 --thread -v 4 --output-directory=outgoing/git-notes-append/v4 --cover-letter --range-diff 196e80358ediff --git a/t/t3301-notes.sh b/t/t3301-notes.sh There's some weird malformation going on here... this should just be a "diff --git ..." line. > @@ -521,6 +521,65 @@ test_expect_success 'listing non-existing notes fails' ' > +test_expect_success 'append: specify an empty separator' ' > + test_when_finished git notes remove HEAD && > + cat >expect <<-\EOF && > + notes-1 > + notes-2 > + EOF Style nit: We don't normally give extra indentation to the body of the here-doc. Instead: cat >expect <<-\EOF && notes-1 notes-2 EOF > + git notes add -m "notes-1" && > + git notes append --separator="" -m "notes-2" && > + git notes show >actual && > + test_cmp expect actual > + > +' Style nit: drop the unnecessary blank line before the closing quote. > +test_expect_success 'append: specify separatoro with line break' ' s/separatoro/separator/ > + test_when_finished git notes remove HEAD && > + cat >expect <<-\EOF && > + notes-1 > + ------- > + notes-2 > + EOF > + > + git notes add -m "notes-1" && > + separator=$(printf "%s\n" "-------") && > + git notes append --separator="$separator" -m "notes-2" && It might be easier to drop the `separator` variable and write this simply as: git notes append --separator="-------$LF" -m "notes-2" && LF is defined in t/test-lib.sh as "\n". > + git notes show >actual && > + test_cmp expect actual > +'
diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt index efbc10f0f5..227fa88317 100644 --- a/Documentation/git-notes.txt +++ b/Documentation/git-notes.txt @@ -11,7 +11,7 @@ SYNOPSIS 'git notes' [list [<object>]] 'git notes' add [-f] [--allow-empty] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>] 'git notes' copy [-f] ( --stdin | <from-object> [<to-object>] ) -'git notes' append [--allow-empty] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>] +'git notes' append [--allow-empty] [--separator] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>] 'git notes' edit [--allow-empty] [<object>] 'git notes' show [<object>] 'git notes' merge [-v | -q] [-s <strategy> ] <notes-ref> @@ -86,7 +86,11 @@ the command can read the input given to the `post-rewrite` hook.) append:: Append to the notes of an existing object (defaults to HEAD). - Creates a new notes object if needed. + Creates a new notes object if needed. If the note of the given + object and the note to be appended are not empty, a blank line + will be inserted between them as the separator ("blank line" is + the default behavior, `--separator` option supports to specify + a customized one). edit:: Edit the notes for a given object (defaults to HEAD). @@ -159,6 +163,16 @@ OPTIONS Allow an empty note object to be stored. The default behavior is to automatically remove empty notes. +--separator <text>:: + Specify the <text> to be inserted between existing note and appended + message, the <text> acts as a separator. + If <text> is empty (`--separator=''`), will append the message to + existing note directly without insert any separator. + If <text> is nonempty, will use as-is. One thing to notice is if + the <text> lacks newline charactor, will add the newline automatically. + If not specify this option, a blank line will be inserted as the + separator. + --ref <ref>:: Manipulate the notes tree in <ref>. This overrides `GIT_NOTES_REF` and the "core.notesRef" configuration. The ref diff --git a/builtin/notes.c b/builtin/notes.c index f2efb3736c..6746ad3232 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -24,6 +24,8 @@ #include "notes-utils.h" #include "worktree.h" +static char *separator = "\n"; + static const char * const git_notes_usage[] = { N_("git notes [--ref <notes-ref>] [list [<object>]]"), N_("git notes [--ref <notes-ref>] add [-f] [--allow-empty] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]"), @@ -209,7 +211,7 @@ static void write_note_data(struct note_data *d, struct object_id *oid) } } -static int parse_msg_arg(const struct option *opt, const char *arg, int unset) +static int parse_msg_arg_add(const struct option *opt, const char *arg, int unset) { struct note_data *d = opt->value; @@ -225,6 +227,43 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset) return 0; } +static void insert_separator(struct strbuf *message) +{ + const char *insert; + + if (!separator) + separator = "\n"; + if (*separator == '\0') + /* separator is empty; use as-is (no blank line) */ + return; + else if (separator[strlen(separator) - 1] == '\n') + /* user supplied newline; use as-is */ + insert = separator; + else + /* separator lacks newline; add it ourselves */ + insert = xstrfmt("%s%s", separator,"\n"); + strbuf_insertstr(message, 0, insert); +} + +static int parse_msg_arg_append(const struct option *opt, const char *arg, int unset) +{ + struct note_data *d = opt->value; + struct strbuf append = STRBUF_INIT; + + BUG_ON_OPT_NEG(unset); + + strbuf_addstr(&append, arg); + if (d->buf.len){ + insert_separator(&append); + } + strbuf_addbuf(&d->buf, &append); + strbuf_stripspace(&d->buf, 0); + + d->given = 1; + strbuf_release(&append); + return 0; +} + static int parse_file_arg(const struct option *opt, const char *arg, int unset) { struct note_data *d = opt->value; @@ -406,7 +445,7 @@ static int add(int argc, const char **argv, const char *prefix) struct option options[] = { OPT_CALLBACK_F('m', "message", &d, N_("message"), N_("note contents as a string"), PARSE_OPT_NONEG, - parse_msg_arg), + parse_msg_arg_add), OPT_CALLBACK_F('F', "file", &d, N_("file"), N_("note contents in a file"), PARSE_OPT_NONEG, parse_file_arg), @@ -572,7 +611,7 @@ static int append_edit(int argc, const char **argv, const char *prefix) struct option options[] = { OPT_CALLBACK_F('m', "message", &d, N_("message"), N_("note contents as a string"), PARSE_OPT_NONEG, - parse_msg_arg), + parse_msg_arg_append), OPT_CALLBACK_F('F', "file", &d, N_("file"), N_("note contents in a file"), PARSE_OPT_NONEG, parse_file_arg), @@ -584,6 +623,8 @@ static int append_edit(int argc, const char **argv, const char *prefix) parse_reuse_arg), OPT_BOOL(0, "allow-empty", &allow_empty, N_("allow storing empty note")), + OPT_STRING(0, "separator", &separator, N_("text"), + N_("insert <text> as separator before appending to an existing note")), OPT_END() }; int edit = !strcmp(argv[0], "edit"); @@ -619,7 +660,7 @@ static int append_edit(int argc, const char **argv, const char *prefix) char *prev_buf = read_object_file(note, &type, &size); if (d.buf.len && prev_buf && size) - strbuf_insertstr(&d.buf, 0, "\n"); + insert_separator(&d.buf); if (prev_buf && size) strbuf_insert(&d.buf, 0, prev_buf, size); free(prev_buf); git format-patch a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1 --thread -v 4 --output-directory=outgoing/git-notes-append/v4 --cover-letter --range-diff 196e80358ediff --git a/t/t3301-notes.sh b/t/t3301-notes.sh index e7807e052a..e8bc9934ed 100755 --- a/t/t3301-notes.sh +++ b/t/t3301-notes.sh @@ -521,6 +521,65 @@ test_expect_success 'listing non-existing notes fails' ' test_must_be_empty actual ' +test_expect_success 'append: specify an empty separator' ' + test_when_finished git notes remove HEAD && + cat >expect <<-\EOF && + notes-1 + notes-2 + EOF + + git notes add -m "notes-1" && + git notes append --separator="" -m "notes-2" && + git notes show >actual && + test_cmp expect actual + +' + +test_expect_success 'append: specify separatoro with line break' ' + test_when_finished git notes remove HEAD && + cat >expect <<-\EOF && + notes-1 + ------- + notes-2 + EOF + + git notes add -m "notes-1" && + separator=$(printf "%s\n" "-------") && + git notes append --separator="$separator" -m "notes-2" && + git notes show >actual && + test_cmp expect actual +' + +test_expect_success 'append: specify separator without line break' ' + test_when_finished git notes remove HEAD && + cat >expect <<-\EOF && + notes-1 + ------- + notes-2 + EOF + + git notes add -m "notes-1" && + git notes append --separator="-------" -m "notes-2" && + git notes show >actual && + test_cmp expect actual +' + +test_expect_success 'append: specify separator with multiple messages' ' + test_when_finished git notes remove HEAD && + cat >expect <<-\EOF && + notes-1 + ------- + notes-2 + ------- + notes-3 + EOF + + git notes add -m "notes-1" && + git notes append --separator="-------" -m "notes-2" -m "notes-3" && + git notes show >actual && + test_cmp expect actual +' + test_expect_success 'append to existing note with "git notes append"' ' cat >expect <<-EOF && Initial set of notes