git-config: replaces ~/.gitconfig symlink with real file

git-config: replaces ~/.gitconfig symlink with real file

[Bradford Smith]

Since the number of dot-files and dot-directories that I have in my
home directory these days is somewhat overwhelming, I like to keep
those I directly edit all together in an ~/etc directory so I can
easily back them up and/or copy them in bulk to new accounts.  So,
several of my home dot-files are just symlinks to something in ~/etc,
including ~/.gitconfig.

However, when I tried running 'git-config --global color.diff auto'
today, it removed my symlink and replaced it with a real file.  This
left me briefly a bit confused when the changes I had made didn't show
up in ~/etc/gitconfig, but git-config reported them anyway.

If I were to fix this, I'd be tempted to use realpath(3) to follow the
symlink, but I don't think it's very reliably available
cross-platform.  Certainly, it isn't used anywhere in the current git
code.  Can anyone suggest a more portable fix?

Thanks,

Bradford

Re: git-config: replaces ~/.gitconfig symlink with real file

[Johannes Schindelin]

Hi,

On Sun, 15 Jul 2007, Bradford Smith wrote:

> If I were to fix this, I'd be tempted to use realpath(3) to follow the 
> symlink, but I don't think it's very reliably available cross-platform.  
> Certainly, it isn't used anywhere in the current git code.  Can anyone 
> suggest a more portable fix?

I'd use readlink(2) and test for EINVAL to fall back to the current 
behaviour.

Hth,
Dscho

Re: git-config: replaces ~/.gitconfig symlink with real file

[Nikolai Weibull]

On 7/15/07, Bradford Smith <bradford.carl.smith@gmail.com> wrote:
> Since the number of dot-files and dot-directories that I have in my
> home directory these days is somewhat overwhelming, I like to keep
> those I directly edit all together in an ~/etc directory so I can
> easily back them up and/or copy them in bulk to new accounts.  So,
> several of my home dot-files are just symlinks to something in ~/etc,
> including ~/.gitconfig.

How about adding an environment variable telling Git where to find
user-global .gitconfig instead?

  nikolai

Re: git-config: replaces ~/.gitconfig symlink with real file

[Bradford Smith]

On 7/16/07, Nikolai Weibull <now@bitwi.se> wrote:
> On 7/15/07, Bradford Smith <bradford.carl.smith@gmail.com> wrote:
> > Since the number of dot-files and dot-directories that I have in my
> > home directory these days is somewhat overwhelming, I like to keep
> > those I directly edit all together in an ~/etc directory so I can
> > easily back them up and/or copy them in bulk to new accounts.  So,
> > several of my home dot-files are just symlinks to something in ~/etc,
> > including ~/.gitconfig.
>
> How about adding an environment variable telling Git where to find
> user-global .gitconfig instead?
> > home directory these days is somewhat overwhelming, I like to keep
> > those I directly edit all together in an ~/etc directory so I can
> > easily back them up and/or copy them in bulk to new accounts.  So,
> > several of my home dot-files are just symlinks to something in ~/etc,
> > including ~/.gitconfig.
>
> How about adding an environment variable telling Git where to find
> user-global .gitconfig instead?

Thanks for suggesting that.

Actually, by looking at the code I discovered I could use the
environment variable GIT_CONFIG to specify where the configuration
file is, and I have already changed my setup to use this.
Unfortunately, I found the documentation for this variable in
git-config(1) confusing or I would have used it before.  If I get the
chance, I'll submit a patch for git-config.txt, and maybe for git.txt
as well, since it lists lots of other environment variables but not
GIT_CONFIG or GIT_CONFIG_LOCAL.

Thanks,

Bradford

Re: git-config: replaces ~/.gitconfig symlink with real file

[Bradford Smith]

On 7/16/07, Bradford Smith <bradford.carl.smith@gmail.com> wrote:
> On 7/16/07, Nikolai Weibull <now@bitwi.se> wrote:
> > On 7/15/07, Bradford Smith <bradford.carl.smith@gmail.com> wrote:
> > > Since the number of dot-files and dot-directories that I have in my
> > > home directory these days is somewhat overwhelming, I like to keep
> > > those I directly edit all together in an ~/etc directory so I can
> > > easily back them up and/or copy them in bulk to new accounts.  So,
> > > several of my home dot-files are just symlinks to something in ~/etc,
> > > including ~/.gitconfig.
> >
> > How about adding an environment variable telling Git where to find
> > user-global .gitconfig instead?
> > > home directory these days is somewhat overwhelming, I like to keep
> > > those I directly edit all together in an ~/etc directory so I can
> > > easily back them up and/or copy them in bulk to new accounts.  So,
> > > several of my home dot-files are just symlinks to something in ~/etc,
> > > including ~/.gitconfig.
> >
> > How about adding an environment variable telling Git where to find
> > user-global .gitconfig instead?
>
> Thanks for suggesting that.
>
> Actually, by looking at the code I discovered I could use the
> environment variable GIT_CONFIG to specify where the configuration
> file is, and I have already changed my setup to use this.
> Unfortunately, I found the documentation for this variable in
> git-config(1) confusing or I would have used it before.  If I get the
> chance, I'll submit a patch for git-config.txt, and maybe for git.txt
> as well, since it lists lots of other environment variables but not
> GIT_CONFIG or GIT_CONFIG_LOCAL.
>
> Thanks,
>
> Bradford
>

