Skip to contentSkip to conversation

Make git-clean a builtin

PATCH In review Shawn Bohrer 7 participants 16 messages Jun 15, 2016 Kbuild

lore
Share & tools
lore

Shawn Bohrer

author
#0 lore raw

PATCH 1/1 Make git-clean a builtin

This replaces git-clean.sh with builtin-clean.c, and moves git-clean.sh to the examples.

Signed-off-by Shawn Bohrer <[email protected]>
---
 Makefile                                      |    3 +-
 builtin-clean.c                               |  177 +++++++++++++++++++++++++
 builtin.h                                     |    1 +
 git-clean.sh => contrib/examples/git-clean.sh |    0 
 git.c                                         |    1 +
 5 files changed, 181 insertions(+), 1 deletions(-)
 create mode 100644 builtin-clean.c
 rename git-clean.sh => contrib/examples/git-clean.sh (100%)
Makefile +2 -1
index 8db4dbe..2b3b8fb 100644
@@ -206,7 +206,7 @@ BASIC_LDFLAGS =
 
 SCRIPT_SH = \
 	git-bisect.sh git-checkout.sh \
-	git-clean.sh git-clone.sh git-commit.sh \
+	git-clone.sh git-commit.sh \
 	git-fetch.sh \
 	git-ls-remote.sh \
 	git-merge-one-file.sh git-mergetool.sh git-parse-remote.sh \

@@ -327,6 +327,7 @@ BUILTIN_OBJS = \
 	builtin-check-attr.o \
 	builtin-checkout-index.o \
 	builtin-check-ref-format.o \
+	builtin-clean.o \
 	builtin-commit-tree.o \
 	builtin-count-objects.o \
 	builtin-describe.o \
builtin-clean.c new file +177 -0

177 lines not on this page · raw message

builtin.h +1 -0
index d6f2c76..8c112f3 100644
@@ -23,6 +23,7 @@ extern int cmd_check_attr(int argc, const char **argv, const char *prefix);
 extern int cmd_check_ref_format(int argc, const char **argv, const char *prefix);
 extern int cmd_cherry(int argc, const char **argv, const char *prefix);
 extern int cmd_cherry_pick(int argc, const char **argv, const char *prefix);
+extern int cmd_clean(int argc, const char **argv, const char *prefix);
 extern int cmd_commit_tree(int argc, const char **argv, const char *prefix);
 extern int cmd_count_objects(int argc, const char **argv, const char *prefix);
 extern int cmd_describe(int argc, const char **argv, const char *prefix);
