Re: [PATCH v3 3/4] setup: Add 'abspath_part_inside_repo' function
On Sat, Feb 1, 2014 at 3:22 AM, Martin Erik Werner
wrote:
> diff --git a/setup.c b/setup.c
> index 5432a31..e606846 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -77,6 +77,69 @@ int path_inside_repo(const char *prefix, const char *path)
> return 0;
> }
>
> +/*
> + * It is ok if dst == src, but they should not overlap otherwise.
> + * No checking if the path is in fact an absolute path is done, and it must
> + * already be normalized.
> + *
> + * Find the part of an absolute path that lies inside the work tree by
> + * dereferencing symlinks outside the work tree, for example:
> + * /dir1/repo/dir2/file (work tree is /dir1/repo) -> dir2/file
> + * /dir/file (work tree is /) -> dir/file
> + * /dir/symlink1/symlink2 (symlink1 points to work tree) -> symlink2
> + * /dir/repo (exactly equal to work tree) -> (empty string)
> + */
> +int abspath_part_inside_repo(char *dst, const char* src)
> +{
> + size_t len;
> + char *dst0;
> + char temp;
> +
> + const char* work_tree = get_git_work_tree();
> + if (!work_tree)
> + return -1;
> + len = strlen(src);
> + dst0 = dst;
> +
> + // check root level
Um.. no C++ style comments. And there should be a test that work_tree
is the prefix of src (common case). If so we can return early and do
not need to do real_path() on every path component.
> + if (has_dos_drive_prefix(src)) {
> + *dst++ = *src++;
> + *dst++ = *src++;
> + *dst++ = *src++;
> + } else {
> + *dst++ = *src++;
> + }
> + temp = *dst;
> + *dst = '\0';
> + if (strcmp(real_path(dst0), work_tree) == 0) {
> + *dst = temp;
> + memmove(dst0, src, len - (dst - dst0) + 1);
> + return 0;
> + }
> + *dst = temp;
> +
> + // check each level
> + while (*dst != '\0') {
> + *dst++ = *src++;
> + if (*dst == '/') {
> + *dst = '\0';
> + if (strcmp(real_path(dst0), work_tree) == 0) {
> + memmove(dst0, src + 1, len - (dst - dst0));
> + return 0;
> + }
> + *dst = '/';
> + }
> + }
> +
> + // check whole path
> + if (strcmp(real_path(dst0), work_tree) == 0) {
> + *dst0 = '\0';
> + return 0;
> + }
> +
> + return -1;
> +}
> +
> int check_filename(const char *prefix, const char *arg)
> {
> const char *name;
> --
> 1.8.5.2
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/4] setup: Add 'abspath_part_inside_repo' function
On Fri, Jan 31, 2014 at 11:37:29PM +0100, Torsten Bögershausen wrote:
> On 2014-01-31 21.22, Martin Erik Werner wrote:
> > In order to extract the part of an absolute path which lies inside the
> > repo, it is not possible to directly use real_path, since that would
> > dereference symlinks both outside and inside the work tree.
> >
> > Add an 'abspath_part_inside_repo' function which incrementally checks
> > each path level by temporarily NUL-terminating at each '/' and comparing
> > against the work tree path. When a match is found, it copies the
> > remainder (which will be the in-repo part) to a destination
> > buffer.
> >
> > The path being the filesystem root or exactly equal to the work tree are
> > special cases handled separately, since then there is no directory
> > separator between the work tree and in-repo part.
> >
> > Signed-off-by: Martin Erik Werner
> > ---
> > cache.h | 1 +
> > setup.c | 63
> > +++
> > 2 files changed, 64 insertions(+)
> >
> > diff --git a/cache.h b/cache.h
> > index ce377e1..242f27d 100644
> > --- a/cache.h
> > +++ b/cache.h
> > @@ -426,6 +426,7 @@ extern void verify_filename(const char *prefix,
> > int diagnose_misspelt_rev);
> > extern void verify_non_filename(const char *prefix, const char *name);
> > extern int path_inside_repo(const char *prefix, const char *path);
> > +extern int abspath_part_inside_repo(char *dst, const char *path);
> abspath_part_inside_repo() is only used in setup.c, isn't it?
> In this case we don't need it in cache.h, it can be declared inside setup.c as
>
> static int abspath_part_inside_repo(char *dst, const char *path);
> (or "static inline" )
>
> -
> (And not in this patch: see the final setup.c:)
>
> if (g) {
> free(npath);
> return NULL;
> }
>
> If this is the only caller of abspath_part_inside_repo(),
> then do we need npath 2 times as a parameter ?
> Or can we re-write it to look like this:
>
> static inline int abspath_part_inside_repo(char *path)
> [
> ]
I guess I've over-generalised it a bit too much, that should rather be
done if-and-when, I presume?
It is indeed only used in setup.c and only by the prefix_path_gently
function so static inline then?
Hmm, for single-parameter it should suffice to simply move the parameter
down into the function, like so?:
const char* src;
src = dst;
and carry on as before (obviously also renaming the variables sensibly),
or did you have something else in mind?
(I added two parameters since I was glancing at 'normalize_path_copy_len'
for inspiration, and was thinking about (purely theoretical) re-use in
other cases rather than minimizing it for the time being.)
What do you mean with the "(And not in this patch"... bit; what "final
setup.c"?
--
Martin Erik Werner
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/4] setup: Add 'abspath_part_inside_repo' function
On 2014-01-31 21.22, Martin Erik Werner wrote:
> In order to extract the part of an absolute path which lies inside the
> repo, it is not possible to directly use real_path, since that would
> dereference symlinks both outside and inside the work tree.
>
> Add an 'abspath_part_inside_repo' function which incrementally checks
> each path level by temporarily NUL-terminating at each '/' and comparing
> against the work tree path. When a match is found, it copies the
> remainder (which will be the in-repo part) to a destination
> buffer.
>
> The path being the filesystem root or exactly equal to the work tree are
> special cases handled separately, since then there is no directory
> separator between the work tree and in-repo part.
>
> Signed-off-by: Martin Erik Werner
> ---
> cache.h | 1 +
> setup.c | 63 +++
> 2 files changed, 64 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index ce377e1..242f27d 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -426,6 +426,7 @@ extern void verify_filename(const char *prefix,
> int diagnose_misspelt_rev);
> extern void verify_non_filename(const char *prefix, const char *name);
> extern int path_inside_repo(const char *prefix, const char *path);
> +extern int abspath_part_inside_repo(char *dst, const char *path);
abspath_part_inside_repo() is only used in setup.c, isn't it?
In this case we don't need it in cache.h, it can be declared inside setup.c as
static int abspath_part_inside_repo(char *dst, const char *path);
(or "static inline" )
-
(And not in this patch: see the final setup.c:)
if (g) {
free(npath);
return NULL;
}
If this is the only caller of abspath_part_inside_repo(),
then do we need npath 2 times as a parameter ?
Or can we re-write it to look like this:
static inline int abspath_part_inside_repo(char *path)
[
]
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
