# [PATCH v3 6/7] remote: read $GIT_DIR/branches/* with strbuf_getline() 38 messages from 2016-02-28 to 2016-03-09. Participants: Moritz Neeb, Eric Sunshine, Junio C Hamano. Thread: https://gitlist.dev/t/41558 ## Moritz Neeb, 2016-02-28 05:07 Subject: [PATCH v3 0/7] replacing strbuf_getline_lf() by strbuf_getline() Message-ID: <56D28092.9090209@moritzneeb.de> URL: https://gitlist.dev/e/56D28092.9090209%40moritzneeb.de In-Reply-To: <56CA5DBB.8040006@moritzneeb.de> ``` 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, 2016-02-28 05:13 Subject: [PATCH v3 1/7] quote: remove leading space in sq_dequote_step Message-ID: <56D281F7.4010605@moritzneeb.de> URL: https://gitlist.dev/e/56D281F7.4010605%40moritzneeb.de In-Reply-To: <56D28092.9090209@moritzneeb.de> ``` 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 --- 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 ``` ## Moritz Neeb, 2016-02-28 05:13 Subject: [PATCH v3 2/7] bisect: read bisect paths with strbuf_getline() Message-ID: <56D281FD.1070707@moritzneeb.de> URL: https://gitlist.dev/e/56D281FD.1070707%40moritzneeb.de In-Reply-To: <56D28092.9090209@moritzneeb.de> ``` 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 Signed-off-by: Moritz Neeb --- 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 ``` ## Moritz Neeb, 2016-02-28 05:13 Subject: [PATCH v3 3/7] clean: read user input with strbuf_getline() Message-ID: <56D28203.7040502@moritzneeb.de> URL: https://gitlist.dev/e/56D28203.7040502%40moritzneeb.de In-Reply-To: <56D28092.9090209@moritzneeb.de> ``` 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 --- 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 ``` ## Moritz Neeb, 2016-02-28 05:13 Subject: [PATCH v3 4/7] notes copy --stdin: split lines with string_list_split() Message-ID: <56D28207.6080600@moritzneeb.de> URL: https://gitlist.dev/e/56D28207.6080600%40moritzneeb.de In-Reply-To: <56D28092.9090209@moritzneeb.de> ``` 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 --- 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 ``` ## Moritz Neeb, 2016-02-28 05:13 Subject: [PATCH v3 5/7] notes copy --stdin: read lines with strbuf_getline() Message-ID: <56D28211.5060101@moritzneeb.de> URL: https://gitlist.dev/e/56D28211.5060101%40moritzneeb.de In-Reply-To: <56D28092.9090209@moritzneeb.de> ``` 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 --- 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 ``` ## Moritz Neeb, 2016-02-28 05:14 Subject: [PATCH v3 6/7] remote: read $GIT_DIR/branches/* with strbuf_getline() Message-ID: <56D2821F.4010806@moritzneeb.de> URL: https://gitlist.dev/e/56D2821F.4010806%40moritzneeb.de In-Reply-To: <56D28092.9090209@moritzneeb.de> ``` 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 --- 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 ``` ## Moritz Neeb, 2016-02-28 05:14 Subject: [PATCH v3 7/7] wt-status: read rebase todolist with strbuf_getline() Message-ID: <56D28224.9010005@moritzneeb.de> URL: https://gitlist.dev/e/56D28224.9010005%40moritzneeb.de In-Reply-To: <56D28092.9090209@moritzneeb.de> ``` 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 --- 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 ``` ## Eric Sunshine, 2016-02-28 06:30 Subject: Re: [PATCH v3 0/7] replacing strbuf_getline_lf() by strbuf_getline() Message-ID: URL: https://gitlist.dev/e/CAPig%2BcSv%3D7zixz_BK%3Df0MhQnTTB-agB5%3D4aSrFE5vAJtOgbuGg%40mail.gmail.com In-Reply-To: <56D28092.9090209@moritzneeb.de> ``` On Sun, Feb 28, 2016 at 12:07 AM, Moritz Neeb 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 ``` ## Eric Sunshine, 2016-02-28 06:33 Subject: Re: [PATCH v3 2/7] bisect: read bisect paths with strbuf_getline() Message-ID: URL: https://gitlist.dev/e/CAPig%2BcRMX1DF7ffEKR4fWGY9wZjpKsaOaf4C1YdWipUGh1%2B8AA%40mail.gmail.com In-Reply-To: <56D281FD.1070707@moritzneeb.de> ``` On Sun, Feb 28, 2016 at 12:13 AM, Moritz Neeb 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 > Signed-off-by: Moritz Neeb > --- > 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 ``` ## Eric Sunshine, 2016-02-28 06:36 Subject: Re: [PATCH v3 3/7] clean: read user input with strbuf_getline() Message-ID: URL: https://gitlist.dev/e/CAPig%2BcSkV0KMG7gONabPxgQkzWgPU%2B1YGTMb4SDmpg1QrHxhSA%40mail.gmail.com In-Reply-To: <56D28203.7040502@moritzneeb.de> ``` On Sun, Feb 28, 2016 at 12:13 AM, Moritz Neeb 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 > --- > 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 ``` ## Eric Sunshine, 2016-02-28 06:56 Subject: Re: [PATCH v3 4/7] notes copy --stdin: split lines with string_list_split() Message-ID: URL: https://gitlist.dev/e/CAPig%2BcT2GQ7mr0i649JRkJA7xGzXLEmy0RD31u537%3D%3DsU1mtqQ%40mail.gmail.com In-Reply-To: <56D28207.6080600@moritzneeb.de> ``` On Sun, Feb 28, 2016 at 12:13 AM, Moritz Neeb 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 > --- > 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, 2016-02-28 07:20 Subject: Re: [PATCH v3 0/7] replacing strbuf_getline_lf() by strbuf_getline() Message-ID: <56D29FB3.3030707@moritzneeb.de> URL: https://gitlist.dev/e/56D29FB3.3030707%40moritzneeb.de In-Reply-To: ``` On 02/28/2016 07:30 AM, Eric Sunshine wrote: > On Sun, Feb 28, 2016 at 12:07 AM, Moritz Neeb 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) { ``` ## Moritz Neeb, 2016-02-28 07:30 Subject: Re: [PATCH v3 2/7] bisect: read bisect paths with strbuf_getline() Message-ID: <56D2A1FC.4020008@moritzneeb.de> URL: https://gitlist.dev/e/56D2A1FC.4020008%40moritzneeb.de In-Reply-To: ``` On 02/28/2016 07:33 AM, Eric Sunshine wrote: > On Sun, Feb 28, 2016 at 12:13 AM, Moritz Neeb 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 >> Signed-off-by: Moritz Neeb >> --- >> 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, 2016-02-28 07:36 Subject: Re: [PATCH v3 3/7] clean: read user input with strbuf_getline() Message-ID: <56D2A390.9010804@moritzneeb.de> URL: https://gitlist.dev/e/56D2A390.9010804%40moritzneeb.de In-Reply-To: ``` On 02/28/2016 07:36 AM, Eric Sunshine wrote: > On Sun, Feb 28, 2016 at 12:13 AM, Moritz Neeb 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 >> --- >> 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, 2016-02-28 07:47 Subject: Re: [PATCH v3 4/7] notes copy --stdin: split lines with string_list_split() Message-ID: <56D2A60A.4000306@moritzneeb.de> URL: https://gitlist.dev/e/56D2A60A.4000306%40moritzneeb.de In-Reply-To: ``` On 02/28/2016 07:56 AM, Eric Sunshine wrote: > On Sun, Feb 28, 2016 at 12:13 AM, Moritz Neeb 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 >> --- >> 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, 2016-02-28 08:03 Subject: Re: [PATCH v3 0/7] replacing strbuf_getline_lf() by strbuf_getline() Message-ID: <56D2A9D3.4050303@moritzneeb.de> URL: https://gitlist.dev/e/56D2A9D3.4050303%40moritzneeb.de In-Reply-To: <56D28092.9090209@moritzneeb.de> ``` 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 ``` ## Eric Sunshine, 2016-02-28 16:02 Subject: Re: [PATCH v3 4/7] notes copy --stdin: split lines with string_list_split() Message-ID: URL: https://gitlist.dev/e/CAPig%2BcSOjdCkMAKEJ%2B7o%3D-cGsLgZWG5mnVmkWLSbyedg%3DmY5Lw%40mail.gmail.com In-Reply-To: <56D2A60A.4000306@moritzneeb.de> ``` On Sun, Feb 28, 2016 at 2:47 AM, Moritz Neeb wrote: > On 02/28/2016 07:56 AM, Eric Sunshine wrote: >> On Sun, Feb 28, 2016 at 12:13 AM, Moritz Neeb 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, 2016-02-29 08:30 Subject: [PATCH v4 0/7] replacing strbuf_getline_lf() by strbuf_getline() Message-ID: <56D401C2.8020100@moritzneeb.de> URL: https://gitlist.dev/e/56D401C2.8020100%40moritzneeb.de In-Reply-To: <56D28092.9090209@moritzneeb.de> ``` 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 ``` ## Moritz Neeb, 2016-02-29 08:36 Subject: [PATCH v4 1/7] quote: remove leading space in sq_dequote_step Message-ID: <56D40301.8020007@moritzneeb.de> URL: https://gitlist.dev/e/56D40301.8020007%40moritzneeb.de In-Reply-To: <56D401C2.8020100@moritzneeb.de> ``` 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 --- 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 ``` ## Moritz Neeb, 2016-02-29 08:36 Subject: [PATCH v4 2/7] bisect: read bisect paths with strbuf_getline() Message-ID: <56D40304.30205@moritzneeb.de> URL: https://gitlist.dev/e/56D40304.30205%40moritzneeb.de In-Reply-To: <56D401C2.8020100@moritzneeb.de> ``` 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 Signed-off-by: Moritz Neeb --- 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 ``` ## Moritz Neeb, 2016-02-29 08:36 Subject: [PATCH v4 3/7] clean: read user input with strbuf_getline() Message-ID: <56D40307.4080903@moritzneeb.de> URL: https://gitlist.dev/e/56D40307.4080903%40moritzneeb.de In-Reply-To: <56D401C2.8020100@moritzneeb.de> ``` 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 --- 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 ``` ## Moritz Neeb, 2016-02-29 08:36 Subject: [PATCH v4 5/7] notes copy --stdin: split lines with string_list_split() Message-ID: <56D4030B.8000303@moritzneeb.de> URL: https://gitlist.dev/e/56D4030B.8000303%40moritzneeb.de In-Reply-To: <56D401C2.8020100@moritzneeb.de> ``` 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 Signed-off-by: Moritz Neeb --- 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 ``` ## Moritz Neeb, 2016-02-29 08:36 Subject: [PATCH v4 6/7] remote: read $GIT_DIR/branches/* with strbuf_getline() Message-ID: <56D4030E.4030301@moritzneeb.de> URL: https://gitlist.dev/e/56D4030E.4030301%40moritzneeb.de In-Reply-To: <56D401C2.8020100@moritzneeb.de> ``` 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 --- 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 ``` ## Moritz Neeb, 2016-02-29 08:36 Subject: [PATCH v4 7/7] wt-status: read rebase todolist with strbuf_getline() Message-ID: <56D40311.9080603@moritzneeb.de> URL: https://gitlist.dev/e/56D40311.9080603%40moritzneeb.de In-Reply-To: <56D401C2.8020100@moritzneeb.de> ``` 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 --- 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 ``` ## Moritz Neeb, 2016-02-29 08:36 Subject: [PATCH v4 4/7] notes copy --stdin: read lines with strbuf_getline() Message-ID: <56D40314.7040608@moritzneeb.de> URL: https://gitlist.dev/e/56D40314.7040608%40moritzneeb.de In-Reply-To: <56D401C2.8020100@moritzneeb.de> ``` 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 --- 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, 2016-02-29 18:19 Subject: Re: [PATCH v4 4/7] notes copy --stdin: read lines with strbuf_getline() Message-ID: URL: https://gitlist.dev/e/CAPig%2BcSptQr21QMOJmxT4RPVR3r3zkEQ2TkTU8RoaJfo7%3DKChw%40mail.gmail.com In-Reply-To: <56D40314.7040608@moritzneeb.de> ``` On Mon, Feb 29, 2016 at 3:36 AM, Moritz Neeb 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 > --- > 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, 2016-02-29 18:26 Subject: Re: [PATCH v4 0/7] replacing strbuf_getline_lf() by strbuf_getline() Message-ID: URL: https://gitlist.dev/e/CAPig%2BcT4%2BbKS53E6bmLNn%3DxWkXK2Tx8N9bDEqGCNwNy-qjMOUg%40mail.gmail.com In-Reply-To: <56D401C2.8020100@moritzneeb.de> ``` On Mon, Feb 29, 2016 at 3:30 AM, Moritz Neeb 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. ``` ## Junio C Hamano, 2016-02-29 19:01 Subject: Re: [PATCH v4 1/7] quote: remove leading space in sq_dequote_step Message-ID: URL: https://gitlist.dev/e/xmqqy4a39w8b.fsf%40gitster.mtv.corp.google.com In-Reply-To: <56D40301.8020007@moritzneeb.de> ``` Moritz Neeb 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 > --- > 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, 2016-02-29 19:26 Subject: Re: [PATCH v4 4/7] notes copy --stdin: read lines with strbuf_getline() Message-ID: <56D49B7A.2060601@moritzneeb.de> URL: https://gitlist.dev/e/56D49B7A.2060601%40moritzneeb.de In-Reply-To: ``` 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 >> --- >> 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, 2016-02-29 19:48 Subject: Re: [PATCH v4 4/7] notes copy --stdin: read lines with strbuf_getline() Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQwhPvpGmiOa-KJzeDsEJZVp9KJMd2Oj_RjDTq6aEtWXA%40mail.gmail.com In-Reply-To: <56D49B7A.2060601@moritzneeb.de> ``` On Mon, Feb 29, 2016 at 2:26 PM, Moritz Neeb 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. ``` ## Moritz Neeb, 2016-02-29 21:45 Subject: Re: [PATCH v4 1/7] quote: remove leading space in sq_dequote_step Message-ID: <56D4BC14.90301@moritzneeb.de> URL: https://gitlist.dev/e/56D4BC14.90301%40moritzneeb.de In-Reply-To: ``` On 02/29/2016 08:01 PM, Junio C Hamano wrote: > Moritz Neeb 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 >> --- >> 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, 2016-02-29 21:48 Subject: Re: [PATCH v4 1/7] quote: remove leading space in sq_dequote_step Message-ID: <56D4BCB6.90209@moritzneeb.de> URL: https://gitlist.dev/e/56D4BCB6.90209%40moritzneeb.de In-Reply-To: <56D4BC14.90301@moritzneeb.de> ``` On 02/29/2016 10:45 PM, Moritz Neeb wrote: > On 02/29/2016 08:01 PM, Junio C Hamano wrote: >> Moritz Neeb 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 >>> --- >>> 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 > ``` ## Moritz Neeb, 2016-03-09 00:25 Subject: Re: [PATCH v4 0/7] replacing strbuf_getline_lf() by strbuf_getline() Message-ID: <56DF6D67.9040103@moritzneeb.de> URL: https://gitlist.dev/e/56DF6D67.9040103%40moritzneeb.de In-Reply-To: <56D401C2.8020100@moritzneeb.de> ``` 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(-) > ``` ## Junio C Hamano, 2016-03-09 00:39 Subject: Re: [PATCH v4 0/7] replacing strbuf_getline_lf() by strbuf_getline() Message-ID: URL: https://gitlist.dev/e/xmqq37s0jxgg.fsf%40gitster.mtv.corp.google.com In-Reply-To: <56DF6D67.9040103@moritzneeb.de> ``` Moritz Neeb 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, 2016-03-09 01:13 Subject: Re: [PATCH v4 0/7] replacing strbuf_getline_lf() by strbuf_getline() Message-ID: <56DF78BD.2030506@moritzneeb.de> URL: https://gitlist.dev/e/56DF78BD.2030506%40moritzneeb.de In-Reply-To: ``` On 03/09/2016 01:39 AM, Junio C Hamano wrote: > Moritz Neeb 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. ``` ## Eric Sunshine, 2016-03-09 01:17 Subject: Re: [PATCH v4 0/7] replacing strbuf_getline_lf() by strbuf_getline() Message-ID: URL: https://gitlist.dev/e/CAPig%2BcRGH%3Dae2jm4YbwTUpNPx7ZgiZkMf%3DxKCPP%3DEwsr9xJu%2Bg%40mail.gmail.com In-Reply-To: <56DF6D67.9040103@moritzneeb.de> ``` On Tue, Mar 8, 2016 at 7:25 PM, Moritz Neeb 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. ``` ## Junio C Hamano, 2016-03-09 20:28 Subject: Re: [PATCH v4 0/7] replacing strbuf_getline_lf() by strbuf_getline() Message-ID: URL: https://gitlist.dev/e/xmqqr3fjgzuq.fsf%40gitster.mtv.corp.google.com In-Reply-To: <56DF78BD.2030506@moritzneeb.de> ``` Moritz Neeb 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 ;-) ```