[v2,14/14] advice: update message to suggest '--sparse'

Commit Message

Derrick Stolee Sept. 12, 2021, 1:23 p.m. UTC

From: Derrick Stolee <[e-mail]>

The previous changes modified the behavior of 'git add', 'git rm', and
'git mv' to not adjust paths outside the sparse-checkout cone, even if
they exist in the working tree and their cache entries lack the
SKIP_WORKTREE bit. The intention is to warn users that they are doing
something potentially dangerous. The '--sparse' option was added to each
command to allow careful users the same ability they had before.

To improve the discoverability of this new functionality, add a message
to advice.updateSparsePath that mentions the existence of the option.

The previous set of changes also modified the purpose of this message to
include possibly a list of paths instead of only a list of pathspecs.
Make the warning message more clear about this new behavior.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 advice.c                       | 10 ++++++----
 t/t3602-rm-sparse-checkout.sh  |  6 +++---
 t/t3705-add-sparse-checkout.sh |  6 +++---
 t/t7002-mv-sparse-checkout.sh  |  6 +++---
 4 files changed, 15 insertions(+), 13 deletions(-)

Comments

Ævar Arnfjörð Bjarmason Sept. 12, 2021, 9:58 p.m. UTC | #1

On Sun, Sep 12 2021, Derrick Stolee via GitGitGadget wrote:

> -	fprintf(stderr, _("The following pathspecs didn't match any"
> -			  " eligible path, but they do match index\n"
> -			  "entries outside the current sparse checkout:\n"));
> +	fprintf(stderr, _("The following paths and/or pathspecs matched "
> +			  "paths that exist outside of your\n"
> +			  "sparse-checkout definition, so will not be "
> +			  "updated in the index:\n"));
>  	for_each_string_list_item(item, pathspec_list)
>  		fprintf(stderr, "%s\n", item->string);

This before and after looks about as well line-wrapped...

>  	advise_if_enabled(ADVICE_UPDATE_SPARSE_PATH,
> -			  _("Disable or modify the sparsity rules if you intend"
> +			  _("Disable or modify the sparsity rules or"
> +			    " use the --sparse option if you intend"
>  			    " to update such entries."));
>  }

...but here..

>  	cat >sparse_error_header <<-EOF &&
> -	The following pathspecs didn't match any eligible path, but they do match index
> -	entries outside the current sparse checkout:
> +	The following paths and/or pathspecs matched paths that exist outside of your
> +	sparse-checkout definition, so will not be updated in the index:
>  	EOF
>  
>  	cat >sparse_hint <<-EOF &&
> -	hint: Disable or modify the sparsity rules if you intend to update such entries.
> +	hint: Disable or modify the sparsity rules or use the --sparse option if you intend to update such entries.
>  	hint: Disable this message with \"git config advice.updateSparsePath false\"
>  	EOF

...this used to line-wrap at 80 characters, but is now a bit beyond
that.

Maybe instead make these two into bullet-points?

Also the third "Disable" looks a bit jarring at first, it seems like a
continuation of the first message, but it's just the standard "disable
this message" we tend to print out.

This commentary pre-dates this commit, but just in general:

I think the advice system is best used where there's an initial
non-optional message, and then the advice elaborates on what happened,
how to fix it. A good example is the "short object ID %s is ambiguous"
in object-name.c.

