[PATCH] repack: find -> /usr/bin/find, as for cygwin

[PATCH] repack: find -> /usr/bin/find, as for cygwin

[ryenus ◇]

If I run Cygwin git directly from cmd.exe instead of from a shell,
e.g. bash, I get the following error when executing git repack

FIND: Parameter format not correct

that's because in git-repack.sh, 'find' is called without its full
path, this patch corrects this

Signed-off-by: ryenus <ryenus@gmail.com>
---
 git-repack.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-repack.sh b/git-repack.sh
index 624feec..212caa7 100755
--- a/git-repack.sh
+++ b/git-repack.sh
@@ -64,7 +64,7 @@ case ",$all_into_one," in
 ,t,)
        args= existing=
        if [ -d "$PACKDIR" ]; then
-               for e in `cd "$PACKDIR" && find . -type f -name '*.pack' \
+               for e in `cd "$PACKDIR" && /usr/bin/find . -type f
-name '*.pack' \
                        | sed -e 's/^\.\///' -e 's/\.pack$//'`
                do
                        if [ -e "$PACKDIR/$e.keep" ]; then
--
1.7.4

Re: [PATCH] repack: find -> /usr/bin/find, as for cygwin

[Nguyen Thai Ngoc Duy]

On Sat, Mar 19, 2011 at 7:08 PM, ryenus ◇ <ryenus@gmail.com> wrote:
> -               for e in `cd "$PACKDIR" && find . -type f -name '*.pack' \
> +               for e in `cd "$PACKDIR" && /usr/bin/find . -type f

I'd rather have something like in test-lib.sh (with conditions)

find() {
/usr/bin/find "$@"
}

Even better, rewrite this script to C.
-- 
Duy

Re: [PATCH] repack: find -> /usr/bin/find, as for cygwin

[René Scharfe]

Am 19.03.2011 13:18, schrieb Nguyen Thai Ngoc Duy:
> On Sat, Mar 19, 2011 at 7:08 PM, ryenus ◇<ryenus@gmail.com>  wrote:
>> -               for e in `cd "$PACKDIR"&&  find . -type f -name '*.pack' \
>> +               for e in `cd "$PACKDIR"&&  /usr/bin/find . -type f
> 
> I'd rather have something like in test-lib.sh (with conditions)
> 
> find() {
> /usr/bin/find "$@"
> }
> 
> Even better, rewrite this script to C.

That's a good idea, but it's a lot more involved than the original
patch.

Do we need to support pack files in subdirectories of $PACKDIR?  If
not -- and I don't immediately see why, except that the current code
does with its find call -- then the following patch might be a quick
bandaid.  Untested, please be careful.

René


 git-repack.sh |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/git-repack.sh b/git-repack.sh
index 624feec..4e49079 100755
--- a/git-repack.sh
+++ b/git-repack.sh
@@ -64,15 +64,16 @@ case ",$all_into_one," in
 ,t,)
 	args= existing=
 	if [ -d "$PACKDIR" ]; then
-		for e in `cd "$PACKDIR" && find . -type f -name '*.pack' \
-			| sed -e 's/^\.\///' -e 's/\.pack$//'`
-		do
-			if [ -e "$PACKDIR/$e.keep" ]; then
-				: keep
-			else
-				existing="$existing $e"
-			fi
-		done
+		existing=$(
+			cd "$PACKDIR" &&
+			for e in *.pack
+			do
+				if test -f "$e" -a ! -e "${e%.pack}.keep"
+				then
+					echo "${e%.pack}"
+				fi
+			done
+		)
 		if test -n "$existing" -a -n "$unpack_unreachable" -a \
 			-n "$remove_redundant"
 		then

Re: [PATCH] repack: find -> /usr/bin/find, as for cygwin

[Nguyen Thai Ngoc Duy]

On Sat, Mar 19, 2011 at 04:50:24PM +0100, René Scharfe wrote:
> Do we need to support pack files in subdirectories of $PACKDIR?  If
> not -- and I don't immediately see why, except that the current code
> does with its find call -- then the following patch might be a quick
> bandaid.  Untested, please be careful.

I looked at test-lib.sh but forgot git-sh-setup.sh, which does
aliasing for find in MINGW build. With your patch, the last use of
find is gone. So we might as well do this

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index aa16b83..e891edc 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -232,9 +232,6 @@ case $(uname -s) in
 	sort () {
 		/usr/bin/sort "$@"
 	}
-	find () {
-		/usr/bin/find "$@"
-	}
 	is_absolute_path () {
 		case "$1" in
 		[/\\]* | [A-Za-z]:*)

-- 
Duy

Re: [PATCH] repack: find -> /usr/bin/find, as for cygwin

[Nguyen Thai Ngoc Duy]

On Sat, Mar 19, 2011 at 11:07 PM, Nguyen Thai Ngoc Duy
<pclouds@gmail.com> wrote:
> I looked at test-lib.sh but forgot git-sh-setup.sh, which does
> aliasing for find in MINGW build. With your patch, the last use of
> find is gone. So we might as well do this
>
> -       find () {
> -               /usr/bin/find "$@"
> -       }

On second thought, no. We probably need to do an unconditional alias

find() {
    die "find is not supported"
}

to make sure no one will ever use it again.
-- 
Duy

Re: [PATCH] repack: find -> /usr/bin/find, as for cygwin

[ryenus ◇]

Thank you, Duy, you're almost right, I just checked git-sh-setup.sh,
in the bottom, sort and find are defined as functions like what you
pointed out, but only for MinGW, therefore a better fix is to check
for cygwin as well:

---
 git-sh-setup.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index aa16b83..5c52ae4 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -227,7 +227,7 @@ fi

 # Fix some commands on Windows
 case $(uname -s) in
-*MINGW*)
+*MINGW*|*CYGWIN*)
        # Windows has its own (incompatible) sort and find
        sort () {
                /usr/bin/sort "$@"
--
1.7.4

Re: [PATCH] repack: find -> /usr/bin/find, as for cygwin

[ryenus ◇]

OK, I've been away for a while and didn't notice latest replies :-) do
you mean find is not used elsewhere in git?

Anyway, looks like checking for both MinGW and Cygwin still applies.

Thanks

On Sun, Mar 20, 2011 at 00:32, ryenus ◇ <ryenus@gmail.com> wrote:
> Thank you, Duy, you're almost right, I just checked git-sh-setup.sh,
> in the bottom, sort and find are defined as functions like what you
> pointed out, but only for MinGW, therefore a better fix is to check
> for cygwin as well:
>
> ---
>  git-sh-setup.sh |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index aa16b83..5c52ae4 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -227,7 +227,7 @@ fi
>
>  # Fix some commands on Windows
>  case $(uname -s) in
> -*MINGW*)
> +*MINGW*|*CYGWIN*)
>        # Windows has its own (incompatible) sort and find
>        sort () {
>                /usr/bin/sort "$@"
> --
> 1.7.4
>

Re: [PATCH] repack: find -> /usr/bin/find, as for cygwin

[Nguyen Thai Ngoc Duy]

On Sat, Mar 19, 2011 at 11:43 PM, ryenus ◇ <ryenus@gmail.com> wrote:
> OK, I've been away for a while and didn't notice latest replies :-) do
> you mean find is not used elsewhere in git?

That's what 'git grep find *.sh' told me. Anyway I suppose our
testsuites cover all commands quite good so we would notice if any
other commands still use 'find'.

> Anyway, looks like checking for both MinGW and Cygwin still applies.

I don't use cygwin so I don't know if cygwin users are happy with
that. But it looks ok (unless some users decide to move find to
another place)
-- 
Duy

Re: [PATCH] repack: find -> /usr/bin/find, as for cygwin

[Junio C Hamano]

ryenus ◇ <ryenus@gmail.com> writes:

> Thank you, Duy, you're almost right, I just checked git-sh-setup.sh,
> in the bottom, sort and find are defined as functions like what you
> pointed out, but only for MinGW, therefore a better fix is to check
> for cygwin as well:
>
> ---
>  git-sh-setup.sh |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index aa16b83..5c52ae4 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -227,7 +227,7 @@ fi
>
>  # Fix some commands on Windows
>  case $(uname -s) in
> -*MINGW*)
> +*MINGW*|*CYGWIN*)

This looks like a more sensible alternative than forbidding the use of
"find", privided if the new pattern is an appropriate one to catch cygwin.

I don't have any Windows boxes, so I cannot verify, but the patch smells
correct.

Re: [PATCH] repack: find -> /usr/bin/find, as for cygwin

[ryenus ◇]

I'm not sure if there's a set of tests for Cygwin/MinGW among all the
test cases in GIT, here is a simple one:

#!/bin/sh
echo $(uname -s)
case $(uname -s) in
*MINGW*|*CYGWIN*)
  echo "detected MinGW/Cygwin"
  ;;
*MinGW*)
  echo "detected MinGW"
  ;;
*Cygwin*)
  echo "detected Cygwin"
  ;;
esac

Run with dash, the output is

CYGWIN_NT-6.1
detected MinGW/Cygwin

While I don't have MinGW, so someone has it please give it a shot.

Thanks

2011/3/20 Junio C Hamano <gitster@pobox.com>:
> ryenus ◇ <ryenus@gmail.com> writes:
>
>> Thank you, Duy, you're almost right, I just checked git-sh-setup.sh,
>> in the bottom, sort and find are defined as functions like what you
>> pointed out, but only for MinGW, therefore a better fix is to check
>> for cygwin as well:
>>
>> ---
>>  git-sh-setup.sh |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
>> index aa16b83..5c52ae4 100644
>> --- a/git-sh-setup.sh
>> +++ b/git-sh-setup.sh
>> @@ -227,7 +227,7 @@ fi
>>
>>  # Fix some commands on Windows
>>  case $(uname -s) in
>> -*MINGW*)
>> +*MINGW*|*CYGWIN*)
>
> This looks like a more sensible alternative than forbidding the use of
> "find", privided if the new pattern is an appropriate one to catch cygwin.
>
> I don't have any Windows boxes, so I cannot verify, but the patch smells
> correct.
>
>
>

Re: [PATCH] repack: find -> /usr/bin/find, as for cygwin

[ryenus ◇]

oops, corrected the script with the test strings in upper cases

#!/bin/sh
echo $(uname -s)
case $(uname -s) in
*MINGW*|*CYGWIN*)
  echo "detected MinGW/Cygwin"
  ;;
*MINGW*)
  echo "detected MinGW"
  ;;
*CYGWIN*)
  echo "detected Cygwin"
  ;;
esac


On Sun, Mar 20, 2011 at 08:31, ryenus ◇ <ryenus@gmail.com> wrote:
> I'm not sure if there's a set of tests for Cygwin/MinGW among all the
> test cases in GIT, here is a simple one:
>
> #!/bin/sh
> echo $(uname -s)
> case $(uname -s) in
> *MINGW*|*CYGWIN*)
>  echo "detected MinGW/Cygwin"
>  ;;
> *MinGW*)
>  echo "detected MinGW"
>  ;;
> *Cygwin*)
>  echo "detected Cygwin"
>  ;;
> esac
>
> Run with dash, the output is
>
> CYGWIN_NT-6.1
> detected MinGW/Cygwin
>
> While I don't have MinGW, so someone has it please give it a shot.
>
> Thanks
>
> 2011/3/20 Junio C Hamano <gitster@pobox.com>:
>> ryenus ◇ <ryenus@gmail.com> writes:
>>
>>> Thank you, Duy, you're almost right, I just checked git-sh-setup.sh,
>>> in the bottom, sort and find are defined as functions like what you
>>> pointed out, but only for MinGW, therefore a better fix is to check
>>> for cygwin as well:
>>>
>>> ---
>>>  git-sh-setup.sh |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
>>> index aa16b83..5c52ae4 100644
>>> --- a/git-sh-setup.sh
>>> +++ b/git-sh-setup.sh
>>> @@ -227,7 +227,7 @@ fi
>>>
>>>  # Fix some commands on Windows
>>>  case $(uname -s) in
>>> -*MINGW*)
>>> +*MINGW*|*CYGWIN*)
>>
>> This looks like a more sensible alternative than forbidding the use of
>> "find", privided if the new pattern is an appropriate one to catch cygwin.
>>
>> I don't have any Windows boxes, so I cannot verify, but the patch smells
>> correct.
>>
>>
>>
>

Re: [PATCH] repack: find -> /usr/bin/find, as for cygwin

[Matthieu Moy]

ryenus ◇ <ryenus@gmail.com> writes:

> oops, corrected the script with the test strings in upper cases
>
> #!/bin/sh
> echo $(uname -s)
> case $(uname -s) in
> *MINGW*|*CYGWIN*)
         ^
This "|" means "or" in a case statement...

>   echo "detected MinGW/Cygwin"
>   ;;
> *MINGW*)

...so I can see no way to reach this point: if the string matches
*MINGW*, it also matches *MINGW*|*CYGWIN*.

>   echo "detected MinGW"
>   ;;
> *CYGWIN*)
>   echo "detected Cygwin"
>   ;;
> esac

But you've just showed that $(uname -s) of Cygwin did contain upper-case
CYGWIN, which I think was the point to verify :-).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] repack: find -> /usr/bin/find, as for cygwin

[ryenus ◇]

Hey Matthieu,

Yes, I mean it.

The purpose of this test script is to testify that "*MINGW*|*CYGWIN*"
will match MinGW and/or Cygwin, so that it won't fall down to the next
2 cases.


On Sun, Mar 20, 2011 at 15:48, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> ryenus ◇ <ryenus@gmail.com> writes:
>
>> oops, corrected the script with the test strings in upper cases
>>
>> #!/bin/sh
>> echo $(uname -s)
>> case $(uname -s) in
>> *MINGW*|*CYGWIN*)
>         ^
> This "|" means "or" in a case statement...
>
>>   echo "detected MinGW/Cygwin"
>>   ;;
>> *MINGW*)
>
> ...so I can see no way to reach this point: if the string matches
> *MINGW*, it also matches *MINGW*|*CYGWIN*.
>
>>   echo "detected MinGW"
>>   ;;
>> *CYGWIN*)
>>   echo "detected Cygwin"
>>   ;;
>> esac
>
> But you've just showed that $(uname -s) of Cygwin did contain upper-case
> CYGWIN, which I think was the point to verify :-).
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/
>

Re: [PATCH] repack: find -> /usr/bin/find, as for cygwin

[Erik Faye-Lund]

On Sun, Mar 20, 2011 at 1:35 AM, ryenus ◇ <ryenus@gmail.com> wrote:
> oops, corrected the script with the test strings in upper cases
>
> #!/bin/sh
> echo $(uname -s)
> case $(uname -s) in
> *MINGW*|*CYGWIN*)
>  echo "detected MinGW/Cygwin"
>  ;;
> *MINGW*)
>  echo "detected MinGW"
>  ;;
> *CYGWIN*)
>  echo "detected Cygwin"
>  ;;
> esac
>

Output:
MINGW32_NT-6.1
detected MinGW/Cygwin