Re: [PATCH v4 4/5] sparse-checkout: error or warn when given individual files

[Elijah Newren <newren@gmail.com>]

On Thu, Feb 17, 2022 at 10:07 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> >               int prefix_len = strlen(prefix);
> >
> >               if (!core_sparse_checkout_cone)
> > @@ -703,20 +706,44 @@ static void sanitize_paths(int argc, const char **argv, const char *prefix)
> >               for (i = 0; i < argc; i++)
> >                       argv[i] = prefix_path(prefix, prefix_len, argv[i]);
> >       }
> > +
> > +     if (skip_checks)
> > +             return;
>
> The placement of skip-checks here makes the earier die() between the
> hunks in this block unskippable.

Fair point; I'll restructure a bit.

> > +     for (i = 0; i < argc; i++) {
> > +             struct cache_entry *ce;
> > +             struct index_state *index = the_repository->index;
> > +             int pos = index_name_pos(index, argv[i], strlen(argv[i]));
> > +
> > +             if (pos < 0)
> > +                     continue;
>
> I _think_ the intent for this is to catch possbily common mistakes
> rather than a hard rule, so while an unmerged path will evade this
> check, I think it is an acceptable trade-off between code simplicity
> and thoroughness.
>
> > +             ce = index->cache[pos];
> > +             if (S_ISSPARSEDIR(ce->ce_mode))
> > +                     continue;
> > +
> > +             if (core_sparse_checkout_cone)
> > +                     die(_("\"%s\" is not a directory; to treat it as a directory anyway, rerun with --skip-checks"), argv[i]);
> > +             else
> > +                     warning(_("pass a leading slash before paths such as \"%s\" if you want a single file (see NON-CONE PROBLEMS in the git-sparse-checkout manual)."), argv[i]);
> > +     }
>
> Hmph. The skip_checks flag was introduced exactly so that what the
> code catches as a possible problem is a false alarm, no?  I would
> understand if both were die() to be conservative, of both were
> warning() to be lenient, but one being die() and the other being
> warning() makes this look somewhat uneven.
>
> Besides, being leninent here somewhat defeats the point of having
> skip_checks, no?  As long as we believe skip_checks option is useful,
> shouldn't these be die() to be equally conservative?

One thing that gives me pause about making the leading slash
suggestion a die() instead of a warning() is that on Windows boxes,
users also have to quote arguments with a leading slash to prevent
them from being transformed into a "C:\Random\Garbage\Path\" prefix
instead.  But I don't want to tell linux/macos/etc. users to quote
their patterns unnecessarily, and being rather unfamiliar with Windows
I don't know if it's all Windows systems affected or only some like
cygwin.

Thoughts?