threads / patch / 41558

v3, 7 partsremote: read $GIT_DIR/branches/* with strbuf_getline()

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

## tl;dr

38 messages between Feb 28, 2016 and Mar 9, 2016. Diffs are folded; open one to read it.

replies: 37people: 3as markdown or json

Moritz Neeb· Feb 28, 2016, 05:07 UTC · lore

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

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

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

Every codepath was examined, if after the change it is still necessary to have trimming or if the additional 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

Moritz Neeb· Feb 28, 2016, 05:13 UTC · re: Moritz Neeb · lore

[PATCH v3 1/7] 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.

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

Show changes to quote.c +2 −0

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

Moritz Neeb· Feb 28, 2016, 05:13 UTC · re: Moritz Neeb · lore

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

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

Show changes to bisect.c +2 −3

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

Eric Sunshine· Feb 28, 2016, 06:33 UTC · re: Moritz Neeb · lore

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

On Sun, Feb 28, 2016 at 12:13 AM, Moritz Neeb <lists@moritzneeb.de> wrote:

Show 11 quoted lines

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

Show 23 quoted lines

> 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

Moritz Neeb· Feb 28, 2016, 07:30 UTC · re: Eric Sunshine · lore

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

On 02/28/2016 07:33 AM, Eric Sunshine wrote:

Show 21 quoted lines

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

Show 24 quoted lines

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

Moritz Neeb· Feb 28, 2016, 05:13 UTC · re: Moritz Neeb · lore

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

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

Show changes to builtin/clean.c +3 −3

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

Eric Sunshine· Feb 28, 2016, 06:36 UTC · re: Moritz Neeb · lore

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

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.

Show 39 quoted lines

> 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

Moritz Neeb· Feb 28, 2016, 07:36 UTC · re: Eric Sunshine · lore

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

On 02/28/2016 07:36 AM, Eric Sunshine wrote:

Show 8 quoted lines

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

Show 5 quoted lines

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

Yep, sounds better.

Thanks.

Show 40 quoted lines

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

Moritz Neeb· Feb 28, 2016, 05:13 UTC · re: Moritz Neeb · lore

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

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

Show changes to builtin/notes.c +11 −11

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

Eric Sunshine· Feb 28, 2016, 06:56 UTC · re: Moritz Neeb · lore

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

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.

Show 47 quoted lines

> 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

Moritz Neeb· Feb 28, 2016, 07:47 UTC · re: Eric Sunshine · lore

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

On 02/28/2016 07:56 AM, Eric Sunshine wrote:

Show 27 quoted lines

> 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

Show 50 quoted lines

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

Eric Sunshine· Feb 28, 2016, 16:02 UTC · re: Moritz Neeb · lore

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

On Sun, Feb 28, 2016 at 2:47 AM, Moritz Neeb <lists@moritzneeb.de> wrote:

Show 16 quoted lines

> 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

Moritz Neeb· Feb 28, 2016, 05:13 UTC · re: Moritz Neeb · lore

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

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

Show changes to builtin/notes.c +1 −2

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

Moritz Neeb· Feb 28, 2016, 05:14 UTC · re: Moritz Neeb · lore

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

Show changes to remote.c +1 −1

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

Moritz Neeb· Feb 28, 2016, 05:14 UTC · re: Moritz Neeb · lore

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

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

Show changes to wt-status.c +1 −2

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

Eric Sunshine· Feb 28, 2016, 06:30 UTC · re: Moritz Neeb · lore

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

On Sun, Feb 28, 2016 at 12:07 AM, Moritz Neeb <lists@moritzneeb.de> wrote:

Show 13 quoted lines

> 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

Moritz Neeb· Feb 28, 2016, 07:20 UTC · re: Eric Sunshine · lore

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

On 02/28/2016 07:30 AM, Eric Sunshine wrote:

Show 24 quoted lines

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

Show changes to 2 files +19 −13

builtin/clean.c, builtin/notes.c

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

Moritz Neeb· Feb 28, 2016, 08:03 UTC · re: Moritz Neeb · lore

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

On 02/28/2016 06:07 AM, Moritz Neeb wrote:

Show 8 quoted lines

> 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

Moritz Neeb· Feb 29, 2016, 08:30 UTC · re: Moritz Neeb · lore

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

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.

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

Show changes to builtin/notes.c +0 −2

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

Moritz Neeb· Feb 29, 2016, 08:36 UTC · re: Moritz Neeb · lore

[PATCH v4 1/7] 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.

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

Show changes to quote.c +2 −0

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

Junio C Hamano· Feb 29, 2016, 19:01 UTC · re: Moritz Neeb · lore

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

Moritz Neeb <lists@moritzneeb.de> writes:

Show 25 quoted lines

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

Moritz Neeb· Feb 29, 2016, 21:45 UTC · re: Junio C Hamano · lore

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

On 02/29/2016 08:01 PM, Junio C Hamano wrote:

Show 39 quoted lines

> 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

Moritz Neeb· Feb 29, 2016, 21:48 UTC · re: Moritz Neeb · lore

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

On 02/29/2016 10:45 PM, Moritz Neeb wrote:

Show 52 quoted lines

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

Show 7 quoted lines

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

Moritz Neeb· Feb 29, 2016, 08:36 UTC · re: Moritz Neeb · lore

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

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

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

Show changes to bisect.c +2 −3

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

Moritz Neeb· Feb 29, 2016, 08:36 UTC · re: Moritz Neeb · lore

[PATCH v4 3/7] clean: read user input with 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(-)

Show changes to builtin/clean.c +3 −3

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

Moritz Neeb· Feb 29, 2016, 08:36 UTC · re: Moritz Neeb · lore

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

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

Show changes to builtin/notes.c +9 −10

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

Moritz Neeb· Feb 29, 2016, 08:36 UTC · re: Moritz Neeb · lore

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

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

Show changes to remote.c +1 −1

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

Moritz Neeb· Feb 29, 2016, 08:36 UTC · re: Moritz Neeb · lore

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

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.

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

Show changes to wt-status.c +1 −2

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

Moritz Neeb· Feb 29, 2016, 08:36 UTC · re: Moritz Neeb · lore

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

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

Show changes to builtin/notes.c +1 −2

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

Eric Sunshine· Feb 29, 2016, 18:19 UTC · re: Moritz Neeb · lore

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

On Mon, Feb 29, 2016 at 3:36 AM, Moritz Neeb <lists@moritzneeb.de> wrote:

Show 7 quoted lines

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

Show 28 quoted lines

> 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

Moritz Neeb· Feb 29, 2016, 19:26 UTC · re: Eric Sunshine · lore

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

On 02/29/2016 07:19 PM, Eric Sunshine wrote:

Show 5 quoted lines

> 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

Show 28 quoted lines

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

Eric Sunshine· Feb 29, 2016, 19:48 UTC · re: Moritz Neeb · lore

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

On Mon, Feb 29, 2016 at 2:26 PM, Moritz Neeb <lists@moritzneeb.de> wrote:

Show 21 quoted lines

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

Eric Sunshine· Feb 29, 2016, 18:26 UTC · re: Moritz Neeb · lore

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

On Mon, Feb 29, 2016 at 3:30 AM, Moritz Neeb <lists@moritzneeb.de> wrote:

Show 7 quoted lines

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

Moritz Neeb· Mar 9, 2016, 00:25 UTC · re: Moritz Neeb · lore

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

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

Show 11 quoted lines

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

Junio C Hamano· Mar 9, 2016, 00:39 UTC · re: Moritz Neeb · lore

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

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.

Moritz Neeb· Mar 9, 2016, 01:13 UTC · re: Junio C Hamano · lore

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

On 03/09/2016 01:39 AM, Junio C Hamano wrote:

Show 11 quoted lines

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

Junio C Hamano· Mar 9, 2016, 20:28 UTC · re: Moritz Neeb · lore

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

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

Eric Sunshine· Mar 9, 2016, 01:17 UTC · re: Moritz Neeb · lore

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

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

Show 12 quoted lines

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