| Message ID | a74c96d6dd23f2f1df6d3492093f3fd27451e24c.1676551077.git.dyroneteng@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | notes.c: introduce "--separator" option | expand |
Teng Long <dyroneteng@gmail.com> writes: > 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. ", which as the separator" does not sound grammatically correct. Without that part, the above is a perfectly readable description of the current behaviour. > Sometimes, we want to use a specified <separator> 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. In a way compatible to what? I think s/compatibly// would be even easier to read. I think you are doing that for convenience. > * --separator='------\n': we will insert as-is because it contains > the line break at last. If this behaviour gets spelled out like this, it needs to be justified. After seeing that "------" gives the user these dashes on its own single line, wouldn't a natural expectation by the user be to see a line with dashes, followed by a blank line, if you give "------\n"? How do you justify removal of that newline in a way that is easy to understand to readers? I am not saying that you should allow --separator="---\n\n\n" to give three blank lines between paragraphs. I think it makes sense to keep the stripspace in the code after a paragraph gets added. I just prefer to see it done as our design choice, not "because there is stripspace that removes them", i.e. what the code happens to do. > * not specified --separator option: will use '\n' as the separator > when do appending and this is the default behavour. s/not specified --separator option/no --separator option/; the way you phrased can be misread for git notes --separator -m foo -m bar i.e. any additional specifics is not given but --separator is still on the command line. But I do not think you meant that---rather this entry is what happens by default, i.e. a blank line separates each paragraph. > * --separator='': we specify an empty separator which has the same > behavour with --separator='\n' and or not specified the option. I do not quite see why it is necessary to spell this out. Isn't this a natural consequence of the first one (i.e. "six dashes without any terminating LF gets a line with dashes plus LF" naturally extends to "0 dashes without any terminating LF gets a blank line")? > In addition, if a user specifies multple "-m" with "--separator", the > separator should be inserted between the messages too, so we use > OPT_STRING_LIST instead of OPT_CALLBACK_F to parse "-m" option, make > sure the option value of "--separator" been parsed already when we need > it. This is hard to grok. Is it an instruction to whoever is implementing this new feature, or is it an instruct to end-users telling that they need to give --separator before they start giving -m <msg>, -F <file>, -c <object>, etc.? > diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt > index efbc10f0..5abe6092 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>] Doesn't add allow you to say $ git notes add -m foo -m bar Shouldn't it also honor --separator to specify an alternate paragraph break? > -'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>] "--separator" -> "--separator=<paragraph-break>" or something? > @@ -86,7 +86,9 @@ 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 and the > + message are not empty, "\n" will be inserted between them. > + Use the `--separator` option to insert other delimiters. "\n" is so, ... programmer lingo? "A blank line" is inserted between these paragraphs. > +--separator <separator>:: > + The '<separator>' inserted between the note and message > + by 'append', "\n" by default. A custom separator can be I see no reason to single out 'append'; "add -m <msg> -m <msg>" follows exactly the same paragraph concatenation logic in the current code, no? If we allow customized paragraph separators, we should use the same logic there as well. It probably is simpler to explain if you treat the "current note in 'append'" as if the text were just "earlier paragraphs", to which more paragraphs taken from each -m <msg> and -F <file> are concatenated, with paragraph break before each of them. In the case of 'add', there happens to be zero "earlier paragraphs", but everything else gets concatenated the same way, no? > + provided, if it doesn't end in a "\n", one will be added > + implicitly . Funny punctuation.
Junio C Hamano <gitster@pobox.com> wrote on Thu, 16 Feb 2023 15:22:16 -0800: > > 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. > > ", which as the separator" does not sound grammatically correct. > Without that part, the above is a perfectly readable description of > the current behaviour. OK, I will remove ", which as the separator". > > Sometimes, we want to use a specified <separator> 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. > > In a way compatible to what? I think s/compatibly// would be even > easier to read. I think you are doing that for convenience. My bad, I think I use a wrong word, maybe s/compatibly/automatically and I look back and think the representation of "------\n" is not correct, because "\n" is two characters here and will not treat as a newline. So, after modification, maybe like: * --separator='------': we will insert "------" and a trailing newline character, because if no newline specified at the end, one will be added automatically. > > * --separator='------\n': we will insert as-is because it contains > > the line break at last. > > If this behaviour gets spelled out like this, it needs to be > justified. > > After seeing that "------" gives the user these dashes on its own > single line, wouldn't a natural expectation by the user be to see a > line with dashes, followed by a blank line, if you give "------\n"? > How do you justify removal of that newline in a way that is easy to > understand to readers? > > I am not saying that you should allow --separator="---\n\n\n" to > give three blank lines between paragraphs. I think it makes sense to > keep the stripspace in the code after a paragraph gets added. I just > prefer to see it done as our design choice, not "because there is > stripspace that removes them", i.e. what the code happens to do. Firstly, like the problem I talked above, please let me figure out that "------\n" is not represent as "------" and a blank line, but only as "------\n" verbatim because "\n" will be treated as two characters but not a blank line while parsing. If the user wants to specify a blank line in the value of the option, they can pass "CTRL+v CTRL+j". Then, I think I did get some interference from old logic. Returning to the user scenario, I think about what the user needs and what is my original idea is: consistent behavior * behavior 1: What the user enters is used as the delimiter itself without any special processing. * behavior 2: No matter what the user enters, we always add a newline at the end or other logic like stripspace, etc. I prefer the first one, sometimes users want to be next to the previous note, then: git notes append -m foo -m bar --separator="" and they can also choose to add as many line breaks as they want, then: export LF=" " git notes append -m foo -m bar --separator="$LF$LF$LF" We didn't help users make choices but I have to say it may be a little inconvenient to enter a newline character in the terminal, but I still prefer this way. > > * not specified --separator option: will use '\n' as the separator > > when do appending and this is the default behavour. > > s/not specified --separator option/no --separator option/; the way > you phrased can be misread for > Will apply as: * no --separator option: will use '\n' as the separator when do appending and this is the default behavour. > git notes --separator -m foo -m bar > > i.e. any additional specifics is not given but --separator is still > on the command line. But I do not think you meant that---rather > this entry is what happens by default, i.e. a blank line separates > each paragraph. Yes, only separate the messages(-m), but not each paragraph in it. > > * --separator='': we specify an empty separator which has the same > > behavour with --separator='\n' and or not specified the option. > > I do not quite see why it is necessary to spell this out. Isn't > this a natural consequence of the first one (i.e. "six dashes > without any terminating LF gets a line with dashes plus LF" > naturally extends to "0 dashes without any terminating LF gets a > blank line")? Agree.. will remove. > > In addition, if a user specifies multple "-m" with "--separator", the > > separator should be inserted between the messages too, so we use > > OPT_STRING_LIST instead of OPT_CALLBACK_F to parse "-m" option, make > > sure the option value of "--separator" been parsed already when we need > > it. > > This is hard to grok. Is it an instruction to whoever is > implementing this new feature, or is it an instruct to end-users > telling that they need to give --separator before they start giving > -m <msg>, -F <file>, -c <object>, etc.? No, it's not the order of the user give, but the backend we deal. We use "parse_msg_arg" as a callback when parsing "-m " by OPT_CALLBACK_F, so if we have to read the separator before we parse it, so we could insert it correctly between the messages, So I use OPT_STRING_LIST instead. > > diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt > > index efbc10f0..5abe6092 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>] > > Doesn't add allow you to say > > $ git notes add -m foo -m bar > > Shouldn't it also honor --separator to specify an alternate > paragraph break? Agree, you also mentioned me that, does git-notes-add need to be implies by this option too? I think it needs too. > > @@ -86,7 +86,9 @@ 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 and the > > + message are not empty, "\n" will be inserted between them. > > + Use the `--separator` option to insert other delimiters. > > "\n" is so, ... programmer lingo? "A blank line" is inserted > between these paragraphs. Will apply. > > +--separator <separator>:: > > + The '<separator>' inserted between the note and message > > + by 'append', "\n" by default. A custom separator can be > > I see no reason to single out 'append'; "add -m <msg> -m <msg>" > follows exactly the same paragraph concatenation logic in the > current code, no? If we allow customized paragraph separators, > we should use the same logic there as well. Agree, as I replied above, I think it will be done in next patch. > It probably is simpler to explain if you treat the "current note in > 'append'" as if the text were just "earlier paragraphs", to which > more paragraphs taken from each -m <msg> and -F <file> are > concatenated, with paragraph break before each of them. In the case > of 'add', there happens to be zero "earlier paragraphs", but > everything else gets concatenated the same way, no? Yes, they are the same way, you are right, so we should let add and append both be implied by the option. > > + provided, if it doesn't end in a "\n", one will be added > > + implicitly . > > Funny punctuation. My bad, will fix. Thanks very much for your detailed review.
Teng Long <dyroneteng@gmail.com> writes: >> > In addition, if a user specifies multple "-m" with "--separator", the >> > separator should be inserted between the messages too, so we use >> > OPT_STRING_LIST instead of OPT_CALLBACK_F to parse "-m" option, make >> > sure the option value of "--separator" been parsed already when we need >> > it. >> >> This is hard to grok. Is it an instruction to whoever is >> implementing this new feature, or is it an instruct to end-users >> telling that they need to give --separator before they start giving >> -m <msg>, -F <file>, -c <object>, etc.? > > No, it's not the order of the user give, but the backend we deal. > > We use "parse_msg_arg" as a callback when parsing "-m " by OPT_CALLBACK_F, > so if we have to read the separator before we parse it, so we could insert > it correctly between the messages, So I use OPT_STRING_LIST instead. That is an implementation detail of how you chose to implement the feature, and not an inherent limitation, is it? It makes a lame excuse to give users a hard-to-use UI. For example, we could parse all the command line parameters without making any action other than recording the strings given to -m and contents of files given via -F in the order they appeared on the command line, all in a single string list, while remembering the last value of --separator you got, and then at the end concatenate these strings using the final value of the separator, no?
Junio C Hamano <gitster@pobox.com> writes: > > We use "parse_msg_arg" as a callback when parsing "-m " by OPT_CALLBACK_F, > > so if we have to read the separator before we parse it, so we could insert > > it correctly between the messages, So I use OPT_STRING_LIST instead. > > That is an implementation detail of how you chose to implement the > feature, and not an inherent limitation, is it? It makes a lame > excuse to give users a hard-to-use UI. > For example, we could parse all the command line parameters without > making any action other than recording the strings given to -m and > contents of files given via -F in the order they appeared on the > command line, all in a single string list, while remembering the > last value of --separator you got, and then at the end concatenate > these strings using the final value of the separator, no? Yes, please let'me clarify it, we shouldn't to give users a hard-to-use UI, so: 1. order of "-m" and "-F" matters, it determine the order of the paragraphs (remain the same as before, which I need to fix in next patch). 2. order of "-m""-F" and "--separator" doesn't matter. Thanks.
Teng Long <dyroneteng@gmail.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> > We use "parse_msg_arg" as a callback when parsing "-m " by OPT_CALLBACK_F, >> > so if we have to read the separator before we parse it, so we could insert >> > it correctly between the messages, So I use OPT_STRING_LIST instead. >> >> That is an implementation detail of how you chose to implement the >> feature, and not an inherent limitation, is it? It makes a lame >> excuse to give users a hard-to-use UI. > >> For example, we could parse all the command line parameters without >> making any action other than recording the strings given to -m and >> contents of files given via -F in the order they appeared on the >> command line, all in a single string list, while remembering the >> last value of --separator you got, and then at the end concatenate >> these strings using the final value of the separator, no? > > Yes, please let'me clarify it, we shouldn't to give users a hard-to-use UI, so: > > 1. order of "-m" and "-F" matters, it determine the order of the paragraphs > (remain the same as before, which I need to fix in next patch). > > 2. order of "-m""-F" and "--separator" doesn't matter. > > Thanks. Sounds good.
diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt index efbc10f0..5abe6092 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,9 @@ 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 and the + message are not empty, "\n" will be inserted between them. + Use the `--separator` option to insert other delimiters. edit:: Edit the notes for a given object (defaults to HEAD). @@ -159,6 +161,12 @@ OPTIONS Allow an empty note object to be stored. The default behavior is to automatically remove empty notes. +--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 553ae2bd..524976fe 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -24,6 +24,7 @@ #include "notes-utils.h" #include "worktree.h" +static char *separator = NULL; 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,6 +210,16 @@ static void write_note_data(struct note_data *d, struct object_id *oid) } } +static void insert_separator(struct strbuf *message, size_t pos) +{ + if (!separator) + strbuf_insertstr(message, pos, "\n"); + else if (separator[strlen(separator) - 1] == '\n') + strbuf_insertstr(message, pos, separator); + else + strbuf_insertf(message, pos, "%s%s", separator, "\n"); +} + static int parse_msg_arg(const struct option *opt, const char *arg, int unset) { struct note_data *d = opt->value; @@ -567,11 +578,12 @@ static int append_edit(int argc, const char **argv, const char *prefix) const struct object_id *note; char *logmsg; const char * const *usage; + size_t message_idx; struct note_data d = { .buf = STRBUF_INIT }; + struct string_list message = STRING_LIST_INIT_DUP; struct option options[] = { - OPT_CALLBACK_F('m', "message", &d, N_("message"), - N_("note contents as a string"), PARSE_OPT_NONEG, - parse_msg_arg), + OPT_STRING_LIST('m', "message", &message, N_("message"), + N_("note contents as a string")), OPT_CALLBACK_F('F', "file", &d, N_("file"), N_("note contents in a file"), PARSE_OPT_NONEG, parse_file_arg), @@ -583,6 +595,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_("separator"), + N_("insert <separator> as separator before appending a message")), OPT_END() }; int edit = !strcmp(argv[0], "edit"); @@ -596,6 +610,15 @@ static int append_edit(int argc, const char **argv, const char *prefix) usage_with_options(usage, options); } + for (message_idx = 0; message_idx < message.nr; message_idx++) { + if (d.buf.len) + insert_separator(&d.buf, d.buf.len); + strbuf_insertstr(&d.buf, d.buf.len, + message.items[message_idx].string); + strbuf_stripspace(&d.buf, 0); + d.given = 1; + } + if (d.given && edit) fprintf(stderr, _("The -m/-F/-c/-C options have been deprecated " "for the 'edit' subcommand.\n" @@ -618,7 +641,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, 0); if (prev_buf && size) strbuf_insert(&d.buf, 0, prev_buf, size); free(prev_buf); diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh index 3288aaec..fe00497b 100755 --- a/t/t3301-notes.sh +++ b/t/t3301-notes.sh @@ -521,6 +521,64 @@ 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" && + git notes append --separator="-------$LF" -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
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 <separator> 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. * not specified --separator option: will use '\n' as the separator when do appending and this is the default behavour. * --separator='': we specify an empty separator which has the same behavour with --separator='\n' and or not specified the option. In addition, if a user specifies multple "-m" with "--separator", the separator should be inserted between the messages too, so we use OPT_STRING_LIST instead of OPT_CALLBACK_F to parse "-m" option, make sure the option value of "--separator" been parsed already when we need it. Signed-off-by: Teng Long <dyroneteng@gmail.com> --- Documentation/git-notes.txt | 12 ++++++-- builtin/notes.c | 31 +++++++++++++++++--- t/t3301-notes.sh | 58 +++++++++++++++++++++++++++++++++++++ 3 files changed, 95 insertions(+), 6 deletions(-)