On Thu, Feb 17, 2022 at 10:07 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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?
