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

[Martin Erik Werner <martinerikwerner@gmail.com>]

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:
> > On 2014-01-31 21.22, Martin Erik Werner wrote:
(...)
> > > 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
> <martinerikwerner@gmail.com> wrote:
(...)
> > +       // 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