threads / patch / 19431

patchapply: handle filenames with double slashes better

Subject: [PATCH] apply: handle filenames with double slashes better

## tl;dr

5 messages between May 21, 2009 and May 25, 2009. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Michal Marek· May 21, 2009, 12:25 UTC · lore

Collapse double slashes to make patches like this work with --index or --cached:

git apply --index <<-EOF
	--- a/perl//Git.pm
	+++ b/perl//Git.pm
	@@ -1358,3 +1358,4 @@
	 1; # Famous last words
	+# test
EOF
Signed-off-by: Michal Marek <mmarek@suse.cz>
---
 builtin-apply.c |   26 ++++++++++++++++++++------
 1 files changed, 20 insertions(+), 6 deletions(-)
Show changes to builtin-apply.c +20 −6
diff --git a/builtin-apply.c b/builtin-apply.c
index 8a3771e..0c623e4 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -320,6 +320,20 @@ static int name_terminate(const char *name, int namelen, int c, int terminate)
 	return 1;
 }
 
+/* remove double slashes to make --index work with such filenames */
+static char *canon_name(char *name)
+{
+	int i = 0, j = 0;
+
+	while (name[i]) {
+		if ((name[j++] = name[i++]) == '/')
+			while (name[i] == '/')
+				i++;
+	}
+	name[j] = '\0';
+	return name;
+}
+
 static char *find_name(const char *line, char *def, int p_value, int terminate)
 {
 	int len;
@@ -349,7 +363,7 @@ static char *find_name(const char *line, char *def, int p_value, int terminate)
 				free(def);
 				if (root)
 					strbuf_insert(&name, 0, root, root_len);
-				return strbuf_detach(&name, NULL);
+				return canon_name(strbuf_detach(&name, NULL));
 			}
 		}
 		strbuf_release(&name);
@@ -369,10 +383,10 @@ static char *find_name(const char *line, char *def, int p_value, int terminate)
 			start = line;
 	}
 	if (!start)
-		return def;
+		return canon_name(def);
 	len = line - start;
 	if (!len)
