Re: [PATCH v2] builtin-blame: Reencode commit messages according to git-log rules.

Jeff King [off-list ref] writes:

On Wed, Oct 22, 2008 at 12:55:57AM +0400, Alexander Gavrilov wrote:

quoted

+test_expect_success \
+	'blame respects i18n.commitencoding' '
+	git blame --incremental file | \
+		grep "^\(author\|summary\) " > actual &&
+	test_cmp actual expected

Even though it is POSIX, using backslashed grouping in 'grep' isn't
portable. It fails at least on Solaris 8, and you have to do:

  egrep "^(author|summary) "

instead. Of course, I can't get your test to pass even with that change,
but I think that is just a broken iconv on Solaris.

Yuck.  Solaris 8 /usr/bin/grep does not even grok "-e", so we cannot do a
more obvious:

	grep -e "^author " -e "^summary "

Do people build with NO_EXTERNAL_GREP on older Solaris?

git-submodule.sh uses grep "-e" to look for two patterns and I suspect
older Solaris would have the same issue.

On Wed, Oct 22, 2008 at 12:07:48PM -0700, Junio C Hamano wrote:

Yuck.  Solaris 8 /usr/bin/grep does not even grok "-e", so we cannot do a
more obvious:

	grep -e "^author " -e "^summary "

Yep. I already introduced one use of egrep for a similar case in
8753941 (tests: grep portability fixes).

Do people build with NO_EXTERNAL_GREP on older Solaris?

Yep. See:

  http://repo.or.cz/w/git/gitbuild.git?a=blob;f=jk/solaris/config.mak;hb=platform

for the gory details (boy, I wish we had nice PATH_INFO-based gitweb
URLs...).

git-submodule.sh uses grep "-e" to look for two patterns and I suspect
older Solaris would have the same issue.

Yes, that code will break on Solaris. Most of my portability fixes have
been in direct response to tests, so I guess we are not testing
git-submodule very well.

-Peff

On Wed, Oct 22, 2008 at 03:14:16PM -0400, Jeff King wrote:

quoted

git-submodule.sh uses grep "-e" to look for two patterns and I suspect
older Solaris would have the same issue.

Yes, that code will break on Solaris. Most of my portability fixes have
been in direct response to tests, so I guess we are not testing
git-submodule very well.

And here's a patch. Though I believe this is the last "grep -e", I
wonder if it wouldn't have been wiser to simply force people on such
platforms to use GNU grep (I already have to use GNU tools to build, and
bash to run the scripts).

-- >8 --
submodule: fix some non-portable grep invocations

Not all greps support "-e", but in this case we can easily
convert it to a single extended regex.

Signed-off-by: Jeff King <redacted>
---
Passes the test scripts, but I'm not sure they are exercising this code,
anyway, since it passed on Solaris. Please double-check my conversion.

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

diff --git a/git-submodule.sh b/git-submodule.sh
index 65178ae..b63e5c3 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -424,7 +424,7 @@ cmd_summary() {
 	cd_to_toplevel
 	# Get modified modules cared by user
 	modules=$(git diff-index $cached --raw $head -- "$@" |
-		grep -e '^:160000' -e '^:[0-7]* 160000' |
+		egrep '^:([0-7]* )?160000' |
 		while read mod_src mod_dst sha1_src sha1_dst status name
 		do
 			# Always show modules deleted or type-changed (blob<->module)

@@ -438,7 +438,7 @@ cmd_summary() {
 	test -z "$modules" && return
 
 	git diff-index $cached --raw $head -- $modules |
-	grep -e '^:160000' -e '^:[0-7]* 160000' |
+	egrep '^:([0-7]* )?160000' |
 	cut -c2- |
 	while read mod_src mod_dst sha1_src sha1_dst status name
 	do

-- 
1.6.0.2.825.g6d19d

Jeff King wrote:

quoted

Do people build with NO_EXTERNAL_GREP on older Solaris?

Yep. See:

  http://repo.or.cz/w/git/gitbuild.git?a=blob;f=jk/solaris/config.mak;hb=platform

for the gory details (boy, I wish we had nice PATH_INFO-based gitweb
URLs...).

Currently you can use path_info URL for blob_plain

  http://repo.or.cz/w/git/gitbuild.git/platform:/jk/solaris/config.mak

Soon (thanks to Giuseppe patches) you would be able to use

  http://repo.or.cz/w/git/gitbuild.git/blob/platform:/jk/solaris/config.mak

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Hi,

On Wed, 22 Oct 2008, Jeff King wrote:

submodule: fix some non-portable grep invocations

Not all greps support "-e", but in this case we can easily
convert it to a single extended regex.

I really wonder if we cannot catch these things (unportable grep, sed, etc 
invokations) with a simple patch to the pre-commit hook.

Ciao,
Dscho

On Wed, Oct 22, 2008 at 10:29:39PM +0200, Johannes Schindelin wrote:

quoted

Not all greps support "-e", but in this case we can easily
convert it to a single extended regex.

I really wonder if we cannot catch these things (unportable grep, sed, etc 
invokations) with a simple patch to the pre-commit hook.

We could probably write a hook for some of the simpler ones, but we
would have quite a few false negatives, I suspect.

FWIW, I am not finding these portability problems by hand. I am nightly
auto-building and testing Junio's maint, master, and next on Solaris 8
and FreeBSD, and Mike Ralphson is doing the same for AIX. So while we
could perhaps catch them sooner, I am very happy to have caught several
issues recently in next, _before_ they hit master.

-Peff

On Wed, Oct 22, 2008 at 10:12:54PM +0200, Jakub Narebski wrote:

Currently you can use path_info URL for blob_plain

  http://repo.or.cz/w/git/gitbuild.git/platform:/jk/solaris/config.mak

Ah, I didn't know that. Thanks.

Soon (thanks to Giuseppe patches) you would be able to use

  http://repo.or.cz/w/git/gitbuild.git/blob/platform:/jk/solaris/config.mak

Yes, those patches were actually what spurred my comment. :) I am
looking forward to playing with them once they are on repo.or.cz.

-Peff