Re: [PATCH 14/22] lockfile: use strbufs when handling (most) paths

2014-04-02 Thread Junio C Hamano
Michael Haggerty mhag...@alum.mit.edu writes:

 Change struct lock_file's filename field from a fixed-length buffer
 into a strbuf.

Good.

As I allued to in a review on an unrelated patch, I do not think it
is a good idea to name the lock filename field lock_filename in a
structure that is about a lockfile, though.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 14/22] lockfile: use strbufs when handling (most) paths

2014-04-01 Thread Jeff King
On Tue, Apr 01, 2014 at 05:58:22PM +0200, Michael Haggerty wrote:

  /*
 - * p = path that may be a symlink
 - * s = full size of p
 - *
 - * If p is a symlink, attempt to overwrite p with a path to the real
 - * file or directory (which may or may not exist), following a chain of
 - * symlinks if necessary.  Otherwise, leave p unmodified.
 + * path contains a path that may be a symlink
   *
 - * This is a best-effort routine.  If an error occurs, p will either be
 - * left unmodified or will name a different symlink in a symlink chain
 - * that started with p's initial contents.
 + * If path is a symlink, attempt to overwrite it with a path to the
 + * real file or directory (which may or may not exist), following a
 + * chain of symlinks if necessary.  Otherwise, leave path unmodified.
   *
 - * Always returns p.
 + * This is a best-effort routine.  If an error occurs, path will
 + * either be left unmodified or will name a different symlink in a
 + * symlink chain that started with path's initial contents.
   */
 -
 -static char *resolve_symlink(char *p, size_t s)
 +static void resolve_symlink(struct strbuf *path)
 [...]

This is not a problem you are introducing, but do we really want
resolve_symlink to be best-effort here?

What happens if a is a symlink to b, and two processes try to lock
a simultaneously? If one succeeds, it will take b.lock. But the
other will take a.lock, and both will think they have the target
locked.

I suspect we need to recognize ENOENT for cases where we are creating
the file for the first time, but it seems like we should bail on locking
from any other transient errors.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html