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

On 9/12/2021 5:58 PM, Ævar Arnfjörð Bjarmason wrote:
...

quoted

 	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