Pull request for msysGit patches

Pull request for msysGit patches

[Pat Thoyts]

Junio,

The msysGit tree currently tracks some 50+ patches on top of 'next'. I
have gathered 42 of these that look good to move upstream. 
Please pull from
  git://repo.or.cz/git/mingw/4msysgit.git work/pt/for-junio
also visible for inspection at
  http://repo.or.cz/w/git/mingw/4msysgit.git/shortlog/refs/heads/work/pt/for-junio

Output of git-request-pull....

The following changes since commit 2a10b71f738b8b77ba8d243574f537a54dbf9a62:

  Merge branch 'mg/reset-doc' into next (2010-09-22 09:38:57 -0700)

are available in the git repository at:

  git://repo.or.cz/git/mingw/4msysgit.git work/pt/for-junio

Eric Sunshine (6):
      Fix 'clone' failure at DOS root directory.
      Fix Windows-specific macro redefinition warning.
      Add MinGW-specific execv() override.
      Side-step MSYS-specific path "corruption" leading to t5560 failure.
      Side-step sed line-ending "corruption" leading to t6038 failure.
      Side-step line-ending corruption leading to t3032 failures.

Erik Faye-Lund (6):
      core.hidedotfiles: hide '.git' dir by default
      mingw: do not hide bare repositories
      mingw: fix st_mode for symlink dirs
      send-email: accept absolute path even on Windows
      config.c: trivial fix for compile-time warning
      mingw: do not crash on open(NULL, ...)

Heiko Voigt (4):
      mingw: move unlink wrapper to mingw.c
      mingw: work around irregular failures of unlink on windows
      mingw: make failures to unlink or move raise a question
      mingw: add fallback for rmdir in case directory is in use

Johannes Schindelin (11):
      Avoid TAGS/tags warning from GNU Make
      When initializing .git/, record the current setting of core.hideDotFiles
      git-am: fix absolute path logic on Windows
      mingw_rmdir: set errno=ENOTEMPTY when appropriate
      Add a Windows-specific fallback to getenv("HOME");
      Tests: make sure that $DIFF is non-empty
      merge-octopus: Work around environment issue on Windows
      Make sure that git_getpass() never returns NULL
      Give commit message reencoding for output on MinGW a chance
      Fix typo in pack-objects' usage
      Fix compile error on MinGW

Johannes Sixt (1):
      criss cross rename failure workaround

Karsten Blees (4):
      Enable color output in Windows cmd.exe
      Support Unicode console output on Windows
      Detect console streams more reliably on Windows
      Warn if the Windows console font doesn't support Unicode

Pat Thoyts (6):
      Skip t1300.70 and 71 on msysGit.
      fix mingw stat() and lstat() implementations for handling symlinks
      Report errors when failing to launch the html browser in mingw.
      mingw: add tests for the hidden attribute on the git directory
      Do not strip CR when grepping HTTP headers.
      Skip 'git archive --remote' test on msysGit

Sebastian Schuberth (2):
      MinGW: Use pid_t more consequently, introduce uid_t for greater compatibility
      MinGW: Add missing file mode bit defines

bert Dvornik (2):
      mingw: Don't ask the user yes/no questions if they can't see the question.
      send-email: handle Windows paths for display just like we do for processing

 Documentation/config.txt            |    6 +
 Makefile                            |    2 +
 abspath.c                           |    7 +-
 builtin/config.c                    |    4 +-
 builtin/init-db.c                   |    1 +
 builtin/pack-objects.c              |    2 +-
 cache.h                             |    7 +
 compat/mingw.c                      |  296 +++++++++++++++++++++++++++++++++--
 compat/mingw.h                      |   63 +++++---
 compat/regex/regexec.c              |   20 ++-
 compat/winansi.c                    |  132 ++++++++++++----
 config.c                            |   11 ++-
 connect.c                           |    8 +-
 environment.c                       |    1 +
 git-am.sh                           |    2 +-
 git-compat-util.h                   |    8 +
 git-merge-octopus.sh                |    5 +
 git-send-email.perl                 |    4 +-
 log-tree.c                          |    3 +-
 path.c                              |    2 +-
 t/t0001-init.sh                     |   28 ++++
 t/t1300-repo-config.sh              |    6 +-
 t/t3032-merge-recursive-options.sh  |   11 +-
 t/t4130-apply-criss-cross-rename.sh |    4 +-
 t/t5000-tar-tree.sh                 |    2 +-
 t/t5503-tagfollow.sh                |    9 +-
 t/t5560-http-backend-noserver.sh    |    5 +-
 t/t6038-merge-text-auto.sh          |    4 +-
 t/test-lib.sh                       |    4 +
 29 files changed, 556 insertions(+), 101 deletions(-)

Re: Pull request for msysGit patches

[Junio C Hamano]

Pat Thoyts <patthoyts@users.sourceforge.net> writes:

> Junio,
>
> The msysGit tree currently tracks some 50+ patches on top of 'next'. I
> have gathered 42 of these that look good to move upstream. 
> Please pull from
>   git://repo.or.cz/git/mingw/4msysgit.git work/pt/for-junio
> also visible for inspection at
>   http://repo.or.cz/w/git/mingw/4msysgit.git/shortlog/refs/heads/work/pt/for-junio

Sorry, I cannot pull anything based directly on top of 'next'.  However,
except for the one that touch t/t3032-merge-recursive-options.sh at the
tip, the series seem to apply cleanly to the tip of 'master'.

There seem to be many patches that touch outside compat/ area.  Have they
been reviewed and discussed here already?

A quick and superficial review follows.

----------------------------------------------------------------
abspath.c

@@ -108,10 +108,15 @@ const char *make_nonrelative_path(const char *path)
 		if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
 			die("Too long path: %.*s", 60, path);
 	} else {
+		size_t len;
+		const char *fmt;
 		const char *cwd = get_pwd_cwd();
 		if (!cwd)
 			die_errno("Cannot determine the current working directory");
-		if (snprintf(buf, PATH_MAX, "%s/%s", cwd, path) >= PATH_MAX)
+		len = strlen(cwd);
+		/* For cwd c:/, return c:/foo rather than URL-like c://foo */

For the patch to be regression free, the logic described by this comment
requires get_pwd_cmd() to return a string with trailing dir-sep only at
slash.  IOW, if you see any non-root path returned with a trailing dir-sep
for whatever reason, you are changing the behaviour in that case as well,
and that clearly is not "fix at the root level".

But if you label this as "avoid duplicated dir-sep", everything flows
smoothly ;-).

+		fmt = len > 0 && is_dir_sep(cwd[len-1]) ? "%s%s" : "%s/%s";

Please have () around "len > 0 && is_dir_sep(cwd[len-1])" for readability.

+		if (snprintf(buf, PATH_MAX, fmt, cwd, path) >= PATH_MAX)
 			die("Too long path: %.*s", 60, path);

----------------------------------------------------------------
get_home_directory()

The patch looks fine, but the commit log message is way insufficient.  Can
you restate what problem it addresses, and why it is the best solution?

----------------------------------------------------------------
Hide dotfiles

It is somewhat unfortunate that this Windows-only hack needs to touch
cache.h and environment.c.

hidedotfiles may currently be the only platform specific configuration
variable, but we might want to futureproof by abstracting this out,
perhaps by adding a call to platform_core_config() at the end of
git_default_core_config(), provide a default implementation that is a
no-op, and allow platforms to override it, or something like that?

----------------------------------------------------------------
pack-objects.c usage string
connect.c use of unchecked git_getpass()

Good eyes, but these should not be part of the series but applied to
maint.  If possible please send them separately.

----------------------------------------------------------------
compat/regex/regexec.c

Thanks for fixing up my mess ;-)

----------------------------------------------------------------
git-am.sh

This is questionable.  Does this mean that on POSIX boxes I cannot have my
patch mailbox named 0:pt.patch, 1:pt.patch, etc.?

Adding "is_absolute_path" helper that has a different implementation
depending on the platform in git-sh-setup and using it here would limit
the extent of damage?

----------------------------------------------------------------
git-merge-octopus.sh

Transliterating a-z to A-z may happen to work because A-z is A-Z with a
lot of garbage concatenated at the end that won't be used, but it feels
sloppy.

Why isn't upcasing necessary for all the other uses of environment
variables?  For example, we pass reflog action by exporting a variable,
and we use GIT_AUTHOR_NAME and friends to override configuration
variables.  Do they get upcased?

What I am getting at is that this might be just fixing a symptom, and
between applying similar band-aid to many other places and finding out why
undesired upcaing happens and fixing that, I'd rather see the latter done.

