Re: [PATCH 0/9] magic pathspec updates

From: Junio C Hamano <gitster@pobox.com>
Date: May 10, 2011, 17:07 UTC
Message-ID: <7vvcxijxhi.fsf@alter.siamese.dyndns.org>
In-Reply-To: <BANLkTimLsNU0rLHRxxPmzFH_-1U5m3GoCw@mail.gmail.com>

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

Show 13 quoted lines

> On Tue, May 10, 2011 at 12:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> This is a series to update the magic pathspec topic, primarily the area
>> around the empty ':' pathspec.
>
> I think you missed git-commit and git-reset. "git commit --include :"
> may get past parse_and_validate_options() (ie. not dying) but later on
> in prepare_index(), there's no pathspec. I did not check very careful
> though.
>
>> Other than that, I think the current code is probably more or less safe to
>> dogfood with known rough edges.
>
> Agreed.

After re-reading the series one more time (including the ones that are already in next), I think teaching get_pathspec() about a lone ":" was probably a bad idea. The only reason we might want to say "there is no pathspec whatsoever" is when a command wants to limit its operation to the current subdirectory and it changes behaviour when run at the root level with and without pathspec (e.g. history simplification).

Note that no such command exists in our vocabulary today, so you need to imagine "git local-log" that acts like "git log" but by default shows history simplified to explain only the subdirectory you are currently in, or something.

Such an application can easily notice that argv[] has only a lone ":" left and do the same thing it does when there is no pathspec, without affecting other (existing) users of get_pathspec().

But the thing is, we do not have such an application like "local-log" now, we know we can easily support such commands when needed without teaching get_pathspec() about a lone ":", and we can be sure that existing commands that know get_pathspec() will never give them an empty pathspec if they feed a non-empty argv[] will not be broken by not teaching get_pathspec() about a lone ":".

I am leaning toward ripping the lone ":" support from the code in "next". I would also remove ":(icase)" from "next". It was only meant to be a POC to show how far we could go only by futzing get_pathspec() function, and was never meant to be a serious implementation of the feature. It should be re-done after we do deeper conversion and use the "struct pathspec" interface not "char **" interface.