Re: [PATCH v7 4/4] notes.c: don't do stripespace when parse file arg

Teng Long <dyroneteng@gmail.com> writes:

It is a fairly easy way to both implement and explain. The approach is certainly attractive from the "expectation management" point of view.

With "git notes add --no-stripspace -F file0 -F file1 ...", we may still be making the resulting note unusable as a binary file due to splicing the inter-paragraph separator, but the option name does not hint that we are giving this option to users to help them use binary contents at all, to prevent them from complaining. If it is truly useful is a separate matter, but I somehow suspect that there will be a lot more users who want "no stripspace on text contents" than those who want "binary contents in note", so it probably is a good way to go after all. I like that.

Before proceeding further on this front, we'd probably need a bit more tests that are specifically designed to catch behaviour change before and after this series with respect to stripspace behaviour. The current code without these patches calls stripspace() on the whole thing every time it receives and appends an additional piece of contents. The parse_msg_arg() function, for example, handles "-m <possibly multiline message>" by first appending a blank line (if we already have some contents) and then appending the new message, and runs stripspace on the whole thing. "-F <file>" is handled by parse_file_arg() the same way, i.e. append the file data and then stripspace the result. This can of course be optimized somewhat (e.g. an early part of the buffer may have already been cleansed by stripspace before appending new data, so as long as we do not introduce double-blanks and other things that are to be corrected at the boundary of appending new data, we shouldn't have to run stripspace on the earlier part in order to behave "as if we did a stripspace the whole thing over and over again"), but we should make sure we do not change the behaviour to cause regression. After doing something silly like this:

    cat >file1 <<-\EOF &&

    With a leading blank line, this is the second line of the file.
    And with a trailing blank line, this is the second from the last.

    EOF
    git notes add -m '
    Message from the command line.

    Two blank lines above.' -F file1    

we should see the leading blank line before "Message from the command line." removed, two blank lines from the "-m" option squeezed into one, only one blank line to appear before the contents taken from file1, and the trailing blank line removed.

Thanks.