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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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.