[PATCH 1/2] [TopGit] Portability: Use tr instead of sed for translating spaces to newlines

[PATCH 1/2] [TopGit] Portability: Use tr instead of sed for translating spaces to newlines

[Brian Campbell]

It is not portable to use \n in the replacement of a sed substitute
command.  On Mac OS X (and presumably FreeBSD, as Mac OS X uses
sed from FreeBSD), \n in the replacement of a substitution gives a
literal "n" instead of a newline.

This patch replaces the sed command with tr, which should be more
portable.

Signed-off-by: Brian Campbell <brian.p.campbell@dartmouth.edu>
---
 tg-create.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tg-create.sh b/tg-create.sh
index 2edd537..ef9a955 100644
--- a/tg-create.sh
+++ b/tg-create.sh
@@ -116,7 +116,7 @@ done
 git update-ref "refs/top-bases/$name" "HEAD" ""
 git checkout -b "$name"
 
-echo "$deps" | sed 's/ /\n/g' >"$root_dir/.topdeps"
+echo "$deps" | tr ' ' '\n' >"$root_dir/.topdeps"
 git add -f "$root_dir/.topdeps"
 
 author="$(git var GIT_AUTHOR_IDENT)"
-- 
1.6.2.185.g8b635.dirty

[PATCH 2/2] [TopGit] Portability: Don't use alternation ("|") in sed regular expressions

[Brian Campbell]

There is no portable way in sed to express alternation (the "|"
operator).  POSIX does not specify any way of expressing alternation;
according to POSIX, sed uses basic regular expressions which do not
support alternation.  GNU sed allows "\|" to express alternation,
and BSD sed (as tested on Mac OS X) allows for the use of extended
regular expressions with the -E flag, but there appears to be no
portable way to express this.

This patch works around the problem by changing a single sed
expression into two.  This patch also factors out all the call
sites that were doing this, to determing the current branch,
into a single function current_branch in tg.sh.

Signed-off-by: Brian Campbell <brian.p.campbell@dartmouth.edu>
---
 tg-info.sh    |    2 +-
 tg-patch.sh   |    2 +-
 tg-summary.sh |    2 +-
 tg-update.sh  |    2 +-
 tg.sh         |    5 +++++
 5 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/tg-info.sh b/tg-info.sh
index 7d6a34c..25c753c 100644
--- a/tg-info.sh
+++ b/tg-info.sh
@@ -20,7 +20,7 @@ while [ -n "$1" ]; do
 	esac
 done
 
-[ -n "$name" ] || name="$(git symbolic-ref HEAD | sed 's#^refs/\(heads\|top-bases\)/##')"
+[ -n "$name" ] || name="$(current_branch)"
 base_rev="$(git rev-parse --short --verify "refs/top-bases/$name" 2>/dev/null)" ||
 	die "not a TopGit-controlled branch"
 
diff --git a/tg-patch.sh b/tg-patch.sh
index d701c54..ba788c8 100644
--- a/tg-patch.sh
+++ b/tg-patch.sh
@@ -35,7 +35,7 @@ done
 [ -n "$name"  -a  -z "$diff_committed_only" ]  &&
 	die "-i/-w are mutually exclusive with NAME"
 
-[ -n "$name" ] || name="$(git symbolic-ref HEAD | sed 's#^refs/\(heads\|top-bases\)/##')"
+[ -n "$name" ] || name="$(current_branch)"
 base_rev="$(git rev-parse --short --verify "refs/top-bases/$name" 2>/dev/null)" ||
 	die "not a TopGit-controlled branch"
 
diff --git a/tg-summary.sh b/tg-summary.sh
index 50ee883..62b499e 100644
--- a/tg-summary.sh
+++ b/tg-summary.sh
@@ -22,7 +22,7 @@ while [ -n "$1" ]; do
 	esac
 done
 
-curname="$(git symbolic-ref HEAD | sed 's#^refs/\(heads\|top-bases\)/##')"
+curname="$(current_branch)"
 
 ! [ -n "$terse" -a -n "$graphviz" ] ||
 	die "-t and --graphviz options are mutual exclusive"
diff --git a/tg-update.sh b/tg-update.sh
index 288ec14..0cff0ac 100644
--- a/tg-update.sh
+++ b/tg-update.sh
@@ -14,7 +14,7 @@ if [ -n "$1" ]; then
 fi
 
 
-name="$(git symbolic-ref HEAD | sed 's#^refs/\(heads\|top-bases\)/##')"
+name="$(current_branch)"
 base_rev="$(git rev-parse --short --verify "refs/top-bases/$name" 2>/dev/null)" ||
 	die "not a TopGit-controlled branch"
 
