Re: Fwd: Possibly nicer pathspec syntax?

[Duy Nguyen <pclouds@gmail.com>]

On Thu, Feb 9, 2017 at 4:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
> With or without the patch in this thread, parse_pathspec() behaves
> the same way for either CWD or FULL if you feed a non-empty
> pathspec with at least one positive element.  IOW, if a caller feeds
> a non-empty pathspec and there is no "negative" element involved, it
> does not matter if we feed CWD or FULL.

Yes. Now that you put it that way, it may make more sense to rename
the options to PATHSPEC_DEFAULT_* instead of PATHSPEC_PREFER_*.

>  - builtin/checkout.c::cmd_checkout(), when argc==0, does not call
>    parse_pathspec().  This codepath will get affected by Linus's
>    change ("cd t && git checkout :\!perf" would try to check out
>    everything except t/perf, but what is a reasonable definition of
>    "everything" in the context of this command).  We need to
>    decide, and I am leaning towards preferring CWD for this case.

So far I have seen arguments with single negative pathspec as
examples. What about "cd t; git checkout :^perf :^../Documentation"?
CWD is probably not the best base to be excluded from. Maybe the
common prefix of all negative pathspecs? But things start to get a bit
unintuitive on that road. Or do will still bail out on multiple
negative pathspecs with no positive one?

Also, even with single negative pathspec, what about "cd t; git
checkout :^../ewah"?

> So, I am tempted to suggest us doing the following:
>
>  * Leave a NEEDSWORK comment to archive.c::path_exists() that is
>    used for checking the validation of pathspec elements.  To fix it
>    properly, we need to be able to skip a negative pathspec to be
>    passed to this function by the caller, and to do so, we need to
>    expose a helper from the pathspec API that gets a single string
>    and returns what magic it has, but that is of lower priority.

Side note. There's something else I'm not happy with pathspec handling
in "git archive". Try "cd t; git archive HEAD -- ../Documentation" and
you'll get a very unfriendly error message. But well, no time for it.

>  * Retire the PATHSPEC_PREFER_CWD bit and replace its use with the
>    lack of the PATHSPEC_PREFER_FULL bit.
>
>  * Keep most of the above callsites that currently do not pass
>    CWD/FULL as they are, except the ones that should take FULL (see
>    above).
>
> Comments?

This comes from my experience reading files-backend.c. I didn't
realize '0' in files_downcast(ref_store, 0, "pack_refs"); meant
'submodule not allowed' because apparently I was too lazy to read
prototype. But if was files_downcast(ref_store, NO_SUBMODULE,
"pack_refs"), it would save people's time.

With that in mind, should we keep _CWD the name, so people can quickly
see and guess that it prefers _CWD than _FULL? _CWD can be defined as
zero. It there's mostly as a friendly reminder.

Other than that, yes if killing one flag helps maintenance, I'm for it.

PS. You may notice I favored ^ over ! already. ! was a pain point for
me too but I was too lazy to bring it up and fight for it. Thanks
Linus.
-- 
Duy