Re: [PATCH 1/2] separate quoting and relative path generation

6 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH 1/2] separate quoting and relative path generation

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:48:54 

Clemens Buchacher [off-list ref] writes:

+/* give path as relative to prefix */
+char *path_relative(const char *in, int len,
+		    struct strbuf *out, const char *prefix, int prefix_len)
+{
 	if (len < 0)
 		len = strlen(in);
+	if (prefix && prefix_len < 0)
+		prefix_len = strlen(prefix);
 
 	strbuf_setlen(out, 0);
 	strbuf_grow(out, len);
 
+	if (prefix_len > 0) {
+		int off = 0, i = 0;
+		while (i < prefix_len && i < len && prefix[i] == in[i]) {
+			if (prefix[i] == '/')
+				off = i + 1;
+			i++;
+		}
+		in += off;
+		len -= off;
+
+		while (i < prefix_len) {
+			if (prefix[i] == '/')
 				strbuf_addstr(out, "../");
+			i++;
+		}
 	}
+	strbuf_add(out, in, len);
+
+	return out->buf;
+}
Hmm...  I wonder if we really want to always make a copy of the string in
the majority of the case where there is no need to add ../ and the path
does not have any funny characters that needs quoting.  In such a case,
shouldn't write_name() be just moving the pointers into the original
string to skip the $(cwd) part and writing the remainder of the string
out, without any extra allocation nor copy?  IIUC, that is what the
original did using write_name_quoted().

[PATCH] optimize path_relative()

From: Clemens Buchacher <hidden>
Date: 2016-06-15 22:48:54 

Avoid copying to strbuf in case a subset of the original string can
be returned.

Since the strbuf is no longer guaranteed to be updated, this
function is different from quote_path_relative(). To avoid
confusion, do not export it.

Signed-off-by: Clemens Buchacher <redacted>
---
On Thu, Jun 03, 2010 at 03:16:33PM -0700, Junio C Hamano wrote:
Clemens Buchacher [off-list ref] writes:
quoted
+	strbuf_add(out, in, len);
+
+	return out->buf;
+}
Hmm...  I wonder if we really want to always make a copy of the string in
the majority of the case where there is no need to add ../ and the path
does not have any funny characters that needs quoting.
Right. Implemented below. I noticed a few more redundancies along
the way, so the following could be squashed into 1/2 "separate
quoting and relative path generation" or applied on top of the
existing two patches.

Please let me know if you'd rather have a resend.

Clemens

 quote.c |   72 ++++++++++++++++++++++++++++++++++++--------------------------
 quote.h |    3 --
 2 files changed, 42 insertions(+), 33 deletions(-)

diff --git a/quote.c b/quote.c
index 2ae2c1f..481541a 100644
--- a/quote.c
+++ b/quote.c
@@ -295,62 +295,74 @@ void write_name_quotedpfx(const char *pfx, size_t pfxlen,
 	fputc(terminator, fp);
 }
 
+static const char *path_relative(const char *in, int len,
+				 struct strbuf *sb, const char *prefix,
+				 int prefix_len);
+
 void write_name_quoted_relative(const char *name, size_t len,
 				const char *prefix, size_t prefix_len,
 				FILE *fp, int terminator)
 {
 	struct strbuf sb = STRBUF_INIT;
 
-	char *path = path_relative(name, len, &sb, prefix, prefix_len);
-	write_name_quoted(path, fp, terminator);
+	name = path_relative(name, len, &sb, prefix, prefix_len);
+	write_name_quoted(name, fp, terminator);
 
 	strbuf_release(&sb);
 }
 
