Hi, On Wed, 15 Aug 2018, Junio C Hamano wrote: > Phillip Wood <phillip.wood@xxxxxxxxxxxx> writes: > > >> I wonder if it makes it easier to read, understand and maintain if > >> there were a local variable that gets opts->current_fixup_count+2 at > >> the beginning of the function, make these three places refer to that > >> variable, and move the increment of opts->current_fixup_count down > >> in the function, after the "if we are squashing, do this, if we are > >> fixing up, do that, otherwise, we do not know what we are doing" > >> cascade. And use the more common post-increment, as we no longer > >> depend on the returned value while at it. > >> > >> IOW, something like this (untested), on top of yours. > > > > I think you'd need to change commit_staged_changes() as well as it > > relies on the current_fixup_count counting the number of fixups, not the > > number of fixups plus two. > > I suspect you misread what I wrote (see below for the patch). I had the same reaction as Phillip: is your patch good enough, or does it only touch one part, but not other that may need the same "touch-upping". > The fixup_count is a new local variable in update_squash_messages() > that holds N+2; in other words, I am not suggesting to change what > opts->current_fixup_count means. Sure, and the better cleanup could possibly be to change the meaning of opts->current_fixup_count altogether. > > Having said that using 'current_fixup_count + > > 2' to create the labels and incrementing the count at the end of > > update_squash_messages() would probably be clearer than my version. I'm > > about to go away so it'll probably be the second week of September > > before I can re-roll this, will that be too late for getting it into 2.19? > > I actually do not mind to go with what you originally sent, unless a > cleaned up version is vastly more readable. After all, a clean-up > can be done separately and safely. At this point, I think Phillip's version would be safer, as it would make it easier to do a more complete cleanup without the pressure of having to fix a bug in one big hurry. So: ACK on Phillip's patch from me. Ciao, Dscho
