v2, 6 partsbisect: read bisect paths with strbuf_getline()

This series deals with strbuf_getline_lf() in certain codepaths: Those, where the input that is read, is/was trimmed before doing anything that could possibly expect a CR character. Those places can be assumed to be "text" input, where a CR never would be a meaningful control character.

The purpose of this series is to document these places to have this property, by using strbuf_getline() instead of strbuf_getline_lf(). Also in some codepaths, the CR could be a leftover of an editor and is thus removed.

Every codepath was examined, if after the change it is still necessary to have trimming or if the additional CRLR-removal suffices.

The series is an idea out of [1], where Junio proposed to replace the calls to strbuf_getline_lf() because it 'would [be] a good way to document them as dealing with "text"'. Changes since v1:

* adapting the behaviour of sq_(de)quote to be a one-to-one transformation
* removing some unneccesary trimming calls in:
    * wt-status.c
    * builting/notes.c
    * builtin/clean.c
    * bisect.c

-Moritz

[1] http://thread.gmane.org/gmane.comp.version-control.git/284104

Moritz Neeb (6):
  quote: remove leading space in sq_dequote_step
  bisect: read bisect paths with strbuf_getline()
  clean: read user input with strbuf_getline()
  notes: read copied notes with strbuf_getline()
  remote: read $GIT_DIR/branches/* with strbuf_getline()
  wt-status: read rebase todolist with strbuf_getline()

 bisect.c        |  5 ++---
 builtin/clean.c | 12 +++---------
 builtin/notes.c |  3 +--
 quote.c         |  2 ++
 remote.c        |  2 +-
 wt-status.c     |  3 +--
 6 files changed, 10 insertions(+), 17 deletions(-)

Because sq_quote_argv adds a leading space (which is expected in trace.c), sq_dequote_step should remove this space again, such that the operations of quoting and dequoting are inverse of each other.

This patch is preparing the way to remove some excessive trimming operation in bisect in the following commit.

Signed-off-by: Moritz Neeb <lists@moritzneeb.de>
---
 quote.c | 2 ++
 1 file changed, 2 insertions(+)

The file BISECT_NAMES is written by "git rev-parse --sq-quote" via sq_quote_argv() when starting a bisection. It can contain pathspecs to narrow down the search. When reading it back, it should be expected that sq_dequote_to_argv_array() is able to parse this file. In fact, the previous commit ensures this.

As the content is of type "text", that means there is no logic expecting CR, strbuf_getline_lf() will be replaced by strbuf_getline().

Apart from whitespace added and removed in quote.c, no more whitespaces are expexted. While it is technically possible, we have never advertised this file to be editable by user, or encouraged them to do so, thus the call to strbuf_trim() turns obsolete in various ways.

For the case that this file is modified nonetheless, in an invalid way such that dequoting fails, the error message is broadened to both cases: bad quoting and unexpected whitespace.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Moritz Neeb <lists@moritzneeb.de>
---
 bisect.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

The notes are copied from stdin. They should only contain SHA1s... Not spaces. CR could be there, because the file/the data from stdin could have been written via an editor that adds them.

The notes that are copied from stdin are trimmed with strbuf_rtrim() after splitting by ' '. There is thus no logic expecting CR, so strbuf_getline_lf() can be replaced by its CRLF counterpart.

Signed-off-by: Moritz Neeb <lists@moritzneeb.de>
---
 builtin/notes.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

On Sun, Feb 21, 2016 at 8:16 PM, Moritz Neeb <lists@moritzneeb.de> wrote:

Given the commit message, I understand that this rtrim is effectively redundant, thus can be dropped, however, I'm not sure that doing so improves the code since the reader now has to think extra hard to understand the asymmetry of only trimming split[0] (and that understanding may require blaming this code in order to consult the commit message).

A deeper issue not touched upon by the commit message (but which should be) is that that strbuf_split() leaves the "terminator" (space, in this case) on the component strings, and that is why split[0] must be rtrim'd. Rather than dropping only one of the rtrim's, a cleaner approach might be to convert the code to use string_list_split() which doesn't have the "odd" behavior of leaving the terminator on the split strings, in which case both rtrim's could be retired. This, of course, would be done as a separate preparatory patch.

Eric Sunshine <sunshine@sunshineco.com> writes:

Yeah, this is a good point to raise.

Thanks.

In read_rebase_todolist() the files $GIT_DIR/rebase-merge/done and $GIT_DIR/rebase-merge/git-rebase-todo are read to collect status information.

The access to this file should always happen via git rebase, e.g. via "git rebase -i" or "git rebase --edit-todo". We can assume, that this interface handles the preprocessing of whitespaces, especially CRLFs correctly. Thus in this codepath we can remove the call to strbuf_trim().

For documenting the input as expecting "text" input, strbuf_getline_lf() is still replaced by strbuf_getline().

Signed-off-by: Moritz Neeb <lists@moritzneeb.de>
---
 wt-status.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Moritz Neeb <lists@moritzneeb.de> writes:

Not related to the substance of the patch series at all, but all except for this patch in the series seem to be corrupt in that the very first line that is removed in each patch has an extra space before the '-' deletion sign. It is a very curious symptom. Please double check the way you send out patch e-mails (e.g. send them first only to yourself and then try to apply them with "git am").

Thanks.

The inputs that are read are all answers that are given by the user when interacting with git on the commandline. As these answers are not supposed to contain a meaningful CR it is safe to replace strbuf_getline_lf() can be replaced by strbuf_getline().

Before the user input was trimmed to remove the CR. This would be now redundant. Another effect of the trimming was that some (accidentally) typed spaces were filtered. But here we want to be consistent with similar UIs like interactive adding, which only accepts space-less input.

For the case of filtering by patterns the input is still trimmed in an untouched codepath after it is split up into multiple patterns. This is considered as desirable, because of two reasons: First this fitering is not part of similar UIs and it is way more likely to accidentally type a space in this way of interacting.

Signed-off-by: Moritz Neeb <lists@moritzneeb.de>
---
When playing around with the interactive git clean I noticed that it is
not possible to have a pattern actually containing a space (i.e. escaping it).
Not sure how relevant this is, because I have no feeling how good the support
and demand for smoothly handling space-containing files in git is.

 builtin/clean.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

On Sun, Feb 21, 2016 at 8:20 PM, Moritz Neeb <lists@moritzneeb.de> wrote:

I don't at all insist upon it, but this behavior change feels somewhat like it ought to be in its own commit. I'm also not convinced that making this consistent with the less forgiving behavior of "interactive adding" is desirable (rather the reverse: that that case should be more flexible). However, I wasn't following the discussion with Junio closely, and perhaps missed you two agreeing that this is preferable.

s/, because of/ for/

On 02/22/2016 03:27 AM, Eric Sunshine wrote:

You're right, two commits would be nicer. I was also thinking about splitting up the three codepaths, but I decided all of the clean-interaction belongs together.

To summarize the discussion with Junio: We were not directly talking about that. Two aspects from the whole discussion were that I should decide something and justify a stance (which I did) and that it's also beneficial to think aloud (which I forgot).

In fact, I was surprised, that interactive adding is that strict. I should've added that to the discussion. I am at the moment sometimes unsure whether I find things weird because git standards are different than what I'd expect or because things really should be changed. So I went for the former and decided to go for consistency with the base. I'd expect, from my own behaviour, interactive adding is used by far more than interactive cleaning, which might be an argument to adapt the latter.

But now that we're discussing this, I don't really see a benefit from the user perspective, it's more code cleanup.

Eric Sunshine <sunshine@sunshineco.com> writes:

There was no such discussion ;-)

I am not 100% sure if we want to be lenient in reading "yes, please remove this one", but if we already are loose, I tend to agree that there is not much point tightening it, especially with a clean-up topic like this one.

Thanks.

The line read from the branch file is directly trimmed after reading with strbuf_trim(). There is thus no logic expecting CR, so strbuf_getline_lf() can be replaced by its CRLF counterpart.

Signed-off-by: Moritz Neeb <lists@moritzneeb.de>
---
To be honest, I did not yet fully understand the purpose of this branches/ file.
What I'd expect is that it is some intermediary file while fetching?
Or is it edited directly by the user and thus it's necessary to strip spaces
that could be added accidentally?

 remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Moritz Neeb <lists@moritzneeb.de> writes:

[Documentation/gitrepository-layout.txt]

branches::
	A slightly deprecated way to store shorthands to be used
	to specify a URL to 'git fetch', 'git pull' and 'git push'.
	A file can be stored as `branches/<name>` and then
	'name' can be given to these commands in place of
	'repository' argument.  See the REMOTES section in
	linkgit:git-fetch[1] for details.  This mechanism is legacy
	and not likely to be found in modern repositories. This
	directory is ignored if $GIT_COMMON_DIR is set and
	"$GIT_COMMON_DIR/branches" will be used instead.