patch, 2 parts[TopGit] Portability: Use tr instead of sed for translating spaces to newlines

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(-)

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(-)

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

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

Hello Brian, hello Junio,

On Wed, Mar 11, 2009 at 11:55:49PM -0700, Junio C Hamano wrote:

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 mean a branch that has the full name refs/heads/refs/top-bases/foo? Well OK, valid concern.

Thanks Junio and Brian.

Brian, do you update the series?

Best regards Uwe

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

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

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

Hello Brian, hello Junio,

On Thu, Mar 12, 2009 at 11:00:00AM -0400, Brian Campbell wrote:

Shouldn't git symbolic-ref -q HEAD be used here?

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.

mmh, I would return "" and let the caller handle that.

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

also sprach Uwe Kleine-König <u.kleine-koenig@pengutronix.de> [2009.03.12.1620 +0100]:

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

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

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

Hello,

On Fri, Mar 13, 2009 at 10:54:41PM -0700, Junio C Hamano wrote:

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

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

Yes, most likely.

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

Fair enough.

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.

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

Hello Brian,

On Thu, Mar 12, 2009 at 12:41:45PM -0400, Brian Campbell wrote:

Yes, this would be a good start. I would call it GIT_SRC, but that's up to you.

Best regards Uwe

Hello Brian,

one more minor nitpick:

On Thu, Mar 12, 2009 at 01:56:29AM -0400, Brian Campbell wrote:

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