[PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Johannes Schindelin]

It seems that accessing NTFS partitions with ufsd (at least on my EeePC)
has an unnerving bug: if you link() a file and unlink() it right away,
the target of the link() will have the correct size, but consist of NULs.

It seems as if the calls are simply not serialized correctly, as single-stepping
through the function move_temp_to_file() works flawlessly.

As ufsd is "Commertial software", I cannot fix it, and have to work
around it in Git.

At the same time, it seems that this fixes msysGit issues 222 and 229 to
assume that Windows cannot handle link() && unlink().

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	I know this is pretty late in the -rc1 cycle, but this is 
	something I need to apply for msysGit anyway.

	And it does not really seem too unobvious a fix, does it?

 Documentation/config.txt |    5 +++++
 Makefile                 |    8 ++++++++
 cache.h                  |    2 ++
 config.c                 |    5 +++++
 environment.c            |    4 ++++
 sha1_file.c              |    4 +++-
 6 files changed, 27 insertions(+), 1 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 35056e1..62cd903 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -429,6 +429,11 @@ relatively high IO latencies.  With this set to 'true', git will do the
 index comparison to the filesystem data in parallel, allowing
 overlapping IO's.
 
+core.unreliableHardlinks::
+	Some filesystem drivers cannot properly handle hardlinking a file
+	and deleting the source right away.  In such a case, you need to
+	set this config variable to 'true'.
+
 alias.*::
 	Command aliases for the linkgit:git[1] command wrapper - e.g.
 	after defining "alias.last = cat-file commit HEAD", the invocation
diff --git a/Makefile b/Makefile
index 49f36f5..e3979e0 100644
--- a/Makefile
+++ b/Makefile
@@ -171,6 +171,10 @@ all::
 # Define UNRELIABLE_FSTAT if your system's fstat does not return the same
 # information on a not yet closed file that lstat would return for the same
 # file after it was closed.
+#
+# Define UNRELIABLE_HARDLINKS if your operating systems has problems when
+# hardlinking a file to another name and unlinking the original file right
+# away (some NTFS drivers seem to zero the contents in that scenario).
 
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -835,6 +839,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_NSEC = YesPlease
 	USE_WIN32_MMAP = YesPlease
 	UNRELIABLE_FSTAT = UnfortunatelyYes
+	UNRELIABLE_HARDLINKS = UnfortunatelySometimes
 	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/regex -Icompat/fnmatch
 	COMPAT_CFLAGS += -DSNPRINTF_SIZE_CORR=1
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
@@ -1018,6 +1023,9 @@ else
 		COMPAT_OBJS += compat/win32mmap.o
 	endif
 endif
+ifdef UNRELIABLE_HARDLINKS
+	COMPAT_CFLAGS += -DUNRELIABLE_HARDLINKS=1
+endif
 ifdef NO_PREAD
 	COMPAT_CFLAGS += -DNO_PREAD
 	COMPAT_OBJS += compat/pread.o
diff --git a/cache.h b/cache.h
index ab1294d..ff9e145 100644
--- a/cache.h
+++ b/cache.h
@@ -554,6 +554,8 @@ extern enum branch_track git_branch_track;
 extern enum rebase_setup_type autorebase;
 extern enum push_default_type push_default;
 
+extern int unreliable_hardlinks;
+
 #define GIT_REPO_VERSION 0
 extern int repository_format_version;
 extern int check_repository_format(void);
diff --git a/config.c b/config.c
index 8c1ae59..1750cfb 100644
--- a/config.c
+++ b/config.c
@@ -495,6 +495,11 @@ static int git_default_core_config(const char *var, const char *value)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.unreliablehardlinks")) {
+		unreliable_hardlinks = git_config_bool(var, value);
+		return 0;
+	}
+
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;
 }
diff --git a/environment.c b/environment.c
index 4696885..10578d2 100644
--- a/environment.c
+++ b/environment.c
@@ -43,6 +43,10 @@ unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
 enum branch_track git_branch_track = BRANCH_TRACK_REMOTE;
 enum rebase_setup_type autorebase = AUTOREBASE_NEVER;
 enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
+#ifndef UNRELIABLE_HARDLINKS
+#define UNRELIABLE_HARDLINKS 0
+#endif
+int unreliable_hardlinks = UNRELIABLE_HARDLINKS;
 
 /* Parallel index stat data preload? */
 int core_preload_index = 0;