----------------------------------------------------------------
git-send-email.perl

Similar comment as is_absolute_path(), although in Perl environment I
suspect we can just use an existing package without adding our own.

----------------------------------------------------------------
t/test-lib.sh

Is "DIFF=${DIFF:-diff}" needed?  How is the existing initialization (in
'maint') of GIT_TEST_CMP working?

----------------------------------------------------------------

I didn't look at tests very deeply.  Other changes outside compat/ that I
didn't mention above looked fine.

Thanks.

Re: [msysGit] Pull request for msysGit patches

[Johannes Sixt]

On Dienstag, 28. September 2010, Pat Thoyts wrote:
> Junio,
>
> The msysGit tree currently tracks some 50+ patches on top of 'next'. I
> have gathered 42 of these that look good to move upstream.
> Please pull from
>   git://repo.or.cz/git/mingw/4msysgit.git work/pt/for-junio
> also visible for inspection at
>  
> http://repo.or.cz/w/git/mingw/4msysgit.git/shortlog/refs/heads/work/pt/for-
>junio

Thanks for picking up the baton.

I've browsed through the list, and I'll annotate my opinion below. When I 
say 'OK', then this means that the patch looks good, but it doesn't imply 
that I have tested it.

> Eric Sunshine (6):
>       Fix 'clone' failure at DOS root directory.
>       Fix Windows-specific macro redefinition warning.
>       Add MinGW-specific execv() override.
>       Side-step MSYS-specific path "corruption" leading to t5560 failure.
>       Side-step sed line-ending "corruption" leading to t6038 failure.

These 5 are OK.

>       Side-step line-ending corruption leading to t3032 failures.

This one has non-portable 'export foo=bar', but it is in a MinGW specific 
path, so it should be fine. But why do we need to export GREP_OPTIONS, but 
not SED_OPTIONS?

> Erik Faye-Lund (6):
>       core.hidedotfiles: hide '.git' dir by default

This one was heavily disputed. Contrary to the subject line, the patch hides 
not only .git, but all dot-files. I'm not exactly a friend of this behavior. 
To hide only .git is OK, but every dot-file? The default setting of 
core.hidedotfiles should be different, IMO.

The subject line must be fixed, and unlike Erik's claim in a post earlier this 
week on the msysgit list, mingw_freopen blows up if filename ever happens to 
be NULL when core.hidedotfiles is true.

>       mingw: do not hide bare repositories

OK if it is decided that the above goes in; but could also be squashed.

>       mingw: fix st_mode for symlink dirs

This is a fix of Pat's "fix mingw stat...for handling symlinks" and should be 
squashed into that one.

>       send-email: accept absolute path even on Windows

I'm neutral. I don't use send-email.

>       config.c: trivial fix for compile-time warning

This is a fix for Dscho's getenv("HOME") patch and must be squashed - 
otherwise the commit message references a non-existent commit.

>       mingw: do not crash on open(NULL, ...)

This one is bogus, and as it stands, it must have my Ack removed. :) Needs the 
same fix in mingw_fopen as mingw_freopen. (There remains an unprotected 
dereference of filename.)

> Heiko Voigt (4):
>       mingw: move unlink wrapper to mingw.c

OK, whether or not the next patch goes in.

>       mingw: work around irregular failures of unlink on windows

The workaround is to retry the unlink() after a delay when it failed with 
EACCES. What happens if the EACCES is for a good reason? Doesn't this delay 
the process by 71ms per unlink() invocation? Can't this become a problem if 
many unlink()s are tried by git code?

>       mingw: make failures to unlink or move raise a question

Gaah! But people seem to like it. Since the question is only triggered after 
all retries fail, I can live with this.

But isn't the implementation a bit sloppy? Can strlen(answer)-2 be negative? 
What happens if the user typed more than 4 characters? Wouldn't it leave data 
in the buffer for the next question?

>       mingw: add fallback for rmdir in case directory is in use

Depends on the previous patch. OK.

> Johannes Schindelin (11):
>       Avoid TAGS/tags warning from GNU Make

OK.

>       When initializing .git/, record the current setting of
> core.hideDotFiles

I'm neutral on this one. The commit message does not say why the change is 
needed.

>       git-am: fix absolute path logic on Windows 

This mistakes a file that has a colon in the second position as absolute, even 
on non-Windows. IIUC, this patch is not intended for upstream git.

>       mingw_rmdir: set errno=ENOTEMPTY when appropriate

OK. Good catch!

>       Add a Windows-specific fallback to getenv("HOME");

Introduces get_home_directory(). I'm a bit worried that it leaks the string 
that it constructed.

>       Tests: make sure that $DIFF is non-empty

Hasn't this been fixed in upstream already?

>       merge-octopus: Work around environment issue on Windows

This works around MSYS DLL's "feature" to uppercase all environment variables. 
This destroys the original names GITHEAD_$SHA1.

There is a small typo in the second range argument of the tr invocation: it 
should be: tr a-z A-Z

>       Make sure that git_getpass() never returns NULL

This is not Windows specific. OK.

>       Give commit message reencoding for output on MinGW a chance

This is a change in log-tree.c, but has an effect only on Windows. OK.

>       Fix typo in pack-objects' usage

OK.

>       Fix compile error on MinGW

This has been fixed in a different way in upstream. Please eject this patch.

> Johannes Sixt (1):
>       criss cross rename failure workaround

The patch text is OK, but I should really write a better commit message...

> Karsten Blees (4):
>       Enable color output in Windows cmd.exe

This makes it so that "winansi" is return by getenv("TERM") when TERM is not 
set in the environment. What are the implications? It won't affect me because 
I've set TERM=cygwin. I'm neutral.

>       Support Unicode console output on Windows

I'm negative on this one because I think it will be a regression for me.

It assumes that all text written to the console is UTF8. I don't think that 
this is a generally valid assumption. It might be for msysgit, but not when 
git on Windows is used outside the msysgit bash or without the git.cmd 
wrapper.

>       Detect console streams more reliably on Windows

Looks good. OK.

>       Warn if the Windows console font doesn't support Unicode

Might be a good idea. The warning appears only when multi-byte characters are 
printed. I can't tell how this behaves in non-western locales.

Depends on the Unicode console output patch above.

> Pat Thoyts (6):
>       Skip t1300.70 and 71 on msysGit.
>       fix mingw stat() and lstat() implementations for handling symlinks
>       Report errors when failing to launch the html browser in mingw.

These 3 are OK.

>       mingw: add tests for the hidden attribute on the git directory

The tests of the non-bare repository should be marked expect_failure or be 
squashed into the core.hidedotfiles patch.

>       Do not strip CR when grepping HTTP headers.

export foo=bar again. Is it necessary to export GREP_OPTIONS?

>       Skip 'git archive --remote' test on msysGit

OK, though the failure is not due missing git daemon support, but because we 
do not have fork().

> Sebastian Schuberth (2):
>       MinGW: Use pid_t more consequently, introduce uid_t for greater
> compatibility

OK.

>       MinGW: Add missing file mode bit defines 

OK, why not. It is not strictly necessary, I think, otherwise I would observe 
build failures, but I do not.

> bert Dvornik (2):
>       mingw: Don't ask the user yes/no questions if they can't see the
> question.

Should be squashed into the ask-yes-no patch.

>       send-email: handle Windows paths for display just like we do for 
> processing

I'm neutral. The solution is analogous to the git-am patch above, but slightly 
more restrictive.

-- Hannes

Re: [msysGit] Pull request for msysGit patches

[Erik Faye-Lund]

On Tue, Sep 28, 2010 at 10:52 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>>       mingw: do not crash on open(NULL, ...)
>
> This one is bogus, and as it stands, it must have my Ack removed. :) Needs the
> same fix in mingw_fopen as mingw_freopen. (There remains an unprotected
> dereference of filename.)
>

I believe the version in for-junio already has this fix squashed in.
The following hunk, taken from
http://repo.or.cz/w/git/mingw/4msysgit.git/blobdiff/4e93566b07dcf47ecb6484d225418c04c1eedee6..b18500977d88b13803ecc60cf383538139ec09d8:/compat/mingw.c
shows that it is... Or are you thinking of something else?

@@ -346,7 +346,7 @@ FILE *mingw_fopen (const char *filename, const char *otype)
 	if (hide_dotfiles == HIDE_DOTFILES_TRUE &&
 	basename((char*)filename)[0] == '.')
 		hide = access(filename, F_OK);
-	if (!strcmp(filename, "/dev/null"))
+	if (filename && !strcmp(filename, "/dev/null"))
 		filename = "nul";
 	file = fopen(filename, otype);
 	if (file && hide && make_hidden(filename))

