Moritz Neeb [off-list ref] writes:
quoted
You would also want to think about the necessity of strbuf_trim()
here. Now strbuf_getline() would trim the trailing CR, would we
still need to call strbuf_trim() here? The code will break if you
just remove the call, but on the other hand, you will realize that
the trimming done by calling it is excessive and unnecessary, once
you inspect the code and learn who writes the file being read here
and how.
I am not sure what you mean by excessive: How much can I assume that
the input is like expected? The files we are talking about are supposed
to be read and written by git only. But could be modified in theory with
an editor, right? Then things could break, right? This question maybe holds
true for the other patches as well, I still have to look into them.
These are all good questions you as a Git contributor to be asking
yourself, and I really like the fact that you are thinking aloud
here.
As to bisect pathspec, while I do not think it is sensible to change
it in the middle of bisection, I do not think it would cause the
bisect command to produce an incorrect result if you did so. You
would be changing the definition of what is the "middle point" of
the history on the fly by changing the pathspec, hence you may end
up making the bisection suboptimal, but it shouldn't affect the
correctness of the bisection.
So I tend to agree with you that it is a good idea to be prepared to
parse what the end user may have futzed with the editor, as long as
the user did not break the syntax of the quoted string.
Your answer to the question quoted at the top of this message could
be a summary of the above reasoning, with a conclusion: "we would
want to tolerate trailing (or leading) whitespaces the user might
add with his editor; even though we use strbuf_getline() to remove
the CR at the end of line, we still need to strbuf_trim() the line
we read".
Side note: of course, you can instead say "while it is
technically possible, we have never advertised this file to
be editable by user, or encouraged them to do so" and
tighten the parsing to assume only what we write out,
erroring out when we see any attempt to edit it.
Either way, the most important thing is to record the reasoning
behind the design choice you made clearly in the proposed commit log
message.
Thanks.
On 02/15/2016 06:05 AM, Junio C Hamano wrote:
Moritz Neeb [off-list ref] writes:
quoted
quoted
You would also want to think about the necessity of strbuf_trim()
here. Now strbuf_getline() would trim the trailing CR, would we
still need to call strbuf_trim() here? The code will break if you
just remove the call, but on the other hand, you will realize that
the trimming done by calling it is excessive and unnecessary, once
you inspect the code and learn who writes the file being read here
and how.
I am not sure what you mean by excessive: How much can I assume that
the input is like expected? The files we are talking about are supposed
to be read and written by git only. But could be modified in theory with
an editor, right? Then things could break, right? This question maybe holds
true for the other patches as well, I still have to look into them.
These are all good questions you as a Git contributor to be asking
yourself, and I really like the fact that you are thinking aloud
here.
Thanks for this feedback I will try to continue this thinking aloud
whenever appropriate.
Sorry by the way for only answering every week - I currently only have
time to work on git during the weekend. What is (assuming familiarity
with the codebase) the expected "cooking"-time of a smaller patch? I'm
sure the answer is "depends", but maybe you can give away some experiences.
On 02/15/2016 06:05 AM, Junio C Hamano wrote:
As to bisect pathspec, while I do not think it is sensible to change
it in the middle of bisection, I do not think it would cause the
bisect command to produce an incorrect result if you did so. You
would be changing the definition of what is the "middle point" of
the history on the fly by changing the pathspec, hence you may end
up making the bisection suboptimal, but it shouldn't affect the
correctness of the bisection.
Yep I also think it would not affect the correctness. To reformulate
your explanation: changing the pathspec does not change the good and bad
"frontiers". And those should be still correct under the assumption
you're still looking for the same bug/regression.
As to the question if it is sensible, I also don't really think it could
be helpful. The only way I would expect it to be useful, it so "refine"
the search by adding specific files to the filepath. But then it's
possible to end up in a dead-end if the commits left are not touching
the specified files with:
No testable commit found.
Maybe you started with bad path parameters?
I have also seen other messed up messages, like "-1 commits left" while
playing around.
So I tend to agree with you that it is a good idea to be prepared to
parse what the end user may have futzed with the editor, as long as
the user did not break the syntax of the quoted string.
Now I am not convinced anymore, because I think if it potentially leads
to troubles, it should not be made easier to get oneself into them.
Side note: of course, you can instead say "while it is
technically possible, we have never advertised this file to
be editable by user, or encouraged them to do so" and
tighten the parsing to assume only what we write out,
erroring out when we see any attempt to edit it.
I will include something like that in the patch v2 that is to follow soon.