contrib/examples/git-clean.sh renamed from git-clean.sh +0 -0
similarity index 100%
rename from git-clean.sh
rename to contrib/examples/git-clean.sh
git.c +1 -0
index 9eaca1d..cda6344 100644
@@ -320,6 +320,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "check-attr", cmd_check_attr, RUN_SETUP | NEED_WORK_TREE },
 		{ "cherry", cmd_cherry, RUN_SETUP },
 		{ "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },
+		{ "clean", cmd_clean, RUN_SETUP },
 		{ "commit-tree", cmd_commit_tree, RUN_SETUP },
 		{ "config", cmd_config },
 		{ "count-objects", cmd_count_objects, RUN_SETUP },
Signature · 1 line
1.5.3.GIT
#1 lore raw
Quoted 11 lines · Shawn Bohrer
builtin-clean.c new file +11 -0
+static int remove_directory(const char *path)
+{
+	DIR *d;
+	struct dirent *dir;
+	d = opendir(path);
+	if (d) {
+		chdir(path);
+		while ((dir = readdir(d)) != NULL) {
+			if(strcmp( dir->d_name, ".") == 0 ||
+			   strcmp( dir->d_name, ".." ) == 0 )
+				continue;

builtin-clean.c line 51

Quoted 10 lines
builtin-clean.c new file +10 -0
+			if (dir->d_type == DT_DIR)
+				remove_directory(dir->d_name);
+			else
+				unlink(dir->d_name);
+		}
+	}
+	closedir(d);
+	chdir("..");
+	return rmdir(path);
+}

The unconditional chdir(..) after the conditional chdir(path) seems like asking for trouble to me...

builtin-clean.c line 146

Quoted 6 lines
builtin-clean.c new file +6 -0
+	while (fgets(path, sizeof(path), cmd_fout) != NULL) {
+		struct stat st;
+		char *p;
+		p = strrchr(path, '\n');
+		if ( p != NULL )
+			*p = '\0';

What happens in case p == NULL? It simply tries to remove the partial path?

builtin-clean.c line 174

Quoted 4 lines
builtin-clean.c new file +4 -0
+	fclose(cmd_fout);
+	finish_command(&cmd);
+	if (!ignored && !access(git_path("info/exclude"), F_OK))
+		free(buf);

There is a race condition here of the value of access() changes between the two calls. Not one likely to trigger but it should be easy to avoid alltogether.

builtin-clean.c line 177

Quoted 3 lines
builtin-clean.c new file +3 -0
+	free(argv_ls_files);
+	return 0;
+}

Gruesse,

Signature · 2 lines
Frank Lichtenheld <[email protected]>
www: http://www.djpig.de/
author
#2 lore raw
Quoted 17 lines · Frank Lichtenheld
Quoted 16 lines · Shawn Bohrer
builtin-clean.c new file +16 -0
+static int remove_directory(const char *path)
+{
+	DIR *d;
+	struct dirent *dir;
+	d = opendir(path);
+	if (d) {
+		chdir(path);
+		while ((dir = readdir(d)) != NULL) {
+			if(strcmp( dir->d_name, ".") == 0 ||
+			   strcmp( dir->d_name, ".." ) == 0 )
+				continue;
+			if (dir->d_type == DT_DIR)
+				remove_directory(dir->d_name);
+			else
+				unlink(dir->d_name);
+		}
Quoted 8 lines

builtin-clean.c line 51

Quoted 5 lines
builtin-clean.c new file +5 -0
+	}
+	closedir(d);
+	chdir("..");
+	return rmdir(path);
+}

The unconditional chdir(..) after the conditional chdir(path) seems like asking for trouble to me...

Agreed, I'm not sure what I was thinking there.

Quoted 9 lines

builtin-clean.c line 146

Quoted 6 lines
builtin-clean.c new file +6 -0
+	while (fgets(path, sizeof(path), cmd_fout) != NULL) {
+		struct stat st;
+		char *p;
+		p = strrchr(path, '\n');
+		if ( p != NULL )
+			*p = '\0';

What happens in case p == NULL? It simply tries to remove the partial path?

If p == NULL then the path didn't have a EOL character. This shouldn't ever really happen since fgets() leaves the EOL character as part of the string, and it is processing the output of git-ls-files which will provide one path per line. If it does happen for some reason then either we will happily remove the file/directory, or if the path is garbage then we will simply fail to remove anything.

Quoted 8 lines

builtin-clean.c line 174

Quoted 4 lines
builtin-clean.c new file +4 -0
+	fclose(cmd_fout);
+	finish_command(&cmd);
+	if (!ignored && !access(git_path("info/exclude"), F_OK))
+		free(buf);

There is a race condition here of the value of access() changes between the two calls. Not one likely to trigger but it should be easy to avoid alltogether.

Yes, though unlikely I agree this wasn't very smart in the first place so I fixed it and will send reworked patch soon.

--
Shawn

author
#3 lore raw

PATCH 1/1 Make git-clean a builtin

This replaces git-clean.sh with builtin-clean.c, and moves git-clean.sh to the examples.

Signed-off-by Shawn Bohrer <[email protected]>

---

Reworked patch with fixes to Frank's suggestions.

 Makefile                                      |    3 +-
 builtin-clean.c                               |  177 +++++++++++++++++++++++++
 builtin.h                                     |    1 +
 git-clean.sh => contrib/examples/git-clean.sh |    0 
 git.c                                         |    1 +
 5 files changed, 181 insertions(+), 1 deletions(-)
 create mode 100644 builtin-clean.c
 rename git-clean.sh => contrib/examples/git-clean.sh (100%)
Makefile +2 -1
index 8db4dbe..2b3b8fb 100644
@@ -206,7 +206,7 @@ BASIC_LDFLAGS =
 
 SCRIPT_SH = \
 	git-bisect.sh git-checkout.sh \
-	git-clean.sh git-clone.sh git-commit.sh \
+	git-clone.sh git-commit.sh \
 	git-fetch.sh \
 	git-ls-remote.sh \
 	git-merge-one-file.sh git-mergetool.sh git-parse-remote.sh \

@@ -327,6 +327,7 @@ BUILTIN_OBJS = \
 	builtin-check-attr.o \
 	builtin-checkout-index.o \
 	builtin-check-ref-format.o \
+	builtin-clean.o \
 	builtin-commit-tree.o \
 	builtin-count-objects.o \
 	builtin-describe.o \
builtin-clean.c new file +177 -0

177 lines not on this page · raw message

builtin.h +1 -0
index d6f2c76..8c112f3 100644
@@ -23,6 +23,7 @@ extern int cmd_check_attr(int argc, const char **argv, const char *prefix);
 extern int cmd_check_ref_format(int argc, const char **argv, const char *prefix);
 extern int cmd_cherry(int argc, const char **argv, const char *prefix);
 extern int cmd_cherry_pick(int argc, const char **argv, const char *prefix);
+extern int cmd_clean(int argc, const char **argv, const char *prefix);
 extern int cmd_commit_tree(int argc, const char **argv, const char *prefix);
 extern int cmd_count_objects(int argc, const char **argv, const char *prefix);
 extern int cmd_describe(int argc, const char **argv, const char *prefix);
contrib/examples/git-clean.sh renamed from git-clean.sh +0 -0
similarity index 100%
rename from git-clean.sh
rename to contrib/examples/git-clean.sh
git.c +1 -0
index 9eaca1d..cda6344 100644
@@ -320,6 +320,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "check-attr", cmd_check_attr, RUN_SETUP | NEED_WORK_TREE },
 		{ "cherry", cmd_cherry, RUN_SETUP },
 		{ "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },
+		{ "clean", cmd_clean, RUN_SETUP },
 		{ "commit-tree", cmd_commit_tree, RUN_SETUP },
 		{ "config", cmd_config },
 		{ "count-objects", cmd_count_objects, RUN_SETUP },
Signature · 1 line
1.5.3.GIT
#4 lore raw

Hi,

Quoted 1 line · Shawn Bohrer

+static int remove_directory(const char *path)

Please use remove_directory_recursively(), introduced in commit 7155b727c9baae9ef6d7829370aefc09c4ab64e2 to 'next'.

I have not looked at the rest of your patch yet.

Ciao,
Dscho

author
#5 lore raw

PATCH 1/1 Make git-clean a builtin

This replaces git-clean.sh with builtin-clean.c, and moves git-clean.sh to the examples.

Signed-off-by Shawn Bohrer <[email protected]>

---

Rewritten to use remove_directory_recursively() per Dscho's suggestion. Patch is now based ontop of 'next'.

 Makefile                                      |    3 +-
 builtin-clean.c                               |  161 +++++++++++++++++++++++++
 builtin.h                                     |    1 +
 git-clean.sh => contrib/examples/git-clean.sh |    0 
 git.c                                         |    1 +
 5 files changed, 165 insertions(+), 1 deletions(-)
 create mode 100644 builtin-clean.c
 rename git-clean.sh => contrib/examples/git-clean.sh (100%)
Makefile +2 -1
index 62bdac6..bed4c78 100644
@@ -206,7 +206,7 @@ BASIC_LDFLAGS =
 
 SCRIPT_SH = \
 	git-bisect.sh git-checkout.sh \
-	git-clean.sh git-clone.sh git-commit.sh \
+	git-clone.sh git-commit.sh \
 	git-ls-remote.sh \
 	git-merge-one-file.sh git-mergetool.sh git-parse-remote.sh \
 	git-pull.sh git-rebase.sh git-rebase--interactive.sh \

@@ -324,6 +324,7 @@ BUILTIN_OBJS = \
 	builtin-check-attr.o \
 	builtin-checkout-index.o \
 	builtin-check-ref-format.o \
+	builtin-clean.o \
 	builtin-commit-tree.o \
 	builtin-count-objects.o \
 	builtin-describe.o \
builtin-clean.c new file +161 -0

161 lines not on this page · raw message

builtin.h +1 -0
index 65cc0fb..cdefdc0 100644
@@ -23,6 +23,7 @@ extern int cmd_check_attr(int argc, const char **argv, const char *prefix);
 extern int cmd_check_ref_format(int argc, const char **argv, const char *prefix);
 extern int cmd_cherry(int argc, const char **argv, const char *prefix);
 extern int cmd_cherry_pick(int argc, const char **argv, const char *prefix);
+extern int cmd_clean(int argc, const char **argv, const char *prefix);
 extern int cmd_commit_tree(int argc, const char **argv, const char *prefix);
 extern int cmd_count_objects(int argc, const char **argv, const char *prefix);
 extern int cmd_describe(int argc, const char **argv, const char *prefix);
contrib/examples/git-clean.sh renamed from git-clean.sh +0 -0
similarity index 100%
rename from git-clean.sh
rename to contrib/examples/git-clean.sh
git.c +1 -0
index d7c6bca..4e39169 100644
@@ -320,6 +320,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "check-attr", cmd_check_attr, RUN_SETUP | NEED_WORK_TREE },
 		{ "cherry", cmd_cherry, RUN_SETUP },
 		{ "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },
+		{ "clean", cmd_clean, RUN_SETUP },
 		{ "commit-tree", cmd_commit_tree, RUN_SETUP },
 		{ "config", cmd_config },
 		{ "count-objects", cmd_count_objects, RUN_SETUP },
Signature · 1 line
1.5.3.GIT
#6 lore raw

Hi,

builtin-clean.c line 83

Quoted 2 lines · Shawn Bohrer
builtin-clean.c new file +2 -0
+	if (ignored && ignored_only)
+		usage(builtin_clean_usage);

Maybe a helpful message in that case, too? (It is not apparent from the usage what the user has done wrong here.)

builtin-clean.c line 87

Quoted 3 lines
builtin-clean.c new file +3 -0
+	if (disabled) {
+		die("clean.requireForce set and -n or -f not given; refusing to clean");
+	}

Please lose the curly brackets here. They are absolutely useless, and the rest of the git source code avoids unnecessary curly brackets.

builtin-clean.c line 93

Quoted 6 lines
builtin-clean.c new file +5 -0
+	/* Paths (argc - i) + 8 (Possible arguments)*/
+	argv_ls_files = xmalloc((argc - i + 8) * sizeof(const char *));
+	argv_ls_files[0] = "ls-files";
+	argv_ls_files[1] = "--others";
+	argv_ls_files[2] = "--directory";
[...]

As Linus already noted, it is probably easy, and much more efficient, to call read_directory() here. The best example how to use read_directory() is... builtin-ls-files.c.

git.c static void handle_internal_command(int argc, const char **argv) · line 323

Quoted 9 lines
git.c +1 -0
index d7c6bca..4e39169 100644
@@ -320,6 +320,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "check-attr", cmd_check_attr, RUN_SETUP | NEED_WORK_TREE },
 		{ "cherry", cmd_cherry, RUN_SETUP },
 		{ "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },
+		{ "clean", cmd_clean, RUN_SETUP },

You definitely want to have NEED_WORK_TREE here, too.

Ciao,
Dscho

#7 lore raw
In reply to Shawn Bohrer
Quoted 3 lines · Shawn Bohrer

This replaces git-clean.sh with builtin-clean.c, and moves git-clean.sh to the examples.

This looks better, but I think you'd be even better off actually using the "read_directory()" interface directly, instead of exec'ing off "git ls-files" and parsing the line output.

I also would still worry a bit about 'chdir(x)' and 'chdir("..")', because quite frankly, they are *not* mirrors of each other (think symlinks, but also error behaviour due to directories that might be non-executable). Now, admittedly, if a directory isn't executable, I can imagine other git things having problems (anybody want to test?), but that whole pattern is just very fragile and not very reliable.

Linus

author
#8 lore raw

Thanks for the input.

Quoted 3 lines · Linus Torvalds

This looks better, but I think you'd be even better off actually using the "read_directory()" interface directly, instead of exec'ing off "git ls-files" and parsing the line output.

Perhaps, I'll take a look at how git-ls-files does it and see if I can do that directly. Since I'm new to git (and C) it will probably take me a while to re-implement though.

Quoted 6 lines

I also would still worry a bit about 'chdir(x)' and 'chdir("..")', because quite frankly, they are *not* mirrors of each other (think symlinks, but also error behaviour due to directories that might be non-executable). Now, admittedly, if a directory isn't executable, I can imagine other git things having problems (anybody want to test?), but that whole pattern is just very fragile and not very reliable.

Yes it does seem fragile, but 'chdir("-")' doesn't work in C and I couldn't find any equivalents. I actually did think about symlinks, and my code does do the right thing since I test if it is a directory before doing the 'chdir(x)'. Symlinks are therefore treated as normal files and removed.

I did not think about non-executable directories, and you are correct that my code will fail to remove a directory if it is non-executable. I also tested a git-ls-files with non-executable directories, and it will fail to show you any files that are more than one level deep for example:

|-- docs
|   |-- contributing
|   |   `-- patches.txt
|   `-- manual.txt

If docs is non-executable it will only return 'docs/manual.txt'

--
Shawn

#9 lore raw
Quoted 18 lines · Shawn Bohrer

Thanks for the input.

Quoted 3 lines · Linus Torvalds

This looks better, but I think you'd be even better off actually using the "read_directory()" interface directly, instead of exec'ing off "git ls-files" and parsing the line output.

Perhaps, I'll take a look at how git-ls-files does it and see if I can do that directly. Since I'm new to git (and C) it will probably take me a while to re-implement though.

Quoted 6 lines

I also would still worry a bit about 'chdir(x)' and 'chdir("..")', because quite frankly, they are *not* mirrors of each other (think symlinks, but also error behaviour due to directories that might be non-executable). Now, admittedly, if a directory isn't executable, I can imagine other git things having problems (anybody want to test?), but that whole pattern is just very fragile and not very reliable.

Quoted 5 lines

Yes it does seem fragile, but 'chdir("-")' doesn't work in C and I couldn't find any equivalents. I actually did think about symlinks, and my code does do the right thing since I test if it is a directory before doing the 'chdir(x)'. Symlinks are therefore treated as normal files and removed.

'chdir -' is just supported by the shell, and the C interface could use chdir(OLDPWD).

#10 lore raw
In reply to Shawn Bohrer

builtin-clean.c line 49

Quoted 6 lines · Shawn Bohrer
builtin-clean.c new file +4 -0
+static int remove_directory(const char *path)
+{
[...]
+		chdir(path);
[...]
+	chdir("..");

This doesn't always put you back where you started, due to symlinks. For example:

cat >foo.c <<'EOF'
int main(int argc, char **argv) {
chdir(argv[1]);
chdir("..");
execlp("ls", 0);
}
EOF
gcc -o foo foo.c
ln -s /tmp sub
./foo sub

will show that you end up in the root directory. Something like this is more robust:

  fd = open(".", O_RDONLY);
  chdir(path);
  ...
  fchdir(fd);

In general, you shouldn't end up there because you don't actually recurse for symlinks, but there is a race condition (and losing it means you start recursively removing unintended directories -- oops).

On top of which, this line from the same function isn't very portable:

+ if (dir->d_type == DT_DIR)

since POSIX specifies nothing but dir->d_name (Solaris, for example, doesn't define d_type). You need to stat the file.

All of that being said, I think a lot of this is already done in dir.[ch]. At the very least, you should be able to use remove_dir_recursively, and for bonus points you can get rid of the start_command call to ls-files by just walking the dir tree yourself. I don't know if the latter is required, but it's nice when the C-ification actually cleans up a bit and uses the internal C interfaces (which are more efficient and often more clear to read) rather than just converting shell to C.

-Peff

#11 lore raw
Quoted 3 lines · Jeff King

This doesn't always put you back where you started, due to symlinks. For example:
[and other complaints]

Oops, didn't see that there was another thread for the reworked patch. My comments still stand, but it looks like others have weighed in as well. Sorry.

-Peff

#12 lore raw
In reply to Jeff King
Quoted 5 lines · Jeff King
  fd = open(".", O_RDONLY);
  chdir(path);
  ...
  fchdir(fd);

fchdir() is not portable.

I think it would be better to not chdir() at all. Yes, that means having to prepend the prefix to the names, but that is what git generally does (for that - and other - reasons).

Linus

#13 lore raw
Quoted 5 lines · Linus Torvalds

fchdir() is not portable.

I think it would be better to not chdir() at all. Yes, that means having to prepend the prefix to the names, but that is what git generally does (for that - and other - reasons).

I was using the "even Solaris has it!" test, but yes, it's not POSIX. I don't know how common it actually is (for curiosity's sake, do you know of a common platform that lacks it?). But I do agree that just building up the path and avoiding the chdir at all is simple and portable.

-Peff

#14 lore raw
Quoted 7 lines · Jeff King
Quoted 1 line · Linus Torvalds

fchdir() is not portable.

I was using the "even Solaris has it!" test, but yes, it's not POSIX. I don't know how common it actually is (for curiosity's sake, do you know of a common platform that lacks it?).

Windows. ;)

-- Hannes

#15 lore raw
Quoted 10 lines · Johannes Sixt
Quoted 7 lines · Jeff King
Quoted 1 line · Linus Torvalds

fchdir() is not portable.

I was using the "even Solaris has it!" test, but yes, it's not POSIX. I don't know how common it actually is (for curiosity's sake, do you know of a common platform that lacks it?).

Windows. ;)

Not just windows. Almost no "older" Unix has it. Including older versions of Linux.

Linus