threads / patch / 26787

patchrepack: find -> /usr/bin/find, as for cygwin

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

## tl;dr

14 messages between Mar 19, 2011 and Mar 21, 2011. Diffs are folded; open one to read it.

replies: 13people: 6as markdown or json

ryenus ◇· Mar 19, 2011, 12:08 UTC · lore

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(-)
Show changes to git-repack.sh +1 −3
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
Nguyen Thai Ngoc Duy· Mar 19, 2011, 12:18 UTC · re: ryenus ◇ · lore

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

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
René Scharfe· Mar 19, 2011, 15:50 UTC · re: Nguyen Thai Ngoc Duy · lore

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

Am 19.03.2011 13:18, schrieb Nguyen Thai Ngoc Duy:
Show 11 quoted lines
> 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(-)
Show changes to git-repack.sh +10 −9
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
Nguyen Thai Ngoc Duy· Mar 19, 2011, 16:07 UTC · re: René Scharfe · lore

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

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

Show changes to git-sh-setup.sh +0 −3
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
Nguyen Thai Ngoc Duy· Mar 19, 2011, 16:15 UTC · re: Nguyen Thai Ngoc Duy · lore

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

On Sat, Mar 19, 2011 at 11:07 PM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:

Show 7 quoted lines
> 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
ryenus ◇· Mar 19, 2011, 16:32 UTC · re: Nguyen Thai Ngoc Duy · lore

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

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(-)
Show changes to git-sh-setup.sh +1 −2
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
ryenus ◇· Mar 19, 2011, 16:43 UTC · re: ryenus ◇ · lore

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

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:
Show 25 quoted lines
> 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
>
Nguyen Thai Ngoc Duy· Mar 19, 2011, 16:47 UTC · re: ryenus ◇ · lore

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

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
Junio C Hamano· Mar 19, 2011, 18:17 UTC · re: ryenus ◇ · lore

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

ryenus ◇ <ryenus@gmail.com> writes:
Show 19 quoted lines
> 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.

ryenus ◇· Mar 20, 2011, 00:31 UTC · re: Junio C Hamano · lore

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

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>:
Show 30 quoted lines
> 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.
>
>
>
ryenus ◇· Mar 20, 2011, 00:35 UTC · re: ryenus ◇ · lore

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

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:
Show 58 quoted lines
> 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.
>>
>>
>>
>
Matthieu Moy· Mar 20, 2011, 07:48 UTC · re: ryenus ◇ · lore

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

ryenus ◇ <ryenus@gmail.com> writes:
Show 6 quoted lines
> 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*.

Show 6 quoted lines
>   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/
ryenus ◇· Mar 20, 2011, 08:42 UTC · re: Matthieu Moy · lore

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

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:

Show 32 quoted lines
> 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/
>
Erik Faye-Lund· Mar 21, 2011, 09:36 UTC · re: ryenus ◇ · lore

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

On Sun, Mar 20, 2011 at 1:35 AM, ryenus ◇ <ryenus@gmail.com> wrote:
Show 16 quoted lines
> 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

← back to recent threads