-		return def;
+		return canon_name(def);
 
 	/*
 	 * Generally we prefer the shorter name, especially
@@ -383,7 +397,7 @@ static char *find_name(const char *line, char *def, int p_value, int terminate)
 	if (def) {
 		int deflen = strlen(def);
 		if (deflen < len && !strncmp(start, def, deflen))
-			return def;
+			return canon_name(def);
 		free(def);
 	}
 
@@ -392,10 +406,10 @@ static char *find_name(const char *line, char *def, int p_value, int terminate)
 		strcpy(ret, root);
 		memcpy(ret + root_len, start, len);
 		ret[root_len + len] = '\0';
-		return ret;
+		return canon_name(ret);
 	}
 
-	return xmemdupz(start, len);
+	return canon_name(xmemdupz(start, len));
 }
 
 static int count_slashes(const char *cp)

-- 
1.6.3
Junio C Hamano· May 21, 2009, 14:56 UTC · re: Michal Marek · lore

Re: [PATCH] apply: handle filenames with double slashes better

Michal Marek <mmarek@suse.cz> writes:
Show 14 quoted lines
> Collapse double slashes to make patches like this work with --index or
> --cached:
>
> git apply --index <<-EOF
> 	--- a/perl//Git.pm
> 	+++ b/perl//Git.pm
> 	@@ -1358,3 +1358,4 @@
>
>
> 	 1; # Famous last words
> 	+# test
> EOF
>
> Signed-off-by: Michal Marek <mmarek@suse.cz>
Hmm, I do not know if this is a good change.

For duplicate slashes in paths, I do not think there is any other sensible way to handle them other than squashing them together, but naming the function to do so "canon_name()" would tempt people to add other not-so-clearly-sensible "canonicalization" such as turning "./a" to "a" (which we shouldn't --- we should treat "./" as one level so that we keep behaving in a similar way as "patch -p1" does) or "a/../b" to "b".

Also calling this in find_name() loses information too early in the processing; how bad would it look if you move the callsite of this duplicate slash squashing down the callchain where the names are actually used?

Michal Marek· May 21, 2009, 19:12 UTC · re: Junio C Hamano · lore

Re: [PATCH] apply: handle filenames with double slashes better

On Thu, May 21, 2009 at 07:56:10AM -0700, Junio C Hamano wrote:
Show 25 quoted lines
> Michal Marek <mmarek@suse.cz> writes:
> 
> > Collapse double slashes to make patches like this work with --index or
> > --cached:
> >
> > git apply --index <<-EOF
> > 	--- a/perl//Git.pm
> > 	+++ b/perl//Git.pm
> > 	@@ -1358,3 +1358,4 @@
> >
> >
> > 	 1; # Famous last words
> > 	+# test
> > EOF
> >
> > Signed-off-by: Michal Marek <mmarek@suse.cz>
> 
> Hmm, I do not know if this is a good change.
> 
> For duplicate slashes in paths, I do not think there is any other sensible
> way to handle them other than squashing them together, but naming the
> function to do so "canon_name()" would tempt people to add other
> not-so-clearly-sensible "canonicalization" such as turning "./a" to "a"
> (which we shouldn't --- we should treat "./" as one level so that we keep
> behaving in a similar way as "patch -p1" does) or "a/../b" to "b".
OK, renamed to squash_slash().
> Also calling this in find_name() loses information too early in the
> processing; how bad would it look if you move the callsite of this
> duplicate slash squashing down the callchain where the names are actually
> used?

I tried this, but I'm not sure it's better now (and there might be some inconsistencies left). IMHO removing the double slashes is an operation similar to the unquoting done in find_name(), i.e. converting the text in the patch to something that can be passed to lstat() or cache_name_lookup(). Anyway, new patch attached.

Michal
Show changes to builtin-apply.c +20 −3
diff --git a/builtin-apply.c b/builtin-apply.c
index 8a3771e..5a35d9f 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -320,6 +320,22 @@ static int name_terminate(const char *name, int namelen, int c, int terminate)
 	return 1;
 }
 
+/* remove double slashes to make --index work with such filenames */
+static char *squash_slash(char *name)
+{
+	int i = 0, j = 0;
+
+	if (!name)
+		return name;
+	while (name[i]) {
+		if ((name[j++] = name[i++]) == '/')
+			while (name[i] == '/')
+				i++;
+	}
+	name[j] = '\0';
+	return name;
+}
+
 static char *find_name(const char *line, char *def, int p_value, int terminate)
 {
 	int len;
@@ -423,6 +439,7 @@ static int guess_p_value(const char *nameline)
 	name = find_name(nameline, NULL, 0, TERM_SPACE | TERM_TAB);
 	if (!name)
 		return -1;
+	name = squash_slash(name);
 	cp = strchr(name, '/');
 	if (!cp)
 		val = 0;
@@ -2416,7 +2433,7 @@ static int verify_index_match(struct cache_entry *ce, struct stat *st)
 
 static int check_preimage(struct patch *patch, struct cache_entry **ce, struct stat *st)
 {
-	const char *old_name = patch->old_name;
+	const char *old_name = squash_slash(patch->old_name);
 	struct patch *tpatch = NULL;
 	int stat_ret = 0;
 	unsigned st_mode = 0;
@@ -2503,8 +2520,8 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
 static int check_patch(struct patch *patch)
 {
 	struct stat st;
-	const char *old_name = patch->old_name;
-	const char *new_name = patch->new_name;
+	const char *old_name = squash_slash(patch->old_name);
+	const char *new_name = squash_slash(patch->new_name);
 	const char *name = old_name ? old_name : new_name;
 	struct cache_entry *ce = NULL;
 	struct patch *tpatch;

-- 
1.6.3
Junio C Hamano· May 21, 2009, 19:22 UTC · re: Michal Marek · lore

Re: [PATCH] apply: handle filenames with double slashes better

Michal Marek <mmarek@suse.cz> writes:
Show 5 quoted lines
> I tried this, but I'm not sure it's better now (and there might be some
> inconsistencies left). IMHO removing the double slashes is an operation
> similar to the unquoting done in find_name(), i.e. converting the text
> in the patch to something that can be passed to lstat() or
> cache_name_lookup().
Ok, you convinced me.

Let's take your first patch with just the s/canon_name/squash_slash/ change.

Thanks.
Michal Marek· May 25, 2009, 09:11 UTC · re: Junio C Hamano · lore

Re: [PATCH] apply: handle filenames with double slashes better

On Thu, May 21, 2009 at 12:22:16PM -0700, Junio C Hamano wrote:
> Let's take your first patch with just the s/canon_name/squash_slash/
> change.
Sorry for the delay, Here's hopefully the final patch:

Collapse double slashes to make patches like this work with --index or --cached:

git apply --index <<-EOF
	--- a/perl//Git.pm
	+++ b/perl//Git.pm
	@@ -1358,3 +1358,4 @@
	 1; # Famous last words
	+# test
EOF
Signed-off-by: Michal Marek <mmarek@suse.cz>
---
 builtin-apply.c |   28 ++++++++++++++++++++++------
 1 files changed, 22 insertions(+), 6 deletions(-)
Show changes to builtin-apply.c +22 −6
diff --git a/builtin-apply.c b/builtin-apply.c
index 8a3771e..10fefa0 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -320,6 +320,22 @@ static int name_terminate(const char *name, int namelen, int c, int terminate)
 	return 1;
 }
 
+/* remove double slashes to make --index work with such filenames */
+static char *squash_slash(char *name)
+{
+	int i = 0, j = 0;
+
+	if (!name)
+		return name;
+	while (name[i]) {
+		if ((name[j++] = name[i++]) == '/')
+			while (name[i] == '/')
+				i++;
+	}
+	name[j] = '\0';
+	return name;
+}
+
 static char *find_name(const char *line, char *def, int p_value, int terminate)
 {
 	int len;
@@ -349,7 +365,7 @@ static char *find_name(const char *line, char *def, int p_value, int terminate)
 				free(def);
 				if (root)
 					strbuf_insert(&name, 0, root, root_len);
-				return strbuf_detach(&name, NULL);
+				return squash_slash(strbuf_detach(&name, NULL));
 			}
 		}
 		strbuf_release(&name);
@@ -369,10 +385,10 @@ static char *find_name(const char *line, char *def, int p_value, int terminate)
 			start = line;
 	}
 	if (!start)
-		return def;
+		return squash_slash(def);
 	len = line - start;
 	if (!len)
-		return def;
+		return squash_slash(def);
 
 	/*
 	 * Generally we prefer the shorter name, especially
@@ -383,7 +399,7 @@ static char *find_name(const char *line, char *def, int p_value, int terminate)
 	if (def) {
 		int deflen = strlen(def);
 		if (deflen < len && !strncmp(start, def, deflen))
-			return def;
+			return squash_slash(def);
 		free(def);
 	}
 
@@ -392,10 +408,10 @@ static char *find_name(const char *line, char *def, int p_value, int terminate)
 		strcpy(ret, root);
 		memcpy(ret + root_len, start, len);
 		ret[root_len + len] = '\0';
-		return ret;
+		return squash_slash(ret);
 	}
 
-	return xmemdupz(start, len);
+	return squash_slash(xmemdupz(start, len));
 }
 
 static int count_slashes(const char *cp)

-- 
1.6.3

← back to recent threads