Re: [PATCH v5 3/3] notes.c: introduce "--separator" option

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 16, 2023, 23:22 UTC
Message-ID: <xmqq7cwhnql3.fsf@gitster.g>
In-Reply-To: <a74c96d6dd23f2f1df6d3492093f3fd27451e24c.1676551077.git.dyroneteng@gmail.com>

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.

Show 6 quoted lines

> 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")?

Show 5 quoted lines

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

Show 7 quoted lines

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

Show 8 quoted lines

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