git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Carlos Martín Nieto" <cmn@elego.de>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 1/3] make_absolute_path: Don't try to copy a string to itself
Date: Tue, 15 Mar 2011 12:59:49 +0100	[thread overview]
Message-ID: <1300190396.19100.31.camel@bee.lab.cmartin.tk> (raw)
In-Reply-To: <7vpqptb976.fsf@alter.siamese.dyndns.org>

On lun, 2011-03-14 at 15:58 -0700, Junio C Hamano wrote:
> Carlos Martín Nieto <cmn@elego.de> writes:
> 
> >> I don't think it is a bad idea per-se to avoid a copy from the same memory
> >> location into the same memory location, but independent of the necessity
> >> of fixes at the low-level, shouldn't we fix the callers that do not check
> >> if what they have is already absolute?
> >
> > If we'd like the semantics to be "whatever I had, I now know what the
> > absolute path is" then we could make the check in the beginning of the
> > function, to centralise the check. If the semantics should be "I don't
> > have an absolute path, so I need to figure out what it is", then there
> > should be a check before calling make_absolute_path() (the name suggests
> > the second).
> 
> Good thinking, and I think the former semantics would be easier to use.

 I was asking about why we didn't just use realpath, but it seems there
is no portable way of dealing with it (even without counting NFS over
different systems).

 I'm thinking of renaming make_absolute_path to real_path as it's in
fact our implementation of realpath (and actually describes what it
does), without a is_absolute_path check, as a path could be absolute but
point to a link, adding a comment to say that if you don't mind links,
you should use make_nonrelative_path (I'd rename it to absolute_path,
but that may be too close to the old make_absolute_path)

> 
> >  There is however the extra functionality the function offers, namely
> > resolving links. It might be good to split it into two functions so each
> > caller can specify what it wants.
> 
> Probably.

 With the changes mentioned earlier, if you want an absolute pathname,
you'd call absolute_path/make_nonrelative_path and if you want to make
sure you have the real path of the target file, you'd use real_path just
as you'd use realpath on a sane system, with

   cmn

  reply	other threads:[~2011-03-15 12:00 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-03-14 19:18 [PATCH 0/3] Fix some errors reported by valgrind Carlos Martín Nieto
2011-03-14 19:18 ` [PATCH 1/3] make_absolute_path: Don't try to copy a string to itself Carlos Martín Nieto
2011-03-14 20:02   ` Jeff King
2011-03-14 20:25   ` Junio C Hamano
2011-03-14 22:02     ` Carlos Martín Nieto
2011-03-14 22:58       ` Junio C Hamano
2011-03-15 11:59         ` Carlos Martín Nieto [this message]
2011-03-15 12:40           ` Carlos Martín Nieto
2011-03-15 17:02             ` Junio C Hamano
2011-03-15 17:27               ` Carlos Martín Nieto
2011-03-16 14:16                 ` Nguyen Thai Ngoc Duy
2011-03-16 14:49                   ` Carlos Martín Nieto
2011-03-16 14:58                     ` Nguyen Thai Ngoc Duy
2011-03-16 14:04               ` Nguyen Thai Ngoc Duy
2011-03-16 15:08                 ` Carlos Martín Nieto
2011-03-14 19:18 ` [PATCH 2/3] setup_path(): Free temporary buffer Carlos Martín Nieto
2011-03-14 20:09   ` Jeff King
2011-03-14 22:18     ` Carlos Martín Nieto
2011-03-16 11:26     ` [PATCH] system_path: use a static buffer Carlos Martín Nieto
2011-03-16 15:58       ` Erik Faye-Lund
2011-03-16 16:24         ` Carlos Martín Nieto
2011-03-16 16:33         ` Carlos Martín Nieto
2011-03-16 20:43           ` Junio C Hamano
2011-03-17 11:01             ` Carlos Martín Nieto
2011-03-17 14:24               ` Carlos Martín Nieto
2011-03-18  7:25                 ` Junio C Hamano
2011-03-21  9:56                   ` Carlos Martín Nieto
2011-03-21 11:14                     ` Jeff King
2011-03-21 15:26                       ` Carlos Martín Nieto
2011-03-21 15:51                         ` Jeff King
2011-03-21 15:57                           ` Carlos Martín Nieto
2011-03-18 10:34                 ` Nguyen Thai Ngoc Duy
2011-03-18 11:38                   ` PATH_MAX (Re: [PATCH] system_path: use a static buffer) Jonathan Nieder
2011-03-18 11:54                     ` Nguyen Thai Ngoc Duy
2011-03-21  9:47                     ` Carlos Martín Nieto
2011-03-21 12:37                       ` Lasse Makholm
2011-03-21 11:19                     ` Nguyen Thai Ngoc Duy
2011-03-18 11:39                   ` [PATCH 1/2] wrapper.c: add xgetcwd() Nguyễn Thái Ngọc Duy
2011-03-18 11:39                     ` [PATCH 2/2] setup_gently: use xgetcwd() Nguyễn Thái Ngọc Duy
2011-03-14 20:14   ` [PATCH 2/3] setup_path(): Free temporary buffer Junio C Hamano
2011-03-14 22:01     ` Carlos Martín Nieto
2011-03-15  1:12       ` Jeff King
2011-03-15  9:32         ` [PATCH] t/README: Add a note about running commands under valgrind Carlos Martín Nieto
2011-03-15 17:06           ` Junio C Hamano
2011-03-15 17:08             ` Carlos Martín Nieto
2011-03-14 19:18 ` [PATCH 3/3] clone: Free a few paths Carlos Martín Nieto
2011-03-14 19:45   ` Jonathan Nieder
2011-03-18  7:25     ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1300190396.19100.31.camel@bee.lab.cmartin.tk \
    --to=cmn@elego.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).