[PATCH] Windows related patches

[PATCH] Windows related patches

[Johannes Sixt]

Eric Raible (1):
  Teach lookup_prog not to select directories

Johannes Sixt (2):
  builtin-clone: Use is_dir_sep() instead of '/'
  Windows: set gitexecdir = $(bindir)

Peter Harris (1):
  Add ANSI control code emulation for the Windows console

[PATCH] Teach lookup_prog not to select directories

[Johannes Sixt]

From: Eric Raible <raible@gmail.com>

Without this simple fix "git gui" in the git source directory
finds the git-gui directory instead of the tcl script in /usr/bin.

Signed-off-by: Eric Raible <raible@gmail.com>
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 compat/mingw.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 3a05fe7..bb33c8d 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -536,7 +536,8 @@ static char *lookup_prog(const char *dir, const char *cmd, int isexe, int exe_on
 		return xstrdup(path);
 	path[strlen(path)-4] = '\0';
 	if ((!exe_only || isexe) && access(path, F_OK) == 0)
-		return xstrdup(path);
+		if (!(GetFileAttributes(path) & FILE_ATTRIBUTE_DIRECTORY))
+			return xstrdup(path);
 	return NULL;
 }
 
-- 
1.5.6.1.275.g0a3e0f

[PATCH] builtin-clone: Use is_dir_sep() instead of '/'

[Johannes Sixt]

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 builtin-clone.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index 643c7d4..fddf47f 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -115,7 +115,7 @@ static char *guess_dir_name(const char *repo, int is_bundle)
 			if (!after_slash_or_colon)
 				end = p;
 			p += 7;