-/* give path as relative to prefix */
-char *path_relative(const char *in, int len,
-		    struct strbuf *out, const char *prefix, int prefix_len)
+/*
+ * Give path as relative to prefix.
+ *
+ * The strbuf may or may not be used, so do not assume it contains the
+ * returned path.
+ */
+static const char *path_relative(const char *in, int len,
+				 struct strbuf *sb, const char *prefix,
+				 int prefix_len)
 {
+	int off, i;
+
 	if (len < 0)
 		len = strlen(in);
 	if (prefix && prefix_len < 0)
 		prefix_len = strlen(prefix);
 
-	strbuf_setlen(out, 0);
-	strbuf_grow(out, len);
+	off = 0;
+	i = 0;
+	while (i < prefix_len && i < len && prefix[i] == in[i]) {
+		if (prefix[i] == '/')
+			off = i + 1;
+		i++;
+	}
+	in += off;
+	len -= off;
 
-	if (prefix_len > 0) {
-		int off = 0, i = 0;
-		while (i < prefix_len && i < len && prefix[i] == in[i]) {
-			if (prefix[i] == '/')
-				off = i + 1;
-			i++;
-		}
-		in += off;
-		len -= off;
+	if (i == prefix_len)
+		return in;
 
-		while (i < prefix_len) {
-			if (prefix[i] == '/')
-				strbuf_addstr(out, "../");
-			i++;
-		}
+	strbuf_reset(sb);
+	strbuf_grow(sb, len);
+
+	while (i < prefix_len) {
+		if (prefix[i] == '/')
+			strbuf_addstr(sb, "../");
+		i++;
 	}
-	strbuf_add(out, in, len);
+	strbuf_add(sb, in, len);
 
-	return out->buf;
+	return sb->buf;
 }
 
 /* quote path as relative to the given prefix */
 char *quote_path_relative(const char *in, int len,
 			  struct strbuf *out, const char *prefix)
 {
-	char *rel;
-	size_t rel_len;
-
-	path_relative(in, len, out, prefix, -1);
-	rel = strbuf_detach(out, &rel_len);
-	quote_c_style_counted(rel, rel_len, out, NULL, 0);
-	free(rel);
+	struct strbuf sb = STRBUF_INIT;
+	const char *rel = path_relative(in, len, &sb, prefix, -1);
+	strbuf_reset(out);
+	quote_c_style_counted(rel, strlen(rel), out, NULL, 0);
+	strbuf_release(&sb);
 
 	if (!out->len)
 		strbuf_addstr(out, "./");
diff --git a/quote.h b/quote.h
index e9e0221..38003bf 100644
--- a/quote.h
+++ b/quote.h
@@ -58,9 +58,6 @@ extern void write_name_quoted_relative(const char *name, size_t len,
 		const char *prefix, size_t prefix_len,
 		FILE *fp, int terminator);
 
-/* give path as relative to prefix */
-extern char *path_relative(const char *in, int len,
-		struct strbuf *out, const char *prefix, int prefix_len);
 /* quote path as relative to the given prefix */
 extern char *quote_path_relative(const char *in, int len,
 			  struct strbuf *out, const char *prefix);
-- 
1.7.1.2.ga1f6e

Re: [PATCH] optimize path_relative()

From: Clemens Buchacher <hidden>
Date: 2016-06-15 22:48:54 

On Fri, Jun 04, 2010 at 09:44:43AM +0200, Clemens Buchacher wrote:

+static const char *path_relative(const char *in, int len,
+				 struct strbuf *sb, const char *prefix,
+				 int prefix_len)
 {
+	int off, i;
+
 	if (len < 0)
 		len = strlen(in);
 	if (prefix && prefix_len < 0)
 		prefix_len = strlen(prefix);
 
-	strbuf_setlen(out, 0);
-	strbuf_grow(out, len);
+	off = 0;
+	i = 0;
+	while (i < prefix_len && i < len && prefix[i] == in[i]) {
+		if (prefix[i] == '/')
+			off = i + 1;
+		i++;
+	}
+	in += off;
+	len -= off;
 
-	if (prefix_len > 0) {
-		int off = 0, i = 0;
-		while (i < prefix_len && i < len && prefix[i] == in[i]) {
-			if (prefix[i] == '/')
-				off = i + 1;
-			i++;
-		}
-		in += off;
-		len -= off;
+	if (i == prefix_len)
+		return in;
 
-		while (i < prefix_len) {
-			if (prefix[i] == '/')
-				strbuf_addstr(out, "../");
-			i++;
-		}
+	strbuf_reset(sb);
+	strbuf_grow(sb, len);
+
+	while (i < prefix_len) {
+		if (prefix[i] == '/')
+			strbuf_addstr(sb, "../");
+		i++;
 	}
By the way, I noticed that we rely on the fact that a non-empty
prefix ends with '/'. Is that ok?

-	strbuf_add(out, in, len);
+	strbuf_add(sb, in, len);
 
-	return out->buf;
+	return sb->buf;
 }

[PATCH v2] optimize path_relative()

From: Clemens Buchacher <hidden>
Date: 2016-06-15 22:48:55 

Avoid copying to strbuf in case a subset of the original string can
be returned.

Since the strbuf is no longer guaranteed to be updated, this
function is different from quote_path_relative(). To avoid
confusion, do not export it.

Signed-off-by: Clemens Buchacher <redacted>
---

On Fri, Jun 04, 2010 at 09:44:43AM +0200, Clemens Buchacher wrote:

 	if (prefix && prefix_len < 0)
 		prefix_len = strlen(prefix);
[...]
+	if (i == prefix_len)
+		return in;
This should have been i >= prefix_len, in case prefix = NULL and
prefix_len = -1.  (Can only happen with git clean at the moment.)

 quote.c |   72 ++++++++++++++++++++++++++++++++++++--------------------------
 quote.h |    3 --
 2 files changed, 42 insertions(+), 33 deletions(-)

diff --git a/quote.c b/quote.c
index 2ae2c1f..63d3b01 100644
--- a/quote.c
+++ b/quote.c
@@ -295,62 +295,74 @@ void write_name_quotedpfx(const char *pfx, size_t pfxlen,
 	fputc(terminator, fp);
 }
 
+static const char *path_relative(const char *in, int len,
+				 struct strbuf *sb, const char *prefix,
+				 int prefix_len);
+
 void write_name_quoted_relative(const char *name, size_t len,
 				const char *prefix, size_t prefix_len,
 				FILE *fp, int terminator)
 {
 	struct strbuf sb = STRBUF_INIT;
 
-	char *path = path_relative(name, len, &sb, prefix, prefix_len);
-	write_name_quoted(path, fp, terminator);
+	name = path_relative(name, len, &sb, prefix, prefix_len);
+	write_name_quoted(name, fp, terminator);
 
 	strbuf_release(&sb);
 }
 
-/* give path as relative to prefix */
-char *path_relative(const char *in, int len,
-		    struct strbuf *out, const char *prefix, int prefix_len)
+/*
+ * Give path as relative to prefix.
+ *
+ * The strbuf may or may not be used, so do not assume it contains the
+ * returned path.
+ */
+static const char *path_relative(const char *in, int len,
+				 struct strbuf *sb, const char *prefix,
+				 int prefix_len)
 {
+	int off, i;
+
 	if (len < 0)
 		len = strlen(in);
 	if (prefix && prefix_len < 0)
 		prefix_len = strlen(prefix);
 
-	strbuf_setlen(out, 0);
-	strbuf_grow(out, len);
+	off = 0;
+	i = 0;
+	while (i < prefix_len && i < len && prefix[i] == in[i]) {
+		if (prefix[i] == '/')
+			off = i + 1;
+		i++;
+	}
+	in += off;
+	len -= off;
 
-	if (prefix_len > 0) {
-		int off = 0, i = 0;
-		while (i < prefix_len && i < len && prefix[i] == in[i]) {
-			if (prefix[i] == '/')
-				off = i + 1;
-			i++;
-		}
-		in += off;
-		len -= off;
+	if (i >= prefix_len)
+		return in;
 
-		while (i < prefix_len) {
-			if (prefix[i] == '/')
-				strbuf_addstr(out, "../");
-			i++;
-		}
+	strbuf_reset(sb);
+	strbuf_grow(sb, len);
+
+	while (i < prefix_len) {
+		if (prefix[i] == '/')
+			strbuf_addstr(sb, "../");
+		i++;
 	}
-	strbuf_add(out, in, len);
+	strbuf_add(sb, in, len);
 
-	return out->buf;
+	return sb->buf;
 }
 
 /* quote path as relative to the given prefix */
 char *quote_path_relative(const char *in, int len,
 			  struct strbuf *out, const char *prefix)
 {
-	char *rel;
-	size_t rel_len;
-
-	path_relative(in, len, out, prefix, -1);
-	rel = strbuf_detach(out, &rel_len);
-	quote_c_style_counted(rel, rel_len, out, NULL, 0);
-	free(rel);
+	struct strbuf sb = STRBUF_INIT;
+	const char *rel = path_relative(in, len, &sb, prefix, -1);
+	strbuf_reset(out);
+	quote_c_style_counted(rel, strlen(rel), out, NULL, 0);
+	strbuf_release(&sb);
 
 	if (!out->len)
 		strbuf_addstr(out, "./");
diff --git a/quote.h b/quote.h
index e9e0221..38003bf 100644
--- a/quote.h
+++ b/quote.h
@@ -58,9 +58,6 @@ extern void write_name_quoted_relative(const char *name, size_t len,
 		const char *prefix, size_t prefix_len,
 		FILE *fp, int terminator);
 
-/* give path as relative to prefix */
-extern char *path_relative(const char *in, int len,
-		struct strbuf *out, const char *prefix, int prefix_len);
 /* quote path as relative to the given prefix */
 extern char *quote_path_relative(const char *in, int len,
 			  struct strbuf *out, const char *prefix);
-- 
1.7.1.2.ga1f6e

[PATCH] setup: document prefix

From: Clemens Buchacher <hidden>
Date: 2016-06-15 22:48:55 

Signed-off-by: Clemens Buchacher <redacted>
---

On Fri, Jun 04, 2010 at 09:50:34AM +0200, Clemens Buchacher wrote:
By the way, I noticed that we rely on the fact that a non-empty
prefix ends with '/'. Is that ok?
I checked and the prefix does always end in a slash currently, but
this guarantee is well hidden in the code. I would feel more at
ease to continue relying on this with the comment below.

Regards,
Clemens

 setup.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/setup.c b/setup.c
index 5716d90..0e4cfe6 100644
--- a/setup.c
+++ b/setup.c
@@ -519,6 +519,12 @@ int check_repository_format(void)
 	return check_repository_format_gently(NULL);
 }
 
+/*
+ * Returns the "prefix", a path to the current working directory
+ * relative to the work tree root, or NULL, if the current working
+ * directory is not a strict subdirectory of the work tree root. The
+ * prefix always ends with a '/' character.
+ */
 const char *setup_git_directory(void)
 {
 	const char *retval = setup_git_directory_gently(NULL);
-- 
1.7.1.2.ga1f6e

Re: [PATCH v2] optimize path_relative()

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:48:55 

Clemens Buchacher [off-list ref] writes:

Avoid copying to strbuf in case a subset of the original string can
be returned.

Since the strbuf is no longer guaranteed to be updated, this
function is different from quote_path_relative(). To avoid
confusion, do not export it.

Signed-off-by: Clemens Buchacher <redacted>
The version of path_relative() after this patch looks much easier to
follow.

I noticed that there is a similar function write_name_quotedpfx() defined
in the same file, and wondered if we can do something similar to avoid the
whole allocation business.  But that would only be a microoptimize useful
for write_name_quoted_relative() and not for quote_path_relative() that
has a lot more callers, so it would probably not be worth it.

Thanks.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help