diff --git a/sha1_file.c b/sha1_file.c
index 8fe135d..f5a7970 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2225,7 +2225,9 @@ int move_temp_to_file(const char *tmpfile, const char *filename)
 {
 	int ret = 0;
 
-	if (link(tmpfile, filename))
+	if (unreliable_hardlinks)
+		ret = ~EEXIST;
+	else if (link(tmpfile, filename))
 		ret = errno;
 
 	/*
-- 
1.6.2.1.613.g25746

Re: [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Johannes Sixt]

On Donnerstag, 23. April 2009, Johannes Schindelin wrote:
> It seems that accessing NTFS partitions with ufsd (at least on my EeePC)
> has an unnerving bug: if you link() a file and unlink() it right away,
> the target of the link() will have the correct size, but consist of NULs.
>
> It seems as if the calls are simply not serialized correctly, as
> single-stepping through the function move_temp_to_file() works flawlessly.
>
> As ufsd is "Commertial software", I cannot fix it, and have to work

"commercial software"

> around it in Git.
>
> At the same time, it seems that this fixes msysGit issues 222 and 229 to
> assume that Windows cannot handle link() && unlink().
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
...
> @@ -2225,7 +2225,9 @@ int move_temp_to_file(const char *tmpfile, const char
> *filename) {
>  	int ret = 0;
>
> -	if (link(tmpfile, filename))
> +	if (unreliable_hardlinks)
> +		ret = ~EEXIST;

It took me a while to see why we need a tilde here, but it's ok. Perhaps this 
helps others:

+		ret = ~EEXIST;	/* anything but EEXIST */

Nevertheless:

Acked-by: Johannes Sixt <j6t@kdbg.org>

> +	else if (link(tmpfile, filename))
>  		ret = errno;

-- Hannes

Re: [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Johannes Schindelin]

Hi,

On Thu, 23 Apr 2009, Johannes Sixt wrote:

> On Donnerstag, 23. April 2009, Johannes Schindelin wrote:
> > It seems that accessing NTFS partitions with ufsd (at least on my EeePC)
> > has an unnerving bug: if you link() a file and unlink() it right away,
> > the target of the link() will have the correct size, but consist of NULs.
> >
> > It seems as if the calls are simply not serialized correctly, as
> > single-stepping through the function move_temp_to_file() works flawlessly.
> >
> > As ufsd is "Commertial software", I cannot fix it, and have to work
> 
> "commercial software"

I just quoted the license string of that wonderfully high-quality kernel 
module.

Maybe I should have added the beloved "[sic!]".

> > At the same time, it seems that this fixes msysGit issues 222 and 229 to
> > assume that Windows cannot handle link() && unlink().
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ...
> > @@ -2225,7 +2225,9 @@ int move_temp_to_file(const char *tmpfile, const char
> > *filename) {
> >  	int ret = 0;
> >
> > -	if (link(tmpfile, filename))
> > +	if (unreliable_hardlinks)
> > +		ret = ~EEXIST;
> 
> It took me a while to see why we need a tilde here, but it's ok. Perhaps this 
> helps others:
> 
> +		ret = ~EEXIST;	/* anything but EEXIST */

Will do.

> Nevertheless:
> 
> Acked-by: Johannes Sixt <j6t@kdbg.org>

Thanks.

But it will have to wait for Saturday.

Ciao,
Dscho

[PATCH v2] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Johannes Schindelin]

It seems that accessing NTFS partitions with ufsd (at least on my EeePC)
has an unnerving bug: if you link() a file and unlink() it right away,
the target of the link() will have the correct size, but consist of NULs.

It seems as if the calls are simply not serialized correctly, as single-stepping
through the function move_temp_to_file() works flawlessly.

As ufsd is "Commertial software" (sic!), I cannot fix it, and have to work
around it in Git.

At the same time, it seems that this fixes msysGit issues 222 and 229 to
assume that Windows cannot handle link() && unlink().

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Acked-by: Johannes Sixt <j6t@kdbg.org>
---

	On Thu, 23 Apr 2009, Johannes Sixt wrote:

	> +		ret = ~EEXIST;	/* anything but EEXIST */

	In addition to this, I also added the "(sic!)" for the "Commertial 
	software" tyop.  So I still dared to add your

	> Acked-by: Johannes Sixt <j6t@kdbg.org>

 Documentation/config.txt |    5 +++++
 Makefile                 |    8 ++++++++
 cache.h                  |    2 ++
 config.c                 |    5 +++++
 environment.c            |    4 ++++
 sha1_file.c              |    4 +++-
 6 files changed, 27 insertions(+), 1 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3188569..d31adb6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -429,6 +429,11 @@ relatively high IO latencies.  With this set to 'true', git will do the
 index comparison to the filesystem data in parallel, allowing
 overlapping IO's.
 
+core.unreliableHardlinks::
+	Some filesystem drivers cannot properly handle hardlinking a file
+	and deleting the source right away.  In such a case, you need to
+	set this config variable to 'true'.
+
 alias.*::
 	Command aliases for the linkgit:git[1] command wrapper - e.g.
 	after defining "alias.last = cat-file commit HEAD", the invocation
diff --git a/Makefile b/Makefile
index 6f602c7..5c8e83a 100644
--- a/Makefile
+++ b/Makefile
@@ -171,6 +171,10 @@ all::
 # Define UNRELIABLE_FSTAT if your system's fstat does not return the same
 # information on a not yet closed file that lstat would return for the same
 # file after it was closed.
+#
+# Define UNRELIABLE_HARDLINKS if your operating systems has problems when
+# hardlinking a file to another name and unlinking the original file right
+# away (some NTFS drivers seem to zero the contents in that scenario).
 
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -835,6 +839,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_NSEC = YesPlease
 	USE_WIN32_MMAP = YesPlease
 	UNRELIABLE_FSTAT = UnfortunatelyYes
+	UNRELIABLE_HARDLINKS = UnfortunatelySometimes
 	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/regex -Icompat/fnmatch
 	COMPAT_CFLAGS += -DSNPRINTF_SIZE_CORR=1
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
@@ -1018,6 +1023,9 @@ else
 		COMPAT_OBJS += compat/win32mmap.o
 	endif
 endif
+ifdef UNRELIABLE_HARDLINKS
+	COMPAT_CFLAGS += -DUNRELIABLE_HARDLINKS=1
+endif
 ifdef NO_PREAD
 	COMPAT_CFLAGS += -DNO_PREAD
 	COMPAT_OBJS += compat/pread.o
diff --git a/cache.h b/cache.h
index ab1294d..ff9e145 100644
--- a/cache.h
+++ b/cache.h
@@ -554,6 +554,8 @@ extern enum branch_track git_branch_track;
 extern enum rebase_setup_type autorebase;
 extern enum push_default_type push_default;
 
+extern int unreliable_hardlinks;
+
 #define GIT_REPO_VERSION 0
 extern int repository_format_version;
 extern int check_repository_format(void);
diff --git a/config.c b/config.c
index 8c1ae59..1750cfb 100644
--- a/config.c
+++ b/config.c
@@ -495,6 +495,11 @@ static int git_default_core_config(const char *var, const char *value)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.unreliablehardlinks")) {
+		unreliable_hardlinks = git_config_bool(var, value);
+		return 0;
+	}
+
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;
 }
diff --git a/environment.c b/environment.c
index 4696885..10578d2 100644
--- a/environment.c
+++ b/environment.c
@@ -43,6 +43,10 @@ unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
 enum branch_track git_branch_track = BRANCH_TRACK_REMOTE;
 enum rebase_setup_type autorebase = AUTOREBASE_NEVER;
 enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
+#ifndef UNRELIABLE_HARDLINKS
+#define UNRELIABLE_HARDLINKS 0
+#endif
+int unreliable_hardlinks = UNRELIABLE_HARDLINKS;
 
 /* Parallel index stat data preload? */
 int core_preload_index = 0;