Re: [msysGit] Pull request for msysGit patches

[Jonathan Nieder]

Johannes Sixt wrote:
> On Dienstag, 28. September 2010, Pat Thoyts wrote:

>> Johannes Schindelin (11):
>>       Avoid TAGS/tags warning from GNU Make
>
> OK.

Wasn't this one reviewed on-list recently?

-- 8< --
From: Junio C Hamano <gitster@pobox.com>
Subject: MinGW: avoid collisions between "tags" and "TAGS"

On case insensitive filesystems, "tags" and "TAGS" target will try to
overwrite the same file.  Allow MinGW to use "ETAGS" instead.

These two targets do produce real files; do not put them on .PHONY target
list.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Makefile |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/Makefile b/Makefile
index b7a62cf..d3dcfb1 100644
--- a/Makefile
+++ b/Makefile
@@ -390,6 +390,8 @@ SCRIPTS = $(patsubst %.sh,%,$(SCRIPT_SH)) \
 	  $(patsubst %.py,%,$(SCRIPT_PYTHON)) \
 	  git-instaweb
 
+ETAGS_TARGET = TAGS
+
 # Empty...
 EXTRA_PROGRAMS =
 
@@ -1122,6 +1124,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_REGEX = YesPlease
 	NO_PYTHON = YesPlease
 	BLK_SHA1 = YesPlease
+	ETAGS_TARGET = ETAGS
 	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/fnmatch -Icompat/win32
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
 	COMPAT_OBJS += compat/mingw.o compat/fnmatch/fnmatch.o compat/winansi.o \
@@ -1961,11 +1964,11 @@ info:
 pdf:
 	$(MAKE) -C Documentation pdf
 
-TAGS:
-	$(RM) TAGS
-	$(FIND) . -name '*.[hcS]' -print | xargs etags -a
+$(ETAGS_TARGET): FORCE
+	$(RM) $(ETAGS_TARGET)
+	$(FIND) . -name '*.[hcS]' -print | xargs etags -a -o $(ETAGS_TARGET)
 
-tags:
+tags: FORCE
 	$(RM) tags
 	$(FIND) . -name '*.[hcS]' -print | xargs ctags -a
 
@@ -2235,7 +2238,7 @@ clean:
 	$(RM) $(TEST_PROGRAMS)
 	$(RM) -r bin-wrappers
 	$(RM) -r $(dep_dirs)
-	$(RM) *.spec *.pyc *.pyo */*.pyc */*.pyo common-cmds.h TAGS tags cscope*
+	$(RM) *.spec *.pyc *.pyo */*.pyc */*.pyo common-cmds.h $(ETAGS_TARGET) tags cscope*
 	$(RM) -r autom4te.cache
 	$(RM) config.log config.mak.autogen config.mak.append config.status config.cache
 	$(RM) -r $(GIT_TARNAME) .doc-tmp-dir
@@ -2259,7 +2262,7 @@ endif
 
 .PHONY: all install clean strip
 .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell
-.PHONY: FORCE TAGS tags cscope
+.PHONY: FORCE cscope
 
 ### Check documentation
 #
-- 
1.7.3.1.gd86b1

Re: [msysGit] Re: Pull request for msysGit patches

[Johannes Sixt]

On Dienstag, 28. September 2010, Junio C Hamano wrote:
> git-merge-octopus.sh
>
> Why isn't upcasing necessary for all the other uses of environment
> variables?  For example, we pass reflog action by exporting a variable,
> and we use GIT_AUTHOR_NAME and friends to override configuration
> variables.  Do they get upcased?

It is the environment variable *names* that get upper-cased, not the values. 
The variable name that was set for use by git-merge-octopus is of the form 
GITHEAD_deadbeef, but inside git-merge-octopus, the name is GITHEAD_DEADBEEF. 
Only MSYS programs like bash, but not native Windows programs like git, 
suffer from this.

-- Hannes

Re: [msysGit] Pull request for msysGit patches

[Johannes Sixt]

