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

From: Teng Long <dyroneteng@gmail.com>
Date: Feb 20, 2023, 14:00 UTC
Message-ID: <20230220140046.16986-1-tenglong.tl@alibaba-inc.com>
In-Reply-To: <xmqq7cwhnql3.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> wrote on Thu, 16 Feb 2023 15:22:16 -0800:

Show 7 quoted lines

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

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

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.

Show 17 quoted lines

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

Show 6 quoted lines

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

Show 6 quoted lines

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

Show 8 quoted lines

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

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

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.

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

Agree, you also mentioned me that, does git-notes-add need to be implies by this option too? I think it needs too.

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

Will apply.

Show 8 quoted lines

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

Show 6 quoted lines

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