threads / patch / 8026

patchremove unnecessary loop

Subject: [PATCH] remove unnecessary loop

## tl;dr

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

replies: 8people: 5as markdown or json

Liu Yubao· May 8, 2007, 03:18 UTC · lore
Hi,
   Here is a minor optimization, the involved second "for" loop doesn't
need to start from beginning.
Signed-off-by: Liu Yubao <yubao.liu@gmail.com>
---
 builtin-add.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)
Show changes to builtin-add.c +4 −5
diff --git a/builtin-add.c b/builtin-add.c
index 5e6748f..9d10fdc 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -239,20 +239,19 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		die("index file corrupt");
 
 	if (!ignored_too) {
-		int has_ignored = 0;
 		for (i = 0; i < dir.nr; i++)
 			if (dir.entries[i]->ignored)
-				has_ignored = 1;
-		if (has_ignored) {
+				break;
+		if (i < dir.nr) {
 			fprintf(stderr, ignore_warning);
-			for (i = 0; i < dir.nr; i++) {
+			do {
 				if (!dir.entries[i]->ignored)
 					continue;
 				fprintf(stderr, "%s", dir.entries[i]->name);
 				if (dir.entries[i]->ignored_dir)
 					fprintf(stderr, " (directory)");
 				fputc('\n', stderr);
-			}
+			} while (++i < dir.nr);
 			fprintf(stderr,
 				"Use -f if you really want to add them.\n");
 			exit(1);
-- 
1.5.2.rc0.95.ga0715-dirty
Liu Yubao· May 8, 2007, 04:49 UTC · re: Liu Yubao · lore

Re: [PATCH] remove unnecessary loop

Liu Yubao wrote:
> Hi,
>    Here is a minor optimization, the involved second "for" loop doesn't
> need to start from beginning.
> 

I found it when I debugged a strange problem on Cygwin, at last, I think it's a bug of Cygwin.

$ touch hello.exe $ git add hello The following paths are ignored by one of your .gitignore files: hello Use -f if you really want to add them.

Here is a ugly fix, I don't hope it will be merged into git tree as it's not git's fault, I will file a bug report for Cygwin.

---
 builtin-add.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)
Show changes to builtin-add.c +13 −1
diff --git a/builtin-add.c b/builtin-add.c
index 9d10fdc..ff1e74f 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -42,6 +42,9 @@ static void prune_directory(struct dir_struct *dir, const char **pathspec, int p
 	for (i = 0; i < specs; i++) {
 		struct stat st;
 		const char *match;
+#ifdef __CYGWIN__
+		int fd;
+#endif
 		if (seen[i])
 			continue;
 
@@ -50,9 +53,18 @@ static void prune_directory(struct dir_struct *dir, const char **pathspec, int p
 			continue;
 
 		/* Existing file? We must have ignored it */
+#ifdef __CYGWIN__
+		/*
+		 * On cygwin, lstat("hello", &st) returns 0 when
+		 * "hello.exe" exists, so test with open() again.
+		 */
+		if (lstat(match, &st) && -1 != (fd = open(match, O_RDONLY))) {
+			struct dir_entry *ent;
+			close(fd);
+#else
 		if (!lstat(match, &st)) {
 			struct dir_entry *ent;
-
+#endif
 			ent = dir_add_name(dir, match, strlen(match));
 			ent->ignored = 1;
 			if (S_ISDIR(st.st_mode))
-- 
1.5.2.rc0.95.ga0715-dirty
Junio C Hamano· May 8, 2007, 05:05 UTC · re: Liu Yubao · lore

Re: [PATCH] remove unnecessary loop

Liu Yubao <yubao.liu@gmail.com> writes:
> Here is a ugly fix, I don't hope it will be merged into git tree as it's not
> git's fault, I will file a bug report for Cygwin.
Show 13 quoted lines
> @@ -50,9 +53,18 @@ static void prune_directory(struct dir_struct *dir, const char **pathspec, int p
>  			continue;
>  
>  		/* Existing file? We must have ignored it */
> +#ifdef __CYGWIN__
> +		/*
> +		 * On cygwin, lstat("hello", &st) returns 0 when
> +		 * "hello.exe" exists, so test with open() again.
> +		 */
> +		if (lstat(match, &st) && -1 != (fd = open(match, O_RDONLY))) {
> +			struct dir_entry *ent;
> +			close(fd);
> +#else

We have lstat() everywhere, so if we were to work this around without (or "waiting for") a proper fix on the Cygwin side, you would be better off wrapping the above sequence in a separate function (say "sane_lstat()"), and do

	#ifdef __CYGWIN__
        #define lstat(a,b) sane_lstat(a,b)
        #endif
somewhere near the top of git-compat-util.h
Alex Riesen· May 8, 2007, 09:08 UTC · re: Liu Yubao · lore

Re: [PATCH] remove unnecessary loop

On 5/8/07, Liu Yubao <yubao.liu@gmail.com> wrote:
Show 6 quoted lines
> +#ifdef __CYGWIN__
> +               /*
> +                * On cygwin, lstat("hello", &st) returns 0 when
> +                * "hello.exe" exists, so test with open() again.
> +                */
> +               if (lstat(match, &st) && -1 != (fd = open(match, O_RDONLY))) {

This does not "test again" if lstat returns 0. If lstat returns 0 (file stat info obtained) the open is not even called. Besides, cygwin lies not only about .exe but also about .lnk files.

P.S. Somehow I have the feeling that even if it is a stupidity in cygwin they will not fix it (nor will they admit it is a bug).

Jan Hudec· May 8, 2007, 10:13 UTC · re: Alex Riesen · lore

Re: [PATCH] remove unnecessary loop

On Tue, May 08, 2007 at 11:08:35 +0200, Alex Riesen wrote:
Show 16 quoted lines
> On 5/8/07, Liu Yubao <yubao.liu@gmail.com> wrote:
> >+#ifdef __CYGWIN__
> >+               /*
> >+                * On cygwin, lstat("hello", &st) returns 0 when
> >+                * "hello.exe" exists, so test with open() again.
> >+                */
> >+               if (lstat(match, &st) && -1 != (fd = open(match, 
> >O_RDONLY))) {
> 
> This does not "test again" if lstat returns 0. If lstat returns 0
> (file stat info
> obtained) the open is not even called. Besides, cygwin lies not only about
> .exe but also about .lnk files.
> 
> P.S. Somehow I have the feeling that even if it is a stupidity in cygwin
> they will not fix it (nor will they admit it is a bug).

They will not. Because it is not a bug. It seems to be (part of) workaround to get programs written for unix work in windows.

One reason for such workaround I can think of is, that some programs try to find themselves and since their argv[0] often does NOT contain the extension, the stat has to succeed for them.

Using open here unfortunately won't work though, because:
 - For stale links open will fail, but the lstat should succeed. This does
   apply to cygwin, because cygwin emulates links.
 - I'd expect open to actually succeed in this case, because there are
   programs that don't only try to find themselves, but also open themselves,
   because they bundle some data.

Another problem is, that the file might exist or might be cygwin artefact and there does not seem to be an easy way to tell.

IMHO the described problem is harmless (you know the file does not exist, so you should have no reason to add it and nothing happens if you don't) and happens very rarely (adding binaries to version control is usually not a good idea), so I suggest to let this be, as the workaround can easily cause other problems.

-- 
						 Jan 'Bulb' Hudec <bulb@ucw.cz>
Alex Riesen· May 8, 2007, 12:38 UTC · re: Jan Hudec · lore

Re: [PATCH] remove unnecessary loop

On 5/8/07, Jan Hudec <bulb@ucw.cz> wrote:
Show 7 quoted lines
> >
> > P.S. Somehow I have the feeling that even if it is a stupidity in cygwin
> > they will not fix it (nor will they admit it is a bug).
>
> They will not. Because it is not a bug. It seems to be (part of) workaround
> to get programs written for unix work in windows.
>

Just as I said. Why don't you just realize that windows is plainly stupid, illogical piece of sh%t and state clearly that people have to break their programs so-and-so to work there? Instead, everyone has to put the most stupid workarounds POSIX ever seen in their code just to get core functionality (which even HP-UX got right).

Eric Blake· May 8, 2007, 12:17 UTC · re: Alex Riesen · lore

Re: [PATCH] remove unnecessary loop

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
According to Alex Riesen on 5/8/2007 3:08 AM:
Show 7 quoted lines
> This does not "test again" if lstat returns 0. If lstat returns 0
> (file stat info
> obtained) the open is not even called. Besides, cygwin lies not only about
> .exe but also about .lnk files.
> 
> P.S. Somehow I have the feeling that even if it is a stupidity in cygwin
> they will not fix it (nor will they admit it is a bug).

It is a limitation of cygwin, and the cygwin developers will admit it; but they will also stand behind calling it a feature rather than a bug due to the attempts to make cygwin behave more like Linux in spite of Window's insistence on file suffixes. The cygwin port of coreutils has to do similar stat() tricks to reverse engineer some of the .exe magic present in cygwin. However, it is possible to override the magic without resorting to a full-blown open(), via careful use of additional stat()s or readlink()s (trailing . is not legal in Windows, and on cygwin is only legal on managed mounts, so stat("foo.") will fail when stat("foo") succeeds if the reason stat("foo") succeeded was due only to the existence of foo.exe):

/* Return -1 if PATH not found, 0 if PATH spelled correctly, and 1 if PATH
   had ".exe" automatically appended by cygwin.  Don't change errno.  */
int
cygwin_spelling (char const *path)
{
  char path_exact[PATH_MAX + 9];
  int saved_errno = errno;
  int result = 0; /* Start with assumption that PATH is okay.  */
  int len = strlen (path);
  if (! path || ! *path || len > PATH_MAX)
    /* PATH will cause EINVAL or ENAMETOOLONG, treat it as non-existing.  */
    return -1;
  if (path[len - 1] == '.' || path[len-1] == '/')
    /* Don't change spelling if there is a trailing `.' or `/'.  */
    return 0;
  if (readlink (path, NULL, 0) < 0)
    { /* PATH is not a symlink.  */
      if (errno == EINVAL)
	{ /* PATH exists.  Appending trailing `.' exposes whether it is
	     PATH or PATH.exe for normal disk files, but also check appending
	     trailing `.exe' to be sure on virtual/managed directories.  */
	  strcat (strcpy (path_exact, path), ".");
	  if (access (path_exact, F_OK) < 0)
	    { /* PATH. does not exist.  */
	      strcat (path_exact, "exe");
	      if (access (path_exact, F_OK) == 0)
		/* But PATH.exe does, so append .exe.  */
		result = 1;
	    }
	}
      else
	/* PATH does not exist.  */
	result = -1;
    }
  else
    { /* PATH is a symlink.  Appending trailing `.lnk' exposes whether
	 it is PATH.lnk or PATH.exe.lnk; but does not help with
	 old-style symlinks where it was just PATH and the system
	 attribute set.  */
      strcat (strcpy (path_exact, path), ".lnk");
      if (readlink (path_exact, NULL, 0) < 0)
	{
	  strcat (strcpy (path_exact, path), ".exe.lnk");
	  if (readlink (path_exact, NULL, 0) == 0)
	    result = 1;
	}
    }
  errno = saved_errno;
  return result;
}

In the upcoming cygwin 1.7.0, you can set CYGWIN=transparent_exe which will cause ENOENT when dealing with any explicit .exe. When enabled, that will make it impossible to have both foo and foo.exe in the current directory, and make it so that stat can never lie - stat("foo.exe") will fail, and if stat("foo") succeeds, you no longer care if it succeeded because of the Windows file foo or because of foo.exe, because the .exe is transparent to cygwin.

- --
Don't work too hard, make some time for fun as well!
Eric Blake             ebb9@byu.net
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGQGpH84KuGfSFAYARAsCdAKCmqdgsppPY0MhxDWZ6QQxXExn2gwCeLN39 Zl3sRk/0IkkHkIyjf4RpAAA= =rQrT -----END PGP SIGNATURE-----

Jan Hudec· May 8, 2007, 09:39 UTC · re: Liu Yubao · lore

Re: [PATCH] remove unnecessary loop

On Tue, May 08, 2007 at 12:49:35 +0800, Liu Yubao wrote:
Show 13 quoted lines
> +#ifdef __CYGWIN__
> +		/*
> +		 * On cygwin, lstat("hello", &st) returns 0 when
> +		 * "hello.exe" exists, so test with open() again.
> +		 */
> +		if (lstat(match, &st) && -1 != (fd = open(match, O_RDONLY))) {
> +			struct dir_entry *ent;
> +			close(fd);
> +#else
>  		if (!lstat(match, &st)) {
>  			struct dir_entry *ent;
> -
> +#endif
You seem to have reversed the sense of the test.
-- 
						 Jan 'Bulb' Hudec <bulb@ucw.cz>
Liu Yubao· May 9, 2007, 01:03 UTC · re: Jan Hudec · lore

Re: [PATCH] remove unnecessary loop

Jan Hudec wrote:
Show 17 quoted lines
> On Tue, May 08, 2007 at 12:49:35 +0800, Liu Yubao wrote:
>> +#ifdef __CYGWIN__
>> +		/*
>> +		 * On cygwin, lstat("hello", &st) returns 0 when
>> +		 * "hello.exe" exists, so test with open() again.
>> +		 */
>> +		if (lstat(match, &st) && -1 != (fd = open(match, O_RDONLY))) {
>> +			struct dir_entry *ent;
>> +			close(fd);
>> +#else
>>  		if (!lstat(match, &st)) {
>>  			struct dir_entry *ent;
>> -
>> +#endif
> 
> You seem to have reversed the sense of the test.
> 
Sorry I made a mistake, Junio's suggestion is pretty clean, and
that test should be
		if (!lstat(match, &st) && -1 != (fd = open(match, O_RDONLY))) {

Yesterday I digged the Cygwin mail archive, I found it's a concession for windows as you said in the previous message. I agree with you, just let it be.

Once more, I get the lesson: Windows is poor, sigh...

← back to recent threads