Re: Fwd: Possibly nicer pathspec syntax?

On Thu, Feb 9, 2017 at 4:11 AM, Junio C Hamano [off-list ref] 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