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

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

[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 <[hidden email]>
---
 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Duy Nguyen
On Sat, Mar 19, 2011 at 7:08 PM, ryenus ◇ <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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 ◇<[hidden email]>  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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Duy Nguyen
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Duy Nguyen
On Sat, Mar 19, 2011 at 11:07 PM, Nguyen Thai Ngoc Duy
<[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

ryenus ◇
In reply to this post by Duy Nguyen
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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 ◇ <[hidden email]> 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
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Duy Nguyen
On Sat, Mar 19, 2011 at 11:43 PM, ryenus ◇ <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
In reply to this post by ryenus ◇
ryenus ◇ <[hidden email]> 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.


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]>:

> ryenus ◇ <[hidden email]> 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.
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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 ◇ <[hidden email]> 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 <[hidden email]>:
>> ryenus ◇ <[hidden email]> 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.
>>
>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Matthieu Moy-2
ryenus ◇ <[hidden email]> 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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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
<[hidden email]> wrote:

> ryenus ◇ <[hidden email]> 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/
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Erik Faye-Lund-2
In reply to this post by ryenus ◇
On Sun, Mar 20, 2011 at 1:35 AM, ryenus ◇ <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html