[PATCH 0/5] Replacing strbuf_getline_lf() by strbuf_getline() on trimmed input

[PATCH 0/5] Replacing strbuf_getline_lf() by strbuf_getline() on trimmed input

[Moritz Neeb]

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

[PATCH 1/5] bisect: read bisect paths with strbuf_getline()

[Moritz Neeb]

    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.

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

diff --git a/bisect.c b/bisect.c
index 06ec54e..bf7c885 100644
--- a/bisect.c
+++ b/bisect.c
@@ -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

[PATCH 2/5] clean: read user input with strbuf_getline()

[Moritz Neeb]

    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.

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

diff --git a/builtin/clean.c b/builtin/clean.c
index 7b08237..956283d 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -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

[PATCH 3/5] notes: read copied notes with strbuf_getline()

[Moritz Neeb]

    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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index ed6f222..bbd7544 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -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

[PATCH 4/5] remote: read $GIT_DIR/branches/* with strbuf_getline()

[Moritz Neeb]

    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>
---
 remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/remote.c b/remote.c
index 7d61dab..78f97f0 100644
--- a/remote.c
+++ b/remote.c
@@ -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

[PATCH 5/5] wt-status: read rebase todolist with strbuf_getline()

[Moritz Neeb]

    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.

Signed-off-by: Moritz Neeb <lists@moritzneeb.de>
---

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

diff --git a/wt-status.c b/wt-status.c
index ab4f80d..f053b2f 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -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

Re: [PATCH 1/5] bisect: read bisect paths with strbuf_getline()

[Junio C Hamano]

Moritz Neeb <lists@moritzneeb.de> writes:

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

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.

> Signed-off-by: Moritz Neeb <lists@moritzneeb.de>
> ---
>  bisect.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/bisect.c b/bisect.c
> index 06ec54e..bf7c885 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -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",

Re: [PATCH 2/5] clean: read user input with strbuf_getline()

[Junio C Hamano]

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

Re: [PATCH 3/5] notes: read copied notes with strbuf_getline()

[Junio C Hamano]

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

Re: [PATCH 5/5] wt-status: read rebase todolist with strbuf_getline()

[Junio C Hamano]

Moritz Neeb <lists@moritzneeb.de> writes:

>     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.
>
> Signed-off-by: Moritz Neeb <lists@moritzneeb.de>
> ---
>
> 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?

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.

>
>  wt-status.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/wt-status.c b/wt-status.c
> index ab4f80d..f053b2f 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -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);

Re: [PATCH 1/5] bisect: read bisect paths with strbuf_getline()

[Moritz Neeb]

On 02/01/2016 10:30 PM, Junio C Hamano wrote:
> Moritz Neeb <lists@moritzneeb.de> writes:
> 
>>     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.
> 
> We do not indent the whole log message.

Thanks for spotting, I will change that in the next iteration.
 
> 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.

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.

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

diff --git a/quote.c b/quote.c
index fe884d2..2714f27 100644
--- a/quote.c
+++ b/quote.c
@@ -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

Re: [PATCH 1/5] bisect: read bisect paths with strbuf_getline()

[Junio C Hamano]

Moritz Neeb <lists@moritzneeb.de> writes:

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

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.

Re: [PATCH 1/5] bisect: read bisect paths with strbuf_getline()

[Moritz Neeb]

On 02/15/2016 06:05 AM, Junio C Hamano wrote:
> Moritz Neeb <lists@moritzneeb.de> writes:
> 
>>> 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.
>>
>> 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.
> 
> 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.

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.

Re: [PATCH 1/5] bisect: read bisect paths with strbuf_getline()

[Moritz Neeb]

On 02/15/2016 06:05 AM, Junio C Hamano wrote:
> 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.

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.

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

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.

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

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

[PATCH v2 0/6] replacing strbuf_getline_lf() by strbuf_getline() on trimmed input

[Moritz Neeb]

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

[PATCH v2 1/6] quote: remove leading space in sq_dequote_step

[Moritz Neeb]

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

diff --git a/quote.c b/quote.c
index fe884d2..2714f27 100644
--- a/quote.c
+++ b/quote.c
@@ -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

[PATCH v2 2/6] bisect: read bisect paths with strbuf_getline()

[Moritz Neeb]

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

diff --git a/bisect.c b/bisect.c
index 06ec54e..e2df02f 100644
--- a/bisect.c
+++ b/bisect.c
@@ -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

[PATCH v2 4/6] notes: read copied notes with strbuf_getline()

[Moritz Neeb]

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

diff --git a/builtin/notes.c b/builtin/notes.c
index ed6f222..706ec11 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -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

[PATCH v2 6/6] wt-status: read rebase todolist with strbuf_getline()

[Moritz Neeb]

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

diff --git a/wt-status.c b/wt-status.c
index ab4f80d..8047cf2 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -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

[PATCH v2 3/6] clean: read user input with strbuf_getline()

[Moritz Neeb]

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

diff --git a/builtin/clean.c b/builtin/clean.c
index 7b08237..01cc2ff 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -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

[PATCH v2 5/6] remote: read $GIT_DIR/branches/* with strbuf_getline()

[Moritz Neeb]

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

diff --git a/remote.c b/remote.c
index 02e698a..aaff6aa 100644
--- a/remote.c
+++ b/remote.c
@@ -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

Re: [PATCH v2 3/6] clean: read user input with strbuf_getline()

[Eric Sunshine]

On Sun, Feb 21, 2016 at 8:20 PM, Moritz Neeb <lists@moritzneeb.de> wrote:
> 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.

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.

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

s/, because of/ for/

> 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>
> ---
> diff --git a/builtin/clean.c b/builtin/clean.c
> @@ -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

Re: [PATCH v2 4/6] notes: read copied notes with strbuf_getline()

[Eric Sunshine]

On Sun, Feb 21, 2016 at 8:16 PM, Moritz Neeb <lists@moritzneeb.de> wrote:
> 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>
> ---
> diff --git a/builtin/notes.c b/builtin/notes.c
> @@ -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]);

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.

>                 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

Re: [PATCH v2 3/6] clean: read user input with strbuf_getline()

[Moritz Neeb]

On 02/22/2016 03:27 AM, Eric Sunshine wrote:
> On Sun, Feb 21, 2016 at 8:20 PM, Moritz Neeb <lists@moritzneeb.de> wrote:
>> 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.
> 
> I don't at all insist upon it, but this behavior change feels somewhat
> like it ought to be in its own commit.

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.

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

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.

Re: [PATCH v2 5/6] remote: read $GIT_DIR/branches/* with strbuf_getline()

[Junio C Hamano]

Moritz Neeb <lists@moritzneeb.de> writes:

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

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

Re: [PATCH v2 4/6] notes: read copied notes with strbuf_getline()

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

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

Yeah, this is a good point to raise.

Thanks.

Re: [PATCH v2 6/6] wt-status: read rebase todolist with strbuf_getline()

[Junio C Hamano]

Moritz Neeb <lists@moritzneeb.de> writes:

> diff --git a/wt-status.c b/wt-status.c
> index ab4f80d..8047cf2 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -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)) {

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.

Re: [PATCH v2 3/6] clean: read user input with strbuf_getline()

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Sun, Feb 21, 2016 at 8:20 PM, Moritz Neeb <lists@moritzneeb.de> wrote:
>> 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.
>
> 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.

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.

[PATCH v3 0/7] replacing strbuf_getline_lf() by strbuf_getline()

[Moritz Neeb]

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

[PATCH v3 1/7] quote: remove leading space in sq_dequote_step

[Moritz Neeb]

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

diff --git a/quote.c b/quote.c
index fe884d2..2714f27 100644
--- a/quote.c
+++ b/quote.c
@@ -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

[PATCH v3 2/7] bisect: read bisect paths with strbuf_getline()

[Moritz Neeb]

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

diff --git a/bisect.c b/bisect.c
index 06ec54e..e2df02f 100644
--- a/bisect.c
+++ b/bisect.c
@@ -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

[PATCH v3 3/7] clean: read user input with strbuf_getline()

[Moritz Neeb]

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.

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

diff --git a/builtin/clean.c b/builtin/clean.c
index 7b08237..956283d 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -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

[PATCH v3 4/7] notes copy --stdin: split lines with string_list_split()

[Moritz Neeb]

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.

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

diff --git a/builtin/notes.c b/builtin/notes.c
index ed6f222..22909c7 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -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

[PATCH v3 5/7] notes copy --stdin: read lines with strbuf_getline()

[Moritz Neeb]

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.

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

diff --git a/builtin/notes.c b/builtin/notes.c
index 22909c7..660c0b7 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -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

[PATCH v3 6/7] remote: read $GIT_DIR/branches/* with strbuf_getline()

[Moritz Neeb]

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>
---
 remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/remote.c b/remote.c
index 02e698a..aaff6aa 100644
--- a/remote.c
+++ b/remote.c
@@ -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

[PATCH v3 7/7] wt-status: read rebase todolist with strbuf_getline()

[Moritz Neeb]

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

diff --git a/wt-status.c b/wt-status.c
index ab4f80d..8047cf2 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -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

Re: [PATCH v3 0/7] replacing strbuf_getline_lf() by strbuf_getline()

[Eric Sunshine]

On Sun, Feb 28, 2016 at 12:07 AM, Moritz Neeb <lists@moritzneeb.de> wrote:
> 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.
> [...]
>
> 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.

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

Re: [PATCH v3 2/7] bisect: read bisect paths with strbuf_getline()

[Eric Sunshine]

On Sun, Feb 28, 2016 at 12:13 AM, Moritz Neeb <lists@moritzneeb.de> wrote:
> 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

s/expexted/expected/

> this file to be editable by user, or encouraged them to do so, thus
> the call to strbuf_trim() turns obsolete in various ways.

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

> 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>
> ---
> diff --git a/bisect.c b/bisect.c
> @@ -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

Re: [PATCH v3 3/7] clean: read user input with strbuf_getline()

[Eric Sunshine]

On Sun, Feb 28, 2016 at 12:13 AM, Moritz Neeb <lists@moritzneeb.de> wrote:
> 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().

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

> In the subsequent codepath, the input is trimmed. This leads to

How about?

    After being read, the input is trimmed.

> 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.
>
> Signed-off-by: Moritz Neeb <lists@moritzneeb.de>
> ---
> diff --git a/builtin/clean.c b/builtin/clean.c
> @@ -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

Re: [PATCH v3 4/7] notes copy --stdin: split lines with string_list_split()

[Eric Sunshine]

On Sun, Feb 28, 2016 at 12:13 AM, Moritz Neeb <lists@moritzneeb.de> wrote:
> 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.

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.

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

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.

> Signed-off-by: Moritz Neeb <lists@moritzneeb.de>
> ---
> diff --git a/builtin/notes.c b/builtin/notes.c
> @@ -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

Re: [PATCH v3 0/7] replacing strbuf_getline_lf() by strbuf_getline()

[Moritz Neeb]

On 02/28/2016 07:30 AM, Eric Sunshine wrote:
> On Sun, Feb 28, 2016 at 12:07 AM, Moritz Neeb <lists@moritzneeb.de> wrote:
>> 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.
>> [...]
>>
>> 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.
> 
> 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):

diff --git a/builtin/clean.c b/builtin/clean.c
index 18b6056..5b17a31 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -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;
 			}
diff --git a/builtin/notes.c b/builtin/notes.c
index 706ec11..660c0b7 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -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) {

Re: [PATCH v3 2/7] bisect: read bisect paths with strbuf_getline()

[Moritz Neeb]

On 02/28/2016 07:33 AM, Eric Sunshine wrote:
> On Sun, Feb 28, 2016 at 12:13 AM, Moritz Neeb <lists@moritzneeb.de> wrote:
>> 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
> 
> s/expexted/expected/
> 
>> this file to be editable by user, or encouraged them to do so, thus
>> the call to strbuf_trim() turns obsolete in various ways.
> 
> 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().

> 
>> 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>
>> ---
>> diff --git a/bisect.c b/bisect.c
>> @@ -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

Re: [PATCH v3 3/7] clean: read user input with strbuf_getline()

[Moritz Neeb]

On 02/28/2016 07:36 AM, Eric Sunshine wrote:
> On Sun, Feb 28, 2016 at 12:13 AM, Moritz Neeb <lists@moritzneeb.de> wrote:
>> 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().
> 
> Grammo: "it is safe to replace ... can be replaced by ..."
> 

I dropped the second duplication.

>> In the subsequent codepath, the input is trimmed. This leads to
> 
> How about?
> 
>     After being read, the input is trimmed.

Yep, sounds better.

Thanks.

> 
>> 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.
>>
>> Signed-off-by: Moritz Neeb <lists@moritzneeb.de>
>> ---
>> diff --git a/builtin/clean.c b/builtin/clean.c
>> @@ -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

Re: [PATCH v3 4/7] notes copy --stdin: split lines with string_list_split()

[Moritz Neeb]

On 02/28/2016 07:56 AM, Eric Sunshine wrote:
> On Sun, Feb 28, 2016 at 12:13 AM, Moritz Neeb <lists@moritzneeb.de> wrote:
>> 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.
> 
> 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.
> 
>> 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.
> 
> 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.

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

> 
> The patch itself looks okay.
> 
>> Signed-off-by: Moritz Neeb <lists@moritzneeb.de>
>> ---
>> diff --git a/builtin/notes.c b/builtin/notes.c
>> @@ -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

Re: [PATCH v3 0/7] replacing strbuf_getline_lf() by strbuf_getline()

[Moritz Neeb]

On 02/28/2016 06:07 AM, Moritz Neeb wrote:
> 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.
> 

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

Re: [PATCH v3 4/7] notes copy --stdin: split lines with string_list_split()

[Eric Sunshine]

On Sun, Feb 28, 2016 at 2:47 AM, Moritz Neeb <lists@moritzneeb.de> wrote:
> On 02/28/2016 07:56 AM, Eric Sunshine wrote:
>> On Sun, Feb 28, 2016 at 12:13 AM, Moritz Neeb <lists@moritzneeb.de> wrote:
>>> 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.
>>
>> 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.
>
> 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.

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

> 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

[PATCH v4 0/7] replacing strbuf_getline_lf() by strbuf_getline()

[Moritz Neeb]

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

diff --git a/builtin/notes.c b/builtin/notes.c
index 660c0b7..715fade 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -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

[PATCH v4 1/7] quote: remove leading space in sq_dequote_step

[Moritz Neeb]

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

diff --git a/quote.c b/quote.c
index fe884d2..2714f27 100644
--- a/quote.c
+++ b/quote.c
@@ -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

[PATCH v4 2/7] bisect: read bisect paths with strbuf_getline()

[Moritz Neeb]

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.

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

diff --git a/bisect.c b/bisect.c
index 7996c29..f63aa10 100644
--- a/bisect.c
+++ b/bisect.c
@@ -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

[PATCH v4 3/7] clean: read user input with strbuf_getline()

[Moritz Neeb]

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.

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

diff --git a/builtin/clean.c b/builtin/clean.c
index 0371010..5b17a31 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -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

[PATCH v4 5/7] notes copy --stdin: split lines with string_list_split()

[Moritz Neeb]

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.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Moritz Neeb <lists@moritzneeb.de>
---
 builtin/notes.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 706ec11..715fade 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -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

[PATCH v4 6/7] remote: read $GIT_DIR/branches/* with strbuf_getline()

[Moritz Neeb]

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>
---
 remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/remote.c b/remote.c
index fc02698..77e011a 100644
--- a/remote.c
+++ b/remote.c
@@ -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

[PATCH v4 7/7] wt-status: read rebase todolist with strbuf_getline()

[Moritz Neeb]

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

diff --git a/wt-status.c b/wt-status.c
index ab4f80d..8047cf2 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -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

[PATCH v4 4/7] notes copy --stdin: read lines with strbuf_getline()

[Moritz Neeb]

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.

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

diff --git a/builtin/notes.c b/builtin/notes.c
index ed6f222..706ec11 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -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

Re: [PATCH v4 4/7] notes copy --stdin: read lines with strbuf_getline()

[Eric Sunshine]

On Mon, Feb 29, 2016 at 3:36 AM, Moritz Neeb <lists@moritzneeb.de> wrote:
> 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.

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.

> Signed-off-by: Moritz Neeb <lists@moritzneeb.de>
> ---
>  builtin/notes.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/builtin/notes.c b/builtin/notes.c
> index ed6f222..706ec11 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -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

Re: [PATCH v4 0/7] replacing strbuf_getline_lf() by strbuf_getline()

[Eric Sunshine]

On Mon, Feb 29, 2016 at 3:30 AM, Moritz Neeb <lists@moritzneeb.de> wrote:
> 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.
>
> 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.

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

Re: [PATCH v4 1/7] quote: remove leading space in sq_dequote_step

[Junio C Hamano]

Moritz Neeb <lists@moritzneeb.de> writes:

> 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(+)
>
> diff --git a/quote.c b/quote.c
> index fe884d2..2714f27 100644
> --- a/quote.c
> +++ b/quote.c
> @@ -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 (;;) {

If we look at this "for (;;)" loop, we notice that (1) it accepts as
many spaces as there are between two quoted strings, and (2) it does
not limit it to SP but uses isspace().

I wonder if you would instead want

	while (isspace(*src))
        	src++;

to be consistent?

Re: [PATCH v4 4/7] notes copy --stdin: read lines with strbuf_getline()

[Moritz Neeb]

On 02/29/2016 07:19 PM, Eric Sunshine wrote:
> 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


>> Signed-off-by: Moritz Neeb <lists@moritzneeb.de>
>> ---
>>  builtin/notes.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/builtin/notes.c b/builtin/notes.c
>> index ed6f222..706ec11 100644
>> --- a/builtin/notes.c
>> +++ b/builtin/notes.c
>> @@ -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

Re: [PATCH v4 4/7] notes copy --stdin: read lines with strbuf_getline()

[Eric Sunshine]

On Mon, Feb 29, 2016 at 2:26 PM, Moritz Neeb <lists@moritzneeb.de> wrote:
> On 02/29/2016 07:19 PM, Eric Sunshine wrote:
>> 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?

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.

Re: [PATCH v4 1/7] quote: remove leading space in sq_dequote_step

[Moritz Neeb]

On 02/29/2016 08:01 PM, Junio C Hamano wrote:
> Moritz Neeb <lists@moritzneeb.de> writes:
> 
>> 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(+)
>>
>> diff --git a/quote.c b/quote.c
>> index fe884d2..2714f27 100644
>> --- a/quote.c
>> +++ b/quote.c
>> @@ -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 (;;) {
> 
> If we look at this "for (;;)" loop, we notice that (1) it accepts as
> many spaces as there are between two quoted strings, and (2) it does
> not limit it to SP but uses isspace().
> 
> I wonder if you would instead want
> 
> 	while (isspace(*src))
>         	src++;
> 
> to be consistent?
> 

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

Re: [PATCH v4 1/7] quote: remove leading space in sq_dequote_step

[Moritz Neeb]

On 02/29/2016 10:45 PM, Moritz Neeb wrote:
> On 02/29/2016 08:01 PM, Junio C Hamano wrote:
>> Moritz Neeb <lists@moritzneeb.de> writes:
>>
>>> 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(+)
>>>
>>> diff --git a/quote.c b/quote.c
>>> index fe884d2..2714f27 100644
>>> --- a/quote.c
>>> +++ b/quote.c
>>> @@ -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 (;;) {
>>
>> If we look at this "for (;;)" loop, we notice that (1) it accepts as
>> many spaces as there are between two quoted strings, and (2) it does
>> not limit it to SP but uses isspace().
>>
>> I wonder if you would instead want
>>
>> 	while (isspace(*src))
>>         	src++;
>>
>> to be consistent?
>>
> 
> 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.

s/array/to_argv/

> 
> Thanks
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v4 0/7] replacing strbuf_getline_lf() by strbuf_getline()

[Moritz Neeb]

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:

On 02/29/2016 09:30 AM, Moritz Neeb wrote:
> 
> Moritz Neeb (7):
>   quote: remove leading space in sq_dequote_step -- as in v2

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

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

>   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

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

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

Re: [PATCH v4 0/7] replacing strbuf_getline_lf() by strbuf_getline()

[Junio C Hamano]

Moritz Neeb <lists@moritzneeb.de> writes:

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

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.

Re: [PATCH v4 0/7] replacing strbuf_getline_lf() by strbuf_getline()

[Moritz Neeb]

On 03/09/2016 01:39 AM, Junio C Hamano wrote:
> Moritz Neeb <lists@moritzneeb.de> writes:
> 
>> 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?
> 
> 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.
> 

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

> To participate in regression hunting or not is your choice.

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.

Re: [PATCH v4 0/7] replacing strbuf_getline_lf() by strbuf_getline()

[Eric Sunshine]

On Tue, Mar 8, 2016 at 7:25 PM, Moritz Neeb <lists@moritzneeb.de> wrote:
> 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 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

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.

Re: [PATCH v4 0/7] replacing strbuf_getline_lf() by strbuf_getline()

[Junio C Hamano]

Moritz Neeb <lists@moritzneeb.de> writes:

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