| Message ID | cover.1667980450.git.dyroneteng@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | notes.c: introduce "--no-blank-line" option | expand |
> Teng Long (5): > notes.c: cleanup 'strbuf_grow' call in 'append_edit' > notes.c: cleanup for "designated init" and "char ptr init" > notes.c: drop unreachable code in 'append_edit()' > notes.c: provide tips when target and append note are both empty > notes.c: introduce "--no-blank-line" option I'm not sure if this patch series should continue, and if there are no updated comments it will be temporarily suspended. Thanks for the reviews on the past patches.
Teng Long <dyroneteng@gmail.com> writes: >> Teng Long (5): >> notes.c: cleanup 'strbuf_grow' call in 'append_edit' >> notes.c: cleanup for "designated init" and "char ptr init" >> notes.c: drop unreachable code in 'append_edit()' >> notes.c: provide tips when target and append note are both empty >> notes.c: introduce "--no-blank-line" option > > I'm not sure if this patch series should continue, and if there are no > updated comments it will be temporarily suspended. > > Thanks for the reviews on the past patches. Taylor, This topic was marked to "expect" a reroll in the second issue of November "What's cooking" report you did. Do you recall what remaining works there were? I personally do not have much opinion on this topic, other than that "--no-blank-link" would be a horrible name (i.e. uses concrete words to pretend that it clearly describes what it does, but is utterly unclear where these blank lines are etc.) for the feature to help end-users discover it.
Junio C Hamano <gitster@pobox.com> writes: > I personally do not have much opinion on this topic, other than that > "--no-blank-link" would be a horrible name (i.e. uses concrete words > to pretend that it clearly describes what it does, but is utterly > unclear where these blank lines are etc.) for the feature to help > end-users discover it. I have some a candidates might like '--newline' and '--no-newline', or 'splitted-newline' or 'no-splitted-newline', but I think the latter is a little long. Thanks.
Teng Long <dyroneteng@gmail.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> I personally do not have much opinion on this topic, other than that >> "--no-blank-link" would be a horrible name (i.e. uses concrete words >> to pretend that it clearly describes what it does, but is utterly >> unclear where these blank lines are etc.) for the feature to help >> end-users discover it. > > I have some a candidates might like '--newline' and '--no-newline', or > 'splitted-newline' or 'no-splitted-newline', but I think the latter is > a little long. > > Thanks. I do not care much between blank and newline. Both alone are equally horrible, in that the option would have no effect when "git notes edit" is used and spawns an editor, or "git notes append -m one -m two" is used and the command adds the second paragraph whose text is "two". Just like "git commit", the argument to each "-m" option becomes a separate paragraph by default. I personally feel that those who want to make them separate lines deserve to have an option like this one, so that they can do $ git notes add -m foo HEAD $ git notes append \ --each-message-is-line-not-paragraph -m bar -m baz $ git notes show HEAD foo bar baz but the point is "blank-line" or "newline" does not say which newline in the resulting notes object you are mucking with. It is not like in this example: $ git notes add -m "title of the note" $ git notes append --no-blank-line -m "body of the note that span multiple lines" HEAD you are removving the blank lines embedded in the body of the message, but from the option name, it is hard to guess that.
On Tue, Nov 29, 2022 at 10:10:52AM +0900, Junio C Hamano wrote: > Teng Long <dyroneteng@gmail.com> writes: > > >> Teng Long (5): > >> notes.c: cleanup 'strbuf_grow' call in 'append_edit' > >> notes.c: cleanup for "designated init" and "char ptr init" > >> notes.c: drop unreachable code in 'append_edit()' > >> notes.c: provide tips when target and append note are both empty > >> notes.c: introduce "--no-blank-line" option > > > > I'm not sure if this patch series should continue, and if there are no > > updated comments it will be temporarily suspended. > > > > Thanks for the reviews on the past patches. > > Taylor, This topic was marked to "expect" a reroll in the second > issue of November "What's cooking" report you did. Do you recall > what remaining works there were? Looking at the dates, I sent that WC on 2022-11-08, which means that I most likely was referencing the discussion between the author and Ævar. The following WC on 2022-11-14 likely should have changed the status to "Waiting for review". Thanks, Taylor
Junio C Hamano <gitster@pobox.com> writes: > but the point is "blank-line" or "newline" does not say which > newline in the resulting notes object you are mucking with. It is > not like in this example: > > $ git notes add -m "title of the note" > $ git notes append --no-blank-line -m "body of the note > > that span multiple > > lines" HEAD > > you are removving the blank lines embedded in the body of the > message, but from the option name, it is hard to guess that. Fine, I see. Maybe "insert-starting-newline" or "insert-initial-newline". If we could not find a suitable naming for this, it's ok for me to hang up [5/5] (a little struggle for me to find a better name for this now in fact (⊙ˍ⊙).), because except it, the [4/5] still an active bugfix in my opinion. Thanks.
On Thu, Dec 15, 2022 at 7:58 AM Teng Long <dyroneteng@gmail.com> wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > but the point is "blank-line" or "newline" does not say which > > newline in the resulting notes object you are mucking with. It is > > not like in this example: > > [...] > > you are removving the blank lines embedded in the body of the > > message, but from the option name, it is hard to guess that. > > Fine, I see. Maybe "insert-starting-newline" or "insert-initial-newline". If we > could not find a suitable naming for this, it's ok for me to hang up [5/5] (a > little struggle for me to find a better name for this now in fact (⊙ˍ⊙).), > because except it, the [4/5] still an active bugfix in my opinion. Taking a step back, perhaps think of this in terms of "separator". The default behavior is to insert "\n" as a separator between notes. If you add a --separator option, then users could supply their own separator, such as "----\n" or, in your case, "" to suppress the blank line.
Eric Sunshine <sunshine@sunshineco.com> writes: > Taking a step back, perhaps think of this in terms of "separator". The > default behavior is to insert "\n" as a separator between notes. If > you add a --separator option, then users could supply their own > separator, such as "----\n" or, in your case, "" to suppress the blank > line. Your idea is an enhancement of the original one of me. I think it's a suitable name and I could implement it, maybe we could hear about Junio's advice of the naming? Thanks.
Teng Long <dyroneteng@gmail.com> writes: > Eric Sunshine <sunshine@sunshineco.com> writes: > >> Taking a step back, perhaps think of this in terms of "separator". The >> default behavior is to insert "\n" as a separator between notes. If >> you add a --separator option, then users could supply their own >> separator, such as "----\n" or, in your case, "" to suppress the blank >> line. > > Your idea is an enhancement of the original one of me. I think it's a suitable > name and I could implement it, maybe we could hear about Junio's advice of the > naming? Yeah, saying "separator" clarifies what that empty line is meant to be (i.e. it is an inter-paragraph separator), and is much better than "newline" or "blankline", I would think. Thanks.
Eric Sunshine <sunshine@sunshineco.com> writes: > Taking a step back, perhaps think of this in terms of "separator". The > default behavior is to insert "\n" as a separator between notes. If > you add a --separator option, then users could supply their own > separator, such as "----\n" or, in your case, "" to suppress the blank > line. There is another question for me, if the separator we passed contains "\n" string , the argument the cmd receives will need to tranfer to '\n' character instead to make sure it's a linebreak but not a "\n" instead. So maybe like: diff --git a/builtin/notes.c b/builtin/notes.c index f38e6e8b04..64ee64eff7 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -559,15 +559,52 @@ static int copy(int argc, const char **argv, const char *prefix) return retval; } +static void insert_separator(struct strbuf *message, const char *separator) +{ + struct strbuf transfered = STRBUF_INIT; + + if (!separator) + strbuf_insertstr(message, 0, "\n"); + else if (!strcmp("", separator)) + return; + else { + while (*separator) { + if (*separator == '\\'){ + switch (separator[1]) { + case 'n': + strbuf_addstr(&transfered, "\n"); + separator++; + break; + case 'r': + strbuf_addstr(&transfered, "\r"); + separator++; + break; + case 't': + strbuf_addstr(&transfered, "\t"); + separator++; + break; + default: + strbuf_addch(&transfered, *separator); + } + } else { + strbuf_addch(&transfered, *separator); + } + separator++; + } + strbuf_insertstr(message, 0, transfered.buf); + strbuf_release(&transfered); + } +} + static int append_edit(int argc, const char **argv, const char *prefix) { int allow_empty = 0; - int blankline = 1; const char *object_ref; struct notes_tree *t; struct object_id object, new_note; const struct object_id *note; char *logmsg = NULL; + const char *separator = NULL; const char * const *usage; struct note_data d = { .buf = STRBUF_INIT }; struct option options[] = { @@ -585,8 +622,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_BOOL(0, "blank-line", &blankline, - N_("insert paragraph break before appending to an existing 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"); @@ -621,8 +658,8 @@ static int append_edit(int argc, const char **argv, const char *prefix) enum object_type type; char *prev_buf = read_object_file(note, &type, &size); - if (blankline && d.buf.len && prev_buf && size) - strbuf_insertstr(&d.buf, 0, "\n"); + if (d.buf.len && prev_buf && size) + insert_separator(&d.buf, separator); if (prev_buf && size) strbuf_insert(&d.buf, 0, prev_buf, size); free(prev_buf); -- If the above is understood correctly, is there an api that handles escape characters already in the existing code (I haven't found one so far, so I need to confirm and replace it if there is one). In addition, the insert_separator function above handles three special characters \t\n\r. Do we need more? Thanks
On Thu, Dec 22, 2022 at 4:30 AM Teng Long <dyroneteng@gmail.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > Taking a step back, perhaps think of this in terms of "separator". The > > default behavior is to insert "\n" as a separator between notes. If > > you add a --separator option, then users could supply their own > > separator, such as "----\n" or, in your case, "" to suppress the blank > > line. > > There is another question for me, if the separator we passed contains "\n" > string , the argument the cmd receives will need to tranfer to '\n' character > instead to make sure it's a linebreak but not a "\n" instead. > > So maybe like: > +static void insert_separator(struct strbuf *message, const char *separator) > +{ > + while (*separator) { > + if (*separator == '\\'){ > + switch (separator[1]) { > + case 'n': > + strbuf_addstr(&transfered, "\n"); > + separator++; > + break; > + [...] > + separator++; > + } > +} > > If the above is understood correctly, is there an api that handles escape > characters already in the existing code (I haven't found one so far, so I need > to confirm and replace it if there is one). In addition, the insert_separator > function above handles three special characters \t\n\r. Do we need more? You could probably use unquote_c_style() from quote.[hc]; something like this: struct strbuf orig = STRBUF_INIT; struct strbuf unquoted = STRBUF_INIT; strbuf_addf(&orig, "\"%s\"", separator); if (unquote_c_style(&unquoted, orig.buf, NULL) < 0) { strbuf_release(&unquoted); strbuf_release(&orig); die(_("some suitable error message")); } /* unquote succeeded -- use "unquoted" here */ However, I suspect that this is overkill, and you should explore simpler ideas first. For instance, it is perfectly acceptable to embed newlines directly in shell strings, so this would work just fine without having to write any extra string-unquoting code: % git notes add --separator='--- ' <object> But, I think you can make this even friendlier without having to do any extra coding to support string-unquoting. In particular, use this heuristic: - if the separator is zero-length, use it as-is - otherwise, if the separator ends with a newline, use it as-is - otherwise add a newline to the separator In other words: if (!separator) separator = "\n"; /* default is one blank line */ if (*separator == '\0') /* separator is empty; use as-is (no blank line) */ else if (separator[strlen(separator) - 1] == '\n') /* user supplied newline; use as-is */ else /* separator lacks newline; add it ourselves */ With the above logic, this defaults to a blank line between notes: % git notes add ... this has no blank line between notes: % git notes add --separator='' ... this uses a "---" + "\n" as separator: % git notes add --separator='---' ... as does this: % git notes add --separator='--- ' ... and this places two blank lines between notes: % git notes add --separator=' ' ...
From: Teng Long <dyroneteng@gmail.com> Diff since v2: * [5/5] make "--no-blank-line" in doc. * [1/5] [2/5][3/5]do some cleanups and split to serval independent commit. * [3/5] futher explain in commit-msg why we can drop it in "append" but not "add". * [4/5] [4/5] use "test_when_finished" to cleanup notes in tests. Thanks. Teng Long (5): notes.c: cleanup 'strbuf_grow' call in 'append_edit' notes.c: cleanup for "designated init" and "char ptr init" notes.c: drop unreachable code in 'append_edit()' notes.c: provide tips when target and append note are both empty notes.c: introduce "--no-blank-line" option Documentation/git-notes.txt | 10 ++++++++-- builtin/notes.c | 20 ++++++++++---------- t/t3301-notes.sh | 18 ++++++++++++++++-- 3 files changed, 34 insertions(+), 14 deletions(-) Range-diff against v2: -: ---------- > 1: 8ae58934a1 notes.c: cleanup 'strbuf_grow' call in 'append_edit' -: ---------- > 2: a53576ea88 notes.c: cleanup for "designated init" and "char ptr init" -: ---------- > 3: 62a952ba3e notes.c: drop unreachable code in 'append_edit()' -: ---------- > 4: 0d8ce0b14b notes.c: provide tips when target and append note are both empty 1: 2381947abd ! 5: 196e80358e notes.c: introduce "--blank-line" option @@ Metadata Author: Teng Long <dyroneteng@gmail.com> ## Commit message ## - notes.c: introduce "--blank-line" option + notes.c: introduce "--no-blank-line" option When appending to a given object which has note and if the appended note is not empty too, we will insert a blank line at first. The @@ Documentation/git-notes.txt: SYNOPSIS '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] [--blank-line] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>] ++'git notes' append [--allow-empty] [--no-blank-line] [-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> @@ Documentation/git-notes.txt: OPTIONS Allow an empty note object to be stored. The default behavior is to automatically remove empty notes. -+--blank-line:: +--no-blank-line:: -+ Controls if a blank line to split paragraphs is inserted -+ when appending (the default is true). ++ Do not insert a blank line before the inserted notes (insert ++ a blank line is the default). + --ref <ref>:: Manipulate the notes tree in <ref>. This overrides @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char }; int edit = !strcmp(argv[0], "edit"); @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char *prefix) + enum object_type type; char *prev_buf = read_object_file(note, &type, &size); - strbuf_grow(&d.buf, size + 1); - if (d.buf.len && prev_buf && size) + if (blankline && d.buf.len && prev_buf && size) strbuf_insertstr(&d.buf, 0, "\n"); @@ t/t3301-notes.sh: test_expect_success 'listing non-existing notes fails' ' ' +test_expect_success 'append to existing note without a beginning blank line' ' ++ test_when_finished git notes remove HEAD && + cat >expect <<-\EOF && + Initial set of notes + Appended notes @@ t/t3301-notes.sh: test_expect_success 'listing non-existing notes fails' ' More notes appended with git notes append EOF -+ git notes remove HEAD && ++ git notes add -m "Initial set of notes" && git notes append -m "More notes appended with git notes append" && git notes show >actual && 2: 5dbe014a09 < -: ---------- notes.c: fixed tip when target and append note are both empty 3: 2475ea0c04 < -: ---------- notes.c: drop unreachable code in "append_edit()"