Re: [PATCH v9 1/9] git-clean: refactor git-clean into two phases

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

Re: [PATCH v9 1/9] git-clean: refactor git-clean into two phases

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

Jiang Xin [off-list ref] writes:
+/*
+ * Give path as relative to prefix.
+ *
+ * This function is a combination of path_relative (in quote.c) and
+ * relative_path (in path.c)
+ */
+static const char *path_relative(const char *in, const char *prefix)
+{
+...
Hmph.  Is it possible to reuse the public one (in path.c) here and
in quote.c, perhaps after enhancing it a bit to serve needs of the
callers of two existing ones and the new callers of this one?
quoted hunk
@@ -242,11 +287,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 				continue; /* Yup, this one exists unmerged */
 		}
 
-		/*
-		 * we might have removed this as part of earlier
-		 * recursive directory removal, so lstat() here could
-		 * fail with ENOENT.
-		 */
 		if (lstat(ent->name, &st))
 			continue;
I am guessing that the reason why you removed the comment is because
during this phase there is no way we "might have removed".  But if
that is the case, does it still make sense to run lstat() and ignore
errors from the call?

Re: [PATCH v9 1/9] git-clean: refactor git-clean into two phases

From: Jiang Xin <hidden>
Date: 2016-06-15 22:57:15

2013/5/15 Junio C Hamano [off-list ref]:
quoted
@@ -242,11 +287,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
                              continue; /* Yup, this one exists unmerged */
              }

-             /*
-              * we might have removed this as part of earlier
-              * recursive directory removal, so lstat() here could
-              * fail with ENOENT.
-              */
              if (lstat(ent->name, &st))
                      continue;
I am guessing that the reason why you removed the comment is because
during this phase there is no way we "might have removed".  But if
that is the case, does it still make sense to run lstat() and ignore
errors from the call?
Run lstat() here is necessary, because we need to check whether
ent->name points to a file or a directory. If ent points to a directory,
only add to del_list when user provides '-x' option to git-clean.

                if (S_ISDIR(st.st_mode)) {
                        if (remove_directories || (matches ==
MATCHED_EXACTLY)) {
                                rel = path_relative(ent->name, prefix);
                                string_list_append(&del_list, rel);
                        }
                } ...


When start to do cleaning, there is a duplicate lstat() call, and the removed
comments above are moved here.

                /*
                 * we might have removed this as part of earlier
                 * recursive directory removal, so lstat() here could
                 * fail with ENOENT.
                 */
                if (lstat(abs_path.buf, &st))
                        continue;

                if (S_ISDIR(st.st_mode)) {
                        if (remove_dirs(&abs_path, prefix, rm_flags,
dry_run, quiet, &gone))
                                errors++;
                        if (gone && !quiet) {
                                qname =
quote_path_relative(item->string, -1, &buf, NULL);
                                printf(dry_run ? _(msg_would_remove) :
_(msg_remove), qname);
                        }
                }

But I am not clear how "earlier recursive directory removal" could
make lstat() failure here. If it is not the case, maybe we can test a
directory by ent->name (ends with '/' or '\\' ?).

-- 
Jiang Xin

[RFC 0/2] refactor relative_path in path.c

From: Jiang Xin <hidden>
Date: 2016-06-15 22:57:16

