threads / patch / 8002

patch, 3 partsdir.c: Omit non-excluded directories with dir->show_ignored

Subject: [PATCH 2/3] dir.c: Omit non-excluded directories with dir->show_ignored

## tl;dr

8 messages between May 6, 2007 and May 7, 2007. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Michael Spang· May 6, 2007, 18:09 UTC · lore

This makes "git-ls-files --others --directory --ignored" behave as documented and consequently also fixes "git-clean -d -X". Previously, git-clean would remove non-excluded directories even when using the -X option.

Signed-off-by: Michael Spang <mspang@uwaterloo.ca>
---
 dir.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

Show changes to dir.c +4 −1

diff --git a/dir.c b/dir.c
index d306352..adb3e62 100644
--- a/dir.c
+++ b/dir.c
@@ -448,6 +448,7 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co
 
 		while ((de = readdir(fdir)) != NULL) {
 			int len;
+			int exclude;
 
 			if ((de->d_name[0] == '.') &&
 			    (de->d_name[1] == 0 ||
@@ -461,7 +462,7 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co
 			memcpy(fullname + baselen, de->d_name, len+1);
 			if (simplify_away(fullname, baselen + len, simplify))
 				continue;
-			if (excluded(dir, fullname) != dir->show_ignored) {
+			if ((exclude = excluded(dir, fullname)) != dir->show_ignored) {
 				if (!dir->show_ignored || DTYPE(de) != DT_DIR) {
 					continue;
 				}
@@ -484,6 +485,8 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co
 				len++;
 				switch (treat_directory(dir, fullname, baselen + len, simplify)) {
 				case show_directory:
+					if (exclude != dir->show_ignored)
+						continue;
 					break;
 				case recurse_into_directory:
 					contents += read_directory_recursive(dir,

-- 
1.5.2.rc1.4.g47e1

Linus Torvalds· May 6, 2007, 19:42 UTC · re: Michael Spang · lore

Re: [PATCH 2/3] dir.c: Omit non-excluded directories with dir->show_ignored

On Sun, 6 May 2007, Michael Spang wrote:

Show 6 quoted lines

> @@ -461,7 +462,7 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co
>  			memcpy(fullname + baselen, de->d_name, len+1);
>  			if (simplify_away(fullname, baselen + len, simplify))
>  				continue;
> -			if (excluded(dir, fullname) != dir->show_ignored) {
> +			if ((exclude = excluded(dir, fullname)) != dir->show_ignored) {

Style issue: please write this as

			exclude = excluded(dir, fullname);
			if (exclude != dir->show_ignored) {

instead.

Yes, both are valid C, and mean the same thing, but one is much more readable than the other.

Combining multiple things inside an if-statement is convenient if:

 - the things inside are _really_ trivial.

 - it's done as part of macro expansion etc (ie it's not visible as such, 
   and the code is readable in its pre-preprocessor format)

but it's not good form otherwise.

		Linus

Michael Spang· May 6, 2007, 20:18 UTC · re: Linus Torvalds · lore

Re: [PATCH 2/3] dir.c: Omit non-excluded directories with dir->show_ignored

Linus Torvalds wrote:

Show 15 quoted lines

> 
> On Sun, 6 May 2007, Michael Spang wrote:
>> @@ -461,7 +462,7 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co
>>  			memcpy(fullname + baselen, de->d_name, len+1);
>>  			if (simplify_away(fullname, baselen + len, simplify))
>>  				continue;
>> -			if (excluded(dir, fullname) != dir->show_ignored) {
>> +			if ((exclude = excluded(dir, fullname)) != dir->show_ignored) {
> 
> Style issue: please write this as
> 
> 			exclude = excluded(dir, fullname);
> 			if (exclude != dir->show_ignored) {
> 
> instead.

Okay. Will fixup after waiting a bit for more comments.

Thanks, Michael Spang

Michael Spang· May 7, 2007, 02:35 UTC · re: Linus Torvalds · lore

Re: [PATCH 2/3] dir.c: Omit non-excluded directories with dir->show_ignored

Signed-off-by: Michael Spang <mspang@uwaterloo.ca>
---

This fixes the style issue noted by Linus.

 dir.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

Show changes to dir.c +6 −1

diff --git a/dir.c b/dir.c
index d306352..11fab7f 100644
--- a/dir.c
+++ b/dir.c
@@ -448,6 +448,7 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co
 
 		while ((de = readdir(fdir)) != NULL) {
 			int len;
+			int exclude;
 
 			if ((de->d_name[0] == '.') &&
 			    (de->d_name[1] == 0 ||
@@ -461,7 +462,9 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co
 			memcpy(fullname + baselen, de->d_name, len+1);
 			if (simplify_away(fullname, baselen + len, simplify))
 				continue;
-			if (excluded(dir, fullname) != dir->show_ignored) {
+
+			exclude = excluded(dir, fullname);
+			if (exclude != dir->show_ignored) {
 				if (!dir->show_ignored || DTYPE(de) != DT_DIR) {
 					continue;
 				}
@@ -484,6 +487,8 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co
 				len++;
 				switch (treat_directory(dir, fullname, baselen + len, simplify)) {
 				case show_directory:
+					if (exclude != dir->show_ignored)
+						continue;
 					break;
 				case recurse_into_directory:
 					contents += read_directory_recursive(dir,

Michael Spang· May 6, 2007, 18:09 UTC · lore

[PATCH 3/3] Fix minor documentation errors

- git-ls-files.txt: typo in description of --ignored
- git-clean.txt: s/forceRequire/requireForce/

Signed-off-by: Michael Spang <mspang@uwaterloo.ca>
---
 Documentation/git-clean.txt    |    2 +-
 Documentation/git-ls-files.txt |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

Show changes to 2 files +3 −3

Documentation/git-clean.txt, Documentation/git-ls-files.txt

diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
index 5aff026..e3252d5 100644
--- a/Documentation/git-clean.txt
+++ b/Documentation/git-clean.txt
@@ -26,7 +26,7 @@ OPTIONS
 	Remove untracked directories in addition to untracked files.
 
 -f::
-	If the git configuration specifies clean.forceRequire as true,
+	If the git configuration specifies clean.requireForce as true,
 	git-clean will refuse to run unless given -f or -n.
 
 -n::
diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 79e0b7b..076cebc 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -42,8 +42,8 @@ OPTIONS
 	Show other files in the output
 
 -i|--ignored::
-	Show ignored files in the output
-	Note the this also reverses any exclude list present.
+	Show ignored files in the output.
+	Note that this also reverses any exclude list present.
 
 -s|--stage::
 	Show stage files in the output

Michael Spang· May 6, 2007, 18:09 UTC · lore

[PATCH/RFD 4/3] t7300: Tests for git-clean using filenames with spaces/punctuation

Signed-off-by: Michael Spang <mspang@uwaterloo.ca>
---

This isn't meant for applying, at least until this flaw is fixed.

These tests are failing because ls-files does escaping and clean does not unescape. Does anyone know of a solution to this?

sed could be used to remove the double quotes and replace escaped characters, but doing many replacements happens in multiple passes and hence does not always work as desired.

Is this even properly solvable without making clean a builtin or writing git-unescape?

 t/t7300-clean.sh |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

Show changes to t/t7300-clean.sh +23 −0

diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 1fb3850..3792221 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -7,6 +7,8 @@ test_description='git-clean basic tests'
 
 . ./test-lib.sh
 
+TAB="	"
+
 test_expect_success \
     'setup' \
     "mkdir -p src &&
@@ -123,6 +125,27 @@ test_expect_success \
     test ! -e obj.o &&
     test ! -e build"
 
+test_expect_success \
+    'filenames with spaces' \
+    'touch abc\ def &&
+    touch 123 123\  &&
+    git-add 123 &&
+    git-clean &&
+    test ! -e abc\ def &&
+    test ! -e 123\  &&
+    test -e 123'
+
+test_expect_success \
+    'filenames with escaped characters' \
+    'touch "'"$TAB"'" " " \\ \" \\\\\" \\t &&
+    git-clean &&
+    test ! -e "'"$TAB"'" &&
+    test ! -e " " &&
+    test ! -e \\ &&
+    test ! -e \" &&
+    test ! -e \\\\\" &&
+    test ! -e \\t'
+
 test_expect_failure \
     'clean.requireForce' \
     "mkdir -p build docs &&

Junio C Hamano· May 6, 2007, 18:54 UTC · re: Michael Spang · lore

Re: [PATCH/RFD 4/3] t7300: Tests for git-clean using filenames with spaces/punctuation

Michael Spang <mspang@uwaterloo.ca> writes:

> Is this even properly solvable without making clean a builtin or
> writing git-unescape?

If you know how to use "xargs -0" and are willing to depend on the -0 GNU extension, then the answer is yes.

I do not use git-clean myself, as I do not see what (I think) it tries to solve as a problem to begin with, so obviously I do not care too deeply about the command's implementation --- I just let it be there because there seem to be others who want it --- but if I were asked an advice on the right direction to proceed, I would probably suggest rewriting it in C.

Michael Spang· May 6, 2007, 19:14 UTC · re: Junio C Hamano · lore

Re: [PATCH/RFD 4/3] t7300: Tests for git-clean using filenames with spaces/punctuation

Junio C Hamano wrote:

Show 7 quoted lines

> Michael Spang <mspang@uwaterloo.ca> writes:
> 
>> Is this even properly solvable without making clean a builtin or
>> writing git-unescape?
> 
> If you know how to use "xargs -0" and are willing to depend on
> the -0 GNU extension, then the answer is yes.

Right. But I do not think xargs can call a shell function. git-clean does not just call git-rm directly. I guess git-clean could call itself through xargs, but that might become confusing.

Show 6 quoted lines

> I do not use git-clean myself, as I do not see what (I think) it
> tries to solve as a problem to begin with, so obviously I do not
> care too deeply about the command's implementation --- I just
> let it be there because there seem to be others who want it ---
> but if I were asked an advice on the right direction to proceed,
> I would probably suggest rewriting it in C.

That seems like the cleanest solution to me as well.

Cheers, Michael Spang