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

2014-04-02 Thread Junio C Hamano
Michael Haggerty  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


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

2014-04-01 Thread Michael Haggerty
Change struct lock_file's filename field from a fixed-length buffer
into a strbuf.  This:

* Make buffer overflows a bit less likely

* Removes arbitrary limitations on the length of the path in some
  places, though the restrictions still have to be removed from other
  places to make a real difference

* Shortens the code

Name the new strbuf field lock_filename to make it clear that it
contains the name of the lockfile as opposed to the name of the file
being locked.  Initialize the strbuf the first time the lock_file
object is used.

Change resolve_symlink() to work on a strbuf.

Replace last_path_elm() with a new function, trim_last_path_elm(),
which operates on a strbuf.

Adjust other places in the code that refer directly to the filename.

Signed-off-by: Michael Haggerty 
---
 builtin/commit.c |  12 ++--
 builtin/reflog.c |   2 +-
 cache.h  |   2 +-
 config.c |   3 +-
 lockfile.c   | 183 ---
 refs.c   |   6 +-
 shallow.c|   6 +-
 7 files changed, 97 insertions(+), 117 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d9550c5..28944de 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -330,7 +330,7 @@ static char *prepare_index(int argc, const char **argv, 
const char *prefix,
die(_("unable to create temporary index"));
 
old_index_env = getenv(INDEX_ENVIRONMENT);
-   setenv(INDEX_ENVIRONMENT, index_lock.filename, 1);
+   setenv(INDEX_ENVIRONMENT, index_lock.lock_filename.buf, 1);
 
if (interactive_add(argc, argv, prefix, patch_interactive) != 0)
die(_("interactive add failed"));
@@ -341,10 +341,10 @@ static char *prepare_index(int argc, const char **argv, 
const char *prefix,
unsetenv(INDEX_ENVIRONMENT);
 
discard_cache();
-   read_cache_from(index_lock.filename);
+   read_cache_from(index_lock.lock_filename.buf);
 
commit_style = COMMIT_NORMAL;
-   return index_lock.filename;
+   return index_lock.lock_filename.buf;
}
 
/*
@@ -368,7 +368,7 @@ static char *prepare_index(int argc, const char **argv, 
const char *prefix,
close_lock_file(&index_lock))
die(_("unable to write new_index file"));
commit_style = COMMIT_NORMAL;
-   return index_lock.filename;
+   return index_lock.lock_filename.buf;
}
 
/*
@@ -453,9 +453,9 @@ static char *prepare_index(int argc, const char **argv, 
const char *prefix,
die(_("unable to write temporary index file"));
 
discard_cache();
-   read_cache_from(false_lock.filename);
+   read_cache_from(false_lock.lock_filename.buf);
 
-   return false_lock.filename;
+   return false_lock.lock_filename.buf;
 }
 
 static int run_status(FILE *fp, const char *index_file, const char *prefix, 
int nowarn,
diff --git a/builtin/reflog.c b/builtin/reflog.c
index c12a9784..29bc07b 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -431,7 +431,7 @@ static int expire_reflog(const char *ref, const unsigned 
char *sha1, int unused,
 write_str_in_full(lock->lock_fd, "\n") != 1 ||
 close_ref(lock) < 0)) {
status |= error("Couldn't write %s",
-   lock->lk->filename);
+   lock->lk->lock_filename.buf);
unlink(newlog_path);
} else if (rename(newlog_path, log_file)) {
status |= error("cannot rename %s to %s",
diff --git a/cache.h b/cache.h
index 08d0e48..3d0a835 100644
--- a/cache.h
+++ b/cache.h
@@ -538,7 +538,7 @@ struct lock_file {
int fd;
pid_t owner;
unsigned char flags;
-   char filename[PATH_MAX];
+   struct strbuf lock_filename;
 };
 #define LOCK_DIE_ON_ERROR 1
 #define LOCK_NODEREF 2
diff --git a/config.c b/config.c
index 1ea3f39..cea0af0 100644
--- a/config.c
+++ b/config.c
@@ -1305,7 +1305,8 @@ static int store_aux(const char *key, const char *value, 
void *cb)
 
 static int write_error(struct lock_file *lk)
 {
-   error("failed to write new configuration file %s", lk->filename);
+   error("failed to write new configuration file %s",
+ lk->lock_filename.buf);
 
/* Same error code as "failed to rename". */
return 4;
diff --git a/lockfile.c b/lockfile.c
index 4e3ada8..931ebbd 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -34,24 +34,25 @@
  * A lock_file object can be in several states:
  *
  * - Uninitialized.  In this state the object's flags field must be
- *   zero but the rest of the contents need not be initialized.  As
- *   soon as the object is used in any way, it is irrevocably
- *   registered in the lock_file_list, and flags & LOCK_FLAGS_ON_LIST
- *   is set.