diff --git a/tg.sh b/tg.sh
index 43d1c9f..232a8b7 100644
--- a/tg.sh
+++ b/tg.sh
@@ -18,6 +18,11 @@ die()
 	exit 1
 }
 
+current_branch()
+{
+	echo "$(git symbolic-ref HEAD | sed -e 's#^refs/heads/##' -e 's#^refs/top-bases/##')"
+}
+
 # cat_file "topic:file"
 # Like `git cat-file blob $1`, but topics '(i)' and '(w)' means index and worktree
 cat_file()
-- 
1.6.2.185.g8b635.dirty

Re: [PATCH 2/2] [TopGit] Portability: Don't use alternation ("|") in sed regular expressions

[Junio C Hamano]

Brian Campbell <brian.p.campbell@dartmouth.edu> writes:

> +current_branch()
> +{
> +	echo "$(git symbolic-ref HEAD | sed -e 's#^refs/heads/##' -e 's#^refs/top-bases/##')"
> +}

Two micronits.

 - what happens when you are on a detached HEAD?

 - You will be utterly confused by a local branch whose name is
   "refs/top-bases/foo"

To fix these, you might want to do something like:

	if head_=$(git symbolic-ref HEAD)
        then
                case "$head_" in
                refs/heads/*)
                        echo "${head_#refs/heads/}"
                        ;;
                refs/top-bases/*)
                        echo "${head_#refs/top-bases/}"
                        ;;
                *)
                        echo "$head_"
                        ;;
                esac
	else
        	whatever you want to do on a detached HEAD
	fi

Re: [PATCH 2/2] [TopGit] Portability: Don't use alternation ("|") in sed regular expressions

[Uwe Kleine-König]

Hello Brian, hello Junio,

On Wed, Mar 11, 2009 at 11:55:49PM -0700, Junio C Hamano wrote:
> Brian Campbell <brian.p.campbell@dartmouth.edu> writes:
> 
> > +current_branch()
> > +{
> > +	echo "$(git symbolic-ref HEAD | sed -e 's#^refs/heads/##' -e 's#^refs/top-bases/##')"
> > +}
> 
> Two micronits.
> 
>  - what happens when you are on a detached HEAD?
The original code had this problem, too, so I would not take this as a
stopper for the patch.  There are some other locations that suffer from
the same problem.  That's already on my todo list.  So I don't care much
here.
 
>  - You will be utterly confused by a local branch whose name is
>    "refs/top-bases/foo"
You mean a branch that has the full name refs/heads/refs/top-bases/foo?
Well OK, valid concern.

> To fix these, you might want to do something like:
> 
> 	if head_=$(git symbolic-ref HEAD)
>         then
>                 case "$head_" in
>                 refs/heads/*)
>                         echo "${head_#refs/heads/}"
>                         ;;
>                 refs/top-bases/*)
>                         echo "${head_#refs/top-bases/}"
>                         ;;
>                 *)
>                         echo "$head_"
>                         ;;
>                 esac
> 	else
>         	whatever you want to do on a detached HEAD
> 	fi
Thanks Junio and Brian.

Brian, do you update the series?

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-König            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

Re: [PATCH 2/2] [TopGit] Portability: Don't use alternation ("|") in sed regular expressions

[Brian Campbell]

On Mar 12, 2009, at 3:45 AM, Uwe Kleine-König wrote:

>> - You will be utterly confused by a local branch whose name is
>>   "refs/top-bases/foo"
> You mean a branch that has the full name refs/heads/refs/top-bases/ 
> foo?
> Well OK, valid concern.

Yes, you're right, this is a problem.

>> To fix these, you might want to do something like:
>>
>> 	if head_=$(git symbolic-ref HEAD)
>>        then
>>                case "$head_" in
>>                refs/heads/*)
>>                        echo "${head_#refs/heads/}"
>>                        ;;
>>                refs/top-bases/*)
>>                        echo "${head_#refs/top-bases/}"
>>                        ;;
>>                *)
>>                        echo "$head_"
>>                        ;;
>>                esac
>> 	else
>>        	whatever you want to do on a detached HEAD
>> 	fi
> Thanks Junio and Brian.
>
> Brian, do you update the series?

Sure, I'll send an updated patch.

I'm thinking that for the detached HEAD case, this function should die  
with a message about not being on a valid branch, and then the call  
site in tg-summary that doesn't care about being on a valid branch  
should ignore the error and leave curname empty. Does that sound about  
right? I'm fairly new to doing Bourne shell scripting, so I don't yet  
have a good sense of how these things should be structured.

Also, has anyone considered writing a test suite for TopGit? I  
actually got fairly deep in to a series of 10 or so patches before I  
hit these problems, since tg-create worked fine as long as I only  
supplied one dependency, and I didn't notice the second issue until I  
tried to do a tg-update after modifying one of my base patches.

-- Brian

Re: [PATCH 2/2] [TopGit] Portability: Don't use alternation ("|") in sed regular expressions

[Uwe Kleine-König]

Hello Brian, hello Junio,

On Thu, Mar 12, 2009 at 11:00:00AM -0400, Brian Campbell wrote:
> On Mar 12, 2009, at 3:45 AM, Uwe Kleine-König wrote:
>
>>> - You will be utterly confused by a local branch whose name is
>>>   "refs/top-bases/foo"
>> You mean a branch that has the full name refs/heads/refs/top-bases/ 
>> foo?
>> Well OK, valid concern.
>
> Yes, you're right, this is a problem.
>
>>> To fix these, you might want to do something like:
>>>
>>> 	if head_=$(git symbolic-ref HEAD)
Shouldn't git symbolic-ref -q HEAD be used here?

>>>        then
>>>                case "$head_" in
>>>                refs/heads/*)
>>>                        echo "${head_#refs/heads/}"
>>>                        ;;
>>>                refs/top-bases/*)
>>>                        echo "${head_#refs/top-bases/}"
>>>                        ;;
>>>                *)
>>>                        echo "$head_"
>>>                        ;;
>>>                esac
>>> 	else
>>>        	whatever you want to do on a detached HEAD
How do I  distinguish between a detached HEAD and another error?  I have
the feeling that git symbolic-ref -q HEAD should exit(0) with a detached
HEAD.

>> Thanks Junio and Brian.
>>
>> Brian, do you update the series?
>
> Sure, I'll send an updated patch.
>
> I'm thinking that for the detached HEAD case, this function should die  
> with a message about not being on a valid branch, and then the call site 
> in tg-summary that doesn't care about being on a valid branch should 
> ignore the error and leave curname empty. Does that sound about right? 
mmh, I would return "" and let the caller handle that.

> Also, has anyone considered writing a test suite for TopGit?
Yes, but I didn't found the time for that until now.  If you'd volunteer
that would be very welcome.

IMHO we should reuse as much as possible from git.git.  For me even
requiring a git.git checkout to use its files would be OK.  I consider
that even better then duplicating the relevant files.

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-König            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

-- 
Pengutronix e.K.                              | Uwe Kleine-König            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |
Peiner Strasse 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686              | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/2] [TopGit] Portability: Don't use alternation ("|") in sed regular expressions

[martin f krafft]

[-- Attachment #1: Type: text/plain, Size: 781 bytes --]

also sprach Uwe Kleine-König <u.kleine-koenig@pengutronix.de> [2009.03.12.1620 +0100]:
> IMHO we should reuse as much as possible from git.git.  For me even
> requiring a git.git checkout to use its files would be OK.  I consider
> that even better then duplicating the relevant files.

Maybe we could even start to think about integrating TopGit back
into git.git?

-- 
martin | http://madduck.net/ | http://two.sentenc.es/
 
"perhaps debian is concerned more about technical excellence rather
 than ease of use by breaking software. in the former we may excel.
 in the latter we have to concede the field to microsoft. guess
 where i want to go today?"
                                                 -- manoj srivastava
 
spamtraps: madduck.bogus@madduck.net

[-- Attachment #2: Digital signature (see http://martin-krafft.net/gpg/) --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 2/2] [TopGit] Portability: Don't use alternation ("|") in sed regular expressions

[Junio C Hamano]

martin f krafft <madduck@madduck.net> writes:

> also sprach Uwe Kleine-König <u.kleine-koenig@pengutronix.de> [2009.03.12.1620 +0100]:
>> IMHO we should reuse as much as possible from git.git.  For me even
>> requiring a git.git checkout to use its files would be OK.  I consider
>> that even better then duplicating the relevant files.
>
> Maybe we could even start to think about integrating TopGit back
> into git.git?

Heh, it would need massive style fixes before that happens. I am fairly
picky on shell script styles.

Re: [PATCH 2/2] [TopGit] Portability: Don't use alternation ("|") in sed regular expressions

[Uwe Kleine-König]

Hello,

On Fri, Mar 13, 2009 at 10:54:41PM -0700, Junio C Hamano wrote:
> martin f krafft <madduck@madduck.net> writes:
> 
> > also sprach Uwe Kleine-König <u.kleine-koenig@pengutronix.de> [2009.03.12.1620 +0100]:
> >> IMHO we should reuse as much as possible from git.git.  For me even
> >> requiring a git.git checkout to use its files would be OK.  I consider
> >> that even better then duplicating the relevant files.
> >
> > Maybe we could even start to think about integrating TopGit back
> > into git.git?
> 
> Heh, it would need massive style fixes before that happens. I am fairly
> picky on shell script styles.
me, too.  That's one of my todo list items, independently from martin's
suggestion.  (More to the bottom of that list, though.)

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-König            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

Re: [PATCH 2/2] [TopGit] Portability: Don't use alternation ("|") in sed regular expressions

[Brian Campbell]

On Mar 12, 2009, at 11:20 AM, Uwe Kleine-König wrote:

>>>> To fix these, you might want to do something like:
>>>>
>>>> 	if head_=$(git symbolic-ref HEAD)
> Shouldn't git symbolic-ref -q HEAD be used here?

Yes, most likely.

>>>>       then
>>>>               case "$head_" in
>>>>               refs/heads/*)
>>>>                       echo "${head_#refs/heads/}"
>>>>                       ;;
>>>>               refs/top-bases/*)
>>>>                       echo "${head_#refs/top-bases/}"
>>>>                       ;;
>>>>               *)
>>>>                       echo "$head_"
>>>>                       ;;
>>>>               esac
>>>> 	else
>>>>       	whatever you want to do on a detached HEAD
> How do I  distinguish between a detached HEAD and another error?  I  
> have
> the feeling that git symbolic-ref -q HEAD should exit(0) with a  
> detached
> HEAD.

If you pass -q, it exits with status 1 on a detached head, 128 on  
other errors, so we can use that to distinguish.

>
>>> Thanks Junio and Brian.
>>>
>>> Brian, do you update the series?
>>
>> Sure, I'll send an updated patch.
>>
>> I'm thinking that for the detached HEAD case, this function should  
>> die
>> with a message about not being on a valid branch, and then the call  
>> site
>> in tg-summary that doesn't care about being on a valid branch should
>> ignore the error and leave curname empty. Does that sound about  
>> right?
> mmh, I would return "" and let the caller handle that.

Fair enough.

>> Also, has anyone considered writing a test suite for TopGit?
> Yes, but I didn't found the time for that until now.  If you'd  
> volunteer
> that would be very welcome.

I would, but I'm not sure I'll be continuing to use TopGit for more  
than the one patch series I'm using it for now; I was trying it out,  
but it feels a little more heavy-weight than what I want. StGIT or  
just rewriting a patch series with git rebase -i works better for my  
uses; I'm not maintaining a lot of long-lived topic branches upon  
which I need full history.

> IMHO we should reuse as much as possible from git.git.  For me even
> requiring a git.git checkout to use its files would be OK.  I consider
> that even better then duplicating the relevant files.

Hmm. How would the tests find your git working tree? I'd be willing to  
start the process off at least by writing test cases for the  
functionality I'm changing here if I had a good idea of how to start.  
Would it be sufficient to make something like "GIT_CHECKOUT=~/src/git  
make check" work?

-- Brian

Re: [PATCH 2/2] [TopGit] Portability: Don't use alternation ("|") in sed regular expressions

[Uwe Kleine-König]

Hello Brian,

On Thu, Mar 12, 2009 at 12:41:45PM -0400, Brian Campbell wrote:
>> IMHO we should reuse as much as possible from git.git.  For me even
>> requiring a git.git checkout to use its files would be OK.  I consider
>> that even better then duplicating the relevant files.
>
> Hmm. How would the tests find your git working tree? I'd be willing to  
> start the process off at least by writing test cases for the  
> functionality I'm changing here if I had a good idea of how to start.  
> Would it be sufficient to make something like "GIT_CHECKOUT=~/src/git  
> make check" work?
Yes, this would be a good start.  I would call it GIT_SRC, but that's up
to you.

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-König            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

Re: [PATCH 2/2] [TopGit] Portability: Don't use alternation ("|") in sed regular expressions

[Uwe Kleine-König]

Hello Brian,

one more minor nitpick:

On Thu, Mar 12, 2009 at 01:56:29AM -0400, Brian Campbell wrote:
> -[ -n "$name" ] || name="$(git symbolic-ref HEAD | sed 's#^refs/\(heads\|top-bases\)/##')"
> +[ -n "$name" ] || name="$(current_branch)"
current_branch doesn't look like the correct name.  I'd prefer
current_tgpatch is better?!  (For current_branch I would expect that
only refs/heads is stripped.)

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-König            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |