Re: [PATCH v2 1/1] Allow reworking with a file after deciding on all its hunks

[Junio C Hamano <gitster@pobox.com>]

Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:

> diff --git a/add-patch.c b/add-patch.c
> index 173a53241e..edb2fab3fd 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1418,6 +1418,8 @@ N_("j - go to the next undecided hunk, roll over at the bottom\n"
>     "e - manually edit the current hunk\n"
>     "p - print the current hunk\n"
>     "P - print the current hunk using the pager\n"
> +   "> - go to the next file\n"
> +   "< - go to the previous file\n"
>     "? - print help\n");

As I said earlier, these may have to be optional.  It may give
existing users a jarring experience to be given a prompt after
deciding on all the hunks in a file, when they expect to be on
the next file already.

> @@ -1441,6 +1443,17 @@ static bool get_first_undecided(const struct file_diff *file_diff, size_t *idx)
>  	return false;
>  }
>  
> +static size_t get_file_diff_index(struct add_p_state *s, struct file_diff *file_diff) {
> +	size_t idx = 0;
> +	for (size_t i = 0; i < s->file_diff_nr; i++) {
> +		if (s->file_diff + i == file_diff) {
> +			idx = i;
> +			break;
> +		}
> +	}
> +	return idx;
> +}

Yuck.  Can't we lose the need for this function if we change the
interface into patch_update_file so that it takes the index of the
file (i.e., instead of "&s.file_diff[i]", pass "i")?  There is only
one caller to patch_update_file() which is run_add_p(), so such a
clean-up should be trivial.

