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

Phillip Wood [off-list ref] writes:

quoted

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

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.

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.

Thanks.

quoted

 sequencer.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 77d3c2346f..f82c283a89 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -1331,8 +1331,9 @@ static int update_squash_messages(enum todo_command command,
 	struct strbuf buf = STRBUF_INIT;
 	int res;
 	const char *message, *body;
+	int fixup_count = opts->current_fixup_count + 2;
 
-	if (opts->current_fixup_count > 0) {
+	if (fixup_count > 2) {
 		struct strbuf header = STRBUF_INIT;
 		char *eol;
 

@@ -1345,7 +1346,7 @@ static int update_squash_messages(enum todo_command command,
 
 		strbuf_addf(&header, "%c ", comment_line_char);
 		strbuf_addf(&header, _("This is a combination of %d commits."),
-			    opts->current_fixup_count + 2);
+			    fixup_count);
 		strbuf_splice(&buf, 0, eol - buf.buf, header.buf, header.len);
 		strbuf_release(&header);
 	} else {

@@ -1387,18 +1388,19 @@ static int update_squash_messages(enum todo_command command,
 		unlink(rebase_path_fixup_msg());
 		strbuf_addf(&buf, "\n%c ", comment_line_char);
 		strbuf_addf(&buf, _("This is the commit message #%d:"),
-			    ++opts->current_fixup_count + 1);
+			    fixup_count);
 		strbuf_addstr(&buf, "\n\n");
 		strbuf_addstr(&buf, body);
 	} else if (command == TODO_FIXUP) {
 		strbuf_addf(&buf, "\n%c ", comment_line_char);
 		strbuf_addf(&buf, _("The commit message #%d will be skipped:"),
-			    ++opts->current_fixup_count + 1);
+			    fixup_count);
 		strbuf_addstr(&buf, "\n\n");
 		strbuf_add_commented_lines(&buf, body, strlen(body));
 	} else
 		return error(_("unknown command: %d"), command);
 	unuse_commit_buffer(commit, message);
+	opts->current_fixup_count++;
 
 	res = write_message(buf.buf, buf.len, rebase_path_squash_msg(), 0);
 	strbuf_release(&buf);

Hi,

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

Phillip Wood [off-list ref] writes:

quoted

quoted

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.

quoted

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