2013/5/15 Junio C Hamano [off-list ref]:
Jiang Xin [off-list ref] writes:
quoted
+/*
+ * Give path as relative to prefix.
+ *
+ * This function is a combination of path_relative (in quote.c) and
+ * relative_path (in path.c)
+ */
+static const char *path_relative(const char *in, const char *prefix)
+{
+...
Hmph.  Is it possible to reuse the public one (in path.c) here and
in quote.c, perhaps after enhancing it a bit to serve needs of the
callers of two existing ones and the new callers of this one?
These two patches enhance relative_path() in path.c, so that function
relative_path() will return real relative path, not a path strip off
the prefix.

The 2nd patch is a bit aggressive, it refactor all related functions,
remove unnecessary arguments: len and/or prefix_len.

Please review them. They will be prerequisites for the interactive
git-clean patch series.

Jiang Xin (2):
  path.c: refactor relative_path(), not only strip prefix
  quote.c: remove path_relative, use relative_path instead

 builtin/clean.c    | 18 +++++------
 builtin/grep.c     |  4 +--
 builtin/ls-files.c | 13 ++++----
 path.c             | 94 ++++++++++++++++++++++++++++++++++++++++++------------
 quote.c            | 71 +++--------------------------------------
 quote.h            |  7 ++--
 wt-status.c        | 17 +++++-----
 7 files changed, 107 insertions(+), 117 deletions(-)

-- 
1.8.3.rc1.404.ga32c147

[RFC 1/2] path.c: refactor relative_path(), not only strip prefix

From: Jiang Xin <hidden>
Date: 2016-06-15 22:57:16

Original design of relative_path() is simple, just strip the prefix
(*base) from the abosolute path (*abs). In most cases, we need a real
relative path, such as: ../foo, ../../bar. That's why there is another
reimplementation (path_relative()) in quote.c.

Refactor relative_path() in path.c to return real relative path, so
that user can reuse this function without reimplement his/her own.
I will use this method for interactive git-clean later. Some of the
implementations are borrowed from path_relative() in quote.c.

Different results for relative_path() before and after this refactor:

    base path  abs path  relative (orignal)  relative (refactor)
    =========  ========  ==================  ===================
    /a/b       /a/b/c/   c/                  c/
    //a///b/   /a/b//c/  c/                  c/
    /a/b       /a/b/     .                   ./
    /a/b/      /a        /a                  ../
    /a/b/      /         /                   ../../
    /a/b/      /a/c      /a/c                ../c

Signed-off-by: Jiang Xin <redacted>
---
 path.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 74 insertions(+), 20 deletions(-)
diff --git a/path.c b/path.c
index 04ff..4dafa8 100644
--- a/path.c
+++ b/path.c
@@ -444,38 +444,92 @@ int adjust_shared_perm(const char *path)
 const char *relative_path(const char *abs, const char *base)
 {
 	static char buf[PATH_MAX + 1];
-	int i = 0, j = 0;
+	int abs_off, base_off, i, j;
+	int abs_len, base_len;
 
 	if (!base || !base[0])
 		return abs;
-	while (base[i]) {
+
+	abs_len = strlen(abs);
+	base_len = strlen(base);
+
+	abs_off = 0;
+	base_off = 0;
+	i = 0;
+	j = 0;
+	while (i < base_len && j < abs_len && base[i] == abs[j]) {
 		if (is_dir_sep(base[i])) {
-			if (!is_dir_sep(abs[j]))
-				return abs;
 			while (is_dir_sep(base[i]))
 				i++;
 			while (is_dir_sep(abs[j]))
 				j++;
-			continue;
-		} else if (abs[j] != base[i]) {
+			base_off = i;
+			abs_off = j;
+		} else {
+			i++;
+			j++;
+		}
+	}
+	if (
+	    /* base seems like prefix of abs */
+	    i >= base_len &&
+	    /*
+	     * but "/foo" is not a prefix of "/foobar"
+	     * (i.e. base not end with '/')
+	     */
+	    base_off < base_len) {
+		if (j >= abs_len) {
+			/* abs="/a/b", base="/a/b" */
+			abs_off = abs_len;
+		} else if (is_dir_sep(abs[j])) {
+			/* abs="/a/b/c", base="/a/b" */
+			while (is_dir_sep(abs[j]))
+				j++;
+			abs_off = j;
+		} else {
+			/* abs="/a/bbb/c", base="/a/b" */
+			i = base_off;
+		}
+	} else if (
+		   /* abs is short than base (prefix of base) */
+		   j >= abs_len &&
+		   /* abs not end with '/' */
+		   abs_off < abs_len) {
+		if (is_dir_sep(base[i])) {
+			/* abs="/a/b", base="/a/b/c/" */
+			while (is_dir_sep(base[i]))
+				i++;
+			abs_off = abs_len;
+		}
+	}
+	abs += abs_off;
+	abs_len -= abs_off;
+
+	/* base is prefix of abs */
+	if (i >= base_len) {
+		if (*abs == '\0') {
+			strcpy(buf, "./");
+			return buf;
+		} else {
 			return abs;
 		}
+	}
+
+	buf[0] = '\0';
+	while (i < base_len) {
+		if (is_dir_sep(base[i])) {
+			strcat(buf, "../");
+			while (is_dir_sep(base[i]))
+				i++;
+			continue;
+		}
 		i++;
-		j++;
 	}
-	if (
-	    /* "/foo" is a prefix of "/foo" */
-	    abs[j] &&
-	    /* "/foo" is not a prefix of "/foobar" */
-	    !is_dir_sep(base[i-1]) && !is_dir_sep(abs[j])
-	   )
-		return abs;
-	while (is_dir_sep(abs[j]))
-		j++;
-	if (!abs[j])
-		strcpy(buf, ".");
-	else
-		strcpy(buf, abs + j);
+	if (!is_dir_sep(base[base_len - 1]))
+		strcat(buf, "../");
+
+	strcat(buf, abs);
+
 	return buf;
 }
 
-- 
1.8.3.rc1.404.ga32c147

[RFC 2/2] quote.c: remove path_relative, use relative_path instead

From: Jiang Xin <hidden>
Date: 2016-06-15 22:57:16

Since there is a enhanced version of relative_path() in path.c,
remove duplicate counterpart path_relative() in quote.c.

Also refactor related functions, remove unnecessary arguments:
len and/or prefix_len.

Signed-off-by: Jiang Xin <redacted>
---
 builtin/clean.c    | 18 +++++++-------
 builtin/grep.c     |  4 +--
 builtin/ls-files.c | 13 +++++-----
 quote.c            | 71 ++++--------------------------------------------------
 quote.h            |  7 +++---
 wt-status.c        | 17 ++++++-------
 6 files changed, 33 insertions(+), 97 deletions(-)
diff --git a/builtin/clean.c b/builtin/clean.c
index 04e39..a93c3 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -56,7 +56,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 	if ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) &&
 			!resolve_gitlink_ref(path->buf, "HEAD", submodule_head)) {
 		if (!quiet) {
-			quote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);
+			quote_path_relative(path->buf, &quoted, prefix);
 			printf(dry_run ?  _(msg_would_skip_git_dir) : _(msg_skip_git_dir),
 					quoted.buf);
 		}