Drat!  The documentation wasn't as wrong as I had hoped.  If I set
GIT_CONFIG, git will ignore $(prefix)/etc/gitconfig and ~/.git/config,
which isn't what I want.  So, I guess I need to add a GIT_CONFIG_HOME
environment variable.  If I get that done, I'll send a patch to the
list including doc updates.

Of course, if someone else wants to do it first, I won't complain. B')

Thanks,

Bradford

Re: git-config: replaces ~/.gitconfig symlink with real file

[Junio C Hamano]

"Bradford Smith" <bradford.carl.smith@gmail.com> writes:

> ...  So, I guess I need to add a GIT_CONFIG_HOME
> environment variable.

I suspect that is going down a wrong path.

We use the sequence:

	fd = creat("temporary location");
        write(fd, ...);
        close(fd);
        rename("temporary location", "final location");

in quite a lot of codepaths.  I think they can be factored out,
to take the "final location" (and perhaps a suggested temporary
directory) as an parameter, and that code can check that "final
location" is a symlink to somewhere else and create the
temporary next to the target file.

[PATCH 0/2] git-config should not replace symlink

[Bradford C. Smith]

These patches fix a problem that caused git-config to replace my
~/.gitconfig symlink with a real file.

[PATCH 1/2] resolve symlinks when creating lockfiles
[PATCH 2/2] use lockfile.c routines in git_commit_set_multivar()

[PATCH 1/2] resolve symlinks when creating lockfiles

[Bradford C. Smith]

From: Bradford C. Smith <bradford.carl.smith@gmail.com>

Without this fix, the lockfile code will replace a symlink with a real file.

Signed-off-by: "Bradford C. Smith" <bradford.carl.smith@gmail.com>
---
 lockfile.c |   87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 86 insertions(+), 1 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index fb8f13b..4c35224 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -25,10 +25,95 @@ static void remove_lock_file_on_signal(int signo)
 	raise(signo);
 }
 
