Re: [PATCH v3 3/4] setup: Add 'abspath_part_inside_repo' function
- From
Martin Erik Werner <martinerikwerner@gmail.com>
- Date
- Feb 1, 2014, 01:31 UTC
- Message-ID
- <20140201013121.GA14729@mule>
- In-Reply-To
- <52EC25A9.1050807@web.de>
On Fri, Jan 31, 2014 at 11:37:29PM +0100, Torsten Bögershausen wrote:
Show 51 quoted lines
> 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 <martinerikwerner@gmail.com>
> > ---
> > 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 <martinerikwerner@gmail.com>