Re: [PATCH 0/8] build-in add -i: implement all commands in the main loop

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 18, 2019, 18:53 UTC
Message-ID: <nycvar.QRO.7.76.6.1911181940020.46@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqqlfsd3omz.fsf@gitster-ct.c.googlers.com>

Hi Junio,

On Mon, 18 Nov 2019, Junio C Hamano wrote:

Show 16 quoted lines

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > ... there is so little overlap between the git add --patch part
> > and the rest of git add --interactive that I could put the former
> > into a totally different file: add-patch.c.
>
> We would want to call, after "add -i in C" stabilizes, directly into
> "add -p" machinery from different subcommands such as "reset",
> "checkout", etc., and bypass the rest of "add -i" when we do so.  We
> can credit the lack of overlap to the design that supports such
> usage while it is still in a scripted Porcelain.
>
> Preparing the "add -p" machinery in such a way that "add -i" merely
> is one of the users would make a lot of sense from organizational
> point of view.

If I understand you correctly, you mean that e.g. `git checkout -p` will call the code from `add-patch.c` directly? That is indeed the case. Already now, `git checkout -p` calls `run_add_interactive()` (see https://github.com/git/git/blob/v2.24.0/builtin/checkout.c#L463):

	if (opts->patch_mode) {
		const char *patch_mode;

		if (opts->checkout_index && opts->checkout_worktree)
			patch_mode = "--patch=checkout";
		else if (opts->checkout_index && !opts->checkout_worktree)
			patch_mode = "--patch=reset";
		else if (!opts->checkout_index && opts->checkout_worktree)
			patch_mode = "--patch=worktree";
		else
			BUG("either flag must have been set, worktree=%d, index=%d",
			    opts->checkout_worktree, opts->checkout_index);
		return run_add_interactive(revision, patch_mode, &opts->pathspec);
	}

And with the changes in PR #174 (i.e. three patch series later, see https://github.com/gitgitgadget/git/pull/174/files#diff-1f1eb09e4703764f9f9b9f1f1e67eb97R205-R218 for details), this will call `run_add_p()`, i.e. the newly-added code in `add-patch.c` directly:

		if (!strcmp(patch_mode, "--patch"))
			mode = ADD_P_STAGE;
		else if (!strcmp(patch_mode, "--patch=stash"))
			mode = ADD_P_STASH;
		else if (!strcmp(patch_mode, "--patch=reset"))
			mode = ADD_P_RESET;
		else if (!strcmp(patch_mode, "--patch=checkout"))
			mode = ADD_P_CHECKOUT;
		else if (!strcmp(patch_mode, "--patch=worktree"))
			mode = ADD_P_WORKTREE;
		else
			die("'%s' not supported", patch_mode);

		return !!run_add_p(the_repository, mode, revision, pathspec);

Now that I see it, it does not look the most elegant that the `patch_mode` is a string instead of that enum, but I think that will be relatively easy to fix once the legacy `git-add--interactive.perl` will have been decomissioned.

Ciao, Dscho