On 02/01/2016 10:30 PM, Junio C Hamano wrote:
> Moritz Neeb <lists@xxxxxxxxxxxxx> writes:
>
>> The lines read from BISECT_NAMES are trimmed with strbuf_trim()
>> immediately. There is thus no logic expecting CR, so
>> strbuf_getline_lf() can be replaced by its CRLF counterpart.
>
> We do not indent the whole log message.
Thanks for spotting, I will change that in the next iteration.
> You would also want to think about the necessity of strbuf_trim()
> here. Now strbuf_getline() would trim the trailing CR, would we
> still need to call strbuf_trim() here? The code will break if you
> just remove the call, but on the other hand, you will realize that
> the trimming done by calling it is excessive and unnecessary, once
> you inspect the code and learn who writes the file being read here
> and how.
I am not sure what you mean by excessive: How much can I assume that
the input is like expected? The files we are talking about are supposed
to be read and written by git only. But could be modified in theory with
an editor, right? Then things could break, right? This question maybe holds
true for the other patches as well, I still have to look into them.
Assuming that things are just like expected, what has to be done in
read_bisect_paths before dequoting is to remove the leading space,
which is added by 'git rev-parse --sq-quote "$@"' in git-bisect.sh.
I am just not sure, where to remove it:
* in sq_quote_argv? - see some analysis below.
* in read_bisect_paths via strbuf_ltrim instead of strbuf_trim,
is that what you meant?
* in sq_dequote_argv_array? - see below as well.
I tried to understand, why the space is added by sq_quote_argv() in quotes.c
in the first place. It is there since the beginning in 7cf6720. I have the
feeling that the space is redundant for bisect, because the only other codepath
where BISECT_NAMES is read is in bisect_visualize(), where the necessary space
to separate the arguments is added before '$(...)':
eval '"$@"' --bisect -- $(cat "$GIT_DIR/BISECT_NAMES")
Other places where sq_quote_argv() and sq_dequote_to_argv_array() are called
as well are builtin/am.c and trace.c.
In builtin/am.c when reading 'apply-opt', the data is trimmed in read_state_file,
so the space in the beginning is superfluous.
Looking at trace.c it gets clear that this code is dependent on this space,
otherwise the trace output will look like "trace: built-in: git'status'"
which is obviously bogus. It would maybe make more sense to add this space
directly there than to trim it in all the other places. I realized that this
code came up in 7cf6720 as well which explains why things are like they are
(and why I'm CC'ing Christian Couder).
Another option, at the moment my favourite because I think it's the least invasive,
would be to "trim" this one space directly in sq_dequote_step(). See the patch below.
This would not affect the tracing functionality, because there is no dequoting used.
-- >8 --
Subject: [PATCH] quote: remove leading space in sq_dequote_step
Because sq_quote_argv adds a leading space (which is expected in trace.c),
sq_dequote_step should remove this space again, such that the operations
of quoting and dequoting are inverse of each other.
Signed-off-by: Moritz Neeb <lists@xxxxxxxxxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html