On Dienstag, 28. September 2010, Erik Faye-Lund wrote:
> On Tue, Sep 28, 2010 at 10:52 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> >>       mingw: do not crash on open(NULL, ...)
> >
> > This one is bogus, and as it stands, it must have my Ack removed. :)
> > Needs the same fix in mingw_fopen as mingw_freopen. (There remains an
> > unprotected dereference of filename.)
>
> I believe the version in for-junio already has this fix squashed in.
> The following hunk, taken from
> http://repo.or.cz/w/git/mingw/4msysgit.git/blobdiff/4e93566b07dcf47ecb6484d
>225418c04c1eedee6..b18500977d88b13803ecc60cf383538139ec09d8:/compat/mingw.c
> shows that it is... Or are you thinking of something else?
>
> @@ -346,7 +346,7 @@ FILE *mingw_fopen (const char *filename, const char
>	if (hide_dotfiles == HIDE_DOTFILES_TRUE &&
>  	basename((char*)filename)[0] == '.')
                        ^^^^^^^^
This can crash, too.

>  		hide = access(filename, F_OK);
> -	if (!strcmp(filename, "/dev/null"))
> +	if (filename && !strcmp(filename, "/dev/null"))
>  		filename = "nul";
>  	file = fopen(filename, otype);
>  	if (file && hide && make_hidden(filename))

-- Hannes

Re: [msysGit] Pull request for msysGit patches

[Erik Faye-Lund]

On Tue, Sep 28, 2010 at 11:13 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> On Dienstag, 28. September 2010, Erik Faye-Lund wrote:
>> On Tue, Sep 28, 2010 at 10:52 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> >>       mingw: do not crash on open(NULL, ...)
>> >
>> > This one is bogus, and as it stands, it must have my Ack removed. :)
>> > Needs the same fix in mingw_fopen as mingw_freopen. (There remains an
>> > unprotected dereference of filename.)
>>
>> I believe the version in for-junio already has this fix squashed in.
>> The following hunk, taken from
>> http://repo.or.cz/w/git/mingw/4msysgit.git/blobdiff/4e93566b07dcf47ecb6484d
>>225418c04c1eedee6..b18500977d88b13803ecc60cf383538139ec09d8:/compat/mingw.c
>> shows that it is... Or are you thinking of something else?
>>
>> @@ -346,7 +346,7 @@ FILE *mingw_fopen (const char *filename, const char
>>       if (hide_dotfiles == HIDE_DOTFILES_TRUE &&
>>       basename((char*)filename)[0] == '.')
>                        ^^^^^^^^
> This can crash, too.
>

Ah, indeed. Thanks for pointing that out! I'll send out a new version,
with this squashed on top:

diff --git a/compat/mingw.c b/compat/mingw.c
index 2fbe381..2584e9f 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -343,11 +343,13 @@ FILE *mingw_fopen (const char *filename, const
char *otype)
 {
 	int hide = 0;
 	FILE *file;
-	if (hide_dotfiles == HIDE_DOTFILES_TRUE &&
-	    basename((char*)filename)[0] == '.')
-		hide = access(filename, F_OK);
-	if (filename && !strcmp(filename, "/dev/null"))
-		filename = "nul";
+	if (filename) {
+		if (hide_dotfiles == HIDE_DOTFILES_TRUE &&
+		    basename((char*)filename)[0] == '.')
+			hide = access(filename, F_OK);
+		if (!strcmp(filename, "/dev/null"))
+			filename = "nul";
+	}
 	file = fopen(filename, otype);
 	if (file && hide && make_hidden(filename))
 		warning("Could not mark '%s' as hidden.", filename);
@@ -359,11 +361,13 @@ FILE *mingw_freopen (const char *filename, const
char *otype, FILE *stream)
 {
 	int hide = 0;
 	FILE *file;
-	if (hide_dotfiles == HIDE_DOTFILES_TRUE &&
-	    basename((char*)filename)[0] == '.')
-		hide = access(filename, F_OK);
-	if (filename && !strcmp(filename, "/dev/null"))
-		filename = "nul";
+	if (filename) {
+		if (hide_dotfiles == HIDE_DOTFILES_TRUE &&
+		    basename((char*)filename)[0] == '.')
+			hide = access(filename, F_OK);
+		if (!strcmp(filename, "/dev/null"))
+			filename = "nul";
+	}
 	file = freopen(filename, otype, stream);
 	if (file && hide && make_hidden(filename))
 		warning("Could not mark '%s' as hidden.", filename);

Re: Pull request for msysGit patches

[Ævar Arnfjörð Bjarmason]

On Tue, Sep 28, 2010 at 19:10, Junio C Hamano <gitster@pobox.com> wrote:

> git-send-email.perl
>
> Similar comment as is_absolute_path(), although in Perl environment I
> suspect we can just use an existing package without adding our own.

It seems to me that the code added by Erik Faye-Lund in
33b2e81f84875bf515b4c0de830eeddfd04227dc and this new code in
"send-email: handle Windows paths for display just like we do for
processing" by bert could both be replaced by:

    use File::Spec::Functions qw(file_name_is_absolute);
    file_name_is_absolute($some_path);

And by doing so we'd also be compatible with e.g. VMS. Can the people
with the odd non-Unix systems maybe try this out and see if it works?
:)

Re: [msysGit] Pull request for msysGit patches

[Erik Faye-Lund]

On Tue, Sep 28, 2010 at 11:20 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
> On Tue, Sep 28, 2010 at 11:13 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> On Dienstag, 28. September 2010, Erik Faye-Lund wrote:
>>> On Tue, Sep 28, 2010 at 10:52 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>>> >>       mingw: do not crash on open(NULL, ...)
>>> >
>>> > This one is bogus, and as it stands, it must have my Ack removed. :)
>>> > Needs the same fix in mingw_fopen as mingw_freopen. (There remains an
>>> > unprotected dereference of filename.)
>>>
>>> I believe the version in for-junio already has this fix squashed in.
>>> The following hunk, taken from
>>> http://repo.or.cz/w/git/mingw/4msysgit.git/blobdiff/4e93566b07dcf47ecb6484d
>>>225418c04c1eedee6..b18500977d88b13803ecc60cf383538139ec09d8:/compat/mingw.c
>>> shows that it is... Or are you thinking of something else?
>>>
>>> @@ -346,7 +346,7 @@ FILE *mingw_fopen (const char *filename, const char
>>>       if (hide_dotfiles == HIDE_DOTFILES_TRUE &&
>>>       basename((char*)filename)[0] == '.')
>>                        ^^^^^^^^
>> This can crash, too.
>>
>
> Ah, indeed. Thanks for pointing that out! I'll send out a new version,
> with this squashed on top:
>

And now that I assemble it, I all of a sudden understand where your
complaint came from. The issue wasn't in the original patch, it was
introduced when the patch was applied.

Basically, I introduced a similar crash-bug to the one this patch
fixes in the original hide-dotfiles patch, and my this patch was
written against junio's master, which doesn't contain that newer bug.

I also seem to remember there being some controversy over the
hide-dotfiles series, whether or not it was in git's scope to allow
hiding non-dotgit files at all etc. So, perhaps what makes the most
sense would be to eject the hide-dotfiles stuff out of for-junio, and
rework it on top, going through the main git mailing list? That series
did get quite a bit messy as patches were applied a little too quickly
IMO.

Re: [msysGit] Re: Pull request for msysGit patches

[Eric Sunshine]

Hi Junio,

On 9/28/2010 3:10 PM, Junio C Hamano wrote:
> Pat Thoyts<patthoyts@users.sourceforge.net>  writes:
>> Junio,
>> The msysGit tree currently tracks some 50+ patches on top of 'next'. I
>> have gathered 42 of these that look good to move upstream.
>
> A quick and superficial review follows.
> ----------------------------------------------------------------
> abspath.c
>
> @@ -108,10 +108,15 @@ const char *make_nonrelative_path(const char *path)
>   		if (strlcpy(buf, path, PATH_MAX)>= PATH_MAX)
>   			die("Too long path: %.*s", 60, path);
>   	} else {
> +		size_t len;
> +		const char *fmt;
>   		const char *cwd = get_pwd_cwd();
>   		if (!cwd)
>   			die_errno("Cannot determine the current working directory");
> -		if (snprintf(buf, PATH_MAX, "%s/%s", cwd, path)>= PATH_MAX)
> +		len = strlen(cwd);
> +		/* For cwd c:/, return c:/foo rather than URL-like c://foo */
>
> For the patch to be regression free, the logic described by this comment
> requires get_pwd_cmd() to return a string with trailing dir-sep only at
> slash.  IOW, if you see any non-root path returned with a trailing dir-sep
> for whatever reason, you are changing the behaviour in that case as well,
> and that clearly is not "fix at the root level".
> But if you label this as "avoid duplicated dir-sep", everything flows
> smoothly ;-).

I am the author of this patch. Do I understand correctly that your 
primary concern is that you find the comment misleading? I consider the code

   fmt = (len > 0 && is_dir_sep(cwd[len-1])) ? "%s%s" : "%s/%s";

sufficiently self-documenting that the comment is superfluous, and would 
be happy to remove the comment. Would you prefer the patch submitted 
with the comment removed?

-- ES

Re: [msysGit] Pull request for msysGit patches

[Eric Sunshine]

Hi Hannes,

On 9/28/2010 4:52 PM, Johannes Sixt wrote:
> On Dienstag, 28. September 2010, Pat Thoyts wrote:
>> Junio,
>>
>> The msysGit tree currently tracks some 50+ patches on top of 'next'. I
>> have gathered 42 of these that look good to move upstream.
>
> I've browsed through the list, and I'll annotate my opinion below. When I
> say 'OK', then this means that the patch looks good, but it doesn't imply
> that I have tested it.
>
>>        Side-step line-ending corruption leading to t3032 failures.
>
> This one has non-portable 'export foo=bar', but it is in a MinGW specific
> path, so it should be fine. But why do we need to export GREP_OPTIONS, but
> not SED_OPTIONS?

I also normally avoid unportable 'export foo=bar'. In the particular 
case of GREP_OPTIONS, when commenting on my original patch submission, 
Dscho suggested 'test_have_prereq MINGW && export GREP_OPTIONS=foo' so 
that is the form which made it into the final patch. While considering 
whether this unportable usage was worthwhile, I took into consideration 
the fact that a much more senior project member preferred it, that the 
MinGW-specific build environment is Bash-based, and that a couple other 
test scripts (t1509, t5560) also employ this form. If preferred, the 
patches can be re-submitted to avoid the unportable usage.

Regarding exporting GREP_OPTIONS but not SED_OPTIONS: grep explicitly 
recognizes the GREP_OPTIONS environment variable. My original patch did 
not export GREP_OPTIONS, but instead referenced it as $GREP_OPTIONS in 
the grep invocation. Upon review, Dscho preferred the variable to be 
exported rather than referenced manually as $GREP_OPTIONS. sed, on the 
other hand, does not recognize any 'options' environment variable. It is 
necessary, therefore, to interpolate $SED_OPTIONS directly during the 
sed invocation. This variable is therefore local to the script and need 
not be exported.

-- ES

Re: [msysGit] Pull request for msysGit patches

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> I also normally avoid unportable 'export foo=bar'. In the particular
> case of GREP_OPTIONS, when commenting on my original patch submission,
> Dscho suggested 'test_have_prereq MINGW && export GREP_OPTIONS=foo' so
> that is the form which made it into the final patch.

Well, since bbc09c2 (grep: rip out support for external grep, 2010-01-12)
we do not run external grep at all, so GREP_OPTIONS is irrelevant.

Unless you are planning to run tests on installed version of git older
than v1.7.0, that is ;-).

Re: [msysGit] Re: Pull request for msysGit patches

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> On Dienstag, 28. September 2010, Junio C Hamano wrote:
>> git-merge-octopus.sh
>>
>> Why isn't upcasing necessary for all the other uses of environment
>> variables?  For example, we pass reflog action by exporting a variable,
>> and we use GIT_AUTHOR_NAME and friends to override configuration
>> variables.  Do they get upcased?
>
> It is the environment variable *names* that get upper-cased, not the values. 

Ah, of course. I should have noticed it if I read the patch more carefully.

Thanks for an explanation.

Re: [msysGit] Re: Pull request for msysGit patches

[Eric Sunshine]

On 9/28/2010 11:38 PM, Junio C Hamano wrote:
> Eric Sunshine<sunshine@sunshineco.com>  writes:
>
>> I am the author of this patch. Do I understand correctly that your
>> primary concern is that you find the comment misleading? I consider
>> the code
>>
>>    fmt = (len>  0&&  is_dir_sep(cwd[len-1])) ? "%s%s" : "%s/%s";
>>
>> sufficiently self-documenting that the comment is superfluous, and
>> would be happy to remove the comment. Would you prefer the patch
>> submitted with the comment removed?
>
> Yeah, that sounds sensible.  Thanks.

Thank you for the response. I will re-submit the patch with the noted 
changes: (1) parentheses surrounding conditional expression, (2) removal 
of superfluous comment.