+/**
+ * p = absolute or relative path name
+ *
+ * Return a pointer into p showing the beginning of the last path name
+ * element.  If p is empty or the root directory ("/"), just return p.
+ */
+static char * last_path_elm(char * p)
+{
+	int	p_len = strlen(p);
+	char *	r;
+
+	if (p_len < 1) return p;
+	/* r points to last non-null character in p */
+	r = p + p_len - 1;
+	/* first skip any trailing slashes */
+	while (*r == '/' && r > p) r--;
+	/* then go back to the first non-slash */
+	while (r > p && *(r-1) != '/') r--;
+	return r;
+}
+
+/**
+ * p = char array containing path to existing file or symlink
+ * s = size of p
+ *
+ * If p indicates a valid symlink to an existing file, overwrite p with
+ * the path to the real file.  Otherwise, leave p unmodified.
+ *
+ * Always returns p in any case.
+ *
+ * NOTE: This is a best-effort routine.  It will give no indication of
+ * failure if it is unable to fully resolve p.  However, it is
+ * guaranteed to leave p in one of the following states if there isn't
+ * enough room in p or some other failure occurs:
+ *
+ * 1. unmodified
+ *      OR
+ * 2. path to a different symlink in a chain that eventually leads to a
+ *    real file or directory.
+ */
+static char * resolve_symlink(char * p, size_t s)
+{
+	struct stat st;
+	char link[PATH_MAX];
+	int link_len;
+
+	/* To avoid an infinite loop of symlinks, try a normal stat()
+	 * first.  This will fail if p is a symlink that cannot be
+	 * resolved, so we won't waste our time following a bad link. */
+	if (stat(p, &st)) return p;
+	/* if I can stat() the file, I sure ought to be able to lstat()
+	 * it, but if something bizarre happens, just return p.  */
+	if (lstat(p, &st)) return p;
+	/* if not a link, return p unmodified */
+	if (!S_ISLNK(st.st_mode)) return p;
+	link_len = st.st_size;
+	/* link is too big, so just return p */
+	if (link_len >= sizeof(link)) return p;
+	/* fail if readlink fails, and just return p */
+	if (link_len != readlink(p, link, sizeof(link))) return p;
+	/* readlink never null-terminates */
+	link[link_len] = '\0';
+	if (link[0] == '/') {
+		/* absolute path simply replaces p */
+		/* fail if link won't fit in p */
+		if (link_len >= s) return p;
+		strcpy(p, link);
+	} else {
+		/* link is relative path, so we must replace the last
+		 * element of p with it. */
+		char * r = last_path_elm(p);
+		/* make sure there's room in p for us to replace the
+		 * last element with the link contents */
+		if (r - p + link_len >= s) return p;
+		strcpy(r, link);
+	}
+	/* try again in case we've resolved to another symlink */
+	return resolve_symlink(p, s);
+}
+
 static int lock_file(struct lock_file *lk, const char *path)
 {
 	int fd;
-	sprintf(lk->filename, "%s.lock", path);
+	if (strlen(path) >= sizeof(lk->filename)) return -1;
+	strcpy(lk->filename, path);
+	/* subtract 5 from size to make sure there's room for adding
+	 * ".lock" for the lock file name */
+	resolve_symlink(lk->filename, sizeof(lk->filename)-5);
+	strcat(lk->filename, ".lock");
 	fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
 	if (0 <= fd) {
 		if (!lock_file_list) {
-- 
1.5.3.rc2.30.g1c06-dirty

[PATCH 2/2] use lockfile.c routines in git_commit_set_multivar()

[Bradford C. Smith]

From: Bradford C. Smith <bradford.carl.smith@gmail.com>

Changed git_commit_set_multivar() to use the routines provided by
lockfile.c to reduce code duplication and ensure consistent behavior.

Signed-off-by: "Bradford C. Smith" <bradford.carl.smith@gmail.com>
---
 config.c |   28 ++++++++++++++++------------
 1 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/config.c b/config.c
index f89a611..9101de9 100644
--- a/config.c
+++ b/config.c
@@ -715,7 +715,7 @@ int git_config_set_multivar(const char* key, const char* value,
 	int fd = -1, in_fd;
 	int ret;
 	char* config_filename;
-	char* lock_file;
+	struct lock_file *lock = NULL;
 	const char* last_dot = strrchr(key, '.');
 
 	config_filename = getenv(CONFIG_ENVIRONMENT);
@@ -725,7 +725,6 @@ int git_config_set_multivar(const char* key, const char* value,
 			config_filename  = git_path("config");
 	}
 	config_filename = xstrdup(config_filename);
-	lock_file = xstrdup(mkpath("%s.lock", config_filename));
 
 	/*
 	 * Since "key" actually contains the section name and the real
@@ -770,11 +769,12 @@ int git_config_set_multivar(const char* key, const char* value,
 	store.key[i] = 0;
 
 	/*
-	 * The lock_file serves a purpose in addition to locking: the new
+	 * The lock serves a purpose in addition to locking: the new
 	 * contents of .git/config will be written into it.
 	 */
-	fd = open(lock_file, O_WRONLY | O_CREAT | O_EXCL, 0666);
-	if (fd < 0 || adjust_shared_perm(lock_file)) {
+	lock = xcalloc(sizeof(struct lock_file), 1);
+	fd = hold_lock_file_for_update(lock, config_filename, 0);
+	if (fd < 0) {
 		fprintf(stderr, "could not lock config file\n");
 		free(store.key);
 		ret = -1;
@@ -914,25 +914,29 @@ int git_config_set_multivar(const char* key, const char* value,
 				goto write_err_out;
 
 		munmap(contents, contents_sz);
-		unlink(config_filename);
 	}
 
-	if (rename(lock_file, config_filename) < 0) {
-		fprintf(stderr, "Could not rename the lock file?\n");
+	if (close(fd) || commit_lock_file(lock) < 0) {
+		fprintf(stderr, "Cannot commit config file!\n");
 		ret = 4;
 		goto out_free;
 	}
 
+	/* fd is closed, so don't try to close it below. */
+	fd = -1;
+	/* lock is committed, so don't try to roll it back below.
+	 * NOTE: Since lockfile.c keeps a linked list of all created
+	 * lock files, it isn't safe to free(lock).  It's better to just
+	 * leave it hanging around. */
+	lock = NULL;
 	ret = 0;
 
 out_free:
 	if (0 <= fd)
 		close(fd);
+	if (lock)
+		rollback_lock_file(lock);
 	free(config_filename);
-	if (lock_file) {
-		unlink(lock_file);
-		free(lock_file);
-	}
 	return ret;
 
 write_err_out:
-- 
1.5.3.rc2.30.g1c06-dirty

Re: [PATCH 1/2] resolve symlinks when creating lockfiles

[Junio C Hamano]

This probably is going in the right direction, but the code is
too densely formatted and unreviewable.  Please imitate the
layout convention of the other parts of the code.

> +/**
> + * p = absolute or relative path name
> + *
> + * Return a pointer into p showing the beginning of the last path name
> + * element.  If p is empty or the root directory ("/"), just return p.
> + */

	/*
         * multi-line comments look like this without the extra
         * asterisk at the beginning of the first line.
         */

> +static char * last_path_elm(char * p)

char *last_path_elem(char *p)

> +{
> +	int	p_len = strlen(p);
> +	char *	r;
> +
> +	if (p_len < 1) return p;

        char *r;
	int p_len = strlen(p);

        if (p_len < 1)
                return p;

Aren't p and r of type "const char *", I wonder...

> +	/* r points to last non-null character in p */
> +	r = p + p_len - 1;
> +	/* first skip any trailing slashes */
> +	while (*r == '/' && r > p) r--;

That is

	r = strrchr(p, '/');

isn't it?

> +/**
> + * p = char array containing path to existing file or symlink
> + * s = size of p
> + *
> + * If p indicates a valid symlink to an existing file, overwrite p with
> + * the path to the real file.  Otherwise, leave p unmodified.

I suspect some callers use lockfile interface to create a new
file.  There will be a symlink to not-yet-created real file,
that is.

[PATCH] use lockfile.c routines in git_commit_set_multivar()

[Bradford C. Smith]

Changed git_commit_set_multivar() to use the routines provided by
lockfile.c to reduce code duplication and ensure consistent behavior.

Signed-off-by: Bradford C. Smith <bradford.carl.smith@gmail.com>
---

I am resubmitting this patch to be considered separately from the
symlink resolution change.  It differs from the first time I submitted
it only in that I have corrected the multi-line comment formatting I
used.

I ran the full test suite (make test) on this patch without failures.

 config.c |   30 ++++++++++++++++++------------
 1 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/config.c b/config.c
index f89a611..dd2de6e 100644
--- a/config.c
+++ b/config.c
@@ -715,7 +715,7 @@ int git_config_set_multivar(const char* key, const char* value,
 	int fd = -1, in_fd;
 	int ret;
 	char* config_filename;
-	char* lock_file;
+	struct lock_file *lock = NULL;
 	const char* last_dot = strrchr(key, '.');
 
 	config_filename = getenv(CONFIG_ENVIRONMENT);
@@ -725,7 +725,6 @@ int git_config_set_multivar(const char* key, const char* value,
 			config_filename  = git_path("config");
 	}
 	config_filename = xstrdup(config_filename);
-	lock_file = xstrdup(mkpath("%s.lock", config_filename));
 
 	/*
 	 * Since "key" actually contains the section name and the real
@@ -770,11 +769,12 @@ int git_config_set_multivar(const char* key, const char* value,
 	store.key[i] = 0;
 
 	/*
-	 * The lock_file serves a purpose in addition to locking: the new
+	 * The lock serves a purpose in addition to locking: the new
 	 * contents of .git/config will be written into it.
 	 */
-	fd = open(lock_file, O_WRONLY | O_CREAT | O_EXCL, 0666);
-	if (fd < 0 || adjust_shared_perm(lock_file)) {
+	lock = xcalloc(sizeof(struct lock_file), 1);
+	fd = hold_lock_file_for_update(lock, config_filename, 0);
+	if (fd < 0) {
 		fprintf(stderr, "could not lock config file\n");
 		free(store.key);
 		ret = -1;
@@ -914,25 +914,31 @@ int git_config_set_multivar(const char* key, const char* value,
 				goto write_err_out;
 
 		munmap(contents, contents_sz);
-		unlink(config_filename);
 	}
 
-	if (rename(lock_file, config_filename) < 0) {
-		fprintf(stderr, "Could not rename the lock file?\n");
+	if (close(fd) || commit_lock_file(lock) < 0) {
+		fprintf(stderr, "Cannot commit config file!\n");
 		ret = 4;
 		goto out_free;
 	}
 
+	/* fd is closed, so don't try to close it below. */
+	fd = -1;
+	/*
+	 * lock is committed, so don't try to roll it back below.
+	 * NOTE: Since lockfile.c keeps a linked list of all created
+	 * lock_file structures, it isn't safe to free(lock).  It's
+	 * better to just leave it hanging around.
+	 */
+	lock = NULL;
 	ret = 0;
 
 out_free:
 	if (0 <= fd)
 		close(fd);
+	if (lock)
+		rollback_lock_file(lock);
 	free(config_filename);
-	if (lock_file) {
-		unlink(lock_file);
-		free(lock_file);
-	}
 	return ret;
 
 write_err_out:
-- 
1.5.3.rc3.9.g1b487

Re: [PATCH] use lockfile.c routines in git_commit_set_multivar()

[Johannes Schindelin]

Hi,

I like the general idea.  Thanks.


On Thu, 26 Jul 2007, Bradford C. Smith wrote:

> +	/* fd is closed, so don't try to close it below. */
> +	fd = -1;
> +	/*
> +	 * lock is committed, so don't try to roll it back below.
> +	 * NOTE: Since lockfile.c keeps a linked list of all created
> +	 * lock_file structures, it isn't safe to free(lock).  It's
> +	 * better to just leave it hanging around.
> +	 */
> +	lock = NULL;
>  	ret = 0;
>  
>  out_free:
>  	if (0 <= fd)
>  		close(fd);
> +	if (lock)
> +		rollback_lock_file(lock);

Wouldn't it be better to put the rollback_lock_file() into the if clause 
when commit failed?

Besides, I think you can safely call rollback_lock_file(lock) on a 
committed lock_file, since the name will be set to "" by the latter, which 
is checked by the former.

But I am fine with the patch as is (have not tested it, though).

Ciao,
Dscho

Re: [PATCH] use lockfile.c routines in git_commit_set_multivar()

[Bradford Smith]

On 7/26/07, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Thu, 26 Jul 2007, Bradford C. Smith wrote:
>
> > +     /* fd is closed, so don't try to close it below. */
> > +     fd = -1;
> > +     /*
> > +      * lock is committed, so don't try to roll it back below.
> > +      * NOTE: Since lockfile.c keeps a linked list of all created
> > +      * lock_file structures, it isn't safe to free(lock).  It's
> > +      * better to just leave it hanging around.
> > +      */
> > +     lock = NULL;
> >       ret = 0;
> >
> >  out_free:
> >       if (0 <= fd)
> >               close(fd);
> > +     if (lock)
> > +             rollback_lock_file(lock);
>
> Wouldn't it be better to put the rollback_lock_file() into the if clause
> when commit failed?

Actually no.  There are multiple goto statements that lead to
out_free.  It isn't even needed at the point that the commit failed,
because commit_lock_file() sets the lock file name to "" even when it
fails.

> Besides, I think you can safely call rollback_lock_file(lock) on a
> committed lock_file, since the name will be set to "" by the latter, which
> is checked by the former.

Quite right.  I really just put in the comment and 'lock= NULL' line
to increase readability.  I wanted to make it very clear to the reader
that the commit wouldn't be undone by the rollback.

> But I am fine with the patch as is (have not tested it, though).

Thanks!

FWIW, I have successfully run 'make test' and also verified that it
behaves as I expect with my ~/.gitconfig symlink (in conjunction with
the my other patch for resolving symlinks).

Best Regards,

Bradford

Re: [PATCH] use lockfile.c routines in git_commit_set_multivar()

[Junio C Hamano]

"Bradford Smith" <bradford.carl.smith@gmail.com> writes:

> FWIW, I have successfully run 'make test' and also verified that it
> behaves as I expect with my ~/.gitconfig symlink (in conjunction with
> the my other patch for resolving symlinks).

Existing "make test" testsuite is not an appropriate thing to
say this patch is safe, as we do not have much symlinking in the
test git repository there.  Care to add a new test or two?

Re: [PATCH] use lockfile.c routines in git_commit_set_multivar()

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "Bradford Smith" <bradford.carl.smith@gmail.com> writes:
>
>> FWIW, I have successfully run 'make test' and also verified that it
>> behaves as I expect with my ~/.gitconfig symlink (in conjunction with
>> the my other patch for resolving symlinks).
>
> Existing "make test" testsuite is not an appropriate thing to
> say this patch is safe, as we do not have much symlinking in the
> test git repository there.  Care to add a new test or two?

How about this?  On top of your "lockfile to keep symlink" and
"set-multivar to use lockfile protocol" patches.

---

 t/t1300-repo-config.sh |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 1c43cc3..187ca2d 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -595,4 +595,19 @@ echo >>result
 
 test_expect_success '--null --get-regexp' 'cmp result expect'
 
+test_expect_success 'symlinked configuration' '
+
+	ln -s notyet myconfig &&
+	GIT_CONFIG=myconfig git config test.frotz nitfol &&
+	test -h myconfig &&
+	test -f notyet &&
+	test "z$(GIT_CONFIG=notyet git config test.frotz)" = znitfol &&
+	GIT_CONFIG=myconfig git config test.xyzzy rezrov &&
+	test -h myconfig &&
+	test -f notyet &&
+	test "z$(GIT_CONFIG=notyet git config test.frotz)" = znitfol &&
+	test "z$(GIT_CONFIG=notyet git config test.xyzzy)" = zrezrov
+
+'
+
 test_done

Re: [PATCH] use lockfile.c routines in git_commit_set_multivar()

[Johannes Schindelin]

Hi,

On Thu, 26 Jul 2007, Junio C Hamano wrote:

> How about this?  On top of your "lockfile to keep symlink" and
> "set-multivar to use lockfile protocol" patches.

Looks very good to me!

Ciao,
Dscho

Re: [PATCH] use lockfile.c routines in git_commit_set_multivar()

[Bradford Smith]

That's great!

I've added this patch to my local branch and confirmed that all tests,
including the new ones, run successfully.

Thanks!

Bradford

On 7/27/07, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > "Bradford Smith" <bradford.carl.smith@gmail.com> writes:
> >
> >> FWIW, I have successfully run 'make test' and also verified that it
> >> behaves as I expect with my ~/.gitconfig symlink (in conjunction with
> >> the my other patch for resolving symlinks).
> >
> > Existing "make test" testsuite is not an appropriate thing to
> > say this patch is safe, as we do not have much symlinking in the
> > test git repository there.  Care to add a new test or two?
>
> How about this?  On top of your "lockfile to keep symlink" and
> "set-multivar to use lockfile protocol" patches.
>
> ---
>
>  t/t1300-repo-config.sh |   15 +++++++++++++++
>  1 files changed, 15 insertions(+), 0 deletions(-)
>
> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index 1c43cc3..187ca2d 100755
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh
> @@ -595,4 +595,19 @@ echo >>result
>
>  test_expect_success '--null --get-regexp' 'cmp result expect'
>
> +test_expect_success 'symlinked configuration' '
> +
> +       ln -s notyet myconfig &&
> +       GIT_CONFIG=myconfig git config test.frotz nitfol &&
> +       test -h myconfig &&
> +       test -f notyet &&
> +       test "z$(GIT_CONFIG=notyet git config test.frotz)" = znitfol &&
> +       GIT_CONFIG=myconfig git config test.xyzzy rezrov &&
> +       test -h myconfig &&
> +       test -f notyet &&
> +       test "z$(GIT_CONFIG=notyet git config test.frotz)" = znitfol &&
> +       test "z$(GIT_CONFIG=notyet git config test.xyzzy)" = zrezrov
> +
> +'
> +
>  test_done
>
>

[PATCH] fully resolve symlinks when creating lockfiles

[Bradford C. Smith]

Make the code for resolving symlinks in lockfile.c more robust as
follows:

1. Handle relative symlinks
2. recursively resolve symlink chains up to OS limit

Signed-off-by: Bradford C. Smith <bradford.carl.smith@gmail.com>
---

I have updated this patch as follows based partly on Junio's comments.

	1. Made comment and coding style consistent with existing git
	   code base.
	2. improved readability
	3. rebased to latest version of master (2007-07-26) and updated
	   commit message appropriately
	4. added warning messages for error conditions
	5. resolve symlinks to non-existent files

 lockfile.c |  128 +++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 114 insertions(+), 14 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 9202472..864ce73 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -25,23 +25,123 @@ static void remove_lock_file_on_signal(int signo)
 	raise(signo);
 }
 
+/*
+ * p = absolute or relative path name
+ *
+ * Return a pointer into p showing the beginning of the last path name
+ * element.  If p is empty or the root directory ("/"), just return p.
+ */
+static const char *last_path_elm(const char *p)
+{
+	/* r starts pointing to null at the end of the string */
+	const char *r = strchr(p, '\0');
+
+	if (r == p)
+		return p; /* just return empty string */
+
+	r--; /* back up to last non-null character */
+
+	/* back up past trailing slashes, if any */
+	while (r > p && *r == '/') {
+		r--;
+	}
+	/*
+	 * then go backwards until I hit a slash, or the beginning of
+	 * the string
+	 */
+	while (r > p && *(r-1) != '/') {
+		r--;
+	}
+	return r;
+}
+
+
+/*
+ * 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.
+ *
+ * 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.
+ *
+ * Always returns p.
+ */
+static char *resolve_symlink(char * p, size_t s)
+{
+	struct stat stb;
+	char link[PATH_MAX];
+	int link_len;
+
+	/*
+	 * leave p unchanged if it doesn't appear to be a valid path to
+	 * a symlink.
+	 */
+	if (lstat(p, &stb) != 0 || !S_ISLNK(stb.st_mode)) {
+		return p;
+	}
+	/*
+	 * don't attempt to resolve a chain or loop of symlinks the OS
+	 * cannot resolve.
+	 */
+	if (stat(p, &stb) != 0 && ELOOP == errno) {
+		warning("%s: %s", p, strerror(ELOOP));
+		return p;
+	}
+
+	link_len = readlink(p, link, sizeof(link));
+	if (link_len < 0) {
+		warning("%s: %s", p, strerror(errno));
+		return p;
+	} else if (link_len < sizeof(link)) {
+		/* readlink() never null-terminates */
+		link[link_len] = '\0';
+	} else {
+		warning("%s: symlink too long", p);
+		return p;
+	}
+
+	if (link[0] == '/') {
+		/* absolute path simply replaces p */
+		if (link_len < s) {
+			strcpy(p, link);
+		} else {
+			warning("%s: symlink too long", p);
+			return p;
+		}
+	} else {
+		/*
+		 * link is a relative path, so I must replace the last
+		 * element of p with it.
+		 */
+		char *r = (char*)last_path_elm(p);
+		if (r - p + link_len < s) {
+			strcpy(r, link);
+		} else {
+			warning("%s: symlink too long", p);
+			return p;
+		}
+	}
+	/* try again in case we've resolved to another symlink */
+	return resolve_symlink(p, s);
+}
+
+
 static int lock_file(struct lock_file *lk, const char *path)
 {
 	int fd;
-	struct stat st;
-
-	if ((!lstat(path, &st)) && S_ISLNK(st.st_mode)) {
-		ssize_t sz;
-		static char target[PATH_MAX];
-		sz = readlink(path, target, sizeof(target));
-		if (sz < 0)
-			warning("Cannot readlink %s", path);
-		else if (target[0] != '/')
-			warning("Cannot lock target of relative symlink %s", path);
-		else
-			path = target;
-	}
-	sprintf(lk->filename, "%s.lock", path);
+
+	if (strlen(path) >= sizeof(lk->filename)) return -1;
+	strcpy(lk->filename, path);
+	/*
+	 * subtract 5 from size to make sure there's room for adding
+	 * ".lock" for the lock file name
+	 */
+	resolve_symlink(lk->filename, sizeof(lk->filename)-5);
+	strcat(lk->filename, ".lock");
 	fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
 	if (0 <= fd) {
 		if (!lock_file_list) {
-- 
1.5.3.rc3.9.g1b487

Re: [PATCH] fully resolve symlinks when creating lockfiles

[Johannes Schindelin]

Hi,

On Thu, 26 Jul 2007, Bradford C. Smith wrote:

> Make the code for resolving symlinks in lockfile.c more robust as
> follows:
> 
> 1. Handle relative symlinks
> 2. recursively resolve symlink chains up to OS limit

FWIW I like what it does, but how.  What is so wrong with just relying on 
is_absolute_path() and make_absolute_path()?  The code would be much 
shorter then, and we need those functions anyway, methinks.

Ciao,
Dscho

Re: [PATCH] fully resolve symlinks when creating lockfiles

[Morten Welinder]

Why the lstat and that stat in the beginning?  That's just asking for race
condition.  readlink will tell you if it wasn't a link, for example.

Morten

Re: [PATCH] fully resolve symlinks when creating lockfiles

[Bradford Smith]

On 7/26/07, Morten Welinder <mwelinder@gmail.com> wrote:
> Why the lstat and that stat in the beginning?  That's just asking for race
> condition.  readlink will tell you if it wasn't a link, for example.

Here's an example of the sort of thing I'm trying to avoid:

foo is a symlink to bar
bar is a symlink back to foo

readlink() on either one will succeed, but I'll end up with infinite
recursion because I'll resolve foo to bar, then bar to foo, then foo
back to bar, etc.

To avoid craziness like this the OS refuses to follow a chain of more
than a very small number of symlinks.  By experimentation, I found the
limit to be 8 on my Linux box.

I am trying to avoid resolving symlinks manually that the OS would
refuse to resolve anyway.

However, I'm quite open to suggestions for a better way to do it.

Thanks,

Bradford

Re: [PATCH] fully resolve symlinks when creating lockfiles

[Junio C Hamano]

"Bradford C. Smith" <bradford.carl.smith@gmail.com> writes:

> Make the code for resolving symlinks in lockfile.c more robust as
> follows:
>
> 1. Handle relative symlinks
> 2. recursively resolve symlink chains up to OS limit

I munged this patch with Morten's comments.  Will queue for
'next'.  Further polishing will be done in 'next' as needed.

Re: git-config: replaces ~/.gitconfig symlink with real file

[Johannes Schindelin]

Hi,

On Mon, 16 Jul 2007, Bradford Smith wrote:

> So, I guess I need to add a GIT_CONFIG_HOME environment variable.  If I 
> get that done, I'll send a patch to the list including doc updates.

Alternatively, you could actually not ignore my hint at readlink(2) and 
have a proper fix, instead of playing games with environment variables.

Hth,
Dscho

Re: git-config: replaces ~/.gitconfig symlink with real file

[Matthieu Moy]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi,
>
> On Mon, 16 Jul 2007, Bradford Smith wrote:
>
>> So, I guess I need to add a GIT_CONFIG_HOME environment variable.  If I 
>> get that done, I'll send a patch to the list including doc updates.
>
> Alternatively, you could actually not ignore my hint at readlink(2) and 
> have a proper fix, instead of playing games with environment variables.

I second that.

Using an environment variable means having a configuration which is
about git in my shell's config file, and that's a source of a lot of
troubles. Murphy's law implies that one day, the environment variable
won't be set properly (because you changed your shell, because you
launch git from something which isn't a shell, because you logged-in
in a way that didn't read the config file in which the variable was
set, ...).

I can do with it, like many other software require an environment
variable, but I find the symlink trick much more robust.

-- 
Matthieu

Re: git-config: replaces ~/.gitconfig symlink with real file

[Fredrik Tolf]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi,
>
> On Mon, 16 Jul 2007, Bradford Smith wrote:
>
>> So, I guess I need to add a GIT_CONFIG_HOME environment variable.  If I 
>> get that done, I'll send a patch to the list including doc updates.
>
> Alternatively, you could actually not ignore my hint at readlink(2) and 
> have a proper fix, instead of playing games with environment variables.

Wouldn't it be nicer to avoid a lot of the complexity in checking
symlinks, environment variables and what not, and just overwrite the
file in place (with open(..., O_TRUNC | O_CREAT))? Does it happen
terribly often that git-config crashes in the middle and leaves the
file broken?

Fredrik Tolf

Re: git-config: replaces ~/.gitconfig symlink with real file

[Johannes Schindelin]

Hi,

On Tue, 17 Jul 2007, Fredrik Tolf wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Mon, 16 Jul 2007, Bradford Smith wrote:
> >
> >> So, I guess I need to add a GIT_CONFIG_HOME environment variable.  If I 
> >> get that done, I'll send a patch to the list including doc updates.
> >
> > Alternatively, you could actually not ignore my hint at readlink(2) and 
> > have a proper fix, instead of playing games with environment variables.
> 
> Wouldn't it be nicer to avoid a lot of the complexity in checking 
> symlinks, environment variables and what not, and just overwrite the 
> file in place (with open(..., O_TRUNC | O_CREAT))? Does it happen 
> terribly often that git-config crashes in the middle and leaves the file 
> broken?

No, it does not.  But when it does, I am not only annoyed.  I am PISSED!

The way we do it is the only safe way to do it, and I gladly spend some 
extra cycles for that.  Too often, a small hard disk glitch (or just an 
empty laptop battery!) took some important data into the void.  Too often, 
I _cursed_ at the machine, even if it was the programmers' fault.

Ciao,
Dscho

Re: git-config: replaces ~/.gitconfig symlink with real file

[Catalin Marinas]

"Bradford Smith" <bradford.carl.smith@gmail.com> wrote:
> However, when I tried running 'git-config --global color.diff auto'
> today, it removed my symlink and replaced it with a real file.  This
> left me briefly a bit confused when the changes I had made didn't show
> up in ~/etc/gitconfig, but git-config reported them anyway.

Another problem I have with 'git config --global' is that it changes
the access permission bits of ~/.gitconfig. Since I use the same file
to store global StGIT configuration like SMTP username and password,
I'd like to make its access 0600 but it always goes back to 0644 after
'git config --global'.

Maybe fixing the symlink case would solve my problem as well.

Thanks.

-- 
Catalin

Re: git-config: replaces ~/.gitconfig symlink with real file

[Johannes Schindelin]

Hi,

On Tue, 17 Jul 2007, Catalin Marinas wrote:

> "Bradford Smith" <bradford.carl.smith@gmail.com> wrote:
> > However, when I tried running 'git-config --global color.diff auto'
> > today, it removed my symlink and replaced it with a real file.  This
> > left me briefly a bit confused when the changes I had made didn't show
> > up in ~/etc/gitconfig, but git-config reported them anyway.
> 
> Another problem I have with 'git config --global' is that it changes
> the access permission bits of ~/.gitconfig. Since I use the same file
> to store global StGIT configuration like SMTP username and password,
> I'd like to make its access 0600 but it always goes back to 0644 after
> 'git config --global'.
> 
> Maybe fixing the symlink case would solve my problem as well.

More likely not.  The way to solve it would be to follow the link if the 
target path is one.  As such, the _file_ would be rewritten.

So your problem is unrelated, and would need a separate fix.

Ciao,
Dscho