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
