Re: [PATCH] setup.c: move statement under condition

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thank you for your replay.

> I have to be honest: this commit message (including the subject) left me
> quite puzzled as to the intent of this patch.

I still only learn English and correctly express my thoughts while somewhat difficult.

> If you also have a background story that motivated you to work on this
> patch (for example, if you hit a huge performance bottleneck with some
> tool that fed thousands of absolute paths to Git that needed to be turned
> into paths relative to the worktree's top-level directory), I would
> definitely put that into the commit message, too, if I were you.

I have no such reason. I just saw it and wanted to change it.

> Up until recently, we encouraged dropping the curly brackets from
> single-line statements, but apparently that changed. It is now no longer
> clear, and often left to the taste of the contributor. But not always.
> Sometimes we start a beautiful thread discussion the pros and cons of
> curly brackets in the middle of patch review, and drop altogether
> reviewing the actual patch.

I was guided by the rule from the Documentation/CodingGuidelines:
	When there are multiple arms to a conditional and some of them
	require braces, enclose even a single line block in braces for
	consistency.
And other code from setup.c:
	from function get_common_dir:
		if (!has_common) {
			/* several commands */
		} else {
			free(candidate->work_tree);
		}
	from function get_common_dir_noenv:
		if (file_exists(path.buf)) {
			/* several commands */
		} else {
			strbuf_addstr(sb, gitdir);
		}

> In short: I think your patch does the right thing, and I hope that you
> find my suggestions to improve the patch useful.

I fixed the patch according to your suggestions.


Signed-off-by: Vadim Petrov <tridronet@xxxxxxxxx>
---
 setup.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/setup.c b/setup.c
index 8cc34186c..1a414c256 100644
--- a/setup.c
+++ b/setup.c
@@ -27,26 +27,26 @@ static int abspath_part_inside_repo(char *path)
 {
 	size_t len;
 	size_t wtlen;
 	char *path0;
 	int off;
 	const char *work_tree = get_git_work_tree();
 
 	if (!work_tree)
 		return -1;
 	wtlen = strlen(work_tree);
 	len = strlen(path);
-	off = offset_1st_component(path);
 
-	/* check if work tree is already the prefix */
-	if (wtlen <= len && !strncmp(path, work_tree, wtlen)) {
+	if (wtlen > len || strncmp(path, work_tree, wtlen))
+		off = offset_1st_component(path);
+	else { /* check if work tree is already the prefix */
 		if (path[wtlen] == '/') {
 			memmove(path, path + wtlen + 1, len - wtlen);
 			return 0;
 		} else if (path[wtlen - 1] == '/' || path[wtlen] == '\0') {
 			/* work tree is the root, or the whole path */
 			memmove(path, path + wtlen, len - wtlen + 1);
 			return 0;
 		}
 		/* work tree might match beginning of a symlink to work tree */
 		off = wtlen;
 	}
-- 
2.15.1.433.g936d1b989



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux