Re: [PATCH v2] commit: allow partial commits with relative paths

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

Re: [PATCH v2] commit: allow partial commits with relative paths

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:51:43

Clemens Buchacher [off-list ref] writes:

quoted hunk

diff --git a/setup.c b/setup.c
index 5ea5502..2c51a9a 100644
--- a/setup.c
+++ b/setup.c

@@ -264,6 +264,38 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
 	return pathspec;
 }
 
+const char *pathspec_prefix(const char *prefix, const char **pathspec)
+{

As a public function, this sorely needs a comment that describes what it
does. More importantly, when I tried to come up with an example
description, it became very clear that this neither prefixes anything to
pathspec, nor prefixes pathspec to anything else.

As an internal helper in ls-files implementation it was perfectly
fine that the function was slightly misnamed, but if you are making it
into a public API, we should get its name right.

Perhaps "common_prefix()"?

Don't you also want to consolidate dir.c:common_prefix() with this?

What happens when pathspec has the recently introduced magic elements,
e.g. ':/' that widens the match to the whole tree?

Re: [PATCH v2] commit: allow partial commits with relative paths

From: Clemens Buchacher <hidden>
Date: 2016-06-15 22:51:44

Hi Junio,

On Tue, Aug 02, 2011 at 02:31:47PM -0700, Junio C Hamano wrote:

Perhaps "common_prefix()"?

Yes, I was thinking the same thing actually.

Don't you also want to consolidate dir.c:common_prefix() with this?

I wasn't aware of it. I'm really swamped right now, but I'll take a
look at it soon.

Clemens

[PATCH 1/3] remove prefix argument from pathspec_prefix

From: Clemens Buchacher <hidden>
Date: 2016-06-15 22:51:58

Passing a prefix to a function that is supposed to find the prefix
is strange. And it's really only used if the pathspec is NULL. Make
the callers handle this case instead.

Signed-off-by: Clemens Buchacher <redacted>
---
 builtin/commit.c   |    5 +++--
 builtin/ls-files.c |    2 +-
 cache.h            |    2 +-
 setup.c            |    4 ++--
 4 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index cbc9613..64fe501 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -255,8 +255,9 @@ static int list_paths(struct string_list *list, const char *with_tree,
 	m = xcalloc(1, i);
 
 	if (with_tree) {
-		const char *max_prefix = pathspec_prefix(prefix, pattern);
-		overlay_tree_on_cache(with_tree, max_prefix);
+		char *max_prefix = pathspec_prefix(pattern);
+		overlay_tree_on_cache(with_tree, max_prefix ? max_prefix : prefix);
+		free(max_prefix);
 	}
 
 	for (i = 0; i < active_nr; i++) {

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index e8a800d..a54c2a2 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c

@@ -545,7 +545,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		strip_trailing_slash_from_submodules();
 
 	/* Find common prefix for all pathspec's */
-	max_prefix = pathspec_prefix(prefix, pathspec);
+	max_prefix = pathspec_prefix(pathspec);
 	max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
 
 	/* Treat unmatching pathspec elements as errors */

diff --git a/cache.h b/cache.h
index 607c2ea..0ccc84d 100644
--- a/cache.h
+++ b/cache.h

@@ -444,7 +444,7 @@ extern void set_git_work_tree(const char *tree);
 #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
 
 extern const char **get_pathspec(const char *prefix, const char **pathspec);
-extern const char *pathspec_prefix(const char *prefix, const char **pathspec);
+extern char *pathspec_prefix(const char **pathspec);
 extern void setup_work_tree(void);
 extern const char *setup_git_directory_gently(int *);
 extern const char *setup_git_directory(void);

diff --git a/setup.c b/setup.c
index 27c1d47..0906790 100644
--- a/setup.c
+++ b/setup.c

@@ -236,13 +236,13 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
 	return pathspec;
 }
 
-const char *pathspec_prefix(const char *prefix, const char **pathspec)
+char *pathspec_prefix(const char **pathspec)
 {
 	const char **p, *n, *prev;
 	unsigned long max;
 
 	if (!pathspec)
-		return prefix ? xmemdupz(prefix, strlen(prefix)) : NULL;
+		return NULL;
 
 	prev = NULL;
 	max = PATH_MAX;

-- 
1.7.6.1

renaming pathspec_prefix (was: Re: [PATCH v2] commit: allow partial commits with relative paths)

From: Clemens Buchacher <hidden>
Date: 2016-06-15 22:51:58

On Tue, Aug 02, 2011 at 02:31:47PM -0700, Junio C Hamano wrote:

Clemens Buchacher [off-list ref] writes:

quoted

diff --git a/setup.c b/setup.c
index 5ea5502..2c51a9a 100644
--- a/setup.c
+++ b/setup.c

@@ -264,6 +264,38 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
 	return pathspec;
 }
 
+const char *pathspec_prefix(const char *prefix, const char **pathspec)
+{

As a public function, this sorely needs a comment that describes what it
does. More importantly, when I tried to come up with an example
description, it became very clear that this neither prefixes anything to
pathspec, nor prefixes pathspec to anything else.

As an internal helper in ls-files implementation it was perfectly
fine that the function was slightly misnamed, but if you are making it
into a public API, we should get its name right.

Perhaps "common_prefix()"?

Don't you also want to consolidate dir.c:common_prefix() with this?

Yes. This should do it:

[PATCH 1/3] remove prefix argument from pathspec_prefix
[PATCH 2/3] consolidate pathspec_prefix and common_prefix
[PATCH 3/3] rename pathspec_prefix -> common_prefix and move to

What happens when pathspec has the recently introduced magic elements,
e.g. ':/' that widens the match to the whole tree?

If I understand correctly that is resolved in get_pathspec. And
pathspec_prefix (now common_prefix) is called later.

Clemens

[PATCH 2/3] consolidate pathspec_prefix and common_prefix

From: Clemens Buchacher <hidden>
Date: 2016-06-15 22:51:58

The implementation from pathspec_prefix (slightly modified)
replaces the current common_prefix, because it also respects glob
characters.

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

I wonder if PATH_MAX is needed and respected everywhere. Do we have
any previous experience with very long paths?

Clemens

 dir.c   |   52 ++++++++++++++++++++++++++--------------------------
 dir.h   |    1 +
 setup.c |   29 ++---------------------------
 3 files changed, 29 insertions(+), 53 deletions(-)

diff --git a/dir.c b/dir.c
index 08281d2..7099fb3 100644
--- a/dir.c
+++ b/dir.c

@@ -34,37 +34,37 @@ int fnmatch_icase(const char *pattern, const char *string, int flags)
 	return fnmatch(pattern, string, flags | (ignore_case ? FNM_CASEFOLD : 0));
 }
 
-static int common_prefix(const char **pathspec)
+unsigned long common_prefix_len(const char **pathspec)
 {
-	const char *path, *slash, *next;
-	int prefix;
+	const char *n, *first;
+	unsigned long max;
 
 	if (!pathspec)
 		return 0;
 
-	path = *pathspec;
-	slash = strrchr(path, '/');
-	if (!slash)
-		return 0;
-
-	/*
-	 * The first 'prefix' characters of 'path' are common leading
-	 * path components among the pathspecs we have seen so far,
-	 * including the trailing slash.
-	 */
-	prefix = slash - path + 1;
-	while ((next = *++pathspec) != NULL) {
-		int len, last_matching_slash = -1;
-		for (len = 0; len < prefix && next[len] == path[len]; len++)
-			if (next[len] == '/')
-				last_matching_slash = len;
-		if (len == prefix)
-			continue;
-		if (last_matching_slash < 0)
-			return 0;
-		prefix = last_matching_slash + 1;
+	first = *pathspec;
+	max = PATH_MAX;
+	while ((n = *pathspec++)) {
+		int i, len = 0;
+		for (i = 0; i < max; i++) {
+			char c = n[i];
+			if (!c || c != first[i] || is_glob_special(c))
+				break;
+			if (c == '/')
+				len = i+1;
+		}
+		if (len < max) {
+			max = len;
+			if (!max)
+				break;
+		}
 	}
-	return prefix;
+
+	/* Nothing in the first PATH_MAX characters? */
+	if (max > 0 && first[max-1] != '/')
+		max = 0;
+
+	return max;
 }
 
 int fill_directory(struct dir_struct *dir, const char **pathspec)

@@ -76,7 +76,7 @@ int fill_directory(struct dir_struct *dir, const char **pathspec)
 	 * Calculate common prefix for the pathspec, and
 	 * use that to optimize the directory walk
 	 */
-	len = common_prefix(pathspec);
+	len = common_prefix_len(pathspec);
 	path = "";
 
 	if (len)

diff --git a/dir.h b/dir.h
index 433b5b4..0e55b71 100644
--- a/dir.h
+++ b/dir.h

@@ -64,6 +64,7 @@ struct dir_struct {
 #define MATCHED_RECURSIVELY 1
 #define MATCHED_FNMATCH 2
 #define MATCHED_EXACTLY 3
+extern unsigned long common_prefix_len(const char **pathspec);
 extern int match_pathspec(const char **pathspec, const char *name, int namelen, int prefix, char *seen);
 extern int match_pathspec_depth(const struct pathspec *pathspec,
 				const char *name, int namelen,

diff --git a/setup.c b/setup.c
index 0906790..0c60dbd 100644
--- a/setup.c
+++ b/setup.c

@@ -238,34 +238,9 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
 
 char *pathspec_prefix(const char **pathspec)
 {
-	const char **p, *n, *prev;
-	unsigned long max;
+	unsigned long len = common_prefix_len(pathspec);
 
-	if (!pathspec)
-		return NULL;
-
-	prev = NULL;
-	max = PATH_MAX;
-	for (p = pathspec; (n = *p) != NULL; p++) {
-		int i, len = 0;
-		for (i = 0; i < max; i++) {
-			char c = n[i];
-			if (prev && prev[i] != c)
-				break;
-			if (!c || c == '*' || c == '?')
-				break;
-			if (c == '/')
-				len = i+1;
-		}
-		prev = n;
-		if (len < max) {
-			max = len;
-			if (!max)
-				break;
-		}
-	}
-
-	return max ? xmemdupz(prev, max) : NULL;
+	return len ? xmemdupz(prefix, len) : NULL;
 }
 
 /*

-- 
1.7.6.1

[PATCH 3/3] rename pathspec_prefix -> common_prefix and move to dir.[ch]

From: Clemens Buchacher <hidden>
Date: 2016-06-15 22:51:58

Signed-off-by: Clemens Buchacher <redacted>
---
 builtin/commit.c   |    2 +-
 builtin/ls-files.c |    2 +-
 cache.h            |    1 -
 dir.c              |   11 +++++++++++
 dir.h              |    1 +
 setup.c            |    7 -------
 6 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 64fe501..b9ab5ef 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -255,7 +255,7 @@ static int list_paths(struct string_list *list, const char *with_tree,
 	m = xcalloc(1, i);
 
 	if (with_tree) {
-		char *max_prefix = pathspec_prefix(pattern);
+		char *max_prefix = common_prefix(pattern);
 		overlay_tree_on_cache(with_tree, max_prefix ? max_prefix : prefix);
 		free(max_prefix);
 	}

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index a54c2a2..7cff175 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c

@@ -545,7 +545,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		strip_trailing_slash_from_submodules();
 
 	/* Find common prefix for all pathspec's */
-	max_prefix = pathspec_prefix(pathspec);
+	max_prefix = common_prefix(pathspec);
 	max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
 
 	/* Treat unmatching pathspec elements as errors */

diff --git a/cache.h b/cache.h
index 0ccc84d..586b987 100644
--- a/cache.h
+++ b/cache.h

@@ -444,7 +444,6 @@ extern void set_git_work_tree(const char *tree);
 #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
 
 extern const char **get_pathspec(const char *prefix, const char **pathspec);
-extern char *pathspec_prefix(const char **pathspec);
 extern void setup_work_tree(void);
 extern const char *setup_git_directory_gently(int *);
 extern const char *setup_git_directory(void);

diff --git a/dir.c b/dir.c
index 7099fb3..f606da9 100644
--- a/dir.c
+++ b/dir.c

@@ -67,6 +67,17 @@ unsigned long common_prefix_len(const char **pathspec)
 	return max;
 }
 
+/*
+ * Returns a copy of the longest leading path common among all
+ * pathspecs.
+ */
+char *common_prefix(const char **pathspec)
+{
+	unsigned long len = common_prefix_len(pathspec);
+
+	return len ? xmemdupz(*pathspec, len) : NULL;
+}
+
 int fill_directory(struct dir_struct *dir, const char **pathspec)
 {
 	const char *path;

diff --git a/dir.h b/dir.h
index 0e55b71..9a33d23 100644
--- a/dir.h
+++ b/dir.h

@@ -65,6 +65,7 @@ struct dir_struct {
 #define MATCHED_FNMATCH 2
 #define MATCHED_EXACTLY 3
 extern unsigned long common_prefix_len(const char **pathspec);
+extern char *common_prefix(const char **pathspec);
 extern int match_pathspec(const char **pathspec, const char *name, int namelen, int prefix, char *seen);
 extern int match_pathspec_depth(const struct pathspec *pathspec,
 				const char *name, int namelen,

diff --git a/setup.c b/setup.c
index 0c60dbd..52bbb70 100644
--- a/setup.c
+++ b/setup.c

@@ -236,13 +236,6 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
 	return pathspec;
 }
 
-char *pathspec_prefix(const char **pathspec)
-{
-	unsigned long len = common_prefix_len(pathspec);
-
-	return len ? xmemdupz(prefix, len) : NULL;
-}
-
 /*
  * Test if it looks like we're at a git directory.
  * We want to see:

-- 
1.7.6.1

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help