Replacing strbuf_getline_lf() by strbuf_getline() on trimmed input

This series deals with strbuf_getline_lf() in certain codepaths:
Those, where the input that is read, is trimmed before doing anything that could possibly expect a CR character. As the codepath after trimming can not in any way expect a CR at the end of the line, because trim dropped it before the change, this can be safely done.

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"'.
Apart from a general review, I would be happy about comments how I can improve this (and future) patches, as this is my first one. Especially I am not sure about how many arguments from the discussion on the mailing list should be put into the commits.

-Moritz

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

Moritz Neeb (5):
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        | 2 +-
 builtin/clean.c | 6 +++---
 builtin/notes.c | 2 +-
 remote.c        | 2 +-
 wt-status.c     | 2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)

2.4.3

The lines read from BISECT_NAMES are trimmed with strbuf_trim() immediately. There is thus no logic expecting CR, so strbuf_getline_lf() can be replaced by its CRLF counterpart.

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

index 06ec54e..bf7c885 100644
@@ -440,7 +440,7 @@ static void read_bisect_paths(struct argv_array *array)
 	if (!fp)
 		die_errno("Could not open file '%s'", filename);
 -	while (strbuf_getline_lf(&str, fp) != EOF) {
+	while (strbuf_getline(&str, fp) != EOF) {
 		strbuf_trim(&str);
 		if (sq_dequote_to_argv_array(str.buf, array))
 			die("Badly quoted content in file '%s': %s",

2.4.3

We do not indent the whole log message.

You would also want to think about the necessity of strbuf_trim() here. Now strbuf_getline() would trim the trailing CR, would we still need to call strbuf_trim() here? The code will break if you just remove the call, but on the other hand, you will realize that the trimming done by calling it is excessive and unnecessary, once you inspect the code and learn who writes the file being read here and how.

Thanks for spotting, I will change that in the next iteration.

I am not sure what you mean by excessive: How much can I assume that the input is like expected? The files we are talking about are supposed to be read and written by git only. But could be modified in theory with an editor, right? Then things could break, right? This question maybe holds true for the other patches as well, I still have to look into them.

Assuming that things are just like expected, what has to be done in read_bisect_paths before dequoting is to remove the leading space, which is added by 'git rev-parse --sq-quote "$@"' in git-bisect.sh.

I am just not sure, where to remove it:
* in sq_quote_argv? - see some analysis below.
* in read_bisect_paths via strbuf_ltrim instead of strbuf_trim, is that what you meant?
* in sq_dequote_argv_array? - see below as well.

I tried to understand, why the space is added by sq_quote_argv() in quotes.c in the first place. It is there since the beginning in 7cf6720. I have the feeling that the space is redundant for bisect, because the only other codepath where BISECT_NAMES is read is in bisect_visualize(), where the necessary space to separate the arguments is added before '$(...)':

eval '"$@"' --bisect -- $(cat "$GIT_DIR/BISECT_NAMES")

Other places where sq_quote_argv() and sq_dequote_to_argv_array() are called as well are builtin/am.c and trace.c.

In builtin/am.c when reading 'apply-opt', the data is trimmed in read_state_file, so the space in the beginning is superfluous.

Looking at trace.c it gets clear that this code is dependent on this space, otherwise the trace output will look like "trace: built-in: git'status'" which is obviously bogus. It would maybe make more sense to add this space directly there than to trim it in all the other places. I realized that this code came up in 7cf6720 as well which explains why things are like they are (and why I'm CC'ing Christian Couder).

Another option, at the moment my favourite because I think it's the least invasive, would be to "trim" this one space directly in sq_dequote_step(). See the patch below. This would not affect the tracing functionality, because there is no dequoting used.

-- >8 --
Subject: [PATCH] quote: remove leading space in sq_dequote_step

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.

---
quote.c | 2 ++

1 file changed, 2 insertions(+)

index fe884d2..2714f27 100644
@@ -63,6 +63,8 @@ static char *sq_dequote_step(char *arg, char **next)

char *src = arg;
char c;

+ if (*src == ' ')
+ src++;
if (*src != '\'')
return NULL;
for (;;) {

2.4.3

These are all good questions you as a Git contributor to be asking yourself, and I really like the fact that you are thinking aloud here.

As to bisect pathspec, while I do not think it is sensible to change it in the middle of bisection, I do not think it would cause the bisect command to produce an incorrect result if you did so. You would be changing the definition of what is the "middle point" of the history on the fly by changing the pathspec, hence you may end up making the bisection suboptimal, but it shouldn't affect the correctness of the bisection.

So I tend to agree with you that it is a good idea to be prepared to parse what the end user may have futzed with the editor, as long as the user did not break the syntax of the quoted string.

Your answer to the question quoted at the top of this message could be a summary of the above reasoning, with a conclusion: "we would want to tolerate trailing (or leading) whitespaces the user might add with his editor; even though we use strbuf_getline() to remove the CR at the end of line, we still need to strbuf_trim() the line we read".

Side note: of course, you can instead say "while it is technically possible, we have never advertised this file to be editable by user, or encouraged them to do so" and tighten the parsing to assume only what we write out, erroring out when we see any attempt to edit it.

Either way, the most important thing is to record the reasoning behind the design choice you made clearly in the proposed commit log message.

Thanks.

Thanks for this feedback I will try to continue this thinking aloud whenever appropriate.

Sorry by the way for only answering every week - I currently only have time to work on git during the weekend. What is (assuming familiarity with the codebase) the expected "cooking"-time of a smaller patch? I'm sure the answer is "depends", but maybe you can give away some experiences.

Yep I also think it would not affect the correctness. To reformulate your explanation: changing the pathspec does not change the good and bad "frontiers". And those should be still correct under the assumption you're still looking for the same bug/regression.

As to the question if it is sensible, I also don't really think it could be helpful. The only way I would expect it to be useful, it so "refine" the search by adding specific files to the filepath. But then it's possible to end up in a dead-end if the commits left are not touching the specified files with:

No testable commit found.
Maybe you started with bad path parameters?

I have also seen other messed up messages, like "-1 commits left" while playing around.

Now I am not convinced anymore, because I think if it potentially leads to troubles, it should not be made easier to get oneself into them.

I will include something like that in the patch v2 that is to follow soon.

The input that is read is trimmed with strbuf_trim() immediately. There is thus no logic expecting CR, so strbuf_getline_lf() can be replaced by its CRLF counterpart.

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

index 7b08237..956283d 100644
@@ -570,7 +570,7 @@ static int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff)
 			       clean_get_color(CLEAN_COLOR_RESET));
 		}
 -		if (strbuf_getline_lf(&choice, stdin) != EOF) {
+		if (strbuf_getline(&choice, stdin) != EOF) {
 			strbuf_trim(&choice);
 		} else {
 			eof = 1;

@@ -652,7 +652,7 @@ static int filter_by_patterns_cmd(void)
 		clean_print_color(CLEAN_COLOR_PROMPT);
 		printf(_("Input ignore patterns>> "));
 		clean_print_color(CLEAN_COLOR_RESET);
-		if (strbuf_getline_lf(&confirm, stdin) != EOF)
+		if (strbuf_getline(&confirm, stdin) != EOF)
 			strbuf_trim(&confirm);
 		else
 			putchar('\n');

@@ -750,7 +750,7 @@ static int ask_each_cmd(void)
 			qname = quote_path_relative(item->string, NULL, &buf);
 			/* TRANSLATORS: Make sure to keep [y/N] as is */
 			printf(_("Remove %s [y/N]? "), qname);
-			if (strbuf_getline_lf(&confirm, stdin) != EOF) {
+			if (strbuf_getline(&confirm, stdin) != EOF) {
 				strbuf_trim(&confirm);
 			} else {
 				putchar('\n');

2.4.3

The same comment (including "think if this trim is still necessary") as 1/5 applies here.

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.

---
 builtin/notes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index ed6f222..bbd7544 100644
@@ -290,7 +290,7 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
 		t = &default_notes_tree;
 	}
 -	while (strbuf_getline_lf(&buf, stdin) != EOF) {
+	while (strbuf_getline(&buf, stdin) != EOF) {
 		unsigned char from_obj[20], to_obj[20];
 		struct strbuf **split;
 		int err;

2.4.3

Same comment as 1/5 and 2/5 applies. You need to think if strbuf_rtim(split[1]) is necessary here.

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.

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

index 7d61dab..78f97f0 100644
@@ -281,7 +281,7 @@ static void read_branches_file(struct remote *remote)
 	if (!f)
 		return;
 -	strbuf_getline_lf(&buf, f);
+	strbuf_getline(&buf, f);
 	fclose(f);
 	strbuf_trim(&buf);
 	if (!buf.len) {

2.4.3

In read_rebase_todolist() every line is, after reading, checked for a comment_line_char. After that it is trimmed via strbuf_trim(). Assuming we do never expect a CR as comment_line_char, strbuf_getline_lf() can be safely replaced by its CRLF counterpart.

---

Notes:
Looking at the code in read_rebase_todolist(), the
lines are read, checked for a comment_line_char and then trimmed. I wonder why the input is not trimmed before checking for this character? Is it safe to replace strbuf_getline_lf() by strbuf_getline() anyway?
The only case I can imagine that could lead to unexpected behaviour then
would be when someone sets the comment_line_char to CR. How likely is that?
Why is the trim _after_ checking for the comment char anyway? Should
something like " # foobar" not be considered as comment?
I decided to roll out the patch because I considered it not as a risk to be
taken seriously, that the comment line char will be '\r'.
Meta-question: Should something of this discussion be put into the commit?

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

index ab4f80d..f053b2f 100644
@@ -1076,7 +1076,7 @@ static void read_rebase_todolist(const char *fname, struct string_list *lines)
 	if (!f)
 		die_errno("Could not open file %s for reading",
 			  git_path("%s", fname));
-	while (!strbuf_getline_lf(&line, f)) {
+	while (!strbuf_getline(&line, f)) {
 		if (line.len && line.buf[0] == comment_line_char)
 			continue;
 		strbuf_trim(&line);

2.4.3

Yes to the meta question. Add something like this as the second paragraph of the log message, but do *not* change the patch text.

The current code checks if the line begins with a comment line char (typically '#') before trimming, which is consistent with the way how commands like 'git commit' prepares commented lines in that this does not treat a line that begins with whitespaces and '#' as commented out. We however made a mistake in the parser of "git rebase -i" long time ago to allow such a line to be treated as a comment, and made an exception with 1db168ee (rebase-i: loosen over-eager check_bad_cmd check, 2015-10-01). It probably is a good idea to match that exception by swapping the order of comment check and trimming in this codepath as well.

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(-)

2.7.1.345.gc14003e

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.

---
 quote.c | 2 ++
 1 file changed, 2 insertions(+)

index fe884d2..2714f27 100644
@@ -63,6 +63,8 @@ static char *sq_dequote_step(char *arg, char **next)
 	char *src = arg;
 	char c;
 +	if (*src == ' ')
+		src++;
 	if (*src != '\'')
 		return NULL;
 	for (;;) {

2.7.1.345.gc14003e

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.

---
 bisect.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

index 06ec54e..e2df02f 100644
@@ -440,10 +440,9 @@ static void read_bisect_paths(struct argv_array *array)
 	if (!fp)
 		die_errno("Could not open file '%s'", filename);
 -	while (strbuf_getline_lf(&str, fp) != EOF) {
-		strbuf_trim(&str);
+	while (strbuf_getline(&str, fp) != EOF) {
 		if (sq_dequote_to_argv_array(str.buf, array))
-			die("Badly quoted content in file '%s': %s",
+			die("Badly quoted content or unexpected whitespace in file '%s': %s",
 			    filename, str.buf);
 	}
 -- 2.7.1.345.gc14003e

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.

---
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(-)

index 7b08237..01cc2ff 100644
@@ -570,9 +570,7 @@ static int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff)
 			       clean_get_color(CLEAN_COLOR_RESET));
 		}
 -		if (strbuf_getline_lf(&choice, stdin) != EOF) {
-			strbuf_trim(&choice);
-		} else {
+		if (strbuf_getline(&choice, stdin) == EOF) {
 			eof = 1;
 			break;
 		}

@@ -652,9 +650,7 @@ static int filter_by_patterns_cmd(void)
 		clean_print_color(CLEAN_COLOR_PROMPT);
 		printf(_("Input ignore patterns>> "));
 		clean_print_color(CLEAN_COLOR_RESET);
-		if (strbuf_getline_lf(&confirm, stdin) != EOF)
-			strbuf_trim(&confirm);
-		else
+		if (strbuf_getline(&confirm, stdin) == EOF)
 			putchar('\n');
  		/* quit filter_by_pattern mode if press ENTER or Ctrl-D */

@@ -750,9 +746,7 @@ static int ask_each_cmd(void)
 			qname = quote_path_relative(item->string, NULL, &buf);
 			/* TRANSLATORS: Make sure to keep [y/N] as is */
 			printf(_("Remove %s [y/N]? "), qname);
-			if (strbuf_getline_lf(&confirm, stdin) != EOF) {
-				strbuf_trim(&confirm);
-			} else {
+			if (strbuf_getline(&confirm, stdin) == EOF) {
 				putchar('\n');
 				eof = 1;
 			}

2.7.1.345.gc14003e

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/

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.

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

---
 builtin/notes.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index ed6f222..706ec11 100644
@@ -290,7 +290,7 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
 		t = &default_notes_tree;
 	}
 -	while (strbuf_getline_lf(&buf, stdin) != EOF) {
+	while (strbuf_getline(&buf, stdin) != EOF) {
 		unsigned char from_obj[20], to_obj[20];
 		struct strbuf **split;
 		int err;

@@ -299,7 +299,6 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
 		if (!split[0] || !split[1])
 			die(_("Malformed input line: '%s'."), buf.buf);
 		strbuf_rtrim(split[0]);
-		strbuf_rtrim(split[1]);
 		if (get_sha1(split[0]->buf, from_obj))
 			die(_("Failed to resolve '%s' as a valid ref."), split[0]->buf);
 		if (get_sha1(split[1]->buf, to_obj))

2.7.1.345.gc14003e

Yeah, this is a good point to raise.

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.

---
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(-)

index 02e698a..aaff6aa 100644
@@ -281,7 +281,7 @@ static void read_branches_file(struct remote *remote)
 	if (!f)
 		return;
 -	strbuf_getline_lf(&buf, f);
+	strbuf_getline(&buf, f);
 	fclose(f);
 	strbuf_trim(&buf);
 	if (!buf.len) {

2.7.1.345.gc14003e

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

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

---
 wt-status.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index ab4f80d..8047cf2 100644
@@ -1076,10 +1076,9 @@ static void read_rebase_todolist(const char *fname, struct string_list *lines)
 	if (!f)
 		die_errno("Could not open file %s for reading",
 			  git_path("%s", fname));
-	while (!strbuf_getline_lf(&line, f)) {
+	while (!strbuf_getline(&line, f)) {
 		if (line.len && line.buf[0] == comment_line_char)
 			continue;
-		strbuf_trim(&line);
 		if (!line.len)
 			continue;
 		abbrev_sha1_in_line(&line);

2.7.1.345.gc14003e

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 CRLF-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 v2:

* Line splitting in notes_copy_from_stdin() is changed to string_list_split as suggested by Eric Sunshine.
* The behavior change in interactive cleaning from patch v2 is undone.
* Some of the previous patches were broken because of some unexpected whitespace. This should be fixed now.

-Moritz

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

Moritz Neeb (7):
quote: remove leading space in sq_dequote_step
bisect: read bisect paths with strbuf_getline()
clean: read user input with strbuf_getline() notes copy --stdin: split lines with string_list_split() notes copy --stdin: read lines 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 |  6 +++---
 builtin/notes.c | 23 +++++++++++------------
 quote.c         |  2 ++
 remote.c        |  2 +-
 wt-status.c     |  3 +--
 6 files changed, 20 insertions(+), 21 deletions(-)

2.7.1.346.gc0ef946

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.

---
 quote.c | 2 ++
 1 file changed, 2 insertions(+)

index fe884d2..2714f27 100644
@@ -63,6 +63,8 @@ static char *sq_dequote_step(char *arg, char **next)
 	char *src = arg;
 	char c;
 
+	if (*src == ' ')
+		src++;
 	if (*src != '\'')
 		return NULL;
 	for (;;) {

2.4.3

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.

---
 bisect.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

index 06ec54e..e2df02f 100644
@@ -440,10 +440,9 @@ static void read_bisect_paths(struct argv_array *array)
 	if (!fp)
 		die_errno("Could not open file '%s'", filename);
 
-	while (strbuf_getline_lf(&str, fp) != EOF) {
-		strbuf_trim(&str);
+	while (strbuf_getline(&str, fp) != EOF) {
 		if (sq_dequote_to_argv_array(str.buf, array))
-			die("Badly quoted content in file '%s': %s",
+			die("Badly quoted content or unexpected whitespace in file '%s': %s",
 			    filename, str.buf);
 	}
 

2.4.3

s/expexted/expected/

Not sure what "various ways" you mean. Perhaps say instead that (as a consequence of "not advertised or encouraged") you're tightening the parsing of this file by removing strbuf_trim().

Yeah that doesn't make sense. What I meant "it's neither needed for CR-removal, nor for removal of other potential whitespace". I will rewrite the paragraph to:

Apart from whitespace added and removed in quote.c, no other whitespaces are expected. While it is technically possible, we have never advertised this file to be editable by user, or encouraged them to do so. As a consequence, the parsing of BISECT_NAMES is tightened by removing strbuf_trim().

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

In the subsequent codepath, the input is trimmed. This leads to accepting user input with spaces, e.g. " y ", as a valid answer in the interactive cleaning process.

Although trimming would not be required anymore to remove a potential CR, we don't want to change the existing behavior with this patch. Thus, the trimming is kept in place.

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

index 7b08237..956283d 100644
@@ -570,7 +570,7 @@ static int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff)
 			       clean_get_color(CLEAN_COLOR_RESET));
 		}
 
-		if (strbuf_getline_lf(&choice, stdin) != EOF) {
+		if (strbuf_getline(&choice, stdin) != EOF) {
 			strbuf_trim(&choice);
 		} else {
 			eof = 1;

@@ -652,7 +652,7 @@ static int filter_by_patterns_cmd(void)
 		clean_print_color(CLEAN_COLOR_PROMPT);
 		printf(_("Input ignore patterns>> "));
 		clean_print_color(CLEAN_COLOR_RESET);
-		if (strbuf_getline_lf(&confirm, stdin) != EOF)
+		if (strbuf_getline(&confirm, stdin) != EOF)
 			strbuf_trim(&confirm);
 		else
 			putchar('\n');

@@ -750,7 +750,7 @@ static int ask_each_cmd(void)
 			qname = quote_path_relative(item->string, NULL, &buf);
 			/* TRANSLATORS: Make sure to keep [y/N] as is */
 			printf(_("Remove %s [y/N]? "), qname);
-			if (strbuf_getline_lf(&confirm, stdin) != EOF) {
+			if (strbuf_getline(&confirm, stdin) != EOF) {
 				strbuf_trim(&confirm);
 			} else {
 				putchar('\n');

2.4.3

Grammo: "it is safe to replace ... can be replaced by ..."

How about?

After being read, the input is trimmed.

I dropped the second duplication.

Yep, sounds better.

Thanks.

This patch changes, how the lines are split, when reading them from stdin to copy the notes. The advantage of string_list_split() over strbuf_split() is that it removes the terminator, making trimming of the left part unneccesary.

The strbuf is now rtrimmed before splitting. This is still required to remove potential CRs. In the next step this will then be done implicitly by strbuf_readline(). Thus, this is a preparatory refactoring, towards a trim-free codepath.

---
 builtin/notes.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

index ed6f222..22909c7 100644
@@ -292,18 +292,18 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
 
 	while (strbuf_getline_lf(&buf, stdin) != EOF) {
 		unsigned char from_obj[20], to_obj[20];
-		struct strbuf **split;
+		struct string_list split = STRING_LIST_INIT_DUP;
 		int err;
 
-		split = strbuf_split(&buf, ' ');
-		if (!split[0] || !split[1])
+		strbuf_rtrim(&buf);
+		string_list_split(&split, buf.buf, ' ', -1);
+
+		if (split.nr != 2)
 			die(_("Malformed input line: '%s'."), buf.buf);
-		strbuf_rtrim(split[0]);
-		strbuf_rtrim(split[1]);
-		if (get_sha1(split[0]->buf, from_obj))
-			die(_("Failed to resolve '%s' as a valid ref."), split[0]->buf);
-		if (get_sha1(split[1]->buf, to_obj))
-			die(_("Failed to resolve '%s' as a valid ref."), split[1]->buf);
+		if (get_sha1(split.items[0].string, from_obj))
+			die(_("Failed to resolve '%s' as a valid ref."), split.items[0].string);
+		if (get_sha1(split.items[1].string, to_obj))
+			die(_("Failed to resolve '%s' as a valid ref."), split.items[1].string);
 
 		if (rewrite_cmd)
 			err = copy_note_for_rewrite(c, from_obj, to_obj);

@@ -313,11 +313,11 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
 
 		if (err) {
 			error(_("Failed to copy notes from '%s' to '%s'"),
-			      split[0]->buf, split[1]->buf);
+			      split.items[0].string, split.items[1].string);
 			ret = 1;
 		}
 
-		strbuf_list_free(split);
+		string_list_clear(&split, 0);
 	}
 
 	if (!rewrite_cmd) {

2.4.3

Here's an alternate commit message:

strbuf_split() has the unfortunate behavior of leaving the separator character on the end of the split components, thus placing the burden of manually removing the separator on the caller. It's also heavyweight in that each split component is a full-on strbuf. We need neither feature of strbuf_split() so let's use string_list_split() instead since it removes the separator character and returns an array of simple NUL-terminated strings.

I would actually swap patches 4 and 5 so that strbuf_getline() is done first (without removing any of the rtrim's) and string_list_split() second. That way, you don't have to add that extra rtrim in one patch and immediately remove it in the next. And, as a bonus, you can drop the above paragraph altogether.

The patch itself looks okay.

Yeah, I also was thinking about that, should've pointed that out. I was just following your "guiding" in v2 [1], that's why I did it this way, because I thought it is somehow expected to be a prepraratory change.

Ok, when switching 4 and 5, I could call it something like "post cleanup/refactoring" instead.

[1] http://article.gmane.org/gmane.comp.version-control.git/286868

Indeed, I meant to add a footnote acknowledging that and saying something along the lines of: "Despite suggesting this as a preparatory change in my v2 review, having now seen actually seen it, it makes more sense as a follow-on change."

The format of a line that is expected when copying notes via stdin is "sha1 sha1". As this is text-only, strbuf_getline() can be used instead of strbuf_getline_lf().

When reading with strbuf_getline() the trimming can be removed. It was necessary before to remove potential CRs inserted through a dos editor.

---
 builtin/notes.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 22909c7..660c0b7 100644
@@ -290,12 +290,11 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
 		t = &default_notes_tree;
 	}
 
-	while (strbuf_getline_lf(&buf, stdin) != EOF) {
+	while (strbuf_getline(&buf, stdin) != EOF) {
 		unsigned char from_obj[20], to_obj[20];
 		struct string_list split = STRING_LIST_INIT_DUP;
 		int err;
 
-		strbuf_rtrim(&buf);
 		string_list_split(&split, buf.buf, ' ', -1);
 
 		if (split.nr != 2)

2.4.3

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.

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

index 02e698a..aaff6aa 100644
@@ -281,7 +281,7 @@ static void read_branches_file(struct remote *remote)
 	if (!f)
 		return;
 
-	strbuf_getline_lf(&buf, f);
+	strbuf_getline(&buf, f);
 	fclose(f);
 	strbuf_trim(&buf);
 	if (!buf.len) {

2.4.3

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

---
 wt-status.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index ab4f80d..8047cf2 100644
@@ -1076,10 +1076,9 @@ static void read_rebase_todolist(const char *fname, struct string_list *lines)
 	if (!f)
 		die_errno("Could not open file %s for reading",
 			  git_path("%s", fname));
-	while (!strbuf_getline_lf(&line, f)) {
+	while (!strbuf_getline(&line, f)) {
 		if (line.len && line.buf[0] == comment_line_char)
 			continue;
-		strbuf_trim(&line);
 		if (!line.len)
 			continue;
 		abbrev_sha1_in_line(&line);

2.4.3

What I could not yet develop, is a feeling on "how often" to send out new versions of patches. For a small patch like this it feels a bit weird to send out version after version, but maybe that is a good practice? Now that I have implemented some (mainly textual) improvements that came up through the review by Eric, would I rather send out a v4 or wait for comments by other reviewers?

Thanks

In the future, as an aid to reviewers, please include an interdiff since the previous version, as well a link to the previous round[1]. It's also very helpful to say which patches have changed (and which have not).

Thanks.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/285118/focus=286865

Maybe not too late for other reviewers, here comes the interdiff (this assumes the non-broken version 2):

index 18b6056..5b17a31 100644
@@ -570,7 +570,9 @@ static int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff)
 			       clean_get_color(CLEAN_COLOR_RESET));
 		}
 
-		if (strbuf_getline(&choice, stdin) == EOF) {
+		if (strbuf_getline(&choice, stdin) != EOF) {
+			strbuf_trim(&choice);
+		} else {
 			eof = 1;
 			break;
 		}

@@ -650,7 +652,9 @@ static int filter_by_patterns_cmd(void)
 		clean_print_color(CLEAN_COLOR_PROMPT);
 		printf(_("Input ignore patterns>> "));
 		clean_print_color(CLEAN_COLOR_RESET);
-		if (strbuf_getline(&confirm, stdin) == EOF)
+		if (strbuf_getline(&confirm, stdin) != EOF)
+			strbuf_trim(&confirm);
+		else
 			putchar('\n');
 
 		/* quit filter_by_pattern mode if press ENTER or Ctrl-D */

@@ -746,7 +750,9 @@ static int ask_each_cmd(void)
 			qname = quote_path_relative(item->string, NULL, &buf);
 			/* TRANSLATORS: Make sure to keep [y/N] as is */
 			printf(_("Remove %s [y/N]? "), qname);
-			if (strbuf_getline(&confirm, stdin) == EOF) {
+			if (strbuf_getline(&confirm, stdin) != EOF) {
+				strbuf_trim(&confirm);
+			} else {
 				putchar('\n');
 				eof = 1;
 			}

index 706ec11..660c0b7 100644
@@ -292,17 +292,17 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
 
 	while (strbuf_getline(&buf, stdin) != EOF) {
 		unsigned char from_obj[20], to_obj[20];
-		struct strbuf **split;
+		struct string_list split = STRING_LIST_INIT_DUP;
 		int err;
 
-		split = strbuf_split(&buf, ' ');
-		if (!split[0] || !split[1])
+		string_list_split(&split, buf.buf, ' ', -1);
+
+		if (split.nr != 2)
 			die(_("Malformed input line: '%s'."), buf.buf);
-		strbuf_rtrim(split[0]);
-		if (get_sha1(split[0]->buf, from_obj))
-			die(_("Failed to resolve '%s' as a valid ref."), split[0]->buf);
-		if (get_sha1(split[1]->buf, to_obj))
-			die(_("Failed to resolve '%s' as a valid ref."), split[1]->buf);
+		if (get_sha1(split.items[0].string, from_obj))
+			die(_("Failed to resolve '%s' as a valid ref."), split.items[0].string);
+		if (get_sha1(split.items[1].string, to_obj))
+			die(_("Failed to resolve '%s' as a valid ref."), split.items[1].string);
 
 		if (rewrite_cmd)
 			err = copy_note_for_rewrite(c, from_obj, to_obj);

@@ -312,11 +312,11 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
 
 		if (err) {
 			error(_("Failed to copy notes from '%s' to '%s'"),
-			      split[0]->buf, split[1]->buf);
+			      split.items[0].string, split.items[1].string);
 			ret = 1;
 		}
 
-		strbuf_list_free(split);
+		string_list_clear(&split, 0);
 	}
 
 	if (!rewrite_cmd) {

Although I was not sure [4], I decided to roll out v4, in the hope that the next reviewers will profit by the more polished commit messages and order.

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 CRLF-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 v3 [3] (the changes to single patches are indicated below):

* Commit messages refined
* Order of patch 4 and 5 in v2 was switched.

The interdiff only removes an empty line (I noticed, when changing the order of commits, that the splitting operation had no newline before this whole series, so I left it that way):

index 660c0b7..715fade 100644
@@ -296,7 +296,6 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
         int err;
 
         string_list_split(&split, buf.buf, ' ', -1);
-
         if (split.nr != 2)
             die(_("Malformed input line: '%s'."), buf.buf);
         if (get_sha1(split.items[0].string, from_obj))

-Moritz

[1], idea: http://thread.gmane.org/gmane.comp.version-control.git/284104
[2], v2: http://thread.gmane.org/gmane.comp.version-control.git/285118/focus=286865
[3], v3: http://thread.gmane.org/gmane.comp.version-control.git/285118/focus=287747
[4] http://thread.gmane.org/gmane.comp.version-control.git/285118/focus=287760

Moritz Neeb (7):
quote: remove leading space in sq_dequote_step -- as in v2
bisect: read bisect paths with strbuf_getline() -- refined commit message
clean: read user input with strbuf_getline() -- simplified commit message notes copy --stdin: read lines with strbuf_getline() -- switched with below notes copy --stdin: split lines with string_list_split() -- switched with above
remote: read $GIT_DIR/branches/* with strbuf_getline() -- as in v3
wt-status: read rebase todolist with strbuf_getline() -- as in v2

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

2.4.3

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.

---
 quote.c | 2 ++
 1 file changed, 2 insertions(+)

index fe884d2..2714f27 100644
@@ -63,6 +63,8 @@ static char *sq_dequote_step(char *arg, char **next)
 	char *src = arg;
 	char c;
 
+	if (*src == ' ')
+		src++;
 	if (*src != '\'')
 		return NULL;
 	for (;;) {

2.4.3

My intention was to explicitly remove the space added by strbuf_addch(dst, ' ') in sq_quote_argv().

I think it would not make sense to remove more spaces, because for for sq_dequote() it is defined:

This unwraps what sq_quote() produces in place, but returns NULL if the input does not look like what sq_quote would have produced.

I understand that this counts also for the sq_dequote_array*() family.

Thanks

s/array/to_argv/

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 other whitespaces are expected. While it is technically possible, we have never advertised this file to be editable by user, or encouraged them to do so. As a consequence, the parsing of BISECT_NAMES is tightened by removing strbuf_trim().

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.

---
 bisect.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

index 7996c29..f63aa10 100644
@@ -440,10 +440,9 @@ static void read_bisect_paths(struct argv_array *array)
 	if (!fp)
 		die_errno("Could not open file '%s'", filename);
 
-	while (strbuf_getline_lf(&str, fp) != EOF) {
-		strbuf_trim(&str);
+	while (strbuf_getline(&str, fp) != EOF) {
 		if (sq_dequote_to_argv_array(str.buf, array))
-			die("Badly quoted content in file '%s': %s",
+			die("Badly quoted content or unexpected whitespace in file '%s': %s",
 			    filename, str.buf);
 	}
 

2.4.3

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() by strbuf_getline().

After being read, the input is trimmed. This leads to accepting user input with spaces, e.g. " y ", as a valid answer in the interactive cleaning process.

Although trimming would not be required anymore to remove a potential CR, we don't want to change the existing behavior with this patch. Thus, the trimming is kept in place.

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

index 0371010..5b17a31 100644
@@ -570,7 +570,7 @@ static int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff)
 			       clean_get_color(CLEAN_COLOR_RESET));
 		}
 
-		if (strbuf_getline_lf(&choice, stdin) != EOF) {
+		if (strbuf_getline(&choice, stdin) != EOF) {
 			strbuf_trim(&choice);
 		} else {
 			eof = 1;

@@ -652,7 +652,7 @@ static int filter_by_patterns_cmd(void)
 		clean_print_color(CLEAN_COLOR_PROMPT);
 		printf(_("Input ignore patterns>> "));
 		clean_print_color(CLEAN_COLOR_RESET);
-		if (strbuf_getline_lf(&confirm, stdin) != EOF)
+		if (strbuf_getline(&confirm, stdin) != EOF)
 			strbuf_trim(&confirm);
 		else
 			putchar('\n');

@@ -750,7 +750,7 @@ static int ask_each_cmd(void)
 			qname = quote_path_relative(item->string, NULL, &buf);
 			/* TRANSLATORS: Make sure to keep [y/N] as is */
 			printf(_("Remove %s [y/N]? "), qname);
-			if (strbuf_getline_lf(&confirm, stdin) != EOF) {
+			if (strbuf_getline(&confirm, stdin) != EOF) {
 				strbuf_trim(&confirm);
 			} else {
 				putchar('\n');

2.4.3

The format of a line that is expected when copying notes via stdin is "sha1 sha1". As this is text-only, strbuf_getline() should be used instead of strbuf_getline_lf(), as documentation of this fact.

When reading with strbuf_getline() the trimming of split[1] can be removed. It was necessary before to remove potential CRs inserted through a dos editor.

---
 builtin/notes.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index ed6f222..706ec11 100644
@@ -290,7 +290,7 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
 		t = &default_notes_tree;
 	}
 
-	while (strbuf_getline_lf(&buf, stdin) != EOF) {
+	while (strbuf_getline(&buf, stdin) != EOF) {
 		unsigned char from_obj[20], to_obj[20];
 		struct strbuf **split;
 		int err;

@@ -299,7 +299,6 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
 		if (!split[0] || !split[1])
 			die(_("Malformed input line: '%s'."), buf.buf);
 		strbuf_rtrim(split[0]);
-		strbuf_rtrim(split[1]);
 		if (get_sha1(split[0]->buf, from_obj))
 			die(_("Failed to resolve '%s' as a valid ref."), split[0]->buf);
 		if (get_sha1(split[1]->buf, to_obj))

2.4.3

s/dos/DOS/

This may not be worth a re-roll, but the suggestion of my v3 review was to keep both rtrim's in this patch and then remove them in the next patch when converting to string_list_split(). A benefit of doing so is that you can then drop the above paragraph altogether, and both patches become simpler (description and content), thus easier to review.

If you do elect to keep things the way they are, then (as mentioned in my v2 review) it would be helpful for the above paragraph to explain that strbuf_split() leave the "terminator" on the split elements, thus clarifying why the rtrim() of split[0] is still needed.

Yes I would rather leave it like it is. I have the feeling it is unmotivated to remove the rtrim of split[1] in the patch 5/7, because it is directly related to the strbuf_getline_lf() replacement. Thats's what I was trying to explain in the 2nd paragraph of the commit message.

First I was following your review, but then I had to add a paragraph in patch 5/7 that says something like "because the effect of the previous patch is that there is not a CR anymore, we can now safely remove rtrim() split[1]."

You're right, maybe I should add a comment about why I left rtrim() of split[0] to make it more obvious. I thought that would get clear by looking at the context, i.e. patch 5/7, where it is explained (by you, thanks for that), that strbuf_split leave this space. Is the assumption, that those two patches are most times viewed in context wrong?

Thanks,
Moritz

I was more concerned about someone reading patch 4/7 in isolation and not consulting 5/7 (which might happen during a "blame" session, but it's a very minor point, not worth a re-roll if you and Junio are happy with the series as is.

strbuf_split() has the unfortunate behavior of leaving the separator character on the end of the split components, thus placing the burden of manually removing the separator on the caller. It's also heavyweight in that each split component is a full-on strbuf. We need neither feature of strbuf_split() so let's use string_list_split() instead since it removes the separator character and returns an array of simple NUL-terminated strings.

---
 builtin/notes.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

index 706ec11..715fade 100644
@@ -292,17 +292,16 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
 
 	while (strbuf_getline(&buf, stdin) != EOF) {
 		unsigned char from_obj[20], to_obj[20];
-		struct strbuf **split;
+		struct string_list split = STRING_LIST_INIT_DUP;
 		int err;
 
-		split = strbuf_split(&buf, ' ');
-		if (!split[0] || !split[1])
+		string_list_split(&split, buf.buf, ' ', -1);
+		if (split.nr != 2)
 			die(_("Malformed input line: '%s'."), buf.buf);
-		strbuf_rtrim(split[0]);
-		if (get_sha1(split[0]->buf, from_obj))
-			die(_("Failed to resolve '%s' as a valid ref."), split[0]->buf);
-		if (get_sha1(split[1]->buf, to_obj))
-			die(_("Failed to resolve '%s' as a valid ref."), split[1]->buf);
+		if (get_sha1(split.items[0].string, from_obj))
+			die(_("Failed to resolve '%s' as a valid ref."), split.items[0].string);
+		if (get_sha1(split.items[1].string, to_obj))
+			die(_("Failed to resolve '%s' as a valid ref."), split.items[1].string);
 
 		if (rewrite_cmd)
 			err = copy_note_for_rewrite(c, from_obj, to_obj);

@@ -312,11 +311,11 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
 
 		if (err) {
 			error(_("Failed to copy notes from '%s' to '%s'"),
-			      split[0]->buf, split[1]->buf);
+			      split.items[0].string, split.items[1].string);
 			ret = 1;
 		}
 
-		strbuf_list_free(split);
+		string_list_clear(&split, 0);
 	}
 
 	if (!rewrite_cmd) {

2.4.3

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.

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

index fc02698..77e011a 100644
@@ -281,7 +281,7 @@ static void read_branches_file(struct remote *remote)
 	if (!f)
 		return;
 
-	strbuf_getline_lf(&buf, f);
+	strbuf_getline(&buf, f);
 	fclose(f);
 	strbuf_trim(&buf);
 	if (!buf.len) {

2.4.3

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

---
 wt-status.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index ab4f80d..8047cf2 100644
@@ -1076,10 +1076,9 @@ static void read_rebase_todolist(const char *fname, struct string_list *lines)
 	if (!f)
 		die_errno("Could not open file %s for reading",
 			  git_path("%s", fname));
-	while (!strbuf_getline_lf(&line, f)) {
+	while (!strbuf_getline(&line, f)) {
 		if (line.len && line.buf[0] == comment_line_char)
 			continue;
-		strbuf_trim(&line);
 		if (!line.len)
 			continue;
 		abbrev_sha1_in_line(&line);

2.4.3

Thanks. With the exception of my commentary on patch 4/7, I think v4 addresses all my v3 review comments.

Hi,

how to deal with patches during the v2.8.0 rc freeze? Will they wait on the mailing list until the feature release cycle is finished?

Or if it's me who should act on this series, because it got below the radar during the rc freeze?

To my knowledge there's only minor points that have to be discussed:

in patch 1/7: How many spaces should be removed, cf.:

	http://thread.gmane.org/gmane.comp.version-control.git/285118/focus=287911

in patches 4/7 and 5/7: Which commit should remove the trimming of "split[0]", cf.:

	http://thread.gmane.org/gmane.comp.version-control.git/285118/focus=287894

Because people are expected to stop getting distracted by new features and no-op clean-up changes and instead to focus on helping find and fix regressions that have been introduced since v2.7.x series during the pre-release period, you may not get review comments unless your patches are really important.

To participate in regression hunting or not is your choice. In any case, you'd likely be re-sending a reroll after a release concludes this cycle in order to get sufficient reviews and Ack's, as people may have expired the last round of patches from you from their mailboxes and their brain by then. And then we go from there.

Thanks.

Ok, I was not sure, sorry for the noise generated. Will resend post-release.

Say, I would like to participate. This might be a very naive question,
but: What is the "workflow" in regression hunting?

There is this known "git status"-regression, where you seem to be at least close to a decision:

	http://thread.gmane.org/gmane.comp.version-control.git/288228/focus=288444

What I can imagine could lead towards finding regessions, though maybe a bit aimless: Go through the list of changes/patches that are supposed to be included in v2.8.0 and confirm they are working as expected. This would be like a post-review.

That would be one way for a very dedicated contributor.

Normal use of Git in your everyday workflow, noticing any unexpected behaviour and digging into it would be what I had in mind, though ;-)

The reason I brought the issue up in review is that it wasn't clear if dropping the rtrims over the course of two patches was intentional or just an oversight (given that the review recommendation from the previous round had suggested removing both in the same patch). From your response, it is clear that it was intentional and, as mentioned in the cited messages, while I do have an opinion on the matter, it's such a minor point that it's not worth a lot of back and forth, and the patch can move forward if you're happy with it the way it is.