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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 12, 2021, 21:58 UTC
Message-ID: <87pmtd5tkj.fsf@evledraar.gmail.com>
In-Reply-To: <f2abc387822378e02d0b221baf9a09ac91d44d7d.1631453010.git.gitgitgadget@gmail.com>

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

Show 9 quoted lines

> -	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...

Show 6 quoted lines

>  	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..

Show 12 quoted lines

>  	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.