Thread (1 message) 1 message, 1 author, 2017-05-24

Re: [PATCH v2 5/7] grep: un-break building with PCRE < 8.32

From: Junio C Hamano <hidden>
Date: 2017-05-24 06:38:31

Junio C Hamano [off-list ref] writes:
So in that sense, I do not think keeping them separate in practice
has the "makes it easier to revert this change" benefit.

If I were doing these two patches, I'd squash them together into
one, rename GIT_PCRE1_CAN_DO_MODERN_JIT to GIT_PCRE1_USE_JIT, and
explain in the log message why we turn it off for versions older
than 8.32 like you did in the log message for thsi patch.  

The reason for the "rename" is because I might also be tempted to
allow users of newer version to manually decline GIT_PCRE1_USE_JIT
in Makefile/config.mak; i.e. we may decide not to USE something even
if we CAN, and the #ifdef symbol you are using is about the decision
to USE or not USE, not necessarily if the library CAN.
Need to say a few things that I forgot to mention.

I said that I do not see practical benefit for keeping the patches
separate.  But at the same time, I do not see it a huge problem that
such a "main one that is partly broken" followed by "fix for one
minority" followed by "fix for another minority" pattern causes to
bisection.  So I'd be OK either way.

Especially, I'd be more than OK if the "main one that is partly broken"
says "by the way, this is (deliberately) left broken for two cases,
and if you hit this during your bisection, do not answer good or bad
and instead reset to a few commits newer that has both fixes" in its
log message.  Then there is no downside in bisection.


Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help