-		} else if (*p == '/' || *p == ':') {
+		} else if (is_dir_sep(*p) || *p == ':') {
 			if (end == limit)
 				end = p;
 			after_slash_or_colon = 1;
-- 
1.5.6.1.275.g0a3e0f

[PATCH] Add ANSI control code emulation for the Windows console

[Johannes Sixt]

From: Peter Harris <git@peter.is-a-geek.org>

This adds only the minimum necessary to keep git pull/merge's diffstat from
wrapping. Notably absent is support for the K (erase) operation, and support
for POSIX write.

Signed-off-by: Peter Harris <git@peter.is-a-geek.org>
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 Makefile         |    2 +-
 compat/mingw.h   |   11 ++
 compat/winansi.c |  345 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 357 insertions(+), 1 deletions(-)
 create mode 100644 compat/winansi.c

diff --git a/Makefile b/Makefile
index 78e08d3..2c6cc9d 100644
--- a/Makefile
+++ b/Makefile
@@ -740,7 +740,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat
 	COMPAT_CFLAGS += -DSNPRINTF_SIZE_CORR=1
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
-	COMPAT_OBJS += compat/mingw.o compat/fnmatch.o compat/regex.o
+	COMPAT_OBJS += compat/mingw.o compat/fnmatch.o compat/regex.o compat/winansi.o
 	EXTLIBS += -lws2_32
 	X = .exe
 	template_dir = ../share/git-core/templates/
diff --git a/compat/mingw.h b/compat/mingw.h
index 6bc049a..5f11114 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -194,6 +194,17 @@ sig_handler_t mingw_signal(int sig, sig_handler_t handler);
 #define signal mingw_signal
 
 /*
+ * ANSI emulation wrappers
+ */
+
+int winansi_fputs(const char *str, FILE *stream);
+int winansi_printf(const char *format, ...) __attribute__((format (printf, 1, 2)));
+int winansi_fprintf(FILE *stream, const char *format, ...) __attribute__((format (printf, 2, 3)));
+#define fputs winansi_fputs
+#define printf(...) winansi_printf(__VA_ARGS__)
+#define fprintf(...) winansi_fprintf(__VA_ARGS__)
+
+/*
  * git specific compatibility
  */
 
diff --git a/compat/winansi.c b/compat/winansi.c
new file mode 100644
index 0000000..e2d96df
--- /dev/null
+++ b/compat/winansi.c
@@ -0,0 +1,345 @@
+/*
+ * Copyright 2008 Peter Harris <git@peter.is-a-geek.org>
+ */
+
+#include <windows.h>
+#include "../git-compat-util.h"
+
+/*
+ Functions to be wrapped:
+*/
+#undef printf
+#undef fprintf
+#undef fputs
+/* TODO: write */
+
+/*
+ ANSI codes used by git: m, K
+
+ This file is git-specific. Therefore, this file does not attempt
+ to implement any codes that are not used by git.
+
+ TODO: K
+*/
+
+static HANDLE console;
+static WORD plain_attr;
+static WORD attr;
+static int negative;
+
+static void init(void)
+{
+	CONSOLE_SCREEN_BUFFER_INFO sbi;
+
+	static int initialized = 0;
+	if (initialized)
+		return;
+
+	console = GetStdHandle(STD_OUTPUT_HANDLE);
+	if (console == INVALID_HANDLE_VALUE)
+		console = NULL;
+
+	if (!console)
+		return;
+
+	GetConsoleScreenBufferInfo(console, &sbi);
+	attr = plain_attr = sbi.wAttributes;
+	negative = 0;
+
+	initialized = 1;
+}
+
+
+#define FOREGROUND_ALL (FOREGROUND_RED | FOREGROUND_GREEN | FOREGROUND_BLUE)
+#define BACKGROUND_ALL (BACKGROUND_RED | BACKGROUND_GREEN | BACKGROUND_BLUE)
+
+static void set_console_attr(void)
+{
+	WORD attributes = attr;
+	if (negative) {
+		attributes &= ~FOREGROUND_ALL;
+		attributes &= ~BACKGROUND_ALL;
+
+		/* This could probably use a bitmask
+		   instead of a series of ifs */
+		if (attr & FOREGROUND_RED)
+			attributes |= BACKGROUND_RED;
+		if (attr & FOREGROUND_GREEN)
+			attributes |= BACKGROUND_GREEN;
+		if (attr & FOREGROUND_BLUE)
+			attributes |= BACKGROUND_BLUE;
+
+		if (attr & BACKGROUND_RED)
+			attributes |= FOREGROUND_RED;
+		if (attr & BACKGROUND_GREEN)
+			attributes |= FOREGROUND_GREEN;
+		if (attr & BACKGROUND_BLUE)
+			attributes |= FOREGROUND_BLUE;
+	}
+	SetConsoleTextAttribute(console, attributes);
+}
+
+static const char *set_attr(const char *str)
+{
+	const char *func;
+	size_t len = strspn(str, "0123456789;");
+	func = str + len;
+
+	switch (*func) {
+	case 'm':
+		do {
+			long val = strtol(str, (char **)&str, 10);
+			switch (val) {
+			case 0: /* reset */
+				attr = plain_attr;
+				negative = 0;
+				break;
+			case 1: /* bold */
+				attr |= FOREGROUND_INTENSITY;
+				break;
+			case 2:  /* faint */
+			case 22: /* normal */
+				attr &= ~FOREGROUND_INTENSITY;
+				break;
+			case 3:  /* italic */
+				/* Unsupported */
+				break;
+			case 4:  /* underline */
+			case 21: /* double underline */
+				/* Wikipedia says this flag does nothing */
+				/* Furthermore, mingw doesn't define this flag
+				attr |= COMMON_LVB_UNDERSCORE; */
+				break;
+			case 24: /* no underline */
+				/* attr &= ~COMMON_LVB_UNDERSCORE; */
+				break;
+			case 5:  /* slow blink */
+			case 6:  /* fast blink */
+				/* We don't have blink, but we do have
+				   background intensity */
+				attr |= BACKGROUND_INTENSITY;
+				break;
+			case 25: /* no blink */
+				attr &= ~BACKGROUND_INTENSITY;
+				break;
+			case 7:  /* negative */
+				negative = 1;
+				break;
+			case 27: /* positive */
+				negative = 0;
+				break;
+			case 8:  /* conceal */
+			case 28: /* reveal */
+				/* Unsupported */
+				break;
+			case 30: /* Black */
+				attr &= ~FOREGROUND_ALL;
+				break;
+			case 31: /* Red */
+				attr &= ~FOREGROUND_ALL;
+				attr |= FOREGROUND_RED;
+				break;
+			case 32: /* Green */
+				attr &= ~FOREGROUND_ALL;
+				attr |= FOREGROUND_GREEN;
+				break;
+			case 33: /* Yellow */
+				attr &= ~FOREGROUND_ALL;
+				attr |= FOREGROUND_RED | FOREGROUND_GREEN;
+				break;
+			case 34: /* Blue */
+				attr &= ~FOREGROUND_ALL;
+				attr |= FOREGROUND_BLUE;
+				break;
+			case 35: /* Magenta */
+				attr &= ~FOREGROUND_ALL;
+				attr |= FOREGROUND_RED | FOREGROUND_BLUE;
+				break;
+			case 36: /* Cyan */
+				attr &= ~FOREGROUND_ALL;
+				attr |= FOREGROUND_GREEN | FOREGROUND_BLUE;
+				break;
+			case 37: /* White */
+				attr |= FOREGROUND_RED |
+					FOREGROUND_GREEN |
+					FOREGROUND_BLUE;
+				break;
+			case 38: /* Unknown */
+				break;
+			case 39: /* reset */
+				attr &= ~FOREGROUND_ALL;
+				attr |= (plain_attr & FOREGROUND_ALL);
+				break;
+			case 40: /* Black */
+				attr &= ~BACKGROUND_ALL;
+				break;
+			case 41: /* Red */
+				attr &= ~BACKGROUND_ALL;
+				attr |= BACKGROUND_RED;
+				break;
+			case 42: /* Green */
+				attr &= ~BACKGROUND_ALL;
+				attr |= BACKGROUND_GREEN;
+				break;
+			case 43: /* Yellow */
+				attr &= ~BACKGROUND_ALL;
+				attr |= BACKGROUND_RED | BACKGROUND_GREEN;
+				break;
+			case 44: /* Blue */
+				attr &= ~BACKGROUND_ALL;
+				attr |= BACKGROUND_BLUE;
+				break;
+			case 45: /* Magenta */
+				attr &= ~BACKGROUND_ALL;
+				attr |= BACKGROUND_RED | BACKGROUND_BLUE;
+				break;
+			case 46: /* Cyan */
+				attr &= ~BACKGROUND_ALL;
+				attr |= BACKGROUND_GREEN | BACKGROUND_BLUE;
+				break;
+			case 47: /* White */
+				attr |= BACKGROUND_RED |
+					BACKGROUND_GREEN |
+					BACKGROUND_BLUE;
+				break;
+			case 48: /* Unknown */
+				break;
+			case 49: /* reset */
+				attr &= ~BACKGROUND_ALL;
+				attr |= (plain_attr & BACKGROUND_ALL);
+				break;
+			default:
+				/* Unsupported code */
+				break;
+			}
+			str++;
+		} while (*(str-1) == ';');
+
+		set_console_attr();
+		break;
+	case 'K':
+		/* TODO */
+		break;
+	default:
+		/* Unsupported code */
+		break;
+	}
+
+	return func + 1;
+}
+
+static int ansi_emulate(const char *str, FILE *stream)
+{
+	int rv = 0;
+	const char *pos = str;
+
+	while (*pos) {
+		pos = strstr(str, "\033[");
+		if (pos) {
+			size_t len = pos - str;
+
+			if (len) {
+				size_t out_len = fwrite(str, 1, len, stream);
+				rv += out_len;
+				if (out_len < len)
+					return rv;
+			}
+
+			str = pos + 2;
+			rv += 2;
+
+			fflush(stream);
+
+			pos = set_attr(str);
+			rv += pos - str;
+			str = pos;
+		} else {
+			rv += strlen(str);
+			fputs(str, stream);
+			return rv;
+		}
+	}
+	return rv;
+}
+
+int winansi_fputs(const char *str, FILE *stream)
+{
+	int rv;
+
+	if (!isatty(fileno(stream)))
+		return fputs(str, stream);
+
+	init();
+
+	if (!console)
+		return fputs(str, stream);
+
+	rv = ansi_emulate(str, stream);
+
+	if (rv >= 0)
+		return 0;
+	else
+		return EOF;
+}
+
+static int winansi_vfprintf(FILE *stream, const char *format, va_list list)
+{
+	int len, rv;
+	char small_buf[256];
+	char *buf = small_buf;
+	va_list cp;
+
+	if (!isatty(fileno(stream)))
+		goto abort;
+
+	init();
+
+	if (!console)
+		goto abort;
+
+	va_copy(cp, list);
+	len = vsnprintf(small_buf, sizeof(small_buf), format, cp);
+	va_end(cp);
+
+	if (len > sizeof(small_buf) - 1) {
+		buf = malloc(len + 1);
+		if (!buf)
+			goto abort;
+
+		len = vsnprintf(buf, len + 1, format, list);
+	}
+
+	rv = ansi_emulate(buf, stream);
+
+	if (buf != small_buf)
+		free(buf);
+	return rv;
+
+abort:
+	rv = vfprintf(stream, format, list);
+	return rv;
+}
+
+int winansi_fprintf(FILE *stream, const char *format, ...)
+{
+	va_list list;
+	int rv;
+
+	va_start(list, format);
+	rv = winansi_vfprintf(stream, format, list);
+	va_end(list);
+
+	return rv;
+}
+
+int winansi_printf(const char *format, ...)
+{
+	va_list list;
+	int rv;
+
+	va_start(list, format);
+	rv = winansi_vfprintf(stdout, format, list);
+	va_end(list);
+
+	return rv;
+}
-- 
1.5.6.1.275.g0a3e0f

[PATCH] Windows: set gitexecdir = $(bindir)

[Johannes Sixt]

The "dash-less" change aims to remove git commands from $PATH. It does so
by defining a GIT_EXEC_PATH that is different from $(bindir). On Windows
we want a relocatable installation of the git tool, so we cannot use an
absolute GIT_EXEC_PATH.  Therefore, the implementation of
builtin_exec_path() on Windows derives the exec-path from the command
invocation, and disregards GIT_EXEC_PATH. But this broke when
$(gitexecdir) became different from $(bindir), so we restore the earlier
behavior here.

This counteracts the aims of the "dash-less" change on Windows, but better
this way than having no working git at all.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 Makefile |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index 36339d3..7d466b3 100644
--- a/Makefile
+++ b/Makefile
@@ -742,6 +742,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	COMPAT_OBJS += compat/mingw.o compat/fnmatch.o compat/regex.o compat/winansi.o
 	EXTLIBS += -lws2_32
 	X = .exe
+	gitexecdir = $(bindir)
 	template_dir = ../share/git-core/templates/
 	ETC_GITCONFIG = ../etc/gitconfig
 endif
-- 
1.5.6.1.275.g0a3e0f

Re: [PATCH] Windows: set gitexecdir = $(bindir)

[Junio C Hamano]

Johannes Sixt <johannes.sixt@telecom.at> writes:

> The "dash-less" change aims to remove git commands from $PATH. It does so
> by defining a GIT_EXEC_PATH that is different from $(bindir). On Windows
> we want a relocatable installation of the git tool, so we cannot use an
> absolute GIT_EXEC_PATH.  Therefore, the implementation of
> builtin_exec_path() on Windows derives the exec-path from the command
> invocation,...

Sorry, I am not sure if I understand what you are trying to solve.  If you
have ../libexec/git-core/ in GIT_EXEC_PATH (or have builtin_exec_path()
use it), then your installation would look like this:

	[[some random place]]
        	bin/git
                libexec/git-core/git-add
                libexec/git-core/git-del
                libexec/git-core/git-dir
                ...

and if "git" can figure out it is "[[some random place]]/bin/git",
it can find its subcommands from neighbouring directory, that is still
inside the random place the user told the installer to use, can't it?

> This counteracts the aims of the "dash-less" change on Windows, but better
> this way than having no working git at all.

I'd agree to the extent that anything is better than having no working
git, but this somewhat feels backwards.

Re: [PATCH] Windows: set gitexecdir = $(bindir)

[Johannes Sixt]

On Samstag, 19. Juli 2008, Junio C Hamano wrote:
> Sorry, I am not sure if I understand what you are trying to solve.  If you
> have ../libexec/git-core/ in GIT_EXEC_PATH (or have builtin_exec_path()
> use it), then your installation would look like this:
>
> 	[[some random place]]
>         	bin/git
>                 libexec/git-core/git-add
>                 libexec/git-core/git-del
>                 libexec/git-core/git-dir
>                 ...
>
> and if "git" can figure out it is "[[some random place]]/bin/git",
> it can find its subcommands from neighbouring directory, that is still
> inside the random place the user told the installer to use, can't it?

Yes, but...

Take as an example 'git pull'.

- The first call to git will derive the exec-path 
$prefix/bin/../libexec/git-core and prepend it to $PATH.

- Calls to builtin git commands from inside 'git pull' will then derive the 
exec-path $prefix/bin/../libexec/git-core/../libexec/git-core, that is 
$prefix/libexec/libexec/git-core, and prepend it to $PATH as well. That 
directory does not exist - usually - and it does not hurt. But it feels dirty 
and potentially dangerous.

> > This counteracts the aims of the "dash-less" change on Windows, but
> > better this way than having no working git at all.
>
> I'd agree to the extent that anything is better than having no working
> git, but this somewhat feels backwards.

It certainly does.

I'm hoping that the msysgit crew has an opinion on this. CMD users like me do 
not care how cluttered $PATH is because there is no command completion that 
would reveal the 100+ git commands. But msysgit users who are working from a 
bash may want to have them hidden outside $PATH. Or maybe they do not care.

-- Hannes

Re: [PATCH] Windows: set gitexecdir = $(bindir)

[Steffen Prohaska]

On Jul 19, 2008, at 10:52 AM, Johannes Sixt wrote:

> On Samstag, 19. Juli 2008, Junio C Hamano wrote:
>> Sorry, I am not sure if I understand what you are trying to solve.   
>> If you
>> have ../libexec/git-core/ in GIT_EXEC_PATH (or have  
>> builtin_exec_path()
>> use it), then your installation would look like this:
>>
>> 	[[some random place]]
>>        	bin/git
>>                libexec/git-core/git-add
>>                libexec/git-core/git-del
>>                libexec/git-core/git-dir
>>                ...
>>
>> and if "git" can figure out it is "[[some random place]]/bin/git",
>> it can find its subcommands from neighbouring directory, that is  
>> still
>> inside the random place the user told the installer to use, can't it?
>
> Yes, but...
>
> Take as an example 'git pull'.
>
> - The first call to git will derive the exec-path
> $prefix/bin/../libexec/git-core and prepend it to $PATH.
>
> - Calls to builtin git commands from inside 'git pull' will then  
> derive the
> exec-path $prefix/bin/../libexec/git-core/../libexec/git-core, that is
> $prefix/libexec/libexec/git-core, and prepend it to $PATH as well.  
> That
> directory does not exist - usually - and it does not hurt. But it  
> feels dirty
> and potentially dangerous.

Maybe we can avoid this based on the name of the executable?
We would add libexec only if the executable is named "git",
but not if it is one of the dashed forms "git-*".

	Steffen

Re: [PATCH] Windows: set gitexecdir = $(bindir)

[Junio C Hamano]

Johannes Sixt <johannes.sixt@telecom.at> writes:

> On Samstag, 19. Juli 2008, Junio C Hamano wrote:
>> Sorry, I am not sure if I understand what you are trying to solve.  If you
>> have ../libexec/git-core/ in GIT_EXEC_PATH (or have builtin_exec_path()
>> use it), then your installation would look like this:
>>
>> 	[[some random place]]
>>         	bin/git
>>                 libexec/git-core/git-add
>>                 libexec/git-core/git-del
>>                 libexec/git-core/git-dir
>>                 ...
>>
>> and if "git" can figure out it is "[[some random place]]/bin/git",
>> it can find its subcommands from neighbouring directory, that is still
>> inside the random place the user told the installer to use, can't it?
>
> Yes, but...
>
> Take as an example 'git pull'.
>
> - The first call to git will derive the exec-path 
> $prefix/bin/../libexec/git-core and prepend it to $PATH.
>
> - Calls to builtin git commands from inside 'git pull' will then derive the 
> exec-path $prefix/bin/../libexec/git-core/../libexec/git-core, that is 
> $prefix/libexec/libexec/git-core, and prepend it to $PATH as well. That 
> directory does not exist - usually - and it does not hurt. But it feels dirty 
> and potentially dangerous.

You run "git" with an argument "pull".  It needs to figure out where
"git-pull" is, it checks where it came from and adds ../libexec/git-core/.
Then it runs "git-pull" script.

Then the script may have a call to "git ls-files -u" or "git-merge".

 - The former case, "git" again needs to find out where "git-ls-files"
   is.

   If "git" is found as bin/git and not as libexec/git-core/git, this
   should be perfectly fine, isn't it?  Perhaps we install a duplicate
   copy there by mistake, which is what we need to fix?

 - The latter case (our scripts source git-sh-setup so they have libexec
   one in the PATH when they are started) would find "git-merge" directly
   and runs it.

In either case, the programs "git-ls-files" and "git-merge" do not need to
do the same discovery -- are we giving them enough clues when we run them
to let them avoid that?

Re: [PATCH] Windows: set gitexecdir = $(bindir)

[Johannes Sixt]

On Samstag, 19. Juli 2008, Junio C Hamano wrote:
> Johannes Sixt <johannes.sixt@telecom.at> writes:
> > Take as an example 'git pull'.
> >
> > - The first call to git will derive the exec-path
> > $prefix/bin/../libexec/git-core and prepend it to $PATH.
> >
> > - Calls to builtin git commands from inside 'git pull' will then derive
> > the exec-path $prefix/bin/../libexec/git-core/../libexec/git-core, that
> > is $prefix/libexec/libexec/git-core, and prepend it to $PATH as well.
> > That directory does not exist - usually - and it does not hurt. But it
> > feels dirty and potentially dangerous.
>
> You run "git" with an argument "pull".  It needs to figure out where
> "git-pull" is, it checks where it came from and adds ../libexec/git-core/.
> Then it runs "git-pull" script.
>
> Then the script may have a call to "git ls-files -u" or "git-merge".
>
>  - The former case, "git" again needs to find out where "git-ls-files"
>    is.
>
>    If "git" is found as bin/git and not as libexec/git-core/git, this
>    should be perfectly fine, isn't it?  Perhaps we install a duplicate
>    copy there by mistake, which is what we need to fix?

Yes, there's libexec/git-core/git. There reason might be that the install 
target is simpler to write (to create the hardlinks) just in case $(bindir) 
and $(gitexecdir) are not on the same mount.

>  - The latter case (our scripts source git-sh-setup so they have libexec
>    one in the PATH when they are started) would find "git-merge" directly
>    and runs it.
>
> In either case, the programs "git-ls-files" and "git-merge" do not need to
> do the same discovery -- are we giving them enough clues when we run them
> to let them avoid that?

Probably the only clue is the name itself, like Steffen proposed.

I'll see how I can improve my earlier exec-path patch series.

-- Hannes

Re: [PATCH] builtin-clone: Use is_dir_sep() instead of '/'

[Junio C Hamano]

Johannes Sixt <johannes.sixt@telecom.at> writes:

> Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
> ---
>  builtin-clone.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/builtin-clone.c b/builtin-clone.c
> index 643c7d4..fddf47f 100644
> --- a/builtin-clone.c
> +++ b/builtin-clone.c
> @@ -115,7 +115,7 @@ static char *guess_dir_name(const char *repo, int is_bundle)
>  			if (!after_slash_or_colon)
>  				end = p;
>  			p += 7;
> -		} else if (*p == '/' || *p == ':') {
> +		} else if (is_dir_sep(*p) || *p == ':') {
>  			if (end == limit)
>  				end = p;
>  			after_slash_or_colon = 1;

Ok, but the surrounding code in this function look very suspicious.

 * The variable "prefix" is actually about suffix.

 * "else if" for explicit ".bundle" case look redundant; it would never
   trigger, I suspect.

 * Values used to increment p for "prefix" case and ".bundle" case are
   inconsistent.  I think the redundant one increments by the right value
   (i.e. strlen(prefix)-1 is bogus).

Re: [PATCH] builtin-clone: Use is_dir_sep() instead of '/'

[Johannes Sixt]

On Samstag, 19. Juli 2008, Junio C Hamano wrote:
> Ok, but the surrounding code in this function look very suspicious.

How about this then?

-- snip --
builtin-clone: Rewrite guess_dir_name()

The function has to do three small and independent tasks, but all of them
were crammed into a single loop. This rewrites the function entirely by
unrolling these tasks.

We also now use is_dir_sep(c) instead of c == '/' to increase portability,
which actually was the primary reason to touch this code.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 builtin-clone.c |   55 ++++++++++++++++++++++++++-----------------------------
 1 files changed, 26 insertions(+), 29 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index 8112716..91667d5 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -95,35 +95,32 @@ static char *get_repo_path(const char *repo, int *is_bundle)
 
 static char *guess_dir_name(const char *repo, int is_bundle)
 {
-	const char *p, *start, *end, *limit;
-	int after_slash_or_colon;
-
-	/* Guess dir name from repository: strip trailing '/',
-	 * strip trailing '[:/]*.{git,bundle}', strip leading '.*[/:]'. */
-
-	after_slash_or_colon = 1;
-	limit = repo + strlen(repo);
-	start = repo;
-	end = limit;
-	for (p = repo; p < limit; p++) {
-		const char *prefix = is_bundle ? ".bundle" : ".git";
-		if (!prefixcmp(p, prefix)) {
-			if (!after_slash_or_colon)
-				end = p;
-			p += strlen(prefix) - 1;
-		} else if (!prefixcmp(p, ".bundle")) {
-			if (!after_slash_or_colon)
-				end = p;
-			p += 7;
-		} else if (*p == '/' || *p == ':') {
-			if (end == limit)
-				end = p;
-			after_slash_or_colon = 1;
-		} else if (after_slash_or_colon) {
-			start = p;
-			end = limit;
-			after_slash_or_colon = 0;
-		}
+	const char *end = repo + strlen(repo), *start, *dot;
+
+	/*
+	 * Strip trailing slashes
+	 */
+	while (repo < end && is_dir_sep(end[-1]))
+		end--;
+
+	/*
+	 * Find last component, but be prepared that repo could have
+	 * the form  "remote.example.com:foo.git", i.e. no slash
+	 * in the directory part.
+	 */
+	start = end;
+	while (repo < start && !is_dir_sep(start[-1]) && start[-1] != ':')
+		start--;
+
+	/*
+	 * Strip .{bundle,git}.
+	 */
+	if (is_bundle) {
+		if (end - start > 7 && !strcmp(end - 7, ".bundle"))
+			end -= 7;
+	} else {
+		if (end - start > 4 && !strcmp(end - 4, ".git"))
+			end -= 4;
 	}
 
 	return xstrndup(start, end - start);
-- 
1.5.6.3.443.g5f70e

Re: [PATCH] builtin-clone: Use is_dir_sep() instead of '/'

[Johannes Schindelin]

Hi,

On Sat, 19 Jul 2008, Johannes Sixt wrote:

> On Samstag, 19. Juli 2008, Junio C Hamano wrote:
> > Ok, but the surrounding code in this function look very suspicious.
> 
> How about this then?

I like it.  Very clear, very nice.  Shorter code (if you look at the 
diffstat modulo documentation).

Ciao,
Dscho

Re: [PATCH] builtin-clone: Use is_dir_sep() instead of '/'

[Johannes Sixt]

On Samstag, 19. Juli 2008, Johannes Sixt wrote:
> On Samstag, 19. Juli 2008, Junio C Hamano wrote:
> > Ok, but the surrounding code in this function look very suspicious.
>
> How about this then?
>
> -- snip --
> builtin-clone: Rewrite guess_dir_name()
>
> The function has to do three small and independent tasks, but all of them
> were crammed into a single loop. This rewrites the function entirely by
> unrolling these tasks.

Sigh. I knew it, I knew it. If it had been that trivial, then Daniel had done
it this way in the first place. :-(

This needs to be squashed in. It makes sure that we handle 'foo/.git';
and .git was not stripped if we cloned from 'foo.git/'.

diff --git a/builtin-clone.c b/builtin-clone.c
index 91667d5..88616b3 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -98,10 +98,16 @@ static char *guess_dir_name(const char *repo, int is_bundle)
 	const char *end = repo + strlen(repo), *start, *dot;
 
 	/*
-	 * Strip trailing slashes
+	 * Strip trailing slashes and /.git
 	 */
 	while (repo < end && is_dir_sep(end[-1]))
 		end--;
+	if (end - repo > 5 && is_dir_sep(end[-5]) &&
+	    !strncmp(end - 4, ".git", 4)) {
+		end -= 5;
+		while (repo < end && is_dir_sep(end[-1]))
+			end--;
+	}
 
 	/*
 	 * Find last component, but be prepared that repo could have
@@ -116,10 +122,10 @@ static char *guess_dir_name(const char *repo, int is_bundle)
 	 * Strip .{bundle,git}.
 	 */
 	if (is_bundle) {
-		if (end - start > 7 && !strcmp(end - 7, ".bundle"))
+		if (end - start > 7 && !strncmp(end - 7, ".bundle", 7))
 			end -= 7;
 	} else {
-		if (end - start > 4 && !strcmp(end - 4, ".git"))
+		if (end - start > 4 && !strncmp(end - 4, ".git", 4))
 			end -= 4;
 	}
 

Re: [PATCH] builtin-clone: Use is_dir_sep() instead of '/'

[Daniel Barkalow]

On Sat, 19 Jul 2008, Johannes Sixt wrote:

> On Samstag, 19. Juli 2008, Johannes Sixt wrote:
> > On Samstag, 19. Juli 2008, Junio C Hamano wrote:
> > > Ok, but the surrounding code in this function look very suspicious.
> >
> > How about this then?
> >
> > -- snip --
> > builtin-clone: Rewrite guess_dir_name()
> >
> > The function has to do three small and independent tasks, but all of them
> > were crammed into a single loop. This rewrites the function entirely by
> > unrolling these tasks.
> 
> Sigh. I knew it, I knew it. If it had been that trivial, then Daniel had done
> it this way in the first place. :-(
> 
> This needs to be squashed in. It makes sure that we handle 'foo/.git';
> and .git was not stripped if we cloned from 'foo.git/'.

I actually got that from Johannes Schindelin, who added bundle support to 
my clone patch. I remember looking at his change, thinking it looked 
overly complicated, but finding that anything I tried to do to simplify it 
failed tests. If this gets through the test suite (lots of the tests other 
than the clone test try to do a wider variety of odd things than I expect 
users do in practice most of the time), then it's probably a better 
implementation.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] builtin-clone: Use is_dir_sep() instead of '/'

[Kristian Høgsberg]

On Sat, 2008-07-19 at 13:44 -0400, Daniel Barkalow wrote:
> On Sat, 19 Jul 2008, Johannes Sixt wrote:
> 
> > On Samstag, 19. Juli 2008, Johannes Sixt wrote:
> > > On Samstag, 19. Juli 2008, Junio C Hamano wrote:
> > > > Ok, but the surrounding code in this function look very suspicious.
> > >
> > > How about this then?
> > >
> > > -- snip --
> > > builtin-clone: Rewrite guess_dir_name()
> > >
> > > The function has to do three small and independent tasks, but all of them
> > > were crammed into a single loop. This rewrites the function entirely by
> > > unrolling these tasks.
> > 
> > Sigh. I knew it, I knew it. If it had been that trivial, then Daniel had done
> > it this way in the first place. :-(
> > 
> > This needs to be squashed in. It makes sure that we handle 'foo/.git';
> > and .git was not stripped if we cloned from 'foo.git/'.
> 
> I actually got that from Johannes Schindelin, who added bundle support to 
> my clone patch. I remember looking at his change, thinking it looked 
> overly complicated, but finding that anything I tried to do to simplify it 
> failed tests. If this gets through the test suite (lots of the tests other 
> than the clone test try to do a wider variety of odd things than I expect 
> users do in practice most of the time), then it's probably a better 
> implementation.

I wrote the original guess_git_dir(), and it *is* pretty subtle, sorry.
I would not rewrite it unless you really have to, since git clone now is
pretty stable and the dir sep separator change doesn't need this kind of
rewrite to fix the issue.  That said, with the last couple of changes
from Johannes Sixt, I believe it handles the same case that I had in
mind when I first wrote the loop.

cheers,
Kristian