# [PATCH] repack: find -> /usr/bin/find, as for cygwin 14 messages from 2011-03-19 to 2011-03-21. Participants: ryenus ◇, Nguyen Thai Ngoc Duy, René Scharfe, Junio C Hamano, Matthieu Moy, Erik Faye-Lund. Thread: https://gitlist.dev/t/26787 ## ryenus ◇, 2011-03-19 12:08 Subject: [PATCH] repack: find -> /usr/bin/find, as for cygwin Message-ID: URL: https://gitlist.dev/e/AANLkTimHof_MNSGbU2KGX%3D7Q3MQpjkzXK%2BxyGGVjbngR%40mail.gmail.com ``` 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 --- 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 ``` ## Nguyen Thai Ngoc Duy, 2011-03-19 12:18 Subject: Re: [PATCH] repack: find -> /usr/bin/find, as for cygwin Message-ID: URL: https://gitlist.dev/e/AANLkTimPbz2s%3DMaafhqg-7wOk_TT4fFSh7AQ-3rWY0A3%40mail.gmail.com In-Reply-To: ``` On Sat, Mar 19, 2011 at 7:08 PM, ryenus ◇ 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, 2011-03-19 15:50 Subject: Re: [PATCH] repack: find -> /usr/bin/find, as for cygwin Message-ID: <4D84D0C0.7080808@lsrfire.ath.cx> URL: https://gitlist.dev/e/4D84D0C0.7080808%40lsrfire.ath.cx In-Reply-To: ``` Am 19.03.2011 13:18, schrieb Nguyen Thai Ngoc Duy: > On Sat, Mar 19, 2011 at 7:08 PM, ryenus ◇ 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 ``` ## Nguyen Thai Ngoc Duy, 2011-03-19 16:07 Subject: Re: [PATCH] repack: find -> /usr/bin/find, as for cygwin Message-ID: <20110319160754.GA5871@do> URL: https://gitlist.dev/e/20110319160754.GA5871%40do In-Reply-To: <4D84D0C0.7080808@lsrfire.ath.cx> ``` 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 ``` ## Nguyen Thai Ngoc Duy, 2011-03-19 16:15 Subject: Re: [PATCH] repack: find -> /usr/bin/find, as for cygwin Message-ID: URL: https://gitlist.dev/e/AANLkTinncw6GSLf3QvxjqKoaqesH9SWxrn5TENnHDj4Y%40mail.gmail.com In-Reply-To: <20110319160754.GA5871@do> ``` On Sat, Mar 19, 2011 at 11:07 PM, Nguyen Thai Ngoc Duy 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 ``` ## ryenus ◇, 2011-03-19 16:32 Subject: Re: [PATCH] repack: find -> /usr/bin/find, as for cygwin Message-ID: URL: https://gitlist.dev/e/AANLkTinxyp%3DPbvzRkyyxXin5aOFm1NP4Be6U2Dv0aD6d%40mail.gmail.com In-Reply-To: ``` 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 ``` ## ryenus ◇, 2011-03-19 16:43 Subject: Re: [PATCH] repack: find -> /usr/bin/find, as for cygwin Message-ID: URL: https://gitlist.dev/e/AANLkTi%3DEQhtOM3dR8V5dxP%3DAfNxWuSFoTbvzL36q3rXL%40mail.gmail.com In-Reply-To: ``` 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 ◇ 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 > ``` ## Nguyen Thai Ngoc Duy, 2011-03-19 16:47 Subject: Re: [PATCH] repack: find -> /usr/bin/find, as for cygwin Message-ID: URL: https://gitlist.dev/e/AANLkTi%3DiMZWTG8DJWSrkjzNR6FFkc_mo_epQLNfkwZiV%40mail.gmail.com In-Reply-To: ``` On Sat, Mar 19, 2011 at 11:43 PM, ryenus ◇ 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, 2011-03-19 18:17 Subject: Re: [PATCH] repack: find -> /usr/bin/find, as for cygwin Message-ID: <7vsjujq8kf.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vsjujq8kf.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` ryenus ◇ 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 ◇, 2011-03-20 00:31 Subject: Re: [PATCH] repack: find -> /usr/bin/find, as for cygwin Message-ID: URL: https://gitlist.dev/e/AANLkTinPCeg3NU2bRvk8rwWSWnu4b0PHwya9%2BPWAc3DB%40mail.gmail.com In-Reply-To: <7vsjujq8kf.fsf@alter.siamese.dyndns.org> ``` 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 : > ryenus ◇ 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 ◇, 2011-03-20 00:35 Subject: Re: [PATCH] repack: find -> /usr/bin/find, as for cygwin Message-ID: URL: https://gitlist.dev/e/AANLkTinFFnm7-TKcbTdbzJHx4i4L8rdi4xogr3R7%3D7s1%40mail.gmail.com In-Reply-To: ``` 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 ◇ 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 : >> ryenus ◇ 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, 2011-03-20 07:48 Subject: Re: [PATCH] repack: find -> /usr/bin/find, as for cygwin Message-ID: URL: https://gitlist.dev/e/vpqlj0ajkqa.fsf%40bauges.imag.fr In-Reply-To: ``` ryenus ◇ 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/ ``` ## ryenus ◇, 2011-03-20 08:42 Subject: Re: [PATCH] repack: find -> /usr/bin/find, as for cygwin Message-ID: URL: https://gitlist.dev/e/AANLkTim%2BsijuHidJNWyqC75yTFVo0YX67J3AXnPJVsoc%40mail.gmail.com In-Reply-To: ``` 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 wrote: > ryenus ◇ 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, 2011-03-21 09:36 Subject: Re: [PATCH] repack: find -> /usr/bin/find, as for cygwin Message-ID: URL: https://gitlist.dev/e/AANLkTikNprk74FnSfw%3Dn3ePvwOZT_WaeSpnKQCqD9mpa%40mail.gmail.com In-Reply-To: ``` On Sun, Mar 20, 2011 at 1:35 AM, ryenus ◇ 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 ```