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

[Derrick Stolee <stolee@gmail.com>]

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