Re: [RFC PATCH 6/7] add: warn when pathspec only matches SKIP_WORKTREE entries

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

Matheus Tavares <matheus.bernardino@usp.br> writes:

> `git add` already refrains from updating SKIP_WORKTREE entries, but it
> silently succeeds when a pathspec only matches these entries. Instead,
> let's warn the user and display a hint on how to update these entries.

"Silently succeeds" reads as if it succeeds to update, but that is
not what you meant.

I guess the warning is justified and is desirable because an attempt
to add an ignored path would result in a similar hint, e.g.

    $ echo '*~' >.gitignore
    $ git add x~
    hint: use -f if you really want to...
    $ git add .

It is curious why the latter does not warn (even when there is
nothing yet to be added that is not ignored), but that is what we
have now.  If we are modeling the new behaviour for sparse entries
after the ignored files, we should do the same, I think.

> A warning message was chosen over erroring out right away to reproduce
> the same behavior `add` already exhibits with ignored files. This also
> allow users to continue their workflow without having to invoke `add`
> again with only the matching pathspecs, as the matched files will have
> already been added.

Makes sense.

> Note: refresh_index() was changed to only mark matches with
> no-SKIP-WORKTREE entries in the `seen` output parameter. This is exactly
> the behavior we want for `add`, and only `add` calls this function with
> a non-NULL `seen` pointer. So the change brings no side effect on
> other callers.

And possible new callers that wants to learn from seen[] output
would want the same semantics, presumably?

> diff --git a/t/t3705-add-sparse-checkout.sh b/t/t3705-add-sparse-checkout.sh
> index f7b0ea782e..f66d369bf4 100755
> --- a/t/t3705-add-sparse-checkout.sh
> +++ b/t/t3705-add-sparse-checkout.sh
> @@ -32,10 +32,22 @@ test_sparse_entry_unchanged() {
>  	test_cmp expected actual
>  }
>  
> +cat >sparse_entry_error <<-EOF
> +The following pathspecs only matched index entries outside the current
> +sparse checkout:
> +sparse_entry
> +EOF
> +
> +cat >error_and_hint sparse_entry_error - <<-EOF
> +hint: Disable or modify the sparsity rules if you intend to update such entries.
> +hint: Disable this message with "git config advice.updateSparsePath false"
> +EOF
> +
>  test_expect_success "git add does not remove SKIP_WORKTREE entries" '
>  	setup_sparse_entry &&
>  	rm sparse_entry &&
> -	git add sparse_entry &&
> +	test_must_fail git add sparse_entry 2>stderr &&
> +	test_i18ncmp error_and_hint stderr &&

OK, this demonstrates what exactly you meant by "silently succeed".
We are not expecting side effects that are any different from before
(i.e. sparse_entry is not removed from the index), but the command
is taught to error out and hint, which makes sense.

>  	test_sparse_entry_unchanged
>  '
>  
> @@ -56,7 +68,8 @@ do
>  	test_expect_success "git add$opt does not update SKIP_WORKTREE entries" '
>  		setup_sparse_entry &&
>  		echo modified >sparse_entry &&
> -		git add $opt sparse_entry &&
> +		test_must_fail git add $opt sparse_entry 2>stderr &&
> +		test_i18ncmp error_and_hint stderr &&
>  		test_sparse_entry_unchanged
>  	'
>  done
> @@ -64,7 +77,8 @@ done
>  test_expect_success 'git add --refresh does not update SKIP_WORKTREE entries' '
>  	setup_sparse_entry &&
>  	test-tool chmtime -60 sparse_entry &&
> -	git add --refresh sparse_entry &&
> +	test_must_fail git add --refresh sparse_entry 2>stderr &&
> +	test_i18ncmp error_and_hint stderr &&
>  
>  	# We must unset the SKIP_WORKTREE bit, otherwise
>  	# git diff-files would skip examining the file
> @@ -77,7 +91,8 @@ test_expect_success 'git add --refresh does not update SKIP_WORKTREE entries' '
>  
>  test_expect_success 'git add --chmod does not update SKIP_WORKTREE entries' '
>  	setup_sparse_entry &&
> -	git add --chmod=+x sparse_entry &&
> +	test_must_fail git add --chmod=+x sparse_entry 2>stderr &&
> +	test_i18ncmp error_and_hint stderr &&
>  	test_sparse_entry_unchanged
>  '
>  
> @@ -85,8 +100,23 @@ test_expect_success 'git add --renormalize does not update SKIP_WORKTREE entries
>  	test_config core.autocrlf false &&
>  	setup_sparse_entry "LINEONE\r\nLINETWO\r\n" &&
>  	echo "sparse_entry text=auto" >.gitattributes &&
> -	git add --renormalize sparse_entry &&
> +	test_must_fail git add --renormalize sparse_entry 2>stderr &&
> +	test_i18ncmp error_and_hint stderr &&
>  	test_sparse_entry_unchanged
>  '
>  
> +test_expect_success 'do not advice about sparse entries when they do not match the pathspec' '
> +	setup_sparse_entry &&
> +	test_must_fail git add nonexistent sp 2>stderr &&
> +	test_i18ngrep "fatal: pathspec .nonexistent. did not match any files" stderr &&
> +	test_i18ngrep ! "The following pathspecs only matched index entries" stderr

This is because both of the two pathspec elements given do not match
the sparse entries?  It is curious how the command behaves when
given a pathspec that is broader, e.g. "." (aka "everything under
the sun").  We could do "add --dry-run" for the test if we do not
want to set up .gitignore appropriately and do not want to smudge
the index with stderr, expect, actual etc.

> +'
> +
> +test_expect_success 'add obeys advice.updateSparsePath' '
> +	setup_sparse_entry &&
> +	test_must_fail git -c advice.updateSparsePath=false add sparse_entry 2>stderr &&
> +	test_i18ncmp sparse_entry_error stderr
> +
> +'

OK.

Thanks.