On 11/2/2018 2:53 PM, Elijah Newren wrote:
Major question:
* You'll note that I edited the last two patches to mark them as RFC.
To be honest, I'm not sure what to do with these. They improve code
coverage of new code, but the same gaps existed in the old code;
they only show up in the coverage-diff because I essentially moved
code around to a new improved function. Since the new code doesn't
really add new abilities but rather just shifts the handling of
these conflicts to a common function, they shouldn't need any more
testcases than previously and modifying the existing patches thus
feels slightly misleading. That line of thought leads me to believe
that perhaps putting them in a separate combined patch of their own
with a decent commit message is the right way to go. On the other
hand, squashing them to the commits they're marked as fixups for
shows which logical part of the code the tests are related to, which
seems like a good thing. So what's the right way to handle these?
I appreciate the effort you made to improve test coverage! It's
unfortunate that this portion wasn't covered earlier, because we could
have broken it and not noticed until a release.
I think making them separate commits is fine, and the comment on the
test case is helpful. The fact that you only had to change the commit
timestamps in order to get the coverage makes me reexamine the code and
realize that maybe the "right" thing to do is to reduce our code clones.
(This is also how I was looking at the wrong block of the patch when
talking about it not being covered.) I'll look at the patch and see if I
can contribute a concrete code suggestion there.
Aside: I hope that I am not being annoying by poking around with the
test coverage reports. It does give me a way to target my review
efforts, especially into changes that touch areas outside my expertise
(like this one).
Thanks,
-Stolee
