Re: [PATCH] Fix merge-recursive on cygwin: broken errno when unlinking a directory
- From
Linus Torvalds <torvalds@linux-foundation.org>
- Date
- Apr 19, 2007, 16:39 UTC
- Message-ID
- <alpine.LFD.0.98.0704190932450.9964@woody.linux-foundation.org>
- In-Reply-To
- <81b0412b0704190128o7fccbe77h8df3114328d6a0da@mail.gmail.com>
On Thu, 19 Apr 2007, Alex Riesen wrote:
Show 12 quoted lines
> On 4/19/07, Alex Riesen <raa.lkml@gmail.com> wrote: > > > > > > So here's a suggested and totally untested patch. It makes the code more > > > readable, and probably fixes *two* bugs in the process. It also simply > > > doesn't really even care what the error actually was - the important part > > > was not that it was a directory, but that the unlink didn't succeed! > > > > > > > Well, it is a bit tested now. I'll repeat the testing tomorrow on that > > windows box. > > Tested on windows too. Works.
Good. Junio, I'd really suggest applying it. The old code was literally wrong, and depended on an error return that seems to be Linux-specific. It was also pretty ugly.
Here's the patch again with proper sign-off and a commentary..
As mentioned, maybe this wants expanding in the future, but regardless, the patch not only fixes a git problem on windows (and quite possibly other unixes too), any extensions will be much easier on top of this.
Linus
--- From: Linus Torvalds <torvalds@linux-foundation.org> Subject: Fix working directory errno handling when unlinking a directory
Alex Riesen noticed that the case where a file replaced a directory entry in the working tree was broken on cygwin. It turns out that the code made some Linux-specific assumptions, and also ignored errors entirely for the case where the entry was a symlink rather than a file.
This cleans it up by separating out the common case into a function of its own, so that both regular files and symlinks can share it, and by making the error handling more obvious (and not depend on any Linux-specific behaviour).
Acked-by: Alex Riesen <raa.lkml@gmail.com> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
--- merge-recursive.c | 54 ++++++++++++++++++++++++++++------------------------ 1 files changed, 29 insertions(+), 25 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c index 595b022..cea6c87 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -574,6 +574,31 @@ static void flush_buffer(int fd, const char *buf, unsigned long size) } } +static int make_room_for_path(const char *path) +{ + int status; + const char *msg = "failed to create path '%s'%s"; + + status = mkdir_p(path, 0777); + if (status) { + if (status == -3) { + /* something else exists */ + error(msg, path, ": perhaps a D/F conflict?"); + return -1; + } + die(msg, path, ""); + } + + /* Successful unlink is good.. */ + if (!unlink(path)) + return 0; + /* .. and so is no existing file */ + if (errno == ENOENT) + return 0; + /* .. but not some other error (who really cares what?) */ + return error(msg, path, ": perhaps a D/F conflict?"); +} + static void update_file_flags(const unsigned char *sha, unsigned mode, const char *path, @@ -594,33 +619,12 @@ static void update_file_flags(const unsigned char *sha, if (type != OBJ_BLOB) die("blob expected for %s '%s'", sha1_to_hex(sha), path); + if (make_room_for_path(path) < 0) { + update_wd = 0; + goto update_index; + } if (S_ISREG(mode) || (!has_symlinks && S_ISLNK(mode))) { int fd; - int status; - const char *msg = "failed to create path '%s'%s"; - - status = mkdir_p(path, 0777); - if (status) { - if (status == -3) { - /* something else exists */ - error(msg, path, ": perhaps a D/F conflict?"); - update_wd = 0; - goto update_index; - } - die(msg, path, ""); - } - if (unlink(path)) { - if (errno == EISDIR) { - /* something else exists */ - error(msg, path, ": perhaps a D/F conflict?"); - update_wd = 0; - goto update_index; - } - if (errno != ENOENT) - die("failed to unlink %s " - "in preparation to update: %s", - path, strerror(errno)); - } if (mode & 0100) mode = 0777; else