Change how File/Directory/Link .delete works.

runtime/bin/file_win.cc

Index: runtime/bin/file_win.cc
diff --git a/runtime/bin/file_win.cc b/runtime/bin/file_win.cc
index 3780fca056c2adf2f827ef271da4ae4ecf839228..86e808c1ddac61fd56ce1d89cb70df83e89cb49e 100644
--- a/runtime/bin/file_win.cc
+++ b/runtime/bin/file_win.cc
@@ -259,19 +259,25 @@ bool File::CreateLink(const char* utf8_name, const char* utf8_target) {
 
 bool File::Delete(const char* name) {
   const wchar_t* system_name = StringUtils::Utf8ToWide(name);
+  int status = _wremove(system_name);
+  free(const_cast<wchar_t*>(system_name));
+  return status != -1;
+}
+
+
+bool File::DeleteLink(const char* name) {
+  const wchar_t* system_name = StringUtils::Utf8ToWide(name);
+  bool result = false;
   DWORD attributes = GetFileAttributesW(system_name);
   if ((attributes != INVALID_FILE_ATTRIBUTES) &&
       (attributes & FILE_ATTRIBUTE_REPARSE_POINT) != 0) {
     // It's a junction(link), delete it.
-    return RemoveDirectoryW(system_name) != 0;
+    result = (RemoveDirectoryW(system_name) != 0);
   } else {
-    int status = _wremove(system_name);
-    free(const_cast<wchar_t*>(system_name));
-    if (status == -1) {
-      return false;
-    }
-    return true;
+    SetLastError(ERROR_NOT_A_REPARSE_POINT);
   }
+  free(const_cast<wchar_t*>(system_name));

[Bill Hesse, 2013/04/08 12:16:33]

I think there is a problem calling "free" between an implicit or explicit
SetLastError, and the GetLastError after the function returns.  See line 333 for
how we wrap a call to free elsewhere.

[Anders Johnsen, 2013/04/08 12:22:34]

Can you link me some info on the problem? I'm not sure what the problem should
be. (it would render any stack-allocated c++ classes dangerous?)

+  return result;
 }
 
 
@@ -480,15 +486,11 @@ File::StdioHandleType File::GetStdioHandleType(int fd) {
 
 File::Type File::GetType(const char* pathname, bool follow_links) {
   const wchar_t* name = StringUtils::Utf8ToWide(pathname);
-  WIN32_FIND_DATAW file_data;
-  HANDLE find_handle = FindFirstFileW(name, &file_data);
-  if (find_handle == INVALID_HANDLE_VALUE) {
-    // TODO(whesse): Distinguish other errors from does not exist.
-    return File::kDoesNotExist;
-  }
-  FindClose(find_handle);
-  DWORD attributes = file_data.dwFileAttributes;
-  if ((attributes & FILE_ATTRIBUTE_REPARSE_POINT) != 0) {
+  DWORD attributes = GetFileAttributesW(name);
+  File::Type result = kIsFile;
+  if (attributes == INVALID_FILE_ATTRIBUTES) {
+    result = kDoesNotExist;
+  } else if ((attributes & FILE_ATTRIBUTE_REPARSE_POINT) != 0) {
     if (follow_links) {
       HANDLE dir_handle = CreateFileW(
           name,
@@ -499,26 +501,19 @@ File::Type File::GetType(const char* pathname, bool follow_links) {
           FILE_FLAG_BACKUP_SEMANTICS,
           NULL);
       if (dir_handle == INVALID_HANDLE_VALUE) {
-        // TODO(whesse): Distinguish other errors from does not exist.
-        return File::kDoesNotExist;
+        result = File::kIsLink;
       } else {
         CloseHandle(dir_handle);

[Bill Hesse, 2013/04/08 12:16:33]

Since we have the handle to the file, and this might be a symbolic link, rather
than a junction, should we check the attributes for file or directory (or even
the original "attributes" field), and report file or directory here?  Since we
don't support creating symbolic links, we couldn't test this.

[Anders Johnsen, 2013/04/08 12:22:34]

I understand now. Yes, we should probably add that back.

-        return File::kIsDirectory;
+        result = File::kIsDirectory;
       }
     } else {
-      DWORD reparse_tag = file_data.dwReserved0;
-      if (reparse_tag == IO_REPARSE_TAG_SYMLINK ||
-          reparse_tag == IO_REPARSE_TAG_MOUNT_POINT) {
-        return File::kIsLink;
-      } else {
-        return File::kDoesNotExist;
-      }
+      result = kIsLink;
     }
   } else if ((attributes & FILE_ATTRIBUTE_DIRECTORY) != 0) {
-    return File::kIsDirectory;
-  } else {
-    return File::kIsFile;
+    result = kIsDirectory;
   }
+  free(const_cast<wchar_t*>(name));
+  return result;
 }