diff --git a/sha1_file.c b/sha1_file.c
index 8fe135d..bb6eecf 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2225,7 +2225,9 @@ int move_temp_to_file(const char *tmpfile, const char *filename)
 {
 	int ret = 0;
 
-	if (link(tmpfile, filename))
+	if (unreliable_hardlinks)
+		ret = ~EEXIST; /* anything but EEXIST */
+	else if (link(tmpfile, filename))
 		ret = errno;
 
 	/*
-- 
1.6.2.1.613.g25746

Re: [PATCH v2] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Junio C Hamano]

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

> It seems that accessing NTFS partitions with ufsd (at least on my EeePC)
> has an unnerving bug: if you link() a file and unlink() it right away,
> the target of the link() will have the correct size, but consist of NULs.
>
> It seems as if the calls are simply not serialized correctly, as single-stepping
> through the function move_temp_to_file() works flawlessly.
>
> As ufsd is "Commertial software" (sic!), I cannot fix it, and have to work
> around it in Git.
>
> At the same time, it seems that this fixes msysGit issues 222 and 229 to
> assume that Windows cannot handle link() && unlink().
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Acked-by: Johannes Sixt <j6t@kdbg.org>

Hannes, are you ok with this?

> diff --git a/environment.c b/environment.c
> index 4696885..10578d2 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -43,6 +43,10 @@ unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
>  enum branch_track git_branch_track = BRANCH_TRACK_REMOTE;
>  enum rebase_setup_type autorebase = AUTOREBASE_NEVER;
>  enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
> +#ifndef UNRELIABLE_HARDLINKS
> +#define UNRELIABLE_HARDLINKS 0
> +#endif
> +int unreliable_hardlinks = UNRELIABLE_HARDLINKS;

Hmm, this ifndef/define/endif is somewhat yucky to see especially in a .c
source file.  Sorry, I do not think of a better alternative, though.

	int unreliable_hardlinks = defined(UNRELIABLE_HARDLINKS)

would not work either X-<.

> diff --git a/sha1_file.c b/sha1_file.c
> index 8fe135d..bb6eecf 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2225,7 +2225,9 @@ int move_temp_to_file(const char *tmpfile, const char *filename)
>  {
>  	int ret = 0;
>  
> -	if (link(tmpfile, filename))
> +	if (unreliable_hardlinks)
> +		ret = ~EEXIST; /* anything but EEXIST */

It is a bit too far away from the:

	if (ret && ret != EEXIST)

you are trying to trigger with this hack, and without seeing that "if" in
the context anybody would go "Huh?".  It is a good sign that this is
fragile (the later "if" may be rewritten by somebody else without
realizing this hack exists).  Besides, it is (rather, "happens to be at
this moment") "anything non-zero but EEXIST".

I have a feeling that it would be much less fragile to write it like this,
as a label warns anybody touching the code to check where else the control
flow may come from.

diff --git a/sha1_file.c b/sha1_file.c
index 8fe135d..11969fc 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2225,7 +2225,9 @@ int move_temp_to_file(const char *tmpfile, const char *filename)
 {
 	int ret = 0;
 
-	if (link(tmpfile, filename))
+	if (unreliable_hardlinks)
+		goto try_rename;
+	else if (link(tmpfile, filename))
 		ret = errno;
 
 	/*
@@ -2240,6 +2242,7 @@ int move_temp_to_file(const char *tmpfile, const char *filename)
 	 * left to unlink.
 	 */
 	if (ret && ret != EEXIST) {
+	try_rename:
 		if (!rename(tmpfile, filename))
 			goto out;
 		ret = errno;

Re: [PATCH v2] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Linus Torvalds]

On Sat, 25 Apr 2009, Junio C Hamano wrote:
> @@ -2225,7 +2225,9 @@ int move_temp_to_file(const char *tmpfile, const char *filename)
>  {
>  	int ret = 0;
>  
> -	if (link(tmpfile, filename))
> +	if (unreliable_hardlinks)
> +		goto try_rename;

Much better.

		Linus

Re: [PATCH v2] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Michael Gaber]

[-- Attachment #1: Type: text/plain, Size: 565 bytes --]

Linus Torvalds schrieb:
> 
> On Sat, 25 Apr 2009, Junio C Hamano wrote:
>> @@ -2225,7 +2225,9 @@ int move_temp_to_file(const char *tmpfile, const char *filename)
>>  {
>>  	int ret = 0;
>>  
>> -	if (link(tmpfile, filename))
>> +	if (unreliable_hardlinks)
>> +		goto try_rename;
> 
> Much better.
> 
> 		Linus

http://www.cs.utexas.edu/users/EWD/ewd02xx/EWD215.PDF

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


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/x-pkcs7-signature, Size: 3643 bytes --]

Re: [PATCH v2] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Linus Torvalds]

On Sat, 25 Apr 2009, Michael Gaber wrote:
> 
> http://www.cs.utexas.edu/users/EWD/ewd02xx/EWD215.PDF

Yeah, and people thought "pascal" was a good language because it didn't 
contain "break" statements to break out of loops, or "return" statements 
to break out of functions early.

Too bad. They were wrong.

			Linus

Re: [PATCH v2] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Jay Soffian]

On Sat, Apr 25, 2009 at 2:38 PM, Michael Gaber <Michael.Gaber@gmx.net> wrote:
> http://www.cs.utexas.edu/users/EWD/ewd02xx/EWD215.PDF

http://www.bartleby.com/59/3/foolishconsi.html

j.

Re: [PATCH v2] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Johannes Sixt]

On Samstag, 25. April 2009, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > It seems that accessing NTFS partitions with ufsd (at least on my EeePC)
> > has an unnerving bug: if you link() a file and unlink() it right away,
> > the target of the link() will have the correct size, but consist of NULs.
> >
> > It seems as if the calls are simply not serialized correctly, as
> > single-stepping through the function move_temp_to_file() works
> > flawlessly.
> >
> > As ufsd is "Commertial software" (sic!), I cannot fix it, and have to
> > work around it in Git.
> >
> > At the same time, it seems that this fixes msysGit issues 222 and 229 to
> > assume that Windows cannot handle link() && unlink().
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > Acked-by: Johannes Sixt <j6t@kdbg.org>
>
> Hannes, are you ok with this?

Yes. We have been using rename() instead of link() on Windows until recently 
anyway (until link() was implemented, 7be401e06, 2009-01-24). There is no 
regression to be expected from this side.

-- Hannes

Re: [PATCH v2] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Junio C Hamano]

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

> It seems that accessing NTFS partitions with ufsd (at least on my EeePC)
> has an unnerving bug: if you link() a file and unlink() it right away,
> the target of the link() will have the correct size, but consist of NULs.
>
> It seems as if the calls are simply not serialized correctly, as single-stepping
> through the function move_temp_to_file() works flawlessly.

A few questions.

When this problem triggers for you,

 (1) do we have an open file descriptor to the tmpfile?

 (2) if so have we fsync'ed (or better yet, closed) it?

 (3) if the answers to the above are "yes, no", does it help the situation
     if we fsync the filedescriptor before calling move_temp_to_file()?

    ... gitster digs after asking questions to find answers himself ...

I realize that the answers seem to be "no, and the fd that created the
tempfile has been closed".  Hmm.  Very curious.

So if you do:

	cat >corrupt-move.c <<\EOF
	#include <unistd.h>
	int main(int ac, char **av)
        {
                return (link(av[1], av[2]) || unlink(av[1]));
	}
	EOF
        cc -o corrupt-move corrupt-move.c
        ./corrupt-move corrupt-move.c corrupt-move.c.new

you end up with a corrupt-move.c.new file that is full of NUL?

Very curious...

Re: [PATCH v2] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Johannes Schindelin]

Hi,

On Sat, 25 Apr 2009, Junio C Hamano wrote:

> So if you do:
> 
> 	cat >corrupt-move.c <<\EOF
> 	#include <unistd.h>
> 	int main(int ac, char **av)
>         {
>                 return (link(av[1], av[2]) || unlink(av[1]));
> 	}
> 	EOF
>         cc -o corrupt-move corrupt-move.c
>         ./corrupt-move corrupt-move.c corrupt-move.c.new
> 
> you end up with a corrupt-move.c.new file that is full of NUL?

I have not compiled and run this code, but I am _real_ sure that this is 
exactly the issue.

Ciao,
Dscho

Re: [PATCH v2] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Linus Torvalds]

On Sat, 25 Apr 2009, Johannes Schindelin wrote:
> diff --git a/sha1_file.c b/sha1_file.c
> index 8fe135d..bb6eecf 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2225,7 +2225,9 @@ int move_temp_to_file(const char *tmpfile, const char *filename)
>  {
>  	int ret = 0;
>  
> -	if (link(tmpfile, filename))
> +	if (unreliable_hardlinks)
> +		ret = ~EEXIST; /* anything but EEXIST */

Don't do this. ~EEXIST could be 0 (admittedly only if EEXIST is -1 which 
is not reasonable, but who knows about odd operating systems). Which is 
not a good return value either.

So why not just use an explicit error value like EIO? Don't play games 
with this.

			Linus

Re: [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Alex Riesen]

2009/4/23 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> -       if (link(tmpfile, filename))
> +       if (unreliable_hardlinks)
> +               ret = ~EEXIST;