But in this case both messages are rather long. I'd think better would
be something like (and I didn't look very deeply at the involved code):

    error("pathspec '%s' matched only outside sparse checkout")

I.e. in e.g. cmd_rm() we loop through the pathspecs, and we error on the
first one, so to a first approximation why do we need to for sparse emit
ALL the pathspecs we didn't match? if we're going to error out anyway
shouldn'w we just error out on the first one?

But going on, I'd think this would be better overall (pseudocode):

    error("pathspec '%s' matched only outside sparse checkout")
    if (advice_enabled(ADVICE_UPDATE_SPARSE_PATH)) {
        char *list_str;
        list_of_bad_pathspecs = make_that_list(my_pathspec_string_list, &list_str);
  
      if (list_of_bad_pathspecs.nr > 1)
            /* Emit a message that details what's wrong, but also has a
             * list of all the other pathspecs we'd also die on if the user */
      else
          /* Ditto, but no list *?

Maybe I'm missing something with the sparse implemention, but I'd think
going above & beyond and listing all failures is a bit much in either
case, i.e. for non-sparse we have:

    $ git rm 'file-i-do-not-have' 'directory-i-do-not-have/'
    fatal: pathspec 'file-i-do-not-have' did not match any files
    $

I'd think in general a user who's screwed up and typo'd both isn't going
to be much harmed by us noting the first, maybe they'll get another
error then.

But usually it's obvious (e.g. you just ran the command in the wrong
directory), so if you have a large list of pathspecs getting a firehose
of all the things that didn't match can be less helpful due to being
overrly verbose.

Derrick Stolee Sept. 15, 2021, 4:54 p.m. UTC | #2

On 9/12/2021 5:58 PM, Ævar Arnfjörð Bjarmason wrote:
...
>>  	cat >sparse_error_header <<-EOF &&
>> -	The following pathspecs didn't match any eligible path, but they do match index
>> -	entries outside the current sparse checkout:
>> +	The following paths and/or pathspecs matched paths that exist outside of your
>> +	sparse-checkout definition, so will not be updated in the index:
>>  	EOF
>>  
>>  	cat >sparse_hint <<-EOF &&
>> -	hint: Disable or modify the sparsity rules if you intend to update such entries.
>> +	hint: Disable or modify the sparsity rules or use the --sparse option if you intend to update such entries.
>>  	hint: Disable this message with \"git config advice.updateSparsePath false\"
>>  	EOF
> 
> ...this used to line-wrap at 80 characters, but is now a bit beyond
> that.
> 
> Maybe instead make these two into bullet-points?

Do you mean something like this?

	hint: If you intend to update such entries, try one of the following:
	hint: * Use the --sparse option.
	hint: * Disable or modify the sparsity rules.

> Also the third "Disable" looks a bit jarring at first, it seems like a
> continuation of the first message, but it's just the standard "disable
> this message" we tend to print out.

With the bullet points, this is no longer a concern.

> This commentary pre-dates this commit, but just in general:
> 
> I think the advice system is best used where there's an initial
> non-optional message, and then the advice elaborates on what happened,
> how to fix it. A good example is the "short object ID %s is ambiguous"
> in object-name.c.
>
> But in this case both messages are rather long. I'd think better would
> be something like (and I didn't look very deeply at the involved code):
> 
>     error("pathspec '%s' matched only outside sparse checkout")
> 
> I.e. in e.g. cmd_rm() we loop through the pathspecs, and we error on the
> first one, so to a first approximation why do we need to for sparse emit
> ALL the pathspecs we didn't match? if we're going to error out anyway
> shouldn'w we just error out on the first one?

If we don't list the entire set, then users will need to use trial
and error to discover how to get out of a bad state.
 
> But going on, I'd think this would be better overall (pseudocode):
> 
>     error("pathspec '%s' matched only outside sparse checkout")
>     if (advice_enabled(ADVICE_UPDATE_SPARSE_PATH)) {
>         char *list_str;
>         list_of_bad_pathspecs = make_that_list(my_pathspec_string_list, &list_str);
>   
>       if (list_of_bad_pathspecs.nr > 1)
>             /* Emit a message that details what's wrong, but also has a
>              * list of all the other pathspecs we'd also die on if the user */
>       else
>           /* Ditto, but no list *?

I'm not a fan of this "here's an error message for the first thing, but
the advice gives all the details" approach.

> Maybe I'm missing something with the sparse implemention, but I'd think
> going above & beyond and listing all failures is a bit much in either
> case, i.e. for non-sparse we have:
> 
>     $ git rm 'file-i-do-not-have' 'directory-i-do-not-have/'
>     fatal: pathspec 'file-i-do-not-have' did not match any files
>     $
> 
> I'd think in general a user who's screwed up and typo'd both isn't going
> to be much harmed by us noting the first, maybe they'll get another
> error then.
> 
> But usually it's obvious (e.g. you just ran the command in the wrong
> directory), so if you have a large list of pathspecs getting a firehose
> of all the things that didn't match can be less helpful due to being
> overrly verbose.

The difference here is a die() versus an error(). It would probably
be better to convert the die() into an error() and report all failures
rather than have the sparse-checkout changes start short-circuiting
and providing less data.

Thanks,
-Stolee

Patch

diff --git a/advice.c b/advice.c
index 0b9c89c48ab..0deaf111795 100644
--- a/advice.c
+++ b/advice.c
@@ -293,14 +293,16 @@  void advise_on_updating_sparse_paths(struct string_list *pathspec_list)
 	if (!pathspec_list->nr)
 		return;
 
-	fprintf(stderr, _("The following pathspecs didn't match any"
-			  " eligible path, but they do match index\n"
-			  "entries outside the current sparse checkout:\n"));
+	fprintf(stderr, _("The following paths and/or pathspecs matched "
+			  "paths that exist outside of your\n"
+			  "sparse-checkout definition, so will not be "
+			  "updated in the index:\n"));
 	for_each_string_list_item(item, pathspec_list)
 		fprintf(stderr, "%s\n", item->string);
 
 	advise_if_enabled(ADVICE_UPDATE_SPARSE_PATH,
-			  _("Disable or modify the sparsity rules if you intend"
+			  _("Disable or modify the sparsity rules or"
+			    " use the --sparse option if you intend"
 			    " to update such entries."));
 }
 
diff --git a/t/t3602-rm-sparse-checkout.sh b/t/t3602-rm-sparse-checkout.sh
index 5f92b60a56a..712ddae2b15 100755
--- a/t/t3602-rm-sparse-checkout.sh
+++ b/t/t3602-rm-sparse-checkout.sh
@@ -11,12 +11,12 @@  test_expect_success 'setup' "
 	git commit -m files &&
 
 	cat >sparse_error_header <<-EOF &&
-	The following pathspecs didn't match any eligible path, but they do match index
-	entries outside the current sparse checkout:
+	The following paths and/or pathspecs matched paths that exist outside of your
+	sparse-checkout definition, so will not be updated in the index:
 	EOF
 
 	cat >sparse_hint <<-EOF &&
-	hint: Disable or modify the sparsity rules if you intend to update such entries.
+	hint: Disable or modify the sparsity rules or use the --sparse option if you intend to update such entries.
 	hint: Disable this message with \"git config advice.updateSparsePath false\"
 	EOF
 
diff --git a/t/t3705-add-sparse-checkout.sh b/t/t3705-add-sparse-checkout.sh
index cf2ccb87cf2..d51c96f8d72 100755
--- a/t/t3705-add-sparse-checkout.sh
+++ b/t/t3705-add-sparse-checkout.sh
@@ -44,12 +44,12 @@  test_sparse_entry_unstaged () {
 
 test_expect_success 'setup' "
 	cat >sparse_error_header <<-EOF &&
-	The following pathspecs didn't match any eligible path, but they do match index
-	entries outside the current sparse checkout:
+	The following paths and/or pathspecs matched paths that exist outside of your
+	sparse-checkout definition, so will not be updated in the index:
 	EOF
 
 	cat >sparse_hint <<-EOF &&
-	hint: Disable or modify the sparsity rules if you intend to update such entries.
+	hint: Disable or modify the sparsity rules or use the --sparse option if you intend to update such entries.
 	hint: Disable this message with \"git config advice.updateSparsePath false\"
 	EOF
 
diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
index 07dbfeb6d17..3db7ddaba9a 100755
--- a/t/t7002-mv-sparse-checkout.sh
+++ b/t/t7002-mv-sparse-checkout.sh
@@ -11,12 +11,12 @@  test_expect_success 'setup' "
 	git commit -m files &&
 
 	cat >sparse_error_header <<-EOF &&
-	The following pathspecs didn't match any eligible path, but they do match index
-	entries outside the current sparse checkout:
+	The following paths and/or pathspecs matched paths that exist outside of your
+	sparse-checkout definition, so will not be updated in the index:
 	EOF
 
 	cat >sparse_hint <<-EOF
-	hint: Disable or modify the sparsity rules if you intend to update such entries.
+	hint: Disable or modify the sparsity rules or use the --sparse option if you intend to update such entries.
 	hint: Disable this message with \"git config advice.updateSparsePath false\"
 	EOF
 "