-- ES

Re: [msysGit] Pull request for msysGit patches

[Eric Sunshine]

On 9/28/2010 11:37 PM, Junio C Hamano wrote:
> Eric Sunshine<sunshine@sunshineco.com>  writes:
>> I also normally avoid unportable 'export foo=bar'. In the particular
>> case of GREP_OPTIONS, when commenting on my original patch submission,
>> Dscho suggested 'test_have_prereq MINGW&&  export GREP_OPTIONS=foo' so
>> that is the form which made it into the final patch.
>
> Well, since bbc09c2 (grep: rip out support for external grep, 2010-01-12)
> we do not run external grep at all, so GREP_OPTIONS is irrelevant.
>
> Unless you are planning to run tests on installed version of git older
> than v1.7.0, that is ;-).

The patches in question involve test scripts which themselves invoke 
external grep for various reasons. An example is t5560 where external 
grep is invoked to extract the Status: header from an HTTP response. The 
patches fix instances where grep invocations give incorrect results on 
Windows due to grep undesirably swallowing CR from CRLF line-terminators 
in the few tests where those terminators are actually significant (such 
as t5560). In the context of these test scripts, external grep is still 
employed, so GREP_OPTIONS may still be relevant.

-- ES

Re: Pull request for msysGit patches

[yj2133011]

This struct for now is just a wrapper for the current pathspec form:
const char **. It is intended to be extended with more useful
pathspec-related information over time.

The data structure for passing pathspec around remains const char **,
struct pathspec will be initialized locally to be used and destroyed.
Hopefully all pathspec related code will be gradually migrated to pass
this struct instead.


-----
The voice input and output is very good in this 
http://www.tomtop.com/black-ps3-wireless-bluetooth-headset-for-playstation-3.html?aid=z
Wireless PS3 Headset . It is compatible with all PS3 games.Buy from Reliable 
http://www.tomtop.com/google-android-7-notebook-3g-tablet-pc-umpc-wifi-mid-pda.html?aid=z
Google Android PC  apad Wholesalers.
-- 
View this message in context: http://git.661346.n2.nabble.com/Pull-request-for-msysGit-patches-tp5578682p5582620.html
Sent from the git mailing list archive at Nabble.com.

Re: [msysGit] Pull request for msysGit patches

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Johannes Sixt wrote:
>> On Dienstag, 28. September 2010, Pat Thoyts wrote:
>
>>> Johannes Schindelin (11):
>>>       Avoid TAGS/tags warning from GNU Make
>>
>> OK.
>
> Wasn't this one reviewed on-list recently?

Yeah, I remember it vaguely ;-)

Although I do not care very much either way, this probably is a better
approach, I think, if there are people on MinGW and/or OS/X who use
tags/etags.

Will queue.

> -- 8< --
> From: Junio C Hamano <gitster@pobox.com>
> Subject: MinGW: avoid collisions between "tags" and "TAGS"
>
> On case insensitive filesystems, "tags" and "TAGS" target will try to
> overwrite the same file.  Allow MinGW to use "ETAGS" instead.
>
> These two targets do produce real files; do not put them on .PHONY target
> list.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  Makefile |   15 +++++++++------
>  1 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index b7a62cf..d3dcfb1 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -390,6 +390,8 @@ SCRIPTS = $(patsubst %.sh,%,$(SCRIPT_SH)) \
>  	  $(patsubst %.py,%,$(SCRIPT_PYTHON)) \
>  	  git-instaweb
>  
> +ETAGS_TARGET = TAGS
> +
>  # Empty...
>  EXTRA_PROGRAMS =
>  
> @@ -1122,6 +1124,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
>  	NO_REGEX = YesPlease
>  	NO_PYTHON = YesPlease
>  	BLK_SHA1 = YesPlease
> +	ETAGS_TARGET = ETAGS
>  	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/fnmatch -Icompat/win32
>  	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
>  	COMPAT_OBJS += compat/mingw.o compat/fnmatch/fnmatch.o compat/winansi.o \
> @@ -1961,11 +1964,11 @@ info:
>  pdf:
>  	$(MAKE) -C Documentation pdf
>  
> -TAGS:
> -	$(RM) TAGS
> -	$(FIND) . -name '*.[hcS]' -print | xargs etags -a
> +$(ETAGS_TARGET): FORCE
> +	$(RM) $(ETAGS_TARGET)
> +	$(FIND) . -name '*.[hcS]' -print | xargs etags -a -o $(ETAGS_TARGET)
>  
> -tags:
> +tags: FORCE
>  	$(RM) tags
>  	$(FIND) . -name '*.[hcS]' -print | xargs ctags -a
>  
> @@ -2235,7 +2238,7 @@ clean:
>  	$(RM) $(TEST_PROGRAMS)
>  	$(RM) -r bin-wrappers
>  	$(RM) -r $(dep_dirs)
> -	$(RM) *.spec *.pyc *.pyo */*.pyc */*.pyo common-cmds.h TAGS tags cscope*
> +	$(RM) *.spec *.pyc *.pyo */*.pyc */*.pyo common-cmds.h $(ETAGS_TARGET) tags cscope*
>  	$(RM) -r autom4te.cache
>  	$(RM) config.log config.mak.autogen config.mak.append config.status config.cache
>  	$(RM) -r $(GIT_TARNAME) .doc-tmp-dir
> @@ -2259,7 +2262,7 @@ endif
>  
>  .PHONY: all install clean strip
>  .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell
> -.PHONY: FORCE TAGS tags cscope
> +.PHONY: FORCE cscope
>  
>  ### Check documentation
>  #
> -- 
> 1.7.3.1.gd86b1

Re: Pull request for msysGit patches

[Ramsay Jones]

Pat Thoyts wrote:
> Junio,
> 
> The msysGit tree currently tracks some 50+ patches on top of 'next'. I
> have gathered 42 of these that look good to move upstream. 
> Please pull from
>   git://repo.or.cz/git/mingw/4msysgit.git work/pt/for-junio
> also visible for inspection at
>   http://repo.or.cz/w/git/mingw/4msysgit.git/shortlog/refs/heads/work/pt/for-junio
> 
> Output of git-request-pull....

I am carrying a patch by Peter Harris (Modify MSVC wrapper script) in my
repo, which I would hope could be included soon. I have never used 4msysgit.git
(I thought it was no longer used, except for building installers!), but I
just had a quick look via the web interface - the commit I'm referring to is
commit 358f1be616da601b2169463521d409a8aa86466a (Modify MSVC wrapper script,
2010-07-05).

[Note the "Fix MSVC build" companion commit was not needed on git.git]

ATB,
Ramsay Jones

msysGit patches for upstream

[Pat Thoyts]

Junio C Hamano <gitster@pobox.com> writes:

> pack-objects.c usage string
> connect.c use of unchecked git_getpass()
>
> Good eyes, but these should not be part of the series but applied to
> maint.  If possible please send them separately.

Following up on this comment here are the two patches from msysGit.

Pat.

[PATCH 1/2] Make sure that git_getpass() never returns NULL

[Pat Thoyts]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The result of git_getpass() is used without checking for NULL, so let's
just die() instead of returning NULL.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 connect.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/connect.c b/connect.c
index 3450cab..57dc20c 100644
--- a/connect.c
+++ b/connect.c
@@ -631,8 +631,12 @@ char *git_getpass(const char *prompt)
 		askpass = askpass_program;
 	if (!askpass)
 		askpass = getenv("SSH_ASKPASS");
-	if (!askpass || !(*askpass))
-		return getpass(prompt);
+	if (!askpass || !(*askpass)) {
+		char *result = getpass(prompt);
+		if (!result)
+			die_errno("Could not read password");
+		return result;
+	}
 
 	args[0] = askpass;
 	args[1]	= prompt;
-- 
1.7.3

[PATCH 2/2] Fix typo in pack-objects' usage

[Pat Thoyts]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/pack-objects.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 0e81673..3756cf3 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -30,7 +30,7 @@ static const char pack_usage[] =
   "        [--no-reuse-delta] [--no-reuse-object] [--delta-base-offset]\n"
   "        [--threads=N] [--non-empty] [--revs [--unpacked | --all]*]\n"
   "        [--reflog] [--stdout | base-name] [--include-tag]\n"