It is more like "broken_hardlinks" or even "no_hardlinks"!

Re: [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 523 bytes --]

Hi,

On Thu, 23 Apr 2009, Alex Riesen wrote:

> 2009/4/23 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> > -       if (link(tmpfile, filename))
> > +       if (unreliable_hardlinks)
> > +               ret = ~EEXIST;
> 
> It is more like "broken_hardlinks" or even "no_hardlinks"!

Wrong.  As I wrote, single-stepping (i.e. leaving enough time between 
link() and unlink()) works as expected.  So it is not even that the 
hardlinks are broken.  Just the serialization between the operations.

Ciao,
Dscho

Re: [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Alex Riesen]

2009/4/23 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> Hi,
>
> On Thu, 23 Apr 2009, Alex Riesen wrote:
>
>> 2009/4/23 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
>> > -       if (link(tmpfile, filename))
>> > +       if (unreliable_hardlinks)
>> > +               ret = ~EEXIST;
>>
>> It is more like "broken_hardlinks" or even "no_hardlinks"!
>
> Wrong.  As I wrote, single-stepping (i.e. leaving enough time between
> link() and unlink()) works as expected.  So it is not even that the
> hardlinks are broken.  Just the serialization between the operations.
>

Since when does link(2) involve file _data_?

Re: [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Linus Torvalds]

On Thu, 23 Apr 2009, Johannes Schindelin wrote:
> 
> It seems that accessing NTFS partitions with ufsd (at least on my EeePC)
> has an unnerving bug: if you link() a file and unlink() it right away,
> the target of the link() will have the correct size, but consist of NULs.

So I assume that the way ufsd works is that it implements a user-level 
NTFS driver and then exposes it as a NFS mount over local networking (and
perhaps also remotely?)

> It seems as if the calls are simply not serialized correctly, as single-stepping
> through the function move_temp_to_file() works flawlessly.

So presumably there is some cached writes somewhere (a NFS client _should_ 
not cache writes past a 'close()', but maybe there is a bug there and/or 
buffering inside ufsd itself that means that the writes are still queued 
up). And when the unlink() happens, it loses the writes to the original 
file, and thus to the new one too.

If you _don't_ do this patch, does 

	[core]
		fsyncobjectfiles = true

hide the bug? 

I don't disagree with your patch (apart from the error number games), but 
I'd like to understand what's going on. I also wonder if we should make 
that fsync thign be the default.

[ That said, I think the http walker and possibly others may be using 
  'move_temp_to_file()' without going through any of the paths that know 
  about fsync, so 'fsyncobjectfiles' wouldn't fix all cases anyway. ]

Hmm. I hate how we have problems with that "link/unlink" sequence, and 
"rename()" would be much better, but I'd hate overwriting existing objects 
even _more_, and the normal POSIX rename() behavior is to overwrite any 
old object. So link/unlink is supposed to be a lot safer, but it's clearly 
problematic.

		Linus

Re: [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Johannes Sixt]

On Samstag, 25. April 2009, Linus Torvalds wrote:
> If you _don't_ do this patch, does
>
> 	[core]
> 		fsyncobjectfiles = true
>
> hide the bug?

Most likely not because our fsync() on Windows is a noop :(

-- Hannes

Re: [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> On Samstag, 25. April 2009, Linus Torvalds wrote:
>> If you _don't_ do this patch, does
>>
>> 	[core]
>> 		fsyncobjectfiles = true
>>
>> hide the bug?
>
> Most likely not because our fsync() on Windows is a noop :(

Actually, the reason I CC'ed Linus (and I also was interested in the
platform bug itself) was because I think Dscho is accessing the NTFS
partition from the Linux side.

Re: [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Johannes Schindelin]

Hi,

On Sat, 25 Apr 2009, Junio C Hamano wrote:

> Johannes Sixt <j6t@kdbg.org> writes:
> 
> > On Samstag, 25. April 2009, Linus Torvalds wrote:
> >> If you _don't_ do this patch, does
> >>
> >> 	[core]
> >> 		fsyncobjectfiles = true
> >>
> >> hide the bug?
> >
> > Most likely not because our fsync() on Windows is a noop :(
> 
> Actually, the reason I CC'ed Linus (and I also was interested in the
> platform bug itself) was because I think Dscho is accessing the NTFS
> partition from the Linux side.

Yep, correct.

Ciao,
Dscho

[PATCH v3] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Johannes Schindelin]

It seems that accessing NTFS partitions with ufsd (at least on my EeePC)
has an unnerving bug: if you link() a file and unlink() it right away,
the target of the link() will have the correct size, but consist of
NULs.

It seems as if the calls are simply not serialized correctly, as
single-stepping through the function move_temp_to_file() works
flawlessly.

On Linux, this issue can be fixed by setting core.fsyncobjects to true
(thanks Linus), but the same is not true on Windows.

So, force the use of rename() instead of the link() && unlink()
incantation on Windows, and for good measure, add a
core.unreliableHardlinks option to optionally force it on other
platforms, too.

This fixes msysGit issues 222 and 229.

It was substantially improved by the help of Junio.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
[TENTATIVE] Acked-by: Johannes Sixt <j6t@kdbg.org>
---

	Hannes, is it okay to remove the [TENTATIVE]?

	Junio, do you want me to remove the config variable?

 Documentation/config.txt |    5 +++++
 Makefile                 |    8 ++++++++
 cache.h                  |    2 ++
 config.c                 |    5 +++++
 environment.c            |    4 ++++
 sha1_file.c              |    3 +++
 6 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3188569..d31adb6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -429,6 +429,11 @@ relatively high IO latencies.  With this set to 'true', git will do the
 index comparison to the filesystem data in parallel, allowing
 overlapping IO's.
 
+core.unreliableHardlinks::
+	Some filesystem drivers cannot properly handle hardlinking a file
+	and deleting the source right away.  In such a case, you need to
+	set this config variable to 'true'.
+
 alias.*::
 	Command aliases for the linkgit:git[1] command wrapper - e.g.
 	after defining "alias.last = cat-file commit HEAD", the invocation
diff --git a/Makefile b/Makefile
index 6f602c7..5c8e83a 100644
--- a/Makefile
+++ b/Makefile
@@ -171,6 +171,10 @@ all::
 # Define UNRELIABLE_FSTAT if your system's fstat does not return the same
 # information on a not yet closed file that lstat would return for the same
 # file after it was closed.
+#
+# Define UNRELIABLE_HARDLINKS if your operating systems has problems when
+# hardlinking a file to another name and unlinking the original file right
+# away (some NTFS drivers seem to zero the contents in that scenario).
 
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -835,6 +839,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_NSEC = YesPlease
 	USE_WIN32_MMAP = YesPlease
 	UNRELIABLE_FSTAT = UnfortunatelyYes
+	UNRELIABLE_HARDLINKS = UnfortunatelySometimes
 	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/regex -Icompat/fnmatch
 	COMPAT_CFLAGS += -DSNPRINTF_SIZE_CORR=1
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
@@ -1018,6 +1023,9 @@ else
 		COMPAT_OBJS += compat/win32mmap.o
 	endif
 endif
+ifdef UNRELIABLE_HARDLINKS
+	COMPAT_CFLAGS += -DUNRELIABLE_HARDLINKS=1
+endif
 ifdef NO_PREAD
 	COMPAT_CFLAGS += -DNO_PREAD
 	COMPAT_OBJS += compat/pread.o
diff --git a/cache.h b/cache.h
index ab1294d..ff9e145 100644
--- a/cache.h
+++ b/cache.h
@@ -554,6 +554,8 @@ extern enum branch_track git_branch_track;
 extern enum rebase_setup_type autorebase;
 extern enum push_default_type push_default;
 
+extern int unreliable_hardlinks;
+
 #define GIT_REPO_VERSION 0
 extern int repository_format_version;
 extern int check_repository_format(void);
diff --git a/config.c b/config.c
index 8c1ae59..1750cfb 100644
--- a/config.c
+++ b/config.c
@@ -495,6 +495,11 @@ static int git_default_core_config(const char *var, const char *value)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.unreliablehardlinks")) {
+		unreliable_hardlinks = git_config_bool(var, value);
+		return 0;
+	}
+
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;
 }
