Re: [PATCH v3 2/2] add-patch: do not print hunks repeatedly

From: Junio C Hamano <gitster@pobox.com>
To: "Rubén Justo" <rjusto@gmail.com>
Cc: Git List <git@vger.kernel.org>,
	 Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	 Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH v3 2/2] add-patch: do not print hunks repeatedly
Date: Thu, 28 Mar 2024 07:46:00 -0700	[thread overview]
Message-ID: <xmqqmsqixvmf.fsf@gitster.g> (raw)
In-Reply-To: <60c43160-1b60-42f6-9488-4cc332201b7e@gmail.com> (=?utf-8?Q?=22Rub=C3=A9n?= Justo"'s message of "Thu, 28 Mar 2024 02:12:13 +0100")

Rubén Justo <rjusto@gmail.com> writes:

> @@ -1448,10 +1448,15 @@ static int patch_update_file(struct add_p_state *s,
>  
>  		strbuf_reset(&s->buf);
>  		if (file_diff->hunk_nr) {
> -			render_hunk(s, hunk, 0, colored, &s->buf);
> -			fputs(s->buf.buf, stdout);
> +			if (rendered_hunk_index != hunk_index) {

So, the one previously rendered is compared with the current one,
which raises an obvious question, what happens to the first new hunk
resulting from splitting a hunk?  The answer is below ...

> +				render_hunk(s, hunk, 0, colored, &s->buf);
> +				fputs(s->buf.buf, stdout);
> +
> +				rendered_hunk_index = hunk_index;
> +			}
>  
>  			strbuf_reset(&s->buf);
> +
>  			if (undecided_previous >= 0) {
>  				permitted |= ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK;
>  				strbuf_addstr(&s->buf, ",k");
> @@ -1649,10 +1654,12 @@ static int patch_update_file(struct add_p_state *s,
>  			if (!(permitted & ALLOW_SPLIT))
>  				err(s, _("Sorry, cannot split this hunk"));
>  			else if (!split_hunk(s, file_diff,
> -					     hunk - file_diff->hunk))
> +					     hunk - file_diff->hunk)) {
>  				color_fprintf_ln(stdout, s->s.header_color,
>  						 _("Split into %d hunks."),
>  						 (int)splittable_into);
> +				rendered_hunk_index = -1;
> +			}

... we explicitly say "we always want to show the current one after
this operation", which makes sense.

>  		} else if (s->answer.buf[0] == 'e') {
>  			if (!(permitted & ALLOW_EDIT))
>  				err(s, _("Sorry, cannot edit this hunk"));
> @@ -1661,7 +1668,7 @@ static int patch_update_file(struct add_p_state *s,
>  				goto soft_increment;
>  			}
>  		} else if (s->answer.buf[0] == 'p') {
> -			/* nothing special is needed */
> +			rendered_hunk_index = -1;

And that matches what is done for 'p', which is the base case that
wants to say "no matter what, show the current one".  Doubly makes
sense.

>  		} else {
>  			const char *p = _(help_patch_remainder), *eol = p;

Looking good.  As we are not doing anything dynamic to the help
text, I think dropping "again" in [1/2] would make sense.

Will queue.  Thanks.