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@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html