React to feedback for UTF-8 <-> WCHAR and reparse work

9 files changed, 35 insertions, 39 deletions

diff --git a/src/path.c b/src/path.c
index b2845466b..2690cd8e8 100644
--- a/src/path.c
+++ b/src/path.c
@@ -495,7 +495,7 @@ bool git_path_is_empty_dir(const char *path)
 		HANDLE hFind = FindFirstFileW(filter_w, &findData);
 
 		/* If the find handle was created successfully, then it's a directory */
-		if (INVALID_HANDLE_VALUE != hFind) {
+		if (hFind != INVALID_HANDLE_VALUE) {
 			empty = true;
 
 			do {
diff --git a/src/util.h b/src/util.h
index d94463c65..6fb2dc0f4 100644
--- a/src/util.h
+++ b/src/util.h
@@ -22,6 +22,15 @@
 
 #define GIT_DATE_RFC2822_SZ  32
 
+/**
+ * Return the length of a constant string.
+ * We are aware that `strlen` performs the same task and is usually
+ * optimized away by the compiler, whilst being safer because it returns
+ * valid values when passed a pointer instead of a constant string; however
+ * this macro will transparently work with wide-char and single-char strings.
+ */
+#define CONST_STRLEN(x) ((sizeof(x)/sizeof(x[0])) - 1)
+
 /*
  * Custom memory allocation wrappers
  * that set error code and error message
diff --git a/src/win32/findfile.c b/src/win32/findfile.c
index bd06d9232..86d4ef5bd 100644
--- a/src/win32/findfile.c
+++ b/src/win32/findfile.c
@@ -119,7 +119,7 @@ static int win32_find_git_in_registry(
 
 		/* InstallLocation points to the root of the git directory */
 		if (!RegQueryValueExW(hKey, L"InstallLocation", NULL, &dwType, (LPBYTE)path, &cbData) &&
-			REG_SZ == dwType) {
+			dwType == REG_SZ) {
 
 			/* Append the suffix */
 			wcscat(path, subdir);
diff --git a/src/win32/posix_w32.c b/src/win32/posix_w32.c
index bef65a354..0d070f6b5 100644
--- a/src/win32/posix_w32.c
+++ b/src/win32/posix_w32.c
@@ -62,7 +62,7 @@ int p_unlink(const char *path)
 
 	/* If the file could not be deleted because it was
 	 * read-only, clear the bit and try again */
-	if (-1 == error && EACCES == errno) {
+	if (error == -1 && errno == EACCES) {
 		_wchmod(buf, 0666);
 		error = _wunlink(buf);
 	}
@@ -120,7 +120,7 @@ static int readlink_w(
 		FILE_SHARE_READ | FILE_SHARE_DELETE, NULL, OPEN_EXISTING,
 		FILE_FLAG_OPEN_REPARSE_POINT | FILE_FLAG_BACKUP_SEMANTICS, NULL);
 
-	if (INVALID_HANDLE_VALUE == handle) {
+	if (handle == INVALID_HANDLE_VALUE) {
 		errno = ENOENT;
 		return -1;
 	}
@@ -149,7 +149,7 @@ static int readlink_w(
 
 	if (target_len) {
 		/* The path may need to have a prefix removed. */
-		target_len = git_win32__to_dos(target, target_len);
+		target_len = git_win32__canonicalize_path(target, target_len);
 
 		/* Need one additional character in the target buffer
 		 * for the terminating NULL. */
@@ -235,7 +235,7 @@ static int lstat_w(
 			path[path_len] = L'\0';
 			attrs = GetFileAttributesW(path);
 
-			if (INVALID_FILE_ATTRIBUTES != attrs) {
+			if (attrs != INVALID_FILE_ATTRIBUTES) {
 				if (!(attrs & FILE_ATTRIBUTE_DIRECTORY))
 					errno = ENOTDIR;
 				break;
@@ -393,23 +393,18 @@ static int getfinalpath_w(
 	hFile = CreateFileW(path, GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_DELETE,
 		NULL, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL);
 
-	if (hFile == INVALID_HANDLE_VALUE)
+	if (INVALID_HANDLE_VALUE == hFile)
 		return -1;
 
 	/* Call GetFinalPathNameByHandle */
 	dwChars = pgfp(hFile, dest, GIT_WIN_PATH_UTF16, FILE_NAME_NORMALIZED);
+	CloseHandle(hFile);
 
-	if (!dwChars || dwChars >= GIT_WIN_PATH_UTF16) {
-		DWORD error = GetLastError();
-		CloseHandle(hFile);
-		SetLastError(error);
+	if (!dwChars || dwChars >= GIT_WIN_PATH_UTF16)
 		return -1;
-	}
-
-	CloseHandle(hFile);
 
 	/* The path may be delivered to us with a prefix; canonicalize */
-	return (int)git_win32__to_dos(dest, dwChars);
+	return (int)git_win32__canonicalize_path(dest, dwChars);
 }
 
 static int follow_and_lstat_link(git_win32_path path, struct stat* buf)
@@ -473,7 +468,7 @@ int p_rmdir(const char* path)
 
 	error = _wrmdir(buf);
 
-	if (-1 == error) {
+	if (error == -1) {
 		switch (GetLastError()) {
 			/* _wrmdir() is documented to return EACCES if "A program has an open
 			 * handle to the directory."  This sounds like what everybody else calls
@@ -513,7 +508,7 @@ char *p_realpath(const char *orig_path, char *buffer)
 	}
 
 	/* The path must exist. */
-	if (INVALID_FILE_ATTRIBUTES == GetFileAttributesW(buffer_w)) {
+	if (GetFileAttributesW(buffer_w) == INVALID_FILE_ATTRIBUTES) {
 		errno = ENOENT;
 		return NULL;
 	}
diff --git a/src/win32/reparse.h b/src/win32/reparse.h
index 4f56ed055..70f9fd652 100644
--- a/src/win32/reparse.h
+++ b/src/win32/reparse.h
@@ -54,4 +54,4 @@ typedef struct _GIT_REPARSE_DATA_BUFFER {
 # define FSCTL_SET_REPARSE_POINT			0x000900a4
 #endif
 
-#endif
\ No newline at end of file
+#endif
diff --git a/src/win32/utf-conv.c b/src/win32/utf-conv.c
index fe94701a8..b9ccfb5e5 100644
--- a/src/win32/utf-conv.c
+++ b/src/win32/utf-conv.c
@@ -9,7 +9,7 @@
 #include "utf-conv.h"
 
 #ifndef WC_ERR_INVALID_CHARS
-#define WC_ERR_INVALID_CHARS	0x80
+# define WC_ERR_INVALID_CHARS	0x80
 #endif
 
 GIT_INLINE(DWORD) get_wc_flags(void)
@@ -87,12 +87,8 @@ int git__utf8_to_16_alloc(wchar_t **dest, const char *src)
 	utf16_size = MultiByteToWideChar(CP_UTF8, MB_ERR_INVALID_CHARS, src, -1, *dest, utf16_size);
 
 	if (!utf16_size) {
-		/* Don't let git__free stomp on the thread-local last error code,
-		* so that the caller can call giterr_set(GITERR_OS, ...) */
-		DWORD last_error = GetLastError();
 		git__free(*dest);
 		*dest = NULL;
-		SetLastError(last_error);
 	}
 
 	/* Subtract 1 from the result to turn 0 into -1 (an error code) and to not count the NULL
@@ -131,12 +127,8 @@ int git__utf16_to_8_alloc(char **dest, const wchar_t *src)
 	utf8_size = WideCharToMultiByte(CP_UTF8, dwFlags, src, -1, *dest, utf8_size, NULL, NULL);
 
 	if (!utf8_size) {
-		/* Don't let git__free stomp on the thread-local last error code,
-		 * so that the caller can call giterr_set(GITERR_OS, ...) */
-		DWORD last_error = GetLastError();
 		git__free(*dest);
 		*dest = NULL;
-		SetLastError(last_error);
 	}
 
 	/* Subtract 1 from the result to turn 0 into -1 (an error code) and to not count the NULL
diff --git a/src/win32/w32_util.c b/src/win32/w32_util.c
index 50b85a334..2e52525d5 100644
--- a/src/win32/w32_util.c
+++ b/src/win32/w32_util.c
@@ -7,8 +7,6 @@
 
 #include "w32_util.h"
 
-#define CONST_STRLEN(x) ((sizeof(x)/sizeof(x[0])) - 1)
-
 /**
  * Creates a FindFirstFile(Ex) filter string from a UTF-8 path.
  * The filter string enumerates all items in the directory.
@@ -26,8 +24,10 @@ bool git_win32__findfirstfile_filter(git_win32_path dest, const char *src)
 	if (len < 0)
 		return false;
 
-	/* Ensure that the path does not end with a trailing slash --
-	 * because we're about to add one! */
+	/* Ensure that the path does not end with a trailing slash,
+	 * because we're about to add one. Don't rely our trim_end
+	 * helper, because we want to remove the backslash even for
+	 * drive letter paths, in this case. */
 	if (len > 0 &&
 		(dest[len - 1] == L'/' || dest[len - 1] == L'\\')) {
 		dest[len - 1] = L'\0';
@@ -35,7 +35,7 @@ bool git_win32__findfirstfile_filter(git_win32_path dest, const char *src)
 	}
 
 	/* Ensure we have enough room to add the suffix */
-	if ((size_t)len > GIT_WIN_PATH_UTF16 - ARRAY_SIZE(suffix))
+	if ((size_t)len >= GIT_WIN_PATH_UTF16 - CONST_STRLEN(suffix))
 		return false;
 
 	wcscat(dest, suffix);
@@ -59,11 +59,11 @@ int git_win32__sethidden(const char *path)
 	attrs = GetFileAttributesW(buf);
 
 	/* Ensure the path exists */
-	if (INVALID_FILE_ATTRIBUTES == attrs)
+	if (attrs == INVALID_FILE_ATTRIBUTES)
 		return -1;
 
 	/* If the item isn't already +H, add the bit */
-	if (0 == (attrs & FILE_ATTRIBUTE_HIDDEN) &&
+	if ((attrs & FILE_ATTRIBUTE_HIDDEN) == 0 &&
 		!SetFileAttributesW(buf, attrs | FILE_ATTRIBUTE_HIDDEN))
 		return -1;
 
@@ -85,7 +85,7 @@ size_t git_win32__path_trim_end(wchar_t *str, size_t len)
 
 		/* Don't trim backslashes from drive letter paths, which
 		 * are 3 characters long and of the form C:\, D:\, etc. */
-		if (3 == len && git_win32__isalpha(str[0]) && str[1] == ':')
+		if (len == 3 && git_win32__isalpha(str[0]) && str[1] == ':')
 			break;
 
 		len--;
@@ -103,7 +103,7 @@ size_t git_win32__path_trim_end(wchar_t *str, size_t len)
  * @param path The path which should be converted.
  * @return The length of the modified string (<= the input length)
  */
-size_t git_win32__to_dos(wchar_t *str, size_t len)
+size_t git_win32__canonicalize_path(wchar_t *str, size_t len)
 {
 	static const wchar_t dosdevices_prefix[] = L"\\\?\?\\";
 	static const wchar_t nt_prefix[] = L"\\\\?\\";
@@ -136,4 +136,4 @@ size_t git_win32__to_dos(wchar_t *str, size_t len)
 	}
 
 	return git_win32__path_trim_end(str, len);
-}
\ No newline at end of file
+}
diff --git a/src/win32/w32_util.h b/src/win32/w32_util.h
index acdee3d69..a1d388af5 100644
--- a/src/win32/w32_util.h
+++ b/src/win32/w32_util.h
@@ -49,6 +49,6 @@ size_t git_win32__path_trim_end(wchar_t *str, size_t len);
  * @param path The path which should be converted.
  * @return The length of the modified string (<= the input length)
  */
-size_t git_win32__to_dos(wchar_t *str, size_t len);
+size_t git_win32__canonicalize_path(wchar_t *str, size_t len);
 
 #endif
diff --git a/tests/core/link.c b/tests/core/link.c
index 20d2706f7..1794a3893 100644
--- a/tests/core/link.c
+++ b/tests/core/link.c
@@ -137,7 +137,7 @@ static void do_junction(const char *old, const char *new)
 
 	reparse_buf->ReparseTag = IO_REPARSE_TAG_MOUNT_POINT;
 	reparse_buf->MountPointReparseBuffer.SubstituteNameOffset = 0;
-	reparse_buf->MountPointReparseBuffer.SubstituteNameLength =	subst_byte_len;
+	reparse_buf->MountPointReparseBuffer.SubstituteNameLength = subst_byte_len;
 	reparse_buf->MountPointReparseBuffer.PrintNameOffset = (USHORT)(subst_byte_len + sizeof(WCHAR));
 	reparse_buf->MountPointReparseBuffer.PrintNameLength = print_byte_len;
 	reparse_buf->ReparseDataLength = reparse_buflen - REPARSE_DATA_HEADER_SIZE;