diff --git a/environment.c b/environment.c
index 4696885..10578d2 100644
--- a/environment.c
+++ b/environment.c
@@ -43,6 +43,10 @@ unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
 enum branch_track git_branch_track = BRANCH_TRACK_REMOTE;
 enum rebase_setup_type autorebase = AUTOREBASE_NEVER;
 enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
+#ifndef UNRELIABLE_HARDLINKS
+#define UNRELIABLE_HARDLINKS 0
+#endif
+int unreliable_hardlinks = UNRELIABLE_HARDLINKS;
 
 /* Parallel index stat data preload? */
 int core_preload_index = 0;
diff --git a/sha1_file.c b/sha1_file.c
index 8fe135d..0d289f4 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2225,6 +2225,8 @@ int move_temp_to_file(const char *tmpfile, const char *filename)
 {
 	int ret = 0;
 
+	if (unreliable_hardlinks)
+		goto try_rename;
 	if (link(tmpfile, filename))
 		ret = errno;
 
@@ -2240,6 +2242,7 @@ int move_temp_to_file(const char *tmpfile, const char *filename)
 	 * left to unlink.
 	 */
 	if (ret && ret != EEXIST) {
+try_rename:
 		if (!rename(tmpfile, filename))
 			goto out;
 		ret = errno;
-- 
1.6.2.1.613.g25746

Re: [PATCH v3] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Linus Torvalds]

On Mon, 27 Apr 2009, Johannes Schindelin wrote:
> 
> So, force the use of rename() instead of the link() && unlink()
> incantation on Windows, and for good measure, add a
> core.unreliableHardlinks option to optionally force it on other
> platforms, too.

Ok, so:

	Acked-by: Linus Torvalds <torvalds@linux-foundation.org>

but I do think it could be improved. See below..

> 	Junio, do you want me to remove the config variable?

I'd keep it. But I'd suggest that the naming is odd. Why talk about 
"unreliable hardlinks", when that's just a particular symptom. Why not 
just talk about whether hardlinks should be used or not?

And to avoid double negative, make it

	[core]
		usehardlinks = true/false

and then default it to 'true' for Unix.

The thing is, maybe people would prefer to use 'rename' over the 
link/unlink games even on some unixes, and not because of 'reliability' 
issues, but because they may have some filesystems that don't do 
hardlinks, and they'd just rather speed things up by avoiding the 'link()' 
system call that will just error out.

So naming matters. Calling it 'unreliablehardlinks' in that case would be 
odd. They're not unreliable - you just don't want to try to use them.

I also do wonder if we could/should make this one of those options that 
get set automatically at 'git init' time, rather than silently hardcoded 
as a compile option. I thought hardlinks at least sometimes worked fine on 
Windows too, don't they? 

I do detest _hidden_ default values for config options, unless those 
hidden defaults are "obviously always correct" as a default. This one 
smells a bit uncertain, and as a result I think it's ok to default to not 
using hardlinks, but doing it with .gitconfig would be nicer.

Hmm?

		Linus

Re: [PATCH v3] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Johannes Schindelin]

Hi,

On Mon, 27 Apr 2009, Linus Torvalds wrote:

> On Mon, 27 Apr 2009, Johannes Schindelin wrote:
> > 
> > So, force the use of rename() instead of the link() && unlink() 
> > incantation on Windows, and for good measure, add a 
> > core.unreliableHardlinks option to optionally force it on other 
> > platforms, too.
> 
> Ok, so:
> 
> 	Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
> 
> but I do think it could be improved. See below..

Sorry, I missed the fact that Junio already applied and pushed it to 
'next'.

> > 	Junio, do you want me to remove the config variable?
> 
> I'd keep it. But I'd suggest that the naming is odd. Why talk about 
> "unreliable hardlinks", when that's just a particular symptom. Why not 
> just talk about whether hardlinks should be used or not?
> 
> And to avoid double negative, make it
> 
> 	[core]
> 		usehardlinks = true/false
> 
> and then default it to 'true' for Unix.

Or maybe core.preferRenameOverLink?  Then we have no negation either.

> The thing is, maybe people would prefer to use 'rename' over the 
> link/unlink games even on some unixes, and not because of 'reliability' 
> issues, but because they may have some filesystems that don't do 
> hardlinks, and they'd just rather speed things up by avoiding the 'link()' 
> system call that will just error out.

We already fall back to renaming when another error than EEXIST is 
returned from link(), so I think this case is covered.

> So naming matters. Calling it 'unreliablehardlinks' in that case would 
> be odd. They're not unreliable - you just don't want to try to use them.
> 
> I also do wonder if we could/should make this one of those options that 
> get set automatically at 'git init' time, rather than silently hardcoded 
> as a compile option. I thought hardlinks at least sometimes worked fine on 
> Windows too, don't they? 

I thought about that long and hard, and I decided against it.  Take my 
NTFS-formatted portable hard drive (for convenience with Windows users 
@work) for example: the ufsd driver is totally broken, but because it is a 
major investment of time to get my EeePC to work with a sane Linux 
distribution, I'd rather keep using the ufsd driver.  Yet, when I use 
ntfs-3g from the other laptop, it works fine.

See?  It is not a file system specific error, but a fs/os combo problem.

> I do detest _hidden_ default values for config options, unless those 
> hidden defaults are "obviously always correct" as a default. This one 
> smells a bit uncertain, and as a result I think it's ok to default to 
> not using hardlinks, but doing it with .gitconfig would be nicer.

I fully agree on hidden default values, albeit in this case, it is 
necessary: the hard links work just fine on Windows XP here, but that 
might just be a matter of not upgrading to a newer service pack.

Ciao,
Dscho

Re: [PATCH v3] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Linus Torvalds]

On Mon, 27 Apr 2009, Johannes Schindelin wrote:
> 
> > The thing is, maybe people would prefer to use 'rename' over the 
> > link/unlink games even on some unixes, and not because of 'reliability' 
> > issues, but because they may have some filesystems that don't do 
> > hardlinks, and they'd just rather speed things up by avoiding the 'link()' 
> > system call that will just error out.
> 
> We already fall back to renaming when another error than EEXIST is 
> returned from link(), so I think this case is covered.

