Re: [PATCH v2] commit: allow partial commits with relative paths

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 2, 2011, 21:31 UTC
Message-ID: <7vbow7ebzw.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20110730171347.GA10432@toss.lan>

Clemens Buchacher <drizzd@aon.at> writes:

Show 10 quoted lines

> diff --git a/setup.c b/setup.c
> index 5ea5502..2c51a9a 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -264,6 +264,38 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
>  	return pathspec;
>  }
>  
> +const char *pathspec_prefix(const char *prefix, const char **pathspec)
> +{

As a public function, this sorely needs a comment that describes what it does. More importantly, when I tried to come up with an example description, it became very clear that this neither prefixes anything to pathspec, nor prefixes pathspec to anything else.

As an internal helper in ls-files implementation it was perfectly fine that the function was slightly misnamed, but if you are making it into a public API, we should get its name right.

Perhaps "common_prefix()"?

Don't you also want to consolidate dir.c:common_prefix() with this?

What happens when pathspec has the recently introduced magic elements, e.g. ':/' that widens the match to the whole tree?