Re: [PATCH 4/4] restore: add --intent-to-add (restoring worktree only)

[Derrick Stolee <stolee@gmail.com>]

On 6/20/2019 5:55 AM, Nguyễn Thái Ngọc Duy wrote:
> "git restore --source" (without --staged) could create new files
> (i.e. not present in index) on worktree to match the given source. But
> the new files are not tracked, so both "git diff" and "git diff
> <source>" ignore new files. "git commit -a" will not recreate a commit
> exactly as the given source either.
> 
> Add --intent-to-add to help track new files in this case, which is the
> default on the least surprise principle.

I was unfamiliar with this behavior, but did check the 'restore' command
myself and saw that it would register the file as untracked. I agree that
could be confusing for a user, so adding it to the staging environment
makes this more in-line with `git checkout <rev> -- <path>`.

> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Documentation/git-restore.txt |  7 ++++
>  builtin/checkout.c            | 78 +++++++++++++++++++++++++++++++++++
>  t/t2070-restore.sh            | 17 ++++++++
>  3 files changed, 102 insertions(+)
> 
> diff --git a/Documentation/git-restore.txt b/Documentation/git-restore.txt
> index d90093f195..43a7f43b2b 100644
> --- a/Documentation/git-restore.txt
> +++ b/Documentation/git-restore.txt
> @@ -93,6 +93,13 @@ in linkgit:git-checkout[1] for details.
>  	are "merge" (default) and "diff3" (in addition to what is
>  	shown by "merge" style, shows the original contents).
>  
> +--intent-to-add::
> +--no-intent-to-add::
> +	When restoring files only on working tree with `--source`,
> +	new files are marked as "intent to add" (see
> +	linkgit:git-add[1]). This is the default behavior. Use
> +	`--no-intent-to-add` to disable it.
> +
>  --ignore-unmerged::
>  	When restoring files on the working tree from the index, do
>  	not abort the operation if there are unmerged entries and
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index f884d27f1f..c519067d3d 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -70,6 +70,7 @@ struct checkout_opts {
>  	int checkout_worktree;
>  	const char *ignore_unmerged_opt;
>  	int ignore_unmerged;
> +	int intent_to_add;
>  
>  	const char *new_branch;
>  	const char *new_branch_force;
> @@ -392,6 +393,69 @@ static int checkout_worktree(const struct checkout_opts *opts)
>  	return errs;
>  }
>  
> +/*
> + * Input condition: r->index contains the file list matching worktree.
> + *
> + * r->index is reloaded with $GIT_DIR/index. Files that exist in the
> + * current worktree but not in $GIT_DIR/index are added back as
> + * intent-to-add.
> + */

Reading this code (and being unfamiliar with the cache array) I thought
it might accidentally add untracked files from the working directory into
the index. A local test verified that was not the case. Is that worth
adding to your test below?
  
> +test_expect_success 'restore worktree only adds new files back as intent-to-add' '
> +	git init ita &&
> +	(
> +		cd ita &&
> +		test_commit one &&
> +		test_commit two &&
> +		git rm one.t &&
> +		git commit -m one-is-gone &&
+		touch garbage &&
> +		git restore --source one one.t &&
> +		git diff --summary >actual &&
> +		echo " create mode 100644 one.t" >expected &&
> +		test_cmp expected actual &&
> +		git diff --cached >empty &&
> +		test_must_be_empty empty
> +	)
> +'
> +
>  test_done

Perhaps the line I inserted above would suffice to add this extra check?

Outside of that extra test (which may not be necessary), this series makes
sense to me.

Thanks,
-Stolee