Re: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.

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

Kevin Willford <kewillf@microsoft.com> writes:

> diff --git a/builtin/reset.c b/builtin/reset.c
> index d72c7d1c96..1b8bb45989 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -24,6 +24,7 @@
>  #include "cache-tree.h"
>  #include "submodule.h"
>  #include "submodule-config.h"
> +#include "dir.h"
>  
>  static const char * const git_reset_usage[] = {
>  	N_("git reset [--mixed | --soft | --hard | --merge | --keep] [-q] [<commit>]"),
> @@ -124,8 +125,32 @@ static void update_index_from_diff(struct diff_queue_struct *q,
>  
>  	for (i = 0; i < q->nr; i++) {
>  		struct diff_filespec *one = q->queue[i]->one;
> +		struct diff_filespec *two = q->queue[i]->two;
> +		int pos;
>  		int is_missing = !(one->mode && !is_null_oid(&one->oid));
> +		int was_missing = !two->mode && is_null_oid(&two->oid);
>  		struct cache_entry *ce;
> +		struct cache_entry *ceBefore;
> +		struct checkout state = CHECKOUT_INIT;

The five new variables are only used in the new block, so it
probably is better to limit their scope to the "we do something
unusual when sparse checkout is in effect" block as well.  The scope
for the latter two can further be narrowed down to "we do need to
force a checkout of this entry" block.

We prefer to name our variables with underscore (e.g. ce_before)
over camelCase (e.g. ceBefore) unless there is a compelling reason
(e.g. a platform specific code in compat/ layer to match platform
convention).

> +
> +		if (core_apply_sparse_checkout && !file_exists(two->path)) {
> +			pos = cache_name_pos(two->path, strlen(two->path));
> +			if ((pos >= 0 && ce_skip_worktree(active_cache[pos])) &&
> +			    (is_missing || !was_missing))
> +			{
> +				state.force = 1;
> +				state.refresh_cache = 1;
> +				state.istate = &the_index;
> +				ceBefore = make_cache_entry(two->mode,
> +							    two->oid.hash,
> +							    two->path, 0, 0);
> +				if (!ceBefore)
> +					die(_("make_cache_entry failed for path '%s'"),
> +						two->path);
> +
> +				checkout_entry(ceBefore, &state, NULL);
> +			}
> +		}

Can we tell between the case where the reason why the path was not
there in the working tree was due to the path being excluded by the
sparse checkout and the path being removed manually by the end user?

I guess ce_skip_worktree() check is sufficient; we force checkout only
when the path is marked to be skipped due to "sparse" thing.

Do we have to worry about the reverse case, in which file_exists(two->path)
is true (i.e. the user created a file there manually) even though
the path is marked to be skipped due to "sparse" setting?

Other than that, the patch looks quite cleanly done and well explained.

Thanks.

> diff --git a/t/t7114-reset-sparse-checkout.sh b/t/t7114-reset-sparse-checkout.sh
> new file mode 100755
> index 0000000000..f2a5426847
> --- /dev/null
> +++ b/t/t7114-reset-sparse-checkout.sh
> @@ -0,0 +1,60 @@
> +#!/bin/sh
> +
> +test_description='reset when using a sparse-checkout'
> +
> +. ./test-lib.sh
> +
> +# reset using a sparse-checkout file
> +
> +test_expect_success 'setup' '
> +	test_tick &&
> +	echo "checkout file" >c &&
> +	echo "modify file" >m &&
> +	echo "delete file" >d &&
> +	git add . &&
> +	git commit -m "initial commit" &&
> +	echo "added file" >a &&
> +	echo "modification of a file" >m &&
> +	git rm d &&
> +	git add . &&
> +	git commit -m "second commit" &&
> +	git checkout -b endCommit
> +'
> +
> +test_expect_success 'reset when there is a sparse-checkout' '
> +	echo "/c" >.git/info/sparse-checkout &&
> +	test_config core.sparsecheckout true &&
> +	git checkout -b resetBranch &&
> +	test_path_is_missing m &&
> +	test_path_is_missing a &&
> +	test_path_is_missing d &&
> +	git reset HEAD~1 &&
> +	test "checkout file" = "$(cat c)" &&
> +	test "modification of a file" = "$(cat m)" &&
> +	test "added file" = "$(cat a)" &&
> +	test_path_is_missing d
> +'
> +
> +test_expect_success 'reset after deleting file without skip-worktree bit' '
> +	git checkout -f endCommit &&
> +	git clean -xdf &&
> +	cat >.git/info/sparse-checkout <<-\EOF &&
> +	/c
> +	/m
> +	EOF
> +	test_config core.sparsecheckout true &&
> +	git checkout -b resetAfterDelete &&
> +	test_path_is_file m &&
> +	test_path_is_missing a &&
> +	test_path_is_missing d &&
> +	rm -f m &&
> +	git reset HEAD~1 &&
> +	test "checkout file" = "$(cat c)" &&
> +	test "added file" = "$(cat a)" &&
> +	test_path_is_missing m &&
> +	test_path_is_missing d
> +'
> +
> +
> +
> +test_done