Re: [PATCH v4 6/6] clean: teach clean -d to skip dirs containing ignored files

From: Junio C Hamano <gitster@pobox.com>
Date: May 22, 2017, 04:48 UTC
Message-ID: <xmqqtw4do5tf.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170518082154.28643-7-sxlijin@gmail.com>

Samuel Lijin <sxlijin@gmail.com> writes:

Show 23 quoted lines

> +	for (j = i = 0; i < dir.nr;) {
> +		for (;
> +		     j < dir.ignored_nr &&
> +		       0 <= cmp_dir_entry(&dir.entries[i], &dir.ignored[j]);
> +		     j++);
> +
> +		if ((j < dir.ignored_nr) &&
> +				check_dir_entry_contains(dir.entries[i], dir.ignored[j])) {
> +			/* skip any dir.entries which contains a dir.ignored */
> +			free(dir.entries[i]);
> +			dir.entries[i++] = NULL;
> +		} else {
> +			/* prune the contents of a dir.entries which will be removed */
> +			struct dir_entry *ent = dir.entries[i++];
> +			for (;
> +			     i < dir.nr &&
> +			       check_dir_entry_contains(ent, dir.entries[i]);
> +			     i++) {
> +				free(dir.entries[i]);
> +				dir.entries[i] = NULL;
> +			}
> +		}
> +	}

The second loop in the else clause is a bit tricky, and the comment "which will be removed" is not all that helpful to explain why the loop is there.

But I think the code is correct.  Here is how I understood it.

    While looking at dir.entries[i], the code noticed that nothing
    in that directory is ignored.  But entries in dir.entries[] that
    come later may be contained in dir.entries[i] and we just want
    to show the top-level untracked one (e.g. "a/" and "a/b/" were
    in entries[], there is nothing in "a/", so naturally there is
    nothing in "a/b/", but we do not want to bother showing
    both---showing "a/" alone saying "the entire a/ is untracked" is
    what we want).

We may want to have a test to ensure "a/b/" is indeed omitted in such a situation from the output, though.

By the way, instead of putting NULL, it may be easier to follow if you used two pointers, src and dst, into dir.entries[], just like you did in your latest version of [PATCH 4/6]. That way, you do not have to change anything in the later loop that walks over elements in the dir.entries[] array. It would also help the logic easier to follow if the above loop were its own helper function.

Putting them all together, here is what I came up with that can be squashed into your patch. I am undecided myself if this is easier to follow than your version, but it seems to pass your test ;-)

Thanks.

 builtin/clean.c | 70 ++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 42 insertions(+), 28 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index dd3308a447..c8712e7ac8 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -851,9 +851,49 @@ static void interactive_main_loop(void)
 	}
 }
 
+static void simplify_untracked(struct dir_struct *dir)
+{
+	int src, dst, ign;
+
+	for (src = dst = ign = 0; src < dir->nr; src++) {
+		/*
+		 * Skip entries in ignored[] that cannot be inside
+		 * entries[src]
+		 */
+		while (ign < dir->ignored_nr &&
+		       0 <= cmp_dir_entry(&dir->entries[src], &dir->ignored[ign]))
+			ign++;
+
+		if (dir->ignored_nr <= ign ||
+		    !check_dir_entry_contains(dir->entries[src], dir->ignored[ign])) {
+			/*
+			 * entries[src] does not contain an ignored
+			 * path -- we need to keep it.  But we do not
+			 * want to show entries[] that are contained
+			 * in entries[src].
+			 */
+			struct dir_entry *ent = dir->entries[src++];
+			dir->entries[dst++] = ent;
+			while (src < dir->nr &&
+			       check_dir_entry_contains(ent, dir->entries[src])) {
+				free(dir->entries[src++]);
+			}
+			/* compensate for the outer loop's loop control */
+			src--;
+		} else {
+			/*
+			 * entries[src] contains an ignored path --
+			 * drop it.
+			 */
+			free(dir->entries[src]);
+		}
+	}
+	dir->nr = dst;
+}
+
 int cmd_clean(int argc, const char **argv, const char *prefix)
 {
-	int i, j, res;
+	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;
@@ -928,30 +968,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 		       prefix, argv);
 
 	fill_directory(&dir, &pathspec);
-
-	for (j = i = 0; i < dir.nr;) {
-		for (;
-		     j < dir.ignored_nr &&
-		       0 <= cmp_dir_entry(&dir.entries[i], &dir.ignored[j]);
-		     j++);
-
-		if ((j < dir.ignored_nr) &&
-				check_dir_entry_contains(dir.entries[i], dir.ignored[j])) {
-			/* skip any dir.entries which contains a dir.ignored */
-			free(dir.entries[i]);
-			dir.entries[i++] = NULL;
-		} else {
-			/* prune the contents of a dir.entries which will be removed */
-			struct dir_entry *ent = dir.entries[i++];
-			for (;
-			     i < dir.nr &&
-			       check_dir_entry_contains(ent, dir.entries[i]);
-			     i++) {
-				free(dir.entries[i]);
-				dir.entries[i] = NULL;
-			}
-		}
-	}
+	simplify_untracked(&dir);
 
 	for (i = 0; i < dir.nr; i++) {
 		struct dir_entry *ent = dir.entries[i];
@@ -959,9 +976,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 		struct stat st;
 		const char *rel;
 
-		if (!ent)
-			continue;
-
 		if (!cache_name_is_other(ent->name, ent->len))
 			continue;