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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 28, 2024, 14:46 UTC
Message-ID: <xmqqmsqixvmf.fsf@gitster.g>
In-Reply-To: <60c43160-1b60-42f6-9488-4cc332201b7e@gmail.com>

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

Show 7 quoted lines

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

Show 22 quoted lines

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

Show 9 quoted lines

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