You didn't read what I wrote.

  "they'd just rather speed things up by avoiding the 'link()' system call 
   that will just error out."

I know we fall back to rename(). The point is that if you know link 
doesn't work, why not just skip it?

		Linus

Re: [PATCH v3] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

>> 	Junio, do you want me to remove the config variable?
>
> I'd keep it. But I'd suggest that the naming is odd. Why talk about 
> "unreliable hardlinks", when that's just a particular symptom. Why not 
> just talk about whether hardlinks should be used or not?
>
> And to avoid double negative, make it
>
> 	[core]
> 		usehardlinks = true/false
>
> and then default it to 'true' for Unix.

I am a bit worried about this name, too.  It may lead people to a
misunderstanding that we would do something magical when they do this with
the configuration set:

	wget http://some.where/huge-file.mpg 1.mpg
        ln 1.mpg 2.mpg
        git add 1.mpg 2.mpg
        rm -f 1.mpg 2.mpg
        git checkout-index -a
	ls -i ?.mpg

> The thing is, maybe people would prefer to use 'rename' over the
> link/unlink games even on some unixes, and not because of 'reliability'
> issues, but because they may have some filesystems that don't do
> hardlinks, and they'd just rather speed things up by avoiding the
> 'link()' system call that will just error out.

> So naming matters. Calling it 'unreliablehardlinks' in that case would be 
> odd. They're not unreliable - you just don't want to try to use them.

This part I agree with.

> I also do wonder if we could/should make this one of those options that 
> get set automatically at 'git init' time, rather than silently hardcoded 
> as a compile option. I thought hardlinks at least sometimes worked fine on 
> Windows too, don't they? 
>
> I do detest _hidden_ default values for config options, unless those 
> hidden defaults are "obviously always correct" as a default. This one 
> smells a bit uncertain, and as a result I think it's ok to default to not 
> using hardlinks, but doing it with .gitconfig would be nicer.

The coda hack comment in move_temp_to_file() shows what we can do to
autodetect (i.e. try cross directory hardlink), but I somehow thought that
we changed the code enough to ensure that we create the tmpfiles in the
same directory as their final destination?

Re: [PATCH v3] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Linus Torvalds]

On Mon, 27 Apr 2009, Junio C Hamano wrote:
> 
> The coda hack comment in move_temp_to_file() shows what we can do to
> autodetect (i.e. try cross directory hardlink)

The thing is, we cannot do it reliably across different systems.

Coda simply doesn't _support_ hardlinks across directories at all. So it 
will always return an error when you try, and you can see the error 
directly and easily.

> but I somehow thought that we changed the code enough to ensure that we 
> create the tmpfiles in the same directory as their final destination?

This was for a totally different case - a certain kind of NFS client bug 
with a certain kind of (arguably buggy, but I can understand it because 
NFS is just a bad protocol in this respect) NFS server, where you may be 
able to do cross-directory renames, but it caused problems later.

Now, the reason cross-directory name movement matters is that it makes 
many things much harder, and filesystems thus have a much harder time 
doing them well (or decide to not support them at all, as in Coda). Within 
a single directory, things are just simpler, and thus less likely to hit 
bugs.

IOW, with cross-directory link/rename, you didn't get an error, you got 
some unreliable behavior - very much like the thing we see with ufsd. But 
with those problems, we could fix it by just always making the link and 
the rename be within a single directory.

Now, it seems, even being in the same directory isn't sufficient for that 
ufsd thing (but rename works. Knock wood).

			Linus

Re: [PATCH v3] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Linus Torvalds]

On Mon, 27 Apr 2009, Junio C Hamano wrote:
> >
> > 	[core]
> > 		usehardlinks = true/false
> 
> I am a bit worried about this name, too.  It may lead people to a
> misunderstanding that we would do something magical when they do this with
> the configuration set:
> 
> 	wget http://some.where/huge-file.mpg 1.mpg
>         ln 1.mpg 2.mpg
>         git add 1.mpg 2.mpg
>         rm -f 1.mpg 2.mpg
>         git checkout-index -a
> 	ls -i ?.mpg

Btw, I do agree that maybe 'usehardlinks' is not a good name either. Maybe 
we should make it clear that we're talking about a specific case for 
object creation.

Maybe the config option shouldn't be a boolean, but a "how to instantiate 
objects". IOW, we could do

	[core]
		createobject = {link|rename}

instead. Maybe we some day could allow "inplace", for some totally broken 
system that supports neither renames nor links, and just wants the object 
to be created with the final name to start with.

(Ok, that sounds unlikely, but I mention it because it's an example of the 
concept. Maybe somebody likes crazy databases, and would like to have a 
"createobject = mysql" for some DB-backed loose object crap).

		Linus

Re: [PATCH v3] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> Maybe the config option shouldn't be a boolean, but a "how to instantiate 
> objects". IOW, we could do
>
> 	[core]
> 		createobject = {link|rename}
>
> instead. Maybe we some day could allow "inplace", for some totally broken 
> system that supports neither renames nor links, and just wants the object 
> to be created with the final name to start with.
>
> (Ok, that sounds unlikely, but I mention it because it's an example of the 
> concept. Maybe somebody likes crazy databases, and would like to have a 
> "createobject = mysql" for some DB-backed loose object crap).
>
> 		Linus

More likely is "bigtable", I guess ;-)

Re: [PATCH v3] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Johannes Schindelin]

Hi,

On Mon, 27 Apr 2009, Junio C Hamano wrote:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > Maybe the config option shouldn't be a boolean, but a "how to instantiate 
> > objects". IOW, we could do
> >
> > 	[core]
> > 		createobject = {link|rename}
> >
> > instead. Maybe we some day could allow "inplace", for some totally broken 
> > system that supports neither renames nor links, and just wants the object 
> > to be created with the final name to start with.
> >
> > (Ok, that sounds unlikely, but I mention it because it's an example of the 
> > concept. Maybe somebody likes crazy databases, and would like to have a 
> > "createobject = mysql" for some DB-backed loose object crap).
> >
> > 		Linus
> 
> More likely is "bigtable", I guess ;-)

As I said, this is highly unlikely, as certain people made sure that the 
Google Code people do not like Git.

Ciao,
Dscho

Re: [PATCH v3] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Linus Torvalds]

On Tue, 28 Apr 2009, Johannes Schindelin wrote:
>
> As I said, this is highly unlikely, as certain people made sure that the 
> Google Code people do not like Git.

There are actually lots of sane and educated people inside google. Many of 
them freely acknowledge what a horrid thing SVN is.

			Linus

[PATCH] Rename core.unreliableHardlinks to core.createObject

[Johannes Schindelin]

"Unreliable hardlinks" is a misleading description for what is happening.
So rename it to something less misleading.

Suggested by Linus Torvalds.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	On Mon, 27 Apr 2009, Linus Torvalds wrote:

	> Maybe the config option shouldn't be a boolean, but a "how to 
	> instantiate objects". IOW, we could do
	> 
	> 	[core]
	> 		createobject = {link|rename}
	> 
	> instead. Maybe we some day could allow "inplace", for some 
	> totally broken system that supports neither renames nor links, and
	> just wants the object to be created with the final name to start
	> with.

	Here you go.  Only compile-tested.

 Documentation/config.txt |   12 ++++++++----
 Makefile                 |   10 +++++-----
 cache.h                  |    7 ++++++-
 config.c                 |    9 +++++++--
 environment.c            |    6 +++---
 sha1_file.c              |    2 +-
 6 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 83454c5..2c03162 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -429,10 +429,14 @@ relatively high IO latencies.  With this set to 'true', git will do the
 index comparison to the filesystem data in parallel, allowing
 overlapping IO's.
 