@@ -70,7 +70,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 		/* an empty dir could be removed even if it is unreadble */
 		res = dry_run ? 0 : rmdir(path->buf);
 		if (res) {
-			quote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);
+			quote_path_relative(path->buf, &quoted, prefix);
 			warning(_(msg_warn_remove_failed), quoted.buf);
 			*dir_gone = 0;
 		}
@@ -94,7 +94,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 			if (remove_dirs(path, prefix, force_flag, dry_run, quiet, &gone))
 				ret = 1;
 			if (gone) {
-				quote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);
+				quote_path_relative(path->buf, &quoted, prefix);
 				string_list_append(&dels, quoted.buf);
 			} else
 				*dir_gone = 0;
@@ -102,10 +102,10 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 		} else {
 			res = dry_run ? 0 : unlink(path->buf);
 			if (!res) {
-				quote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);
+				quote_path_relative(path->buf, &quoted, prefix);
 				string_list_append(&dels, quoted.buf);
 			} else {
-				quote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);
+				quote_path_relative(path->buf, &quoted, prefix);
 				warning(_(msg_warn_remove_failed), quoted.buf);
 				*dir_gone = 0;
 				ret = 1;
@@ -127,7 +127,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 		if (!res)
 			*dir_gone = 1;
 		else {
-			quote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);
+			quote_path_relative(path->buf, &quoted, prefix);
 			warning(_(msg_warn_remove_failed), quoted.buf);
 			*dir_gone = 0;
 			ret = 1;
@@ -262,7 +262,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 				if (remove_dirs(&directory, prefix, rm_flags, dry_run, quiet, &gone))
 					errors++;
 				if (gone && !quiet) {
-					qname = quote_path_relative(directory.buf, directory.len, &buf, prefix);
+					qname = quote_path_relative(directory.buf, &buf, prefix);
 					printf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);
 				}
 			}
@@ -272,11 +272,11 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 				continue;
 			res = dry_run ? 0 : unlink(ent->name);
 			if (res) {
-				qname = quote_path_relative(ent->name, -1, &buf, prefix);
+				qname = quote_path_relative(ent->name, &buf, prefix);
 				warning(_(msg_warn_remove_failed), qname);
 				errors++;
 			} else if (!quiet) {
-				qname = quote_path_relative(ent->name, -1, &buf, prefix);
+				qname = quote_path_relative(ent->name, &buf, prefix);
 				printf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);
 			}
 		}