-  "        [--keep-unreachable | --unpack-unreachable \n"
+  "        [--keep-unreachable | --unpack-unreachable]\n"
   "        [<ref-list | <object-list]";
 
 struct object_entry {
-- 
1.7.3

Re: [msysGit] Pull request for msysGit patches

[Pat Thoyts]

On 29 September 2010 18:51, Junio C Hamano <gitster@pobox.com> wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> Johannes Sixt wrote:
>>> On Dienstag, 28. September 2010, Pat Thoyts wrote:
>>
>>>> Johannes Schindelin (11):
>>>>       Avoid TAGS/tags warning from GNU Make
>>>
>>> OK.
>>
>> Wasn't this one reviewed on-list recently?
>
> Yeah, I remember it vaguely ;-)
>
> Although I do not care very much either way, this probably is a better
> approach, I think, if there are people on MinGW and/or OS/X who use
> tags/etags.
>
> Will queue.

I've just tested this suggested patch with msysGit and it's fine here.
We can go with this rather than Johannes' original.

Re: msysGit patches for upstream

[Junio C Hamano]

Pat Thoyts <patthoyts@users.sourceforge.net> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> pack-objects.c usage string
>> connect.c use of unchecked git_getpass()
>>
>> Good eyes, but these should not be part of the series but applied to
>> maint.  If possible please send them separately.
>
> Following up on this comment here are the two patches from msysGit.

Thanks.

Since you are forwarding the patch to me, I'll add your Sign-off
(which was in the original series anyway) before applying them.

[PATCH] git-am: fix detection of absolute paths for windows

[Pat Thoyts]

Add an is_absolute_path function to abstract out platform differences
in checking for an absolute or relative path.
Specifically fixes t4150-am on Windows.

Signed-off-by: Pat Thoyts <patthoyts@users.sourceforge.net>
---

In response to the following comment...

>Johannes Sixt <j6t@kdbg.org> writes:
>>>       git-am: fix absolute path logic on Windows 
>>
>>This mistakes a file that has a colon in the second position as absolute, even 
>>on non-Windows. IIUC, this patch is not intended for upstream git.
>>

This patch is an alternative solution that only tries the Windows paths
on msysGit. This is_absolute_path might find use elsewhere in the future
so I elected to place it in the sh-setup script. Also added support for
identifying a UNC path as absolute.


 git-am.sh       |   12 ++++++------
 git-sh-setup.sh |   15 +++++++++++++++
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index e7f008c..9317b38 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -444,12 +444,12 @@ else
 				set x
 				first=
 			}