-core.unreliableHardlinks::
-	Some filesystem drivers cannot properly handle hardlinking a file
-	and deleting the source right away.  In such a case, you need to
-	set this config variable to 'true'.
+core.createObject::
+	You can set this to 'link', in which case a hardlink followed by
+	a delete of the source are used to make sure that object creation
+	will not overwrite existing objects.
++
+On some file system/operating system combinations, this is unreliable.
+Set this config setting to 'rename' there; However, This will remove the
+check that makes sure that existing object files will not get overwritten.
 
 alias.*::
 	Command aliases for the linkgit:git[1] command wrapper - e.g.
diff --git a/Makefile b/Makefile
index 5c8e83a..9ca1826 100644
--- a/Makefile
+++ b/Makefile
@@ -172,8 +172,8 @@ all::
 # information on a not yet closed file that lstat would return for the same
 # file after it was closed.
 #
-# Define UNRELIABLE_HARDLINKS if your operating systems has problems when
-# hardlinking a file to another name and unlinking the original file right
+# Define OBJECT_CREATION_USES_RENAMES if your operating systems has problems
+# when hardlinking a file to another name and unlinking the original file right
 # away (some NTFS drivers seem to zero the contents in that scenario).
 
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
@@ -839,7 +839,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_NSEC = YesPlease
 	USE_WIN32_MMAP = YesPlease
 	UNRELIABLE_FSTAT = UnfortunatelyYes
-	UNRELIABLE_HARDLINKS = UnfortunatelySometimes
+	OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
 	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/regex -Icompat/fnmatch
 	COMPAT_CFLAGS += -DSNPRINTF_SIZE_CORR=1
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
@@ -1023,8 +1023,8 @@ else
 		COMPAT_OBJS += compat/win32mmap.o
 	endif
 endif
-ifdef UNRELIABLE_HARDLINKS
-	COMPAT_CFLAGS += -DUNRELIABLE_HARDLINKS=1
+ifdef OBJECT_CREATION_USES_RENAMES
+	COMPAT_CFLAGS += -DOBJECT_CREATION_MODE=1
 endif
 ifdef NO_PREAD
 	COMPAT_CFLAGS += -DNO_PREAD
diff --git a/cache.h b/cache.h
index ff9e145..d0d48b4 100644
--- a/cache.h
+++ b/cache.h
@@ -554,7 +554,12 @@ extern enum branch_track git_branch_track;
 extern enum rebase_setup_type autorebase;
 extern enum push_default_type push_default;
 
-extern int unreliable_hardlinks;
+enum object_creation_mode {
+	OBJECT_CREATION_USES_HARDLINKS = 0,
+	OBJECT_CREATION_USES_RENAMES = 1,
+};
+
+extern enum object_creation_mode object_creation_mode;
 
 #define GIT_REPO_VERSION 0
 extern int repository_format_version;
diff --git a/config.c b/config.c
index 1750cfb..876f0ed 100644
--- a/config.c
+++ b/config.c
@@ -495,8 +495,13 @@ static int git_default_core_config(const char *var, const char *value)
 		return 0;
 	}
 