diff --git a/builtin/grep.c b/builtin/grep.c
index 159e65..b5222 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -286,7 +286,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,
 	struct strbuf pathbuf = STRBUF_INIT;
 
 	if (opt->relative && opt->prefix_length) {
-		quote_path_relative(filename + tree_name_len, -1, &pathbuf,
+		quote_path_relative(filename + tree_name_len, &pathbuf,
 				    opt->prefix);
 		strbuf_insert(&pathbuf, 0, filename, tree_name_len);
 	} else {
@@ -318,7 +318,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)
 	struct strbuf buf = STRBUF_INIT;
 
 	if (opt->relative && opt->prefix_length)
-		quote_path_relative(filename, -1, &buf, opt->prefix);
+		quote_path_relative(filename, &buf, opt->prefix);
 	else
 		strbuf_addstr(&buf, filename);
 
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 220207..bb563 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -46,10 +46,9 @@ static const char *tag_modified = "";
 static const char *tag_skip_worktree = "";
 static const char *tag_resolve_undo = "";
 
-static void write_name(const char* name, size_t len)
+static void write_name(const char* name)
 {
-	write_name_quoted_relative(name, len, prefix, prefix_len, stdout,
-			line_terminator);
+	write_name_quoted_relative(name, prefix, stdout, line_terminator);
 }
 
 static void show_dir_entry(const char *tag, struct dir_entry *ent)
@@ -63,7 +62,7 @@ static void show_dir_entry(const char *tag, struct dir_entry *ent)
 		return;
 
 	fputs(tag, stdout);
-	write_name(ent->name, ent->len);
+	write_name(ent->name);
 }
 
 static void show_other_files(struct dir_struct *dir)
@@ -163,7 +162,7 @@ static void show_ce_entry(const char *tag, struct cache_entry *ce)
 		       find_unique_abbrev(ce->sha1,abbrev),
 		       ce_stage(ce));
 	}
-	write_name(ce->name, ce_namelen(ce));
+	write_name(ce->name);
 	if (debug_mode) {
 		printf("  ctime: %d:%d\n", ce->ce_ctime.sec, ce->ce_ctime.nsec);
 		printf("  mtime: %d:%d\n", ce->ce_mtime.sec, ce->ce_mtime.nsec);
@@ -196,7 +195,7 @@ static void show_ru_info(void)
 			printf("%s%06o %s %d\t", tag_resolve_undo, ui->mode[i],
 			       find_unique_abbrev(ui->sha1[i], abbrev),
 			       i + 1);
-			write_name(path, len);
+			write_name(path);
 		}
 	}
 }
