Re: [PATCH 5/7] sparse-checkout: reject non-cone-mode patterns starting with a '#'

From: Elijah Newren <newren@gmail.com>
Date: Feb 15, 2022, 04:31 UTC
Message-ID: <CABPp-BEhsp4=dzCkSpyu4Bd7Q=hP1Ec6W09-zX2+N+5_-ytk8A@mail.gmail.com>
In-Reply-To: <xmqqsfsl2utb.fsf@gitster.g>

On Mon, Feb 14, 2022 at 9:59 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 22 quoted lines

>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +     if (!core_sparse_checkout_cone)
> > +             for (i = 0; i < argc; i++)
> > +                     if (argv[i][0] == '#')
> > +                             die(_("paths beginning with a '#' must be preceeded by a backslash"));
> > +
>
> Whenever I see an error message like this, where it is clear that
> the command knows the only viable solution is to the issue, and yet
> still refuses to do-what-the-user-clearly-meant-to-do (is there a
> valid reason to copy and paste "# comment" line, which clearly is
> not about choosing which paths to use/ignore, from an existing file
> and feed it to the command?), I question if it should be solved the
> opposite way.
>
> That is, to pretend as if "\" + argv[i] was given and then give the
> user either a warning saying what we did, or an unsquelcheable advice
> message (no need for advice.* config---the user can avoid triggering
> it by learning what the advice message would say, which is to use \#
> when they mean to give a pattern that begins with a pound).

If this were the only special character case, I'd totally agree, but I do worry a bit that escaping this particular case might lead users to expect us to escape and fix other special characters from '*?[]!\'. If users have files with those characters and specify an argument with one of those, are we to automatically escape them as well? If we don't escape the other characters but do escape '#', aren't we being inconsistent? And if we do escape those other characters too, aren't we breaking users who are trying to specify patterns (which is what non-cone mode is all about)? Personally, I'd rather drop this patch than introduce such an inconsistency.