Johannes Schindelin [off-list ref] writes:
Changes vs v4:
I still do not understand (note that I am not saying "I do not
accept"--acceptance or rejection happens after an understandable
explanation is given, and "do not understand" means no such
explanation has been given yet) your justification behind adding a
technical debt to reimplement the author-script parser and not
sharing it with "git am" in 13/27.
As I pointed out, I think 22/27 is out of place in this series.
But other than these two points, the changes since v4 look minimum,
and they all looked good to me.
Thanks.
Hi Junio,
On Fri, 21 Oct 2016, Junio C Hamano wrote:
Johannes Schindelin [off-list ref] writes:
I still do not understand (note that I am not saying "I do not
accept"--acceptance or rejection happens after an understandable
explanation is given, and "do not understand" means no such
explanation has been given yet) your justification behind adding a
technical debt to reimplement the author-script parser and not
sharing it with "git am" in 13/27.
At this point, I am most of all reluctant to introduce such a huge change,
which surely would introduce a regression.
This is what happened a couple of times to me, most recently with the
hide-dot-gitdir patch series that worked flawlessly for years, had to be
dramatically changed during review to enter git.git, and introduced the
major regression that `core.hideDotFiles = gitDirOnly` was broken.
The lesson I learned: review should not be valued more than the test of
time. This lesson has been reinforced by all the regressions that have not
been caught by review nor the test suite running on Linux only.
It would be a different matter if I still had the cross-validator in place
(which I did when I sent out v1 of this patch series) and tons of time to
spend on accommodating your wishes, however I may disagree with them. And
in this instance, I thought I made clear that I disagree, and why:
Internally, git-am and git-rebase-i handle the author-script very
differently. That may change at some stage in the future, and it would be
a good time then and there to take care of unifying this code. Currently,
not so much, as the only excuse to use the same parser would be that they
both read the same file, while they have to do very different things with
the parsed output (in fact, your suggestion would ask the parser in the
sequencer to rip apart the information into key/value pairs, only to
re-glue them back together when they are used as the environment variables
as which rebase-i treats the contents of the author-script file).
So no, at this point I am not willing to risk introducing breakages in
code that has been proven to work in practice.
Ciao,
Dscho
Johannes Schindelin [off-list ref] writes:
quoted
I still do not understand (note that I am not saying "I do not
accept"--acceptance or rejection happens after an understandable
explanation is given, and "do not understand" means no such
explanation has been given yet) your justification behind adding a
technical debt to reimplement the author-script parser and not
sharing it with "git am" in 13/27.
At this point, I am most of all reluctant to introduce such a huge change,
which surely would introduce a regression.
That is a perfectly sensible and honest answer that I can
understand, accept and even support.
You've been working on the series for several months, running with
these dozen or so lines of code, and replacing it with another dozen
or so lines of code would require you to make sure the result is
actually doing the same thing for the remainder of your series. And
I agree that is an unnecessary risk in order to ship a working code.
The code being battle tested counts.
I cared on this point mostly because I wanted to make sure that
people later can find out why there are two functions that ought to
be doing the same thing.
If there were a technical reason why these two must stay to be
different implementations that are potentially doing different
things, I want to see that reason described, so that those who come
later and want to refactor the helper functions into one later will
know why they are separate in the first place.
If on the other hand there isn't any technical reason why they must
stay to be different, and they are different mostly because you
happened to have written a different one in 13/27 and have been
running with it, that is also a good thing for them to know.