@@ -389,7 +388,7 @@ int report_path_error(const char *ps_matched, const char **pathspec, const char
 		if (found_dup)
 			continue;
 
-		name = quote_path_relative(pathspec[num], -1, &sb, prefix);
+		name = quote_path_relative(pathspec[num], &sb, prefix);
 		error("pathspec '%s' did not match any file(s) known to git.",
 		      name);
 		errors++;
diff --git a/quote.c b/quote.c
index 91122..97c57 100644
--- a/quote.c
+++ b/quote.c
@@ -312,81 +312,20 @@ 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,
+void write_name_quoted_relative(const char *name, const char *prefix,
 				FILE *fp, int terminator)
 {
-	struct strbuf sb = STRBUF_INIT;
-
-	name = path_relative(name, len, &sb, prefix, prefix_len);
+	name = relative_path(name, prefix);
 	write_name_quoted(name, fp, terminator);
-
-	strbuf_release(&sb);
-}
-
-/*
- * 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_len < 0) {
-		if (prefix)
-			prefix_len = strlen(prefix);
-		else
-			prefix_len = 0;
-	}
-
-	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;
-
-	strbuf_reset(sb);
-	strbuf_grow(sb, len);
-
-	while (i < prefix_len) {
-		if (prefix[i] == '/')
-			strbuf_addstr(sb, "../");
-		i++;
-	}
-	strbuf_add(sb, in, len);
-
-	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 *quote_path_relative(const char *in, struct strbuf *out,
+			  const char *prefix)
 {
-	struct strbuf sb = STRBUF_INIT;
-	const char *rel = path_relative(in, len, &sb, prefix, -1);
+	const char *rel = relative_path(in, prefix);
 	strbuf_reset(out);
 	quote_c_style_counted(rel, strlen(rel), out, NULL, 0);
-	strbuf_release(&sb);
-
-	if (!out->len)
-		strbuf_addstr(out, "./");
 
 	return out->buf;
 }
diff --git a/quote.h b/quote.h
index 13315..27022 100644
--- a/quote.h
+++ b/quote.h
@@ -60,12 +60,11 @@ extern void quote_two_c_style(struct strbuf *, const char *, const char *, int);
 extern void write_name_quoted(const char *name, FILE *, int terminator);
 extern void write_name_quotedpfx(const char *pfx, size_t pfxlen,
                                  const char *name, FILE *, int terminator);
-extern void write_name_quoted_relative(const char *name, size_t len,
-		const char *prefix, size_t prefix_len,
-		FILE *fp, int terminator);
+extern void write_name_quoted_relative(const char *name, const char *prefix,
+				       FILE *fp, int terminator);
 
 /* quote path as relative to the given prefix */
-extern char *quote_path_relative(const char *in, int len,
+extern char *quote_path_relative(const char *in,
 			  struct strbuf *out, const char *prefix);
 
 /* quoting as a string literal for other languages */
diff --git a/wt-status.c b/wt-status.c
index bf84a..002ac 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -243,7 +243,7 @@ static void wt_status_print_unmerged_data(struct wt_status *s,
 	struct strbuf onebuf = STRBUF_INIT;
 	const char *one, *how = _("bug");
 
-	one = quote_path(it->string, -1, &onebuf, s->prefix);
+	one = quote_path(it->string, &onebuf, s->prefix);
 	status_printf(s, color(WT_STATUS_HEADER, s), "\t");
 	switch (d->stagemask) {
 	case 1: how = _("both deleted:"); break;
@@ -297,8 +297,8 @@ static void wt_status_print_change_data(struct wt_status *s,
 		    change_type);
 	}
 
-	one = quote_path(one_name, -1, &onebuf, s->prefix);
-	two = quote_path(two_name, -1, &twobuf, s->prefix);
+	one = quote_path(one_name, &onebuf, s->prefix);
+	two = quote_path(two_name, &twobuf, s->prefix);
 
 	status_printf(s, color(WT_STATUS_HEADER, s), "\t");
 	switch (status) {
@@ -706,8 +706,7 @@ static void wt_status_print_other(struct wt_status *s,
 		struct string_list_item *it;
 		const char *path;
 		it = &(l->items[i]);
-		path = quote_path(it->string, strlen(it->string),
-				  &buf, s->prefix);
+		path = quote_path(it->string, &buf, s->prefix);
 		if (column_active(s->colopts)) {
 			string_list_append(&output, path);
 			continue;
@@ -1289,7 +1288,7 @@ static void wt_shortstatus_unmerged(struct string_list_item *it,
 	} else {
 		struct strbuf onebuf = STRBUF_INIT;
 		const char *one;
-		one = quote_path(it->string, -1, &onebuf, s->prefix);
+		one = quote_path(it->string, &onebuf, s->prefix);
 		printf(" %s\n", one);
 		strbuf_release(&onebuf);
 	}
@@ -1317,7 +1316,7 @@ static void wt_shortstatus_status(struct string_list_item *it,
 		struct strbuf onebuf = STRBUF_INIT;
 		const char *one;
 		if (d->head_path) {
-			one = quote_path(d->head_path, -1, &onebuf, s->prefix);
+			one = quote_path(d->head_path, &onebuf, s->prefix);
 			if (*one != '"' && strchr(one, ' ') != NULL) {
 				putchar('"');
 				strbuf_addch(&onebuf, '"');
@@ -1326,7 +1325,7 @@ static void wt_shortstatus_status(struct string_list_item *it,
 			printf("%s -> ", one);
 			strbuf_release(&onebuf);
 		}
-		one = quote_path(it->string, -1, &onebuf, s->prefix);
+		one = quote_path(it->string, &onebuf, s->prefix);
 		if (*one != '"' && strchr(one, ' ') != NULL) {
 			putchar('"');
 			strbuf_addch(&onebuf, '"');
@@ -1345,7 +1344,7 @@ static void wt_shortstatus_other(struct string_list_item *it,
 	} else {
 		struct strbuf onebuf = STRBUF_INIT;
 		const char *one;
-		one = quote_path(it->string, -1, &onebuf, s->prefix);
+		one = quote_path(it->string, &onebuf, s->prefix);
 		color_fprintf(s->fp, color(WT_STATUS_UNTRACKED, s), "%s", sign);
 		printf(" %s\n", one);
 		strbuf_release(&onebuf);
-- 
1.8.3.rc1.404.ga32c147
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help