>  static int patch_update_file(struct add_p_state *s,
>  			     struct file_diff *file_diff)
>  {
> @@ -1448,9 +1461,10 @@ static int patch_update_file(struct add_p_state *s,
>  	ssize_t i, undecided_previous, undecided_next, rendered_hunk_index = -1;
>  	struct hunk *hunk;
>  	char ch;
> -	struct child_process cp = CHILD_PROCESS_INIT;

This is related to the hoisting of the actual patch application to
the caller, but it is not explained why such a change is needed, and
it byitself, even without the "jump to the next file before deciding
on all the hunks" feature.  What problem is it solving???

If it is necessary to move the code to run "git apply" to the
caller, would it make sense to split this patch into at least two
patches, one to do such a move, possibly another patch to change the
function signature of patch_update_file() so that it takes the file
index instead of file_diff struct, and finally another patch to
allow jumping around the files?

>  	int colored = !!s->colored.len, quit = 0, use_pager = 0;
>  	enum prompt_mode_type prompt_mode_type;
> +	size_t file_diff_index = get_file_diff_index(s, file_diff);
> +	int all_decided = 0;
>  
>  	/* Empty added files have no hunks */
>  	if (!file_diff->hunk_nr && !file_diff->added)
> @@ -1467,7 +1481,9 @@ static int patch_update_file(struct add_p_state *s,
>  			ALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3,
>  			ALLOW_SEARCH_AND_GOTO = 1 << 4,
>  			ALLOW_SPLIT = 1 << 5,
> -			ALLOW_EDIT = 1 << 6
> +			ALLOW_EDIT = 1 << 6,
> +			ALLOW_GOTO_PREVIOUS_FILE = 1 << 7,
> +			ALLOW_GOTO_NEXT_FILE = 1 << 8
>  		} permitted = 0;
>  
>  		if (hunk_index >= file_diff->hunk_nr)
> @@ -1499,8 +1515,7 @@ static int patch_update_file(struct add_p_state *s,
>  		/* Everything decided? */
>  		if (undecided_previous < 0 && undecided_next < 0 &&
>  		    hunk->use != UNDECIDED_HUNK)
> -			break;
> -
> +				all_decided = 1;
>  		strbuf_reset(&s->buf);
>  		if (file_diff->hunk_nr) {
>  			if (rendered_hunk_index != hunk_index) {
> @@ -1548,6 +1563,16 @@ static int patch_update_file(struct add_p_state *s,
>  				permitted |= ALLOW_EDIT;
>  				strbuf_addstr(&s->buf, ",e");
>  			}
> +			if (file_diff_index >= 0 &&
> +				file_diff_index < s->file_diff_nr - 1) {
> +				permitted |= ALLOW_GOTO_NEXT_FILE;
> +				strbuf_addstr(&s->buf, ",>");
> +			}
> +			if (file_diff_index > 0 &&
> +				file_diff_index <= s->file_diff_nr - 1) {
> +				permitted |= ALLOW_GOTO_PREVIOUS_FILE;
> +				strbuf_addstr(&s->buf, ",<");
> +			}

As can be seen in what patch_update_file() does when the user says
'J' or 'K', hunks in a file are treated as a ring, and these
commands are enabled as long as there are more than one hunks.

Perhaps that is more familiar than "when we hit the floor, we cannot
sink deeper, and when we hit the ceiling, we cannot float more",
which seems to be what the above implements.

>  			strbuf_addstr(&s->buf, ",p,P");
>  		}
>  		if (file_diff->deleted)
> @@ -1566,6 +1591,9 @@ static int patch_update_file(struct add_p_state *s,
>  						: 1));
>  		printf(_(s->mode->prompt_mode[prompt_mode_type]),
>  		       s->buf.buf);
> +		if (all_decided)
> +			printf(_("\n%s All hunks decided. What now? "),
> +				s->s.prompt_color);
>  		if (*s->s.reset_color_interactive)
>  			fputs(s->s.reset_color_interactive, stdout);
>  		fflush(stdout);
> @@ -1618,7 +1646,24 @@ static int patch_update_file(struct add_p_state *s,
>  		} else if (ch == 'q') {
>  			quit = 1;
>  			break;
> -		} else if (s->answer.buf[0] == 'K') {
> +		} else if (s->answer.buf[0] == '>') {
> +			if (permitted & ALLOW_GOTO_NEXT_FILE) {
> +				quit = 0;
> +				break;
> +			} else {
> +				err(s, _("No next file"));
> +				continue;
> +			}
> +		} else if (s->answer.buf[0] == '<') {
> +			if (permitted & ALLOW_GOTO_PREVIOUS_FILE) {
> +				quit = 2;
> +				break;

What's the magic number "2"?  Should "quit" become an enum with
elements that are more meaningfully named?

> +			} else {
> +				err(s, _("No previous file"));
> +				continue;
> +			}
> +		}
> +		else if (s->answer.buf[0] == 'K') {
>  			if (permitted & ALLOW_GOTO_PREVIOUS_HUNK)
>  				hunk_index = dec_mod(hunk_index,
>  						     file_diff->hunk_nr);
> @@ -1775,33 +1820,6 @@ static int patch_update_file(struct add_p_state *s,
>  		}
>  	}
>  
> -	/* Any hunk to be used? */
> -	for (i = 0; i < file_diff->hunk_nr; i++)
> -		if (file_diff->hunk[i].use == USE_HUNK)
> -			break;
> -
> -	if (i < file_diff->hunk_nr ||
> -	    (!file_diff->hunk_nr && file_diff->head.use == USE_HUNK)) {
> -		/* At least one hunk selected: apply */
> -		strbuf_reset(&s->buf);
> -		reassemble_patch(s, file_diff, 0, &s->buf);
> -
> -		discard_index(s->s.r->index);
> -		if (s->mode->apply_for_checkout)
> -			apply_for_checkout(s, &s->buf,
> -					   s->mode->is_reverse);
> -		else {
> -			setup_child_process(s, &cp, "apply", NULL);
> -			strvec_pushv(&cp.args, s->mode->apply_args);
> -			if (pipe_command(&cp, s->buf.buf, s->buf.len,
> -					 NULL, 0, NULL, 0))
> -				error(_("'git apply' failed"));
> -		}
> -		if (repo_read_index(s->s.r) >= 0)
> -			repo_refresh_and_write_index(s->s.r, REFRESH_QUIET, 0,
> -						     1, NULL, NULL, NULL);
> -	}

It is not obvious why the above code needs to be hoisted to the
caller.  

>  	putchar('\n');
>  	return quit;
>  }
> @@ -1813,7 +1831,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
>  	struct add_p_state s = {
>  		{ r }, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT
>  	};
> -	size_t i, binary_count = 0;
> +	size_t i, j, binary_count = 0;
> +	size_t patch_update_result = 0;

Hmph, I think patch_update_file() returns "int quit".  Why do we
want overly wide type to store the result, which cannot even express
negative number to potentially signal a failure?

> +	struct child_process cp = CHILD_PROCESS_INIT;
>  
>  	init_add_i_state(&s.s, r, o);
>  
> @@ -1852,11 +1872,56 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
>  		return -1;
>  	}
>  
> -	for (i = 0; i < s.file_diff_nr; i++)
> -		if (s.file_diff[i].binary && !s.file_diff[i].hunk_nr)
> +	for (i = 0; i < s.file_diff_nr;) {
> +		if (s.file_diff[i].binary && !s.file_diff[i].hunk_nr) {
>  			binary_count++;
> -		else if (patch_update_file(&s, s.file_diff + i))
> -			break;
> +			i++;
> +			continue;
> +		}
> +		else {
> +			patch_update_result = patch_update_file(&s, s.file_diff + i);
> +			if (patch_update_result == 0) {
> +				i++;
> +				continue;
> +			}
> +			if (patch_update_result == 1)
> +				break;
> +			if (patch_update_result == 2) {
> +				i--;
> +				continue;
> +			}
> +		}
> +	}
> +	for (i = 0; i < s.file_diff_nr; i++) {
> +
> +			/* Any hunk to be used? */
> +		for (j = 0; j < s.file_diff[i].hunk_nr; j++)
> +			if (s.file_diff[i].hunk[j].use == USE_HUNK)
> +				break;
> +
> +		if (j < s.file_diff[i].hunk_nr ||
> +	    (!s.file_diff[i].hunk_nr && s.file_diff[i].head.use == USE_HUNK)) {
> +			/* At least one hunk selected: apply */
> +			strbuf_reset(&s.buf);
> +			reassemble_patch(&s, s.file_diff + i, 0, &s.buf);
> +
> +			discard_index(s.s.r->index);
> +			if (s.mode->apply_for_checkout)
> +				apply_for_checkout(&s, &s.buf,
> +						s.mode->is_reverse);
> +			else {
> +				setup_child_process(&s, &cp, "apply", NULL);
> +				strvec_pushv(&cp.args, s.mode->apply_args);
> +				if (pipe_command(&cp, s.buf.buf, s.buf.len,
> +						NULL, 0, NULL, 0))
> +					error(_("'git apply' failed"));
> +			}
> +			if (repo_read_index(s.s.r) >= 0)
> +				repo_refresh_and_write_index(s.s.r, REFRESH_QUIET, 0,
> +								1, NULL, NULL, NULL);
> +		}
> +
> +	}

One upside of having "git apply" at the end of patch_update_file()
is that you can "^C" out of "git add -p" or your terminal connection
can be cut off, after dealing with hunks in a few early files, and
these early part of your work that you have already done are already
reflected to the working tree files.  By hoisting the logic to the
caller, this is making the update all-or-none, which is good in
transactional systems, but can make a horrible experience for an
interactive use where you make progress while thinking.

So I am not yet convinced if this change makes sense---it could be
because of the lack of justification for this change.



>  
>  	if (s.file_diff_nr == 0)
>  		err(&s, _("No changes."));