On Thu, Feb 9, 2017 at 4:11 AM, Junio C Hamano <gitster@xxxxxxxxx> 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