Re: Fwd: Possibly nicer pathspec syntax?

From: Duy Nguyen <pclouds@gmail.com>
Date: Feb 9, 2017, 13:48 UTC
Message-ID: <CACsJy8DuP5jGFGJYNeAx+6DPFkRpCiPSrTipjP9A_PD=1uNX3Q@mail.gmail.com>
In-Reply-To: <xmqqwpd0v15j.fsf@gitster.mtv.corp.google.com>

On Thu, Feb 9, 2017 at 4:11 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 5 quoted lines

> 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_*.

Show 6 quoted lines

>  - 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"?

Show 8 quoted lines

> 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.

Show 8 quoted lines

>  * 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