-	if (!strcmp(var, "core.unreliablehardlinks")) {
-		unreliable_hardlinks = git_config_bool(var, value);
+	if (!strcmp(var, "core.createobject")) {
+		if (!strcmp(value, "rename"))
+			object_creation_mode = OBJECT_CREATION_USES_RENAMES;
+		else if (!strcmp(value, "link"))
+			object_creation_mode = OBJECT_CREATION_USES_HARDLINKS;
+		else
+			die ("Invalid mode for object creation: %s", value);
 		return 0;
 	}
 
diff --git a/environment.c b/environment.c
index 10578d2..801a005 100644
--- a/environment.c
+++ b/environment.c
@@ -43,10 +43,10 @@ unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
 enum branch_track git_branch_track = BRANCH_TRACK_REMOTE;
 enum rebase_setup_type autorebase = AUTOREBASE_NEVER;
 enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
-#ifndef UNRELIABLE_HARDLINKS
-#define UNRELIABLE_HARDLINKS 0
+#ifndef OBJECT_CREATION_MODE
+#define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS
 #endif
-int unreliable_hardlinks = UNRELIABLE_HARDLINKS;
+enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
 
 /* Parallel index stat data preload? */
 int core_preload_index = 0;
diff --git a/sha1_file.c b/sha1_file.c
index 11969fc..f708cf4 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2225,7 +2225,7 @@ int move_temp_to_file(const char *tmpfile, const char *filename)
 {
 	int ret = 0;
 
-	if (unreliable_hardlinks)
+	if (object_creation_mode == OBJECT_CREATION_USES_RENAMES)
 		goto try_rename;
 	else if (link(tmpfile, filename))
 		ret = errno;
-- 
1.6.3.rc3.326.g039c1

Re: [PATCH] Rename core.unreliableHardlinks to core.createObject

[Junio C Hamano]

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

> diff --git a/Makefile b/Makefile
> index 5c8e83a..9ca1826 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -172,8 +172,8 @@ all::
>  # information on a not yet closed file that lstat would return for the same
>  # file after it was closed.
>  #
> -# Define UNRELIABLE_HARDLINKS if your operating systems has problems when
> -# hardlinking a file to another name and unlinking the original file right
> +# Define OBJECT_CREATION_USES_RENAMES if your operating systems has problems
> +# when hardlinking a file to another name and unlinking the original file right

With the configuration variable for this relatively obscure feature in
place, I wonder if we can simply get rid of the hardcoded compilation
preference.  After all, even on your eeepc, I presume that you have some
filesystems in native format where you do not have the breakages, and some
others mounted with unfsd breakage.  When diagnosing a possible issue on
somebody else's box, having to look into .git/config to see which codepath
is used is bad enough, but it is even worse to have a default that can be
different with compilation switch.

It would essentially boil down to this hunk; instead of introducing
OBJECT_CREATION_MODE, we default to hardlinks, and let the configuration
override it (and do nothing else).

> diff --git a/environment.c b/environment.c
> index 10578d2..801a005 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -43,10 +43,10 @@ unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
>  enum branch_track git_branch_track = BRANCH_TRACK_REMOTE;
>  enum rebase_setup_type autorebase = AUTOREBASE_NEVER;
>  enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
> -#ifndef UNRELIABLE_HARDLINKS
> -#define UNRELIABLE_HARDLINKS 0
> +#ifndef OBJECT_CREATION_MODE
> +#define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS
>  #endif
> -int unreliable_hardlinks = UNRELIABLE_HARDLINKS;
> +enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;

Re: [PATCH] Rename core.unreliableHardlinks to core.createObject

[Johannes Schindelin]

Hi,

On Mon, 27 Apr 2009, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > diff --git a/Makefile b/Makefile
> > index 5c8e83a..9ca1826 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -172,8 +172,8 @@ all::
> >  # information on a not yet closed file that lstat would return for the same
> >  # file after it was closed.
> >  #
> > -# Define UNRELIABLE_HARDLINKS if your operating systems has problems when
> > -# hardlinking a file to another name and unlinking the original file right
> > +# Define OBJECT_CREATION_USES_RENAMES if your operating systems has problems
> > +# when hardlinking a file to another name and unlinking the original file right
> 
> With the configuration variable for this relatively obscure feature in 
> place, I wonder if we can simply get rid of the hardcoded compilation 
> preference.

I'd rather not, for Windows.  Remember, it fixes issues 222 and 229.  And 
from the comments in those issues I understand that more than 2 persons 
had problems due to these issues.

Ciao,
Dscho

Re: [PATCH] Rename core.unreliableHardlinks to core.createObject

[Junio C Hamano]

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

>> With the configuration variable for this relatively obscure feature in 
>> place, I wonder if we can simply get rid of the hardcoded compilation 
>> preference.
>
> I'd rather not, for Windows.  Remember, it fixes issues 222 and 229.

Wait a bit. Wasn't this about you accessing NTFS on your EeePC via unfs
from the Linux side?

Re: [PATCH] Rename core.unreliableHardlinks to core.createObject

[Johannes Schindelin]

Hi,

On Tue, 28 Apr 2009, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> With the configuration variable for this relatively obscure feature 
> >> in place, I wonder if we can simply get rid of the hardcoded 
> >> compilation preference.
> >
> > I'd rather not, for Windows.  Remember, it fixes issues 222 and 229.
> 
> Wait a bit. Wasn't this about you accessing NTFS on your EeePC via unfs 
> from the Linux side?

Both.  I realized that there was a problem with the ufsd driver of the 
Xandros Linux on my EeePC, accessing NTFS partitions.  (This is the issue 
that made me add a config variable, but which was solved by Linus' 
core.fsyncobjects suggestion.)

Later I had a hunch that the issues 222 and 229 of msysGit might have 
exactly the same reason, let the reporters test, and indeed, the problems 
went away.

But come to think of it, we can _easily_ just set core.createObject=rename 
in msysGit, so I agree that there is no longer a need for the Makefile 
variable.

Want me to resend?

Ciao,
Dscho

Re: [PATCH] Rename core.unreliableHardlinks to core.createObject

[Junio C Hamano]

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

>> Wait a bit. Wasn't this about you accessing NTFS on your EeePC via unfs 
>> from the Linux side?
>
> Both.  I realized that there was a problem with the ufsd driver of the 
> Xandros Linux on my EeePC, accessing NTFS partitions.  (This is the issue 
> that made me add a config variable, but which was solved by Linus' 
> core.fsyncobjects suggestion.)
>
> Later I had a hunch that the issues 222 and 229 of msysGit might have 
> exactly the same reason, let the reporters test, and indeed, the problems 
> went away.
>
> But come to think of it, we can _easily_ just set core.createObject=rename 
> in msysGit, so I agree that there is no longer a need for the Makefile 
> variable.
>
> Want me to resend?

If it helps msys, I think we should allow compiling things in, but this
"compiled in default for the platform, and possible per-repository
override" made me a bit confused:

 (1) in your "This is Linux and on sane filesystems I do not weaken it to
     rename but on this one filesystem I do" case can be handled by adding
     .git/config in that repository;

 (2) problems with msysgit can be handled by compiled-in defaults as long
     as the user does not have .git/config entry to say "link";

 (3) if you use the same repository from both sides with (1), presumably
     by dual-booting, so having .git/config that says "rename" happens to
     work;

 (4) if somebody has a dual-boot setup and shares a repository hosted
     natively on the Linux side by mounting it on the Windows side (Ext2
     IFS?), I wonder what should happen.  While you are using the
     repository from the Linux side, you may not want to weaken it to use
     "rename" (so you do not add .git/config that says "rename").  When
     you are accessing it over Ext2 IFS, perhaps you would want to use
     "rename" (I do not know about the details of #222 and #229, so it may
     not applicable, though).

So,... as long as you do not have a triple-boot setup, third system among
which wants to use "link" on a repository where both Linux and Windows
side want to use "rename", I think you are Ok.

Re: [PATCH] Rename core.unreliableHardlinks to core.createObject

[Johannes Schindelin]

Hi,

On Tue, 28 Apr 2009, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> Wait a bit. Wasn't this about you accessing NTFS on your EeePC via 
> >> unfs from the Linux side?
> >
> > Both.  I realized that there was a problem with the ufsd driver of the 
> > Xandros Linux on my EeePC, accessing NTFS partitions.  (This is the 
> > issue that made me add a config variable, but which was solved by 
> > Linus' core.fsyncobjects suggestion.)
> >
> > Later I had a hunch that the issues 222 and 229 of msysGit might have 
> > exactly the same reason, let the reporters test, and indeed, the 
> > problems went away.
> >
> > But come to think of it, we can _easily_ just set 
> > core.createObject=rename in msysGit, so I agree that there is no 
> > longer a need for the Makefile variable.
> >
> > Want me to resend?
> 
> If it helps msys, I think we should allow compiling things in, but this 
> "compiled in default for the platform, and possible per-repository 
> override" made me a bit confused:
> 
>  (1) in your "This is Linux and on sane filesystems I do not weaken it 
>      to rename but on this one filesystem I do" case can be handled by 
>      adding .git/config in that repository;
> 
>  (2) problems with msysgit can be handled by compiled-in defaults as 
>      long as the user does not have .git/config entry to say "link";
> 
>  (3) if you use the same repository from both sides with (1), presumably
>      by dual-booting, so having .git/config that says "rename" happens 
>      to work;
> 
>  (4) if somebody has a dual-boot setup and shares a repository hosted
>      natively on the Linux side by mounting it on the Windows side (Ext2 
>      IFS?), I wonder what should happen.  While you are using the 
>      repository from the Linux side, you may not want to weaken it to 
>      use "rename" (so you do not add .git/config that says "rename").  
>      When you are accessing it over Ext2 IFS, perhaps you would want to 
>      use "rename" (I do not know about the details of #222 and #229, so 
>      it may not applicable, though).
> 
> So,... as long as you do not have a triple-boot setup, third system 
> among which wants to use "link" on a repository where both Linux and 
> Windows side want to use "rename", I think you are Ok.

Well, my idea was to "weaken" the repository by using rename() always, 
even if the fs/OS combo happens to handle the case gracefully.

But actually, what I will use is core.fsyncObjects, as suggested by Linus, 
which should not hurt sane fs/OS combos either.

Ciao,
Dscho

Re: [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable

[Johannes Schindelin]

Hi,

On Sat, 25 Apr 2009, Linus Torvalds wrote:

> If you _don't_ do this patch, does 
> 
> 	[core]
> 		fsyncobjectfiles = true
> 
> hide the bug?

Yes.  On my EeePC with that stupid ufsd driver.

However, there is the other issue with Windows, so we still need half of 
my patch.

Junio, do you want me to throw out the core.unreliableHardlinks stuff, and 
only keep it as a Makefile option?

Ciao,
Dscho