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:

quoted

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 <redacted>
---
 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 [off-list ref]