Re: [PATCH] rebase -i: fix numbering in squash message

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 16, 2018, 08:42 UTC
Message-ID: <nycvar.QRO.7.76.6.1808161039300.71@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqqy3d7h21r.fsf@gitster-ct.c.googlers.com>

Hi,

On Wed, 15 Aug 2018, Junio C Hamano wrote:

Show 18 quoted lines

> Phillip Wood <phillip.wood@talktalk.net> 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.

Show 9 quoted lines

> > 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