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

[Junio C Hamano <gitster@pobox.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.

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