-			case "$arg" in
-			/*)
-				set "$@" "$arg" ;;
-			*)
-				set "$@" "$prefix$arg" ;;
-			esac
+			if is_absolute_path "$arg"
+			then
+				set "$@" "$arg"
+			else
+				set "$@" "$prefix$arg"
+			fi
 		done
 		shift
 	fi
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 6131670..b9eb0bf 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -209,5 +209,20 @@ case $(uname -s) in
 	find () {
 		/usr/bin/find "$@"
 	}
+	is_absolute_path () {
+		case "$1" in
+		/* | ?:* | \\\\*)
+			return 0 ;;
+		esac
+		return 1
+	}
 	;;
+*)
+	is_absolute_path () {
+		case "$1" in
+		/*)
+			return 0 ;;
+		esac
+		return 1
+	}
 esac
-- 
1.7.3

[PATCH] git-am: fix detection of absolute paths for windows

[Pat Thoyts]

Add an is_absolute_path function to abstract out platform differences
in checking for an absolute or relative path.
Specifically fixes t4150-am on Windows.

[PT: updated following suggestion from j6t to support \* and //*]

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Pat Thoyts <patthoyts@users.sourceforge.net>
---

Johannes Sixt <j6t@kdbg.org> writes:
>On Donnerstag, 30. September 2010, Pat Thoyts wrote:
>> Add an is_absolute_path function to abstract out platform differences
>> in checking for an absolute or relative path.
>> Specifically fixes t4150-am on Windows.
>
>Thanks for tackling this!
>
>> @@ -209,5 +209,20 @@ case $(uname -s) in
>>  	find () {
>>  		/usr/bin/find "$@"
>>  	}
>> +	is_absolute_path () {
>> +		case "$1" in
>> +		/* | ?:* | \\\\*)
>
>Absolute paths can also start with a backslash, and UNC paths can start with 
>double-slash. Therefore, this should be:
>
>		[/\\]* | [A-Za-z]:*)
>
>> +			return 0 ;;
>> +		esac
>> +		return 1
>> +	}
>
>-- Hannes

I've modified the patch and added your signoff - hopefully that is ok?

 git-am.sh       |   12 ++++++------
 git-sh-setup.sh |   15 +++++++++++++++
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index e7f008c..9317b38 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -444,12 +444,12 @@ else
 				set x
 				first=
 			}
-			case "$arg" in
-			/*)
-				set "$@" "$arg" ;;
-			*)
-				set "$@" "$prefix$arg" ;;
-			esac
+			if is_absolute_path "$arg"
+			then
+				set "$@" "$arg"
+			else
+				set "$@" "$prefix$arg"
+			fi
 		done
 		shift
 	fi
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 6131670..58d30c9 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -209,5 +209,20 @@ case $(uname -s) in
 	find () {
 		/usr/bin/find "$@"
 	}
+	is_absolute_path () {
+		case "$1" in
+		[/\\]* | [A-Za-z]:*)
+			return 0 ;;
+		esac
+		return 1
+	}
 	;;
+*)
+	is_absolute_path () {
+		case "$1" in
+		/*)
+			return 0 ;;
+		esac
+		return 1
+	}
 esac
-- 
1.7.3

Re: Pull request for msysGit patches

[Pat Thoyts]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>On Tue, Sep 28, 2010 at 19:10, Junio C Hamano <gitster@pobox.com> wrote:
>
>> git-send-email.perl
>>
>> Similar comment as is_absolute_path(), although in Perl environment I
>> suspect we can just use an existing package without adding our own.
>
>It seems to me that the code added by Erik Faye-Lund in
>33b2e81f84875bf515b4c0de830eeddfd04227dc and this new code in
>"send-email: handle Windows paths for display just like we do for
>processing" by bert could both be replaced by:
>
>    use File::Spec::Functions qw(file_name_is_absolute);
>    file_name_is_absolute($some_path);
>
>And by doing so we'd also be compatible with e.g. VMS. Can the people
>with the odd non-Unix systems maybe try this out and see if it works?
>:)

I just looked into using this function with msysGit. Unfortunately it
doesn't work as our perl is msys-compiled and doesn't recognise c:\ as
an absolute path. It's using the unix path functions.
Possibly we could use a native perl if the git perl functions were
making more use of these platform-abstracting functions. Ultimately this
is the right way to go.

Re: Pull request for msysGit patches

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 30, 2010 at 22:15, Pat Thoyts
<patthoyts@users.sourceforge.net> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>On Tue, Sep 28, 2010 at 19:10, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>> git-send-email.perl
>>>
>>> Similar comment as is_absolute_path(), although in Perl environment I
>>> suspect we can just use an existing package without adding our own.
>>
>>It seems to me that the code added by Erik Faye-Lund in
>>33b2e81f84875bf515b4c0de830eeddfd04227dc and this new code in
>>"send-email: handle Windows paths for display just like we do for
>>processing" by bert could both be replaced by:
>>
>>    use File::Spec::Functions qw(file_name_is_absolute);
>>    file_name_is_absolute($some_path);
>>
>>And by doing so we'd also be compatible with e.g. VMS. Can the people
>>with the odd non-Unix systems maybe try this out and see if it works?
>>:)
>
> I just looked into using this function with msysGit. Unfortunately it
> doesn't work as our perl is msys-compiled and doesn't recognise c:\ as
> an absolute path. It's using the unix path functions.
> Possibly we could use a native perl if the git perl functions were
> making more use of these platform-abstracting functions. Ultimately this
> is the right way to go.

That sounds like something msysGit needs to patch in its Perl
build. The path functions on Windows account for drive letters, but if
they're just using the Unix versions on not-quite-Unix that might
break.

Re: Pull request for msysGit patches

[Erik Faye-Lund]

On Fri, Oct 1, 2010 at 12:15 AM, Pat Thoyts
<patthoyts@users.sourceforge.net> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>On Tue, Sep 28, 2010 at 19:10, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>> git-send-email.perl
>>>
>>> Similar comment as is_absolute_path(), although in Perl environment I
>>> suspect we can just use an existing package without adding our own.
>>
>>It seems to me that the code added by Erik Faye-Lund in
>>33b2e81f84875bf515b4c0de830eeddfd04227dc and this new code in
>>"send-email: handle Windows paths for display just like we do for
>>processing" by bert could both be replaced by:
>>
>>    use File::Spec::Functions qw(file_name_is_absolute);
>>    file_name_is_absolute($some_path);
>>
>>And by doing so we'd also be compatible with e.g. VMS. Can the people
>>with the odd non-Unix systems maybe try this out and see if it works?
>>:)
>
> I just looked into using this function with msysGit. Unfortunately it
> doesn't work as our perl is msys-compiled and doesn't recognise c:\ as
> an absolute path. It's using the unix path functions.
> Possibly we could use a native perl if the git perl functions were
> making more use of these platform-abstracting functions. Ultimately this
> is the right way to go.
>

Unfortunately, changing our perl is easier said than done, see issue
218 in the msysGit issue tracker for some of the nasty details:
http://code.google.com/p/msysgit/issues/detail?id=218

Re: [PATCH] git-am: fix detection of absolute paths for windows

[Johannes Sixt]

On Donnerstag, 30. September 2010, Pat Thoyts wrote:
> Add an is_absolute_path function to abstract out platform differences
> in checking for an absolute or relative path.
> Specifically fixes t4150-am on Windows.

Thanks for tackling this!

> @@ -209,5 +209,20 @@ case $(uname -s) in
>  	find () {
>  		/usr/bin/find "$@"
>  	}
> +	is_absolute_path () {
> +		case "$1" in
> +		/* | ?:* | \\\\*)

Absolute paths can also start with a backslash, and UNC paths can start with 
double-slash. Therefore, this should be:

		[/\\]* | [A-Za-z]:*)

> +			return 0 ;;
> +		esac
> +		return 1
> +	}

-- Hannes

[PATCH v2 0/4] make open/unlink failures user friendly on windows using retry/abort

[Heiko Voigt]

Hi,

here is a new iteration of my original patch series. This series
replaces the newest patches currently present in 4msysgit.git's master
and does not cleanly apply to git.git's master. Once everybody is happy
with the outcome I will port it to git.git so msysgit can fetch it from
upstream.

On Tue, Sep 28, 2010 at 10:52:25PM +0200, Johannes Sixt wrote:
> > Heiko Voigt (4):
> >       mingw: work around irregular failures of unlink on windows
> 
> The workaround is to retry the unlink() after a delay when it failed with 
> EACCES. What happens if the EACCES is for a good reason? Doesn't this delay 
> the process by 71ms per unlink() invocation? Can't this become a problem if 
> many unlink()s are tried by git code?

I have changed the triggering error code to be ERROR_SHARING_VIOLATION
which seems to be the appropriate code for files that are in use by
another process.

http://msdn.microsoft.com/en-us/library/ms681382%28v=VS.85%29.aspx

Thus we do not need to worry about whether we are retrying on a valid
access error. I have tested this on my windows box and it works here.

j6t: I have not changed your error code in mingw_rename since you
explicitely compare with the windows error code ERROR_ACCESS_DENIED and
do not use the err_win_to_posix() function. Did you do this on purpose or
should I also refer to ERROR_SHARING_VIOLATION ?

> >       mingw: make failures to unlink or move raise a question
> 
> Gaah! But people seem to like it. Since the question is only triggered after 
> all retries fail, I can live with this.
> 
> But isn't the implementation a bit sloppy? Can strlen(answer)-2 be negative? 
> What happens if the user typed more than 4 characters? Wouldn't it leave data 
> in the buffer for the next question?

I have extracted reading of the answer into its own function and made
the reading more robust which should now take care of the above issues.

> >       mingw: add fallback for rmdir in case directory is in use
> 
> Depends on the previous patch. OK.

No changes.

During pick up of the series I had to gather my testing script which can
only be used manually for testing. Is there any place in git.git where
we can store such "manual testing tools" ?

Cheers Heiko

Heiko Voigt (4):
  mingw: move unlink wrapper to mingw.c
  mingw: work around irregular failures of unlink on windows
  mingw: make failures to unlink or move raise a question
  mingw: add fallback for rmdir in case directory is in use

 compat/mingw.c |  139 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 compat/mingw.h |   14 ++---
 2 files changed, 144 insertions(+), 9 deletions(-)

-- 
1.7.2.2.177.geec0d

[PATCH v2 1/4] mingw: move unlink wrapper to mingw.c

[Heiko Voigt]

The next patch implements a workaround in case unlink fails on Windows.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 compat/mingw.c |    8 ++++++++
 compat/mingw.h |   11 +++--------
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 98163da..49c594f 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -155,6 +155,14 @@ int mingw_mkdir(const char *path, int mode)
 	return ret;
 }
 
+#undef unlink
+int mingw_unlink(const char *pathname)
+{
+	/* read-only files cannot be removed */
+	chmod(pathname, 0666);
+	return unlink(pathname);
+}
+
 #undef open
 int mingw_open (const char *filename, int oflags, ...)
 {
diff --git a/compat/mingw.h b/compat/mingw.h
index 56e58ba..220ae90 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -119,14 +119,6 @@ static inline int fcntl(int fd, int cmd, ...)
 int mingw_mkdir(const char *path, int mode);
 #define mkdir mingw_mkdir
 
-static inline int mingw_unlink(const char *pathname)
-{
-	/* read-only files cannot be removed */
-	chmod(pathname, 0666);
-	return unlink(pathname);
-}
-#define unlink mingw_unlink
-
 static inline pid_t waitpid(pid_t pid, int *status, unsigned options)
 {
 	if (options == 0)
@@ -177,6 +169,9 @@ int link(const char *oldpath, const char *newpath);
  * replacements of existing functions
  */
 
+int mingw_unlink(const char *pathname);
+#define unlink mingw_unlink
+
 int mingw_open (const char *filename, int oflags, ...);
 #define open mingw_open
 
-- 
1.7.2.2.177.geec0d

[PATCH v2 2/4] mingw: work around irregular failures of unlink on windows

[Heiko Voigt]

If a file is opened by another process (e.g. indexing of an IDE) for
reading it is not allowed to be deleted. So in case unlink fails retry
after waiting for some time. This extends the workaround from 6ac6f878.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 compat/mingw.c |   26 ++++++++++++++++++++++++--
 1 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 49c594f..bf9ffb8 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -5,6 +5,7 @@
 #include "../cache.h"
 
 unsigned int _CRT_fmode = _O_BINARY;
+static const int delay[] = { 0, 1, 10, 20, 40 };
 
 int err_win_to_posix(DWORD winerr)
 {
@@ -119,6 +120,13 @@ int err_win_to_posix(DWORD winerr)
 	return error;
 }
 
+static inline int is_file_in_use_error()
+{
+	if (GetLastError() == ERROR_SHARING_VIOLATION)
+		return 1;
+	return 0;
+}
+
 static int make_hidden(const char *path)
 {
 	DWORD attribs = GetFileAttributes(path);
@@ -158,9 +166,24 @@ int mingw_mkdir(const char *path, int mode)
 #undef unlink
 int mingw_unlink(const char *pathname)
 {
+	int ret, tries = 0;
+
 	/* read-only files cannot be removed */
 	chmod(pathname, 0666);
-	return unlink(pathname);
+	while ((ret = unlink(pathname)) == -1 && tries < ARRAY_SIZE(delay)) {
+		if (!is_file_in_use_error())
+			break;
+		/*
+		 * We assume that some other process had the source or
+		 * destination file open at the wrong moment and retry.
+		 * In order to give the other process a higher chance to
+		 * complete its operation, we give up our time slice now.
+		 * If we have to retry again, we do sleep a bit.
+		 */
+		Sleep(delay[tries]);
+		tries++;
+	}
+	return ret;
 }
 
 #undef open
@@ -1279,7 +1302,6 @@ int mingw_rename(const char *pold, const char *pnew)
 {
 	DWORD attrs, gle;
 	int tries = 0;
-	static const int delay[] = { 0, 1, 10, 20, 40 };
 
 	/*
 	 * Try native rename() first to get errno right.
-- 
1.7.2.2.177.geec0d

[PATCH v2 3/4] mingw: make failures to unlink or move raise a question

[Heiko Voigt]

On Windows in case a program is accessing a file unlink or
move operations may fail. To give the user a chance to correct
this we simply wait until the user asks us to retry or fail.

This is useful because of the following use case which seem
to happen rarely but when it does it is a mess:

After making some changes the user realizes that he was on the
incorrect branch. When trying to change the branch some file
is still in use by some other process and git stops in the
middle of changing branches. Now the user has lots of files
with changes mixed with his own. This is especially confusing
on repositories that contain lots of files.

Although the recent implementation of automatic retry makes
this scenario much more unlikely lets provide a fallback as
a last resort.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 compat/mingw.c |   82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 82 insertions(+), 0 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index bf9ffb8..b66bf89 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -3,6 +3,7 @@
 #include <conio.h>
 #include "../strbuf.h"
 #include "../cache.h"
+#include "../run-command.h"
 
 unsigned int _CRT_fmode = _O_BINARY;
 static const int delay[] = { 0, 1, 10, 20, 40 };
@@ -163,6 +164,78 @@ int mingw_mkdir(const char *path, int mode)
 	return ret;
 }
 
+static int read_yes_no_answer()
+{
+	char answer[1024];
+
+	if (fgets(answer, sizeof(answer), stdin)) {
+		size_t answer_len = strlen(answer);
+		int got_full_line = 0, c;
+
+		/* remove the newline */
+		if (answer_len >= 2 && answer[answer_len-2] == '\r') {
+			answer[answer_len-2] = '\0';
+			got_full_line = 1;
+		}
+		else if (answer_len >= 1 && answer[answer_len-1] == '\n') {
+			answer[answer_len-1] = '\0';
+			got_full_line = 1;
+		}
+		/* flush the buffer in case we did not get the full line */
+		if (!got_full_line)
+			while((c = getchar()) != EOF && c != '\n');
+	} else
+		/* we could not read, return the
+		 * default answer which is no */
+		return 0;
+
+	if (answer[0] == 'y' && strlen(answer) == 1)
+		return 1;
+	if (!strncasecmp(answer, "yes", sizeof(answer)))
+		return 1;
+	if (answer[0] == 'n' && strlen(answer) == 1)
+		return 0;
+	if (!strncasecmp(answer, "no", sizeof(answer)))
+		return 0;
+
+	/* did not find an answer we understand */
+	return -1;
+}
+
+static int ask_user_yes_no(const char *format, ...)
+{
+	char question[4096];
+	const char *retry_hook[] = { NULL, NULL, NULL };
+	va_list args;
+
+	if ((retry_hook[0] = getenv("GIT_ASK_YESNO"))) {
+
+		va_start(args, format);
+		vsnprintf(question, sizeof(question), format, args);
+		va_end(args);
+
+		retry_hook[1] = question;
+		return !run_command_v_opt(retry_hook, 0);
+	}
+
+	if (!isatty(_fileno(stdin)))
+		return 0;
+
+	while (1) {
+		int answer;
+		va_start(args, format);
+		vfprintf(stderr, format, args);
+		va_end(args);
+		fprintf(stderr, " (y/n)? ");
+
+		if ((answer = read_yes_no_answer()) >= 0)
+			return answer;
+
+		fprintf(stderr, "Sorry, I did not understand your answer. "
+				"Please type 'y' or 'n'\n");
+	}
+}
+
 #undef unlink
 int mingw_unlink(const char *pathname)
 {
@@ -183,6 +256,10 @@ int mingw_unlink(const char *pathname)
 		Sleep(delay[tries]);
 		tries++;
 	}
+	while (ret == -1 && is_file_in_use_error() &&
+	       ask_user_yes_no("Unlink of file '%s' failed. "
+			"Should I try again?", pathname))
+	       ret = unlink(pathname);
 	return ret;
 }
 
@@ -1343,6 +1420,11 @@ repeat:
 		tries++;
 		goto repeat;
 	}
