Re: [PATCH 1/2] rebase -i: fix counting of fixups after rebase --skip

Phillip Wood <[email protected]> writes:

> @@ -3281,7 +3281,13 @@ static int read_populate_opts(struct replay_opts *opts)
>  			const char *p = ctx->current_fixups.buf;
>  			ctx->current_fixup_count = 1;
>  			while ((p = strchr(p, '\n'))) {
> -				ctx->current_fixup_count++;
> +				/*
> +				 * Older versions of git accidentally
> +				 * inserted blank lines when a fixup
> +				 * was skipped.
> +				 */
> +				if (p[1] != '\n')
> +					ctx->current_fixup_count++;
>  				p++;
>  			}
>  		}

If we hit the LF at the very end (e.g. "fixup A\n" at the end of the
file), strchr() would have moved p to the newline, and p[1] will be
'\0', no?  And because p[1] != '\n' and wouldn't current_fixup_count
be incremented again?  It might be safer to check p[1] != '\n' &&
p[1] != '\0' to avoid counting a trailing newline as an extra
command when reading legacy files.

> @@ -5353,6 +5359,9 @@ static int commit_staged_changes(struct repository *r,
>  			if (!len)
>  				BUG("Incorrect current_fixups:\n%s", p);
>  			while (len && p[len - 1] != '\n')
> +				len--;
> +			/* Remove trailing newline */
> +			if (len)
>  				len--;

So we removed all the non newline from the end, and the loop would
break if !len or p[len - 1] == '\n'.  And in the latter case, we
also drop that '\n'.  Which sounds right.

Thanks.