Re: [PATCH 2-3/3] Name make_*_path functions more accurately
On Wed, 2011-03-16 at 17:59 +0100, Michael J Gruber wrote:
Show 26 quoted lines
> Carlos Martín Nieto venit, vidit, dixit 16.03.2011 17:51:
>>[...]
> > -const char *make_nonrelative_path(const char *path)
> > +/*
> > + * Use this to get an absolute path from a relative one. If you want
> > + * to resolve links, you should use real_path.
> > + *
> > + * If the path is already absolute, then return path. As the user is
> > + * never meant to free the return value, we're safe.
> > + */
> > +const char *absolute_path(const char *path)
> > {
> > static char buf[PATH_MAX + 1];
> >
> > if (is_absolute_path(path)) {
> > - if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
> > + if (strlen(path) >= PATH_MAX)
> > die("Too long path: %.*s", 60, path);
> > + else
> > + return path;
> > } else {
> > size_t len;
> > const char *fmt;
>
> This is not simply a renaming change that you're sneaking in here. What
> is it about? Gah, I've been staring at this patch too long. This is related another
thread where Junio said it'd be a good idea to change the semantics so
we centralise checking wether we already have an absolute path. See
http://thread.gmane.org/gmane.comp.version-control.git/169012/focus=169063 (Message-ID: <7vpqptb976.fsf@alter.siamese.dyndns.org>).
Show 15 quoted lines
>>[...]
> > diff --git a/dir.c b/dir.c
> > index 570b651..5a9372a 100644
> > --- a/dir.c
> > +++ b/dir.c
> > @@ -1023,8 +1023,8 @@ char *get_relative_cwd(char *buffer, int size, const char *dir)
> > if (!getcwd(buffer, size))
> > die_errno("can't find the current directory");
> >
> > - if (!is_absolute_path(dir))
> > - dir = make_absolute_path(dir);
> > + /* getcwd resolves links and gives us the real path */
> > + dir = real_path(dir);
>
> Why remove the check? Again, this is not just renaming. Semantics change. Same as above. The one below (snipped) is the same.
>>[...]
> I think you should strictly separate the renaming issue from other
> patches (and describe/motivate the latter). It's fine to have a large
> patch with mechanical changes (renaming) if you stick to just those.
Unfortunately I introduced some modifications trying to reduce the
number of times we call make_absolute_path/real_path and they snuck in.
Sorry.
cmn
Carlos Martín Nieto <cmn@elego.de>