[PATCH v4 0/4] Handling of in-tree symlinks for absolute paths

Hmm, maybe fourth time's the ch...nevermind.

On Sat, Feb 01, 2014 at 02:31:21AM +0100, Martin Erik Werner wrote:

On Fri, Jan 31, 2014 at 11:37:29PM +0100, Torsten Bögershausen wrote:

quoted

On 2014-01-31 21.22, Martin Erik Werner wrote:

(...)

quoted

quoted

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"?

As per Torsten's suggestions I've re-worked abspath_part_inside_repo function
to only take one parameter and also put it as 'static inline' before
'prefix_path_gently' since currently only used once. (The change turned
out larger/nicer than I first guessed, since the 'src' pointer and copying
could be dropped completely.)

On Sat, Feb 01, 2014 at 09:31:26AM +0700, Duy Nguyen wrote:

On Sat, Feb 1, 2014 at 3:22 AM, Martin Erik Werner
[off-list ref] wrote:

(...)

quoted

+       // 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.

(...)

Oops, comments fixed.

I've added the check for work tree as existing prefix, which also had the nice
side-effect of checking the case of the work tree being the root of the
filesystem as a bonus.

This new single-buffer version also uses 'offset_1st_component' to move past
the root (since not having to worry about copying).

Martin Erik Werner (4):
  t0060: Add test for manipulating symlinks via absolute paths
  t0060: Add test for prefix_path when path == work tree
  setup: Add 'abspath_part_inside_repo' function
  setup: Don't dereference in-tree symlinks for absolute paths

 setup.c               | 93 ++++++++++++++++++++++++++++++++++++++++-----------
 t/t0060-path-utils.sh | 11 ++++++
 2 files changed, 84 insertions(+), 20 deletions(-)

-- 
1.8.5.2