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

[Jeff King <peff@peff.net>]

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