+	if (gle == ERROR_ACCESS_DENIED &&
+	       ask_user_yes_no("Rename from '%s' to '%s' failed. "
+		       "Should I try again?", pold, pnew))
+		goto repeat;
+
 	errno = EACCES;
 	return -1;
 }
-- 
1.7.2.2.177.geec0d

[PATCH v2 4/4] mingw: add fallback for rmdir in case directory is in use

[Heiko Voigt]

From: Heiko Voigt <heiko.voigt@mahr.de>

The same logic as for unlink and rename also applies to rmdir. For
example in case you have a shell open in a git controlled folder. This
will easily fail. So lets be nice for such cases as well.

Signed-off-by: Heiko Voigt <heiko.voigt@mahr.de>
Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 compat/mingw.c |   25 +++++++++++++++++++++++++
 compat/mingw.h |    3 +++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index b66bf89..8e4f1e2 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -263,6 +263,31 @@ int mingw_unlink(const char *pathname)
 	return ret;
 }
 
+#undef rmdir
+int mingw_rmdir(const char *pathname)
+{
+    int ret, tries = 0;
+
+	while ((ret = rmdir(pathname)) == -1 && tries < ARRAY_SIZE(delay)) {
+		if (!is_file_in_use_error())
+			break;
+		/*
+		 * We assume that some other process had the source or
+		 * destination file open at the wrong moment and retry.
+		 * In order to give the other process a higher chance to
+		 * complete its operation, we give up our time slice now.
+		 * If we have to retry again, we do sleep a bit.
+		 */
+		Sleep(delay[tries]);
+		tries++;
+	}
+	while (ret == -1 && is_file_in_use_error() &&
+	       ask_user_yes_no("Deletion of directory '%s' failed. "
+			"Should I try again?", pathname))
+	       ret = rmdir(pathname);
+	return ret;
+}
+
 #undef open
 int mingw_open (const char *filename, int oflags, ...)
 {
diff --git a/compat/mingw.h b/compat/mingw.h
index 220ae90..cf69d39 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -172,6 +172,9 @@ int link(const char *oldpath, const char *newpath);
 int mingw_unlink(const char *pathname);
 #define unlink mingw_unlink
 
+int mingw_rmdir(const char *path);
+#define rmdir mingw_rmdir
+
 int mingw_open (const char *filename, int oflags, ...);
 #define open mingw_open
 
-- 
1.7.2.2.177.geec0d

Re: [PATCH v2 0/4] make open/unlink failures user friendly on windows using retry/abort

[Johannes Sixt]

On Sonntag, 7. November 2010, Heiko Voigt wrote:
> j6t: I have not changed your error code in mingw_rename since you
> explicitely compare with the windows error code ERROR_ACCESS_DENIED and
> do not use the err_win_to_posix() function. Did you do this on purpose or
> should I also refer to ERROR_SHARING_VIOLATION ?

IIRC, I dumped the error code before I implemented the retry logic, and the 
error was ERROR_ACCESS_DENIED. Did you verify that the occasions where you 
want to retry the operations indeed fail due to ERROR_SHARING_VIOLATION, or 
do you trust the documentation?

In any case, it is better to check for the Windows error code (which ever you 
finally choose) rather than errno, IMO.

-- Hannes

Re: Re: [PATCH v2 0/4] make open/unlink failures user friendly on windows using retry/abort

[Heiko Voigt]

Hi,

On Sun, Nov 07, 2010 at 04:49:20PM +0100, Johannes Sixt wrote:
> On Sonntag, 7. November 2010, Heiko Voigt wrote:
> > j6t: I have not changed your error code in mingw_rename since you
> > explicitely compare with the windows error code ERROR_ACCESS_DENIED and
> > do not use the err_win_to_posix() function. Did you do this on purpose or
> > should I also refer to ERROR_SHARING_VIOLATION ?
> 
> IIRC, I dumped the error code before I implemented the retry logic, and the 
> error was ERROR_ACCESS_DENIED. Did you verify that the occasions where you 
> want to retry the operations indeed fail due to ERROR_SHARING_VIOLATION, or 
> do you trust the documentation?

I tested this for my use case which is described by the manual testing
script[1]. I ran it once to create the test structure and then from
inside the test repository

	./../block_read 5000 file

and manually checked out the other branch in parallel. In this situation
git will stop with the retry question. But I am not sure which use case you
where addressing. Do you have a script or some information to reproduce?

> In any case, it is better to check for the Windows error code (which ever you 
> finally choose) rather than errno, IMO.

I agree because that should give us more fine grained control and since in
err_win_to_posix() many codes are mapped to EACCES.

Cheers Heiko

[1] http://groups.google.com/group/msysgit/msg/fcad5f1455b263ca 

Re: [PATCH v2 0/4] make open/unlink failures user friendly on windows using retry/abort

[Johannes Sixt]

On Sonntag, 7. November 2010, Heiko Voigt wrote:
> I tested this for my use case which is described by the manual testing
> script[1]. I ran it once to create the test structure and then from
> inside the test repository
>
> 	./../block_read 5000 file
>
> and manually checked out the other branch in parallel. In this situation
> git will stop with the retry question. But I am not sure which use case you
> where addressing. Do you have a script or some information to reproduce?

Sorry, I don't have a reliable test case. I usually encountered this problem 
in the test suite at random places when it was run with 'make -j2' and in a 
production repository sometimes during rebase --interactive.

-- Hannes