All of lore.kernel.org
 help / color / mirror / Atom feed
From: Vadim Petrov <tridronet@yandex.ru>
To: Johannes Schindelin <johannes.schindelin@gmx.de>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>
Subject: Re: [PATCH] setup.c: move statement under condition
Date: Sun, 24 Dec 2017 12:15:35 +0400	[thread overview]
Message-ID: <1483761514103335@web45o.yandex.ru> (raw)
In-Reply-To: <alpine.DEB.2.21.1.1712232332000.406@MININT-6BKU6QN.europe.corp.microsoft.com>

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@yandex.ru>
---
 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

  reply	other threads:[~2017-12-24  8:26 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-23 20:02 [PATCH] setup.c: move statement under condition Vadim Petrov
2017-12-23 22:48 ` Johannes Schindelin
2017-12-24  8:15   ` Vadim Petrov [this message]
2017-12-24 17:11     ` Kevin Daudt
2017-12-24 19:35       ` Ævar Arnfjörð Bjarmason
2017-12-25 17:52         ` Martin Werner
2017-12-27 18:57   ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1483761514103335@web45o.yandex.ru \
    --to=tridronet@yandex.ru \
    --cc=git@vger.kernel.org \
    --cc=johannes.schindelin@gmx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.