Re: [PATCH v9 1/9] git-clean: refactor git-clean into two phases
Jiang Xin worldhello@gmail.com writes: 2013/5/15 Junio C Hamano gits...@pobox.com: @@ -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. Sorry, but that was not the question; we can see st is used immediately below so somebody needs to fill it. I was pointing out that the lstat() is expected to fail with ENOENT but it is not an error worth reporting justification the original code had to silently ignore an error, because you no longer remove anything immediately in this part of the code. Is if () continue still valid thing to do here, not if () die()? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 1/9] git-clean: refactor git-clean into two phases
2013/5/15 Junio C Hamano gits...@pobox.com: Jiang Xin worldhello@gmail.com writes: 2013/5/15 Junio C Hamano gits...@pobox.com: @@ -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. Sorry, but that was not the question; we can see st is used immediately below so somebody needs to fill it. I was pointing out that the lstat() is expected to fail with ENOENT but it is not an error worth reporting justification the original code had to silently ignore an error, because you no longer remove anything immediately in this part of the code. Is if () continue still valid thing to do here, not if () die()? I'm clear, it could be: if (lstat(ent-name, st)) die_errno(Cannot lstat '%s', ent-name); -- Jiang Xin -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 1/9] git-clean: refactor git-clean into two phases
Before introducing interactive git-clean, refactor git-clean operations
into two phases:
* hold cleaning items in del_list,
* and remove them in a separate loop at the end.
We will introduce interactive git-clean between the two phases. The
interactive git-clean will show what would be done and must confirm
before do real cleaning.
Signed-off-by: Jiang Xin worldhello@gmail.com
---
builtin/clean.c | 103 ++--
1 file changed, 85 insertions(+), 18 deletions(-)
diff --git a/builtin/clean.c b/builtin/clean.c
index 04e39..ccd4 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -15,6 +15,7 @@
#include quote.h
static int force = -1; /* unset */
+static struct string_list del_list = STRING_LIST_INIT_DUP;
static const char *const builtin_clean_usage[] = {
N_(git clean [-d] [-f] [-n] [-q] [-e pattern] [-x | -X] [--]
paths...),
@@ -142,18 +143,61 @@ static int remove_dirs(struct strbuf *path, const char
*prefix, int force_flag,
return ret;
}
+/*
+ * 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)
+{
+ static char buf[PATH_MAX + 1];
+ int off, i;
+ int len, prefix_len;
+
+ len = strlen(in);
+ 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;
+
+ buf[0] = '\0';
+ while (i prefix_len) {
+ if (prefix[i] == '/')
+ strcat(buf, ../);
+ i++;
+ }
+ strcat(buf, in);
+
+ return buf;
+}
+
int cmd_clean(int argc, const char **argv, const char *prefix)
{
int i, res;
int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;
int ignored_only = 0, config_set = 0, errors = 0, gone = 1;
int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;
- struct strbuf directory = STRBUF_INIT;
+ struct strbuf abs_path = STRBUF_INIT;
struct dir_struct dir;
static const char **pathspec;
struct strbuf buf = STRBUF_INIT;
struct string_list exclude_list = STRING_LIST_INIT_NODUP;
struct exclude_list *el;
+ struct string_list_item *item;
const char *qname;
char *seen = NULL;
struct option options[] = {
@@ -223,6 +267,7 @@ int cmd_clean(int argc, const char **argv, const char
*prefix)
int matches = 0;
struct cache_entry *ce;
struct stat st;
+ const char *rel;
/*
* Remove the '/' at the end that directory
@@ -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;
@@ -257,33 +297,60 @@ int cmd_clean(int argc, const char **argv, const char
*prefix)
}
if (S_ISDIR(st.st_mode)) {
- strbuf_addstr(directory, ent-name);
if (remove_directories || (matches == MATCHED_EXACTLY))
{
- 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);
- printf(dry_run ? _(msg_would_remove) :
_(msg_remove), qname);
- }
+ rel = path_relative(ent-name, prefix);
+ string_list_append(del_list, rel);
}
- strbuf_reset(directory);
} else {
if (pathspec !matches)
continue;
- res = dry_run ? 0 : unlink(ent-name);
+ rel = path_relative(ent-name, prefix);
+ string_list_append(del_list, rel);
+ }
+ }
+
+ /* TODO: do interactive git-clean here, which will modify del_list */
+
+ for_each_string_list_item(item, del_list) {
+ struct stat st;
+
+ if (prefix) {
+
Re: [PATCH v9 1/9] git-clean: refactor git-clean into two phases
2013/5/15 Junio C Hamano gits...@pobox.com:
@@ -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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
