* [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
@ 2009-04-23 10:53 Johannes Schindelin
2009-04-23 19:16 ` Johannes Sixt
` (2 more replies)
0 siblings, 3 replies; 38+ messages in thread
From: Johannes Schindelin @ 2009-04-23 10:53 UTC (permalink / raw)
To: git, gitster, j6t
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
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-23 10:53 [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable Johannes Schindelin
@ 2009-04-23 19:16 ` Johannes Sixt
2009-04-23 19:33 ` Johannes Schindelin
2009-04-25 9:57 ` [PATCH v2] " Johannes Schindelin
2009-04-23 19:39 ` [PATCH] " Alex Riesen
2009-04-25 17:56 ` Linus Torvalds
2 siblings, 2 replies; 38+ messages in thread
From: Johannes Sixt @ 2009-04-23 19:16 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, gitster
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-23 19:16 ` Johannes Sixt
@ 2009-04-23 19:33 ` Johannes Schindelin
2009-04-25 9:57 ` [PATCH v2] " Johannes Schindelin
1 sibling, 0 replies; 38+ messages in thread
From: Johannes Schindelin @ 2009-04-23 19:33 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, gitster
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-23 10:53 [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable Johannes Schindelin
2009-04-23 19:16 ` Johannes Sixt
@ 2009-04-23 19:39 ` Alex Riesen
2009-04-23 21:59 ` Johannes Schindelin
2009-04-25 17:56 ` Linus Torvalds
2 siblings, 1 reply; 38+ messages in thread
From: Alex Riesen @ 2009-04-23 19:39 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, gitster, j6t
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"!
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-23 19:39 ` [PATCH] " Alex Riesen
@ 2009-04-23 21:59 ` Johannes Schindelin
2009-04-24 5:44 ` Alex Riesen
0 siblings, 1 reply; 38+ messages in thread
From: Johannes Schindelin @ 2009-04-23 21:59 UTC (permalink / raw)
To: Alex Riesen; +Cc: git, gitster, j6t
[-- 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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-23 21:59 ` Johannes Schindelin
@ 2009-04-24 5:44 ` Alex Riesen
0 siblings, 0 replies; 38+ messages in thread
From: Alex Riesen @ 2009-04-24 5:44 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, gitster, j6t
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_?
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-23 19:16 ` Johannes Sixt
2009-04-23 19:33 ` Johannes Schindelin
@ 2009-04-25 9:57 ` Johannes Schindelin
2009-04-25 16:49 ` Junio C Hamano
` (2 more replies)
1 sibling, 3 replies; 38+ messages in thread
From: Johannes Schindelin @ 2009-04-25 9:57 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, gitster
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
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v2] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-25 9:57 ` [PATCH v2] " Johannes Schindelin
@ 2009-04-25 16:49 ` Junio C Hamano
2009-04-25 17:40 ` Linus Torvalds
2009-04-25 18:50 ` Johannes Sixt
2009-04-25 17:05 ` Junio C Hamano
2009-04-25 17:39 ` Linus Torvalds
2 siblings, 2 replies; 38+ messages in thread
From: Junio C Hamano @ 2009-04-25 16:49 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Johannes Sixt, git
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;
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v2] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-25 9:57 ` [PATCH v2] " Johannes Schindelin
2009-04-25 16:49 ` Junio C Hamano
@ 2009-04-25 17:05 ` Junio C Hamano
2009-04-26 17:39 ` Johannes Schindelin
2009-04-25 17:39 ` Linus Torvalds
2 siblings, 1 reply; 38+ messages in thread
From: Junio C Hamano @ 2009-04-25 17:05 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Johannes Sixt, git, torvalds
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...
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-25 9:57 ` [PATCH v2] " Johannes Schindelin
2009-04-25 16:49 ` Junio C Hamano
2009-04-25 17:05 ` Junio C Hamano
@ 2009-04-25 17:39 ` Linus Torvalds
2 siblings, 0 replies; 38+ messages in thread
From: Linus Torvalds @ 2009-04-25 17:39 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Johannes Sixt, git, gitster
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-25 16:49 ` Junio C Hamano
@ 2009-04-25 17:40 ` Linus Torvalds
2009-04-25 18:38 ` Michael Gaber
2009-04-25 18:50 ` Johannes Sixt
1 sibling, 1 reply; 38+ messages in thread
From: Linus Torvalds @ 2009-04-25 17:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, Johannes Sixt, git
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-23 10:53 [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable Johannes Schindelin
2009-04-23 19:16 ` Johannes Sixt
2009-04-23 19:39 ` [PATCH] " Alex Riesen
@ 2009-04-25 17:56 ` Linus Torvalds
2009-04-25 18:52 ` Johannes Sixt
2009-04-26 17:38 ` [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable Johannes Schindelin
2 siblings, 2 replies; 38+ messages in thread
From: Linus Torvalds @ 2009-04-25 17:56 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, gitster, j6t
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-25 17:40 ` Linus Torvalds
@ 2009-04-25 18:38 ` Michael Gaber
2009-04-25 18:43 ` Linus Torvalds
2009-04-27 3:37 ` Jay Soffian
0 siblings, 2 replies; 38+ messages in thread
From: Michael Gaber @ 2009-04-25 18:38 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Junio C Hamano, Johannes Schindelin, Johannes Sixt, git
[-- 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 --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-25 18:38 ` Michael Gaber
@ 2009-04-25 18:43 ` Linus Torvalds
2009-04-27 3:37 ` Jay Soffian
1 sibling, 0 replies; 38+ messages in thread
From: Linus Torvalds @ 2009-04-25 18:43 UTC (permalink / raw)
To: Michael Gaber; +Cc: Junio C Hamano, Johannes Schindelin, Johannes Sixt, git
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-25 16:49 ` Junio C Hamano
2009-04-25 17:40 ` Linus Torvalds
@ 2009-04-25 18:50 ` Johannes Sixt
1 sibling, 0 replies; 38+ messages in thread
From: Johannes Sixt @ 2009-04-25 18:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-25 17:56 ` Linus Torvalds
@ 2009-04-25 18:52 ` Johannes Sixt
2009-04-26 1:17 ` Junio C Hamano
2009-04-26 17:38 ` [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable Johannes Schindelin
1 sibling, 1 reply; 38+ messages in thread
From: Johannes Sixt @ 2009-04-25 18:52 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Johannes Schindelin, git, gitster
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-25 18:52 ` Johannes Sixt
@ 2009-04-26 1:17 ` Junio C Hamano
2009-04-26 17:40 ` Johannes Schindelin
0 siblings, 1 reply; 38+ messages in thread
From: Junio C Hamano @ 2009-04-26 1:17 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Linus Torvalds, Johannes Schindelin, git, gitster
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.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-25 17:56 ` Linus Torvalds
2009-04-25 18:52 ` Johannes Sixt
@ 2009-04-26 17:38 ` Johannes Schindelin
1 sibling, 0 replies; 38+ messages in thread
From: Johannes Schindelin @ 2009-04-26 17:38 UTC (permalink / raw)
To: Linus Torvalds; +Cc: git, gitster, j6t
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-25 17:05 ` Junio C Hamano
@ 2009-04-26 17:39 ` Johannes Schindelin
0 siblings, 0 replies; 38+ messages in thread
From: Johannes Schindelin @ 2009-04-26 17:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, git, torvalds
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-26 1:17 ` Junio C Hamano
@ 2009-04-26 17:40 ` Johannes Schindelin
2009-04-27 12:00 ` [PATCH v3] " Johannes Schindelin
0 siblings, 1 reply; 38+ messages in thread
From: Johannes Schindelin @ 2009-04-26 17:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Linus Torvalds, git
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-25 18:38 ` Michael Gaber
2009-04-25 18:43 ` Linus Torvalds
@ 2009-04-27 3:37 ` Jay Soffian
1 sibling, 0 replies; 38+ messages in thread
From: Jay Soffian @ 2009-04-27 3:37 UTC (permalink / raw)
To: Michael Gaber
Cc: Linus Torvalds, Junio C Hamano, Johannes Schindelin, Johannes Sixt, git
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.
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v3] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-26 17:40 ` Johannes Schindelin
@ 2009-04-27 12:00 ` Johannes Schindelin
2009-04-27 15:15 ` Linus Torvalds
0 siblings, 1 reply; 38+ messages in thread
From: Johannes Schindelin @ 2009-04-27 12:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Linus Torvalds, git
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
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v3] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-27 12:00 ` [PATCH v3] " Johannes Schindelin
@ 2009-04-27 15:15 ` Linus Torvalds
2009-04-27 16:11 ` Johannes Schindelin
2009-04-27 19:55 ` Junio C Hamano
0 siblings, 2 replies; 38+ messages in thread
From: Linus Torvalds @ 2009-04-27 15:15 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, Johannes Sixt, git
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-27 15:15 ` Linus Torvalds
@ 2009-04-27 16:11 ` Johannes Schindelin
2009-04-27 16:53 ` Linus Torvalds
2009-04-27 19:55 ` Junio C Hamano
1 sibling, 1 reply; 38+ messages in thread
From: Johannes Schindelin @ 2009-04-27 16:11 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Junio C Hamano, Johannes Sixt, git
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-27 16:11 ` Johannes Schindelin
@ 2009-04-27 16:53 ` Linus Torvalds
0 siblings, 0 replies; 38+ messages in thread
From: Linus Torvalds @ 2009-04-27 16:53 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, Johannes Sixt, git
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-27 15:15 ` Linus Torvalds
2009-04-27 16:11 ` Johannes Schindelin
@ 2009-04-27 19:55 ` Junio C Hamano
2009-04-27 20:13 ` Linus Torvalds
2009-04-27 20:18 ` Linus Torvalds
1 sibling, 2 replies; 38+ messages in thread
From: Junio C Hamano @ 2009-04-27 19:55 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Johannes Schindelin, Johannes Sixt, git
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?
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-27 19:55 ` Junio C Hamano
@ 2009-04-27 20:13 ` Linus Torvalds
2009-04-27 20:18 ` Linus Torvalds
1 sibling, 0 replies; 38+ messages in thread
From: Linus Torvalds @ 2009-04-27 20:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, Johannes Sixt, git
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-27 19:55 ` Junio C Hamano
2009-04-27 20:13 ` Linus Torvalds
@ 2009-04-27 20:18 ` Linus Torvalds
2009-04-27 22:10 ` Junio C Hamano
2009-04-27 22:32 ` [PATCH] Rename core.unreliableHardlinks to core.createObject Johannes Schindelin
1 sibling, 2 replies; 38+ messages in thread
From: Linus Torvalds @ 2009-04-27 20:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, Johannes Sixt, git
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-27 20:18 ` Linus Torvalds
@ 2009-04-27 22:10 ` Junio C Hamano
2009-04-27 22:28 ` Johannes Schindelin
2009-04-27 22:32 ` [PATCH] Rename core.unreliableHardlinks to core.createObject Johannes Schindelin
1 sibling, 1 reply; 38+ messages in thread
From: Junio C Hamano @ 2009-04-27 22:10 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Johannes Schindelin, Johannes Sixt, git
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 ;-)
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-27 22:10 ` Junio C Hamano
@ 2009-04-27 22:28 ` Johannes Schindelin
2009-04-27 23:06 ` Linus Torvalds
0 siblings, 1 reply; 38+ messages in thread
From: Johannes Schindelin @ 2009-04-27 22:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Johannes Sixt, git
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH] Rename core.unreliableHardlinks to core.createObject
2009-04-27 20:18 ` Linus Torvalds
2009-04-27 22:10 ` Junio C Hamano
@ 2009-04-27 22:32 ` Johannes Schindelin
2009-04-27 23:48 ` Junio C Hamano
1 sibling, 1 reply; 38+ messages in thread
From: Johannes Schindelin @ 2009-04-27 22:32 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Junio C Hamano, Johannes Sixt, git
"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
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v3] Add an option not to use link(src, dest) && unlink(src) when that is unreliable
2009-04-27 22:28 ` Johannes Schindelin
@ 2009-04-27 23:06 ` Linus Torvalds
0 siblings, 0 replies; 38+ messages in thread
From: Linus Torvalds @ 2009-04-27 23:06 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, Johannes Sixt, git
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] Rename core.unreliableHardlinks to core.createObject
2009-04-27 22:32 ` [PATCH] Rename core.unreliableHardlinks to core.createObject Johannes Schindelin
@ 2009-04-27 23:48 ` Junio C Hamano
2009-04-28 8:23 ` Johannes Schindelin
0 siblings, 1 reply; 38+ messages in thread
From: Junio C Hamano @ 2009-04-27 23:48 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Linus Torvalds, Johannes Sixt, git
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;
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] Rename core.unreliableHardlinks to core.createObject
2009-04-27 23:48 ` Junio C Hamano
@ 2009-04-28 8:23 ` Johannes Schindelin
2009-04-28 8:44 ` Junio C Hamano
0 siblings, 1 reply; 38+ messages in thread
From: Johannes Schindelin @ 2009-04-28 8:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Johannes Sixt, git
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] Rename core.unreliableHardlinks to core.createObject
2009-04-28 8:23 ` Johannes Schindelin
@ 2009-04-28 8:44 ` Junio C Hamano
2009-04-28 14:50 ` Johannes Schindelin
0 siblings, 1 reply; 38+ messages in thread
From: Junio C Hamano @ 2009-04-28 8:44 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Linus Torvalds, Johannes Sixt, git
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?
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] Rename core.unreliableHardlinks to core.createObject
2009-04-28 8:44 ` Junio C Hamano
@ 2009-04-28 14:50 ` Johannes Schindelin
2009-04-28 20:59 ` Junio C Hamano
0 siblings, 1 reply; 38+ messages in thread
From: Johannes Schindelin @ 2009-04-28 14:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Johannes Sixt, git
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] Rename core.unreliableHardlinks to core.createObject
2009-04-28 14:50 ` Johannes Schindelin
@ 2009-04-28 20:59 ` Junio C Hamano
2009-04-28 22:07 ` Johannes Schindelin
0 siblings, 1 reply; 38+ messages in thread
From: Junio C Hamano @ 2009-04-28 20:59 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Linus Torvalds, Johannes Sixt, git
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.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] Rename core.unreliableHardlinks to core.createObject
2009-04-28 20:59 ` Junio C Hamano
@ 2009-04-28 22:07 ` Johannes Schindelin
0 siblings, 0 replies; 38+ messages in thread
From: Johannes Schindelin @ 2009-04-28 22:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Johannes Sixt, git
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
^ permalink raw reply [flat|nested] 38+ messages in thread
end of thread, other threads:[~2009-04-28 22:06 UTC | newest]
Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-04-23 10:53 [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable Johannes Schindelin
2009-04-23 19:16 ` Johannes Sixt
2009-04-23 19:33 ` Johannes Schindelin
2009-04-25 9:57 ` [PATCH v2] " Johannes Schindelin
2009-04-25 16:49 ` Junio C Hamano
2009-04-25 17:40 ` Linus Torvalds
2009-04-25 18:38 ` Michael Gaber
2009-04-25 18:43 ` Linus Torvalds
2009-04-27 3:37 ` Jay Soffian
2009-04-25 18:50 ` Johannes Sixt
2009-04-25 17:05 ` Junio C Hamano
2009-04-26 17:39 ` Johannes Schindelin
2009-04-25 17:39 ` Linus Torvalds
2009-04-23 19:39 ` [PATCH] " Alex Riesen
2009-04-23 21:59 ` Johannes Schindelin
2009-04-24 5:44 ` Alex Riesen
2009-04-25 17:56 ` Linus Torvalds
2009-04-25 18:52 ` Johannes Sixt
2009-04-26 1:17 ` Junio C Hamano
2009-04-26 17:40 ` Johannes Schindelin
2009-04-27 12:00 ` [PATCH v3] " Johannes Schindelin
2009-04-27 15:15 ` Linus Torvalds
2009-04-27 16:11 ` Johannes Schindelin
2009-04-27 16:53 ` Linus Torvalds
2009-04-27 19:55 ` Junio C Hamano
2009-04-27 20:13 ` Linus Torvalds
2009-04-27 20:18 ` Linus Torvalds
2009-04-27 22:10 ` Junio C Hamano
2009-04-27 22:28 ` Johannes Schindelin
2009-04-27 23:06 ` Linus Torvalds
2009-04-27 22:32 ` [PATCH] Rename core.unreliableHardlinks to core.createObject Johannes Schindelin
2009-04-27 23:48 ` Junio C Hamano
2009-04-28 8:23 ` Johannes Schindelin
2009-04-28 8:44 ` Junio C Hamano
2009-04-28 14:50 ` Johannes Schindelin
2009-04-28 20:59 ` Junio C Hamano
2009-04-28 22:07 ` Johannes Schindelin
2009-04-26 17:38 ` [PATCH] Add an option not to use link(src, dest) && unlink(src) when that is unreliable Johannes Schindelin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.