Use of tac in git-rebase--interactive

Use of tac in git-rebase--interactive

[Brian Gernhardt]

Commit d481bcc9: "Do rebase with preserve merges with advanced TODO  
list" uses the command tac, apparently to reverse the TODO command  
list.  (I don't use rebase -i much, if you can't tell.)

The problem is that tac doesn't exist on my OS X system.  I do appear  
to have a rev command which does the same thing.  Simply posting a  
patch that does 472s/tac/rev/ would fix the problem on my system, but  
the fact that this is an issue raises the question of the relative  
portability of the two commands.  I don't think we have a declared  
dependancy on GNU's coreutils, which is where Debian lists tac as  
coming from.

~~ Brian

Re: Use of tac in git-rebase--interactive

[Jeff King]

On Sun, Apr 27, 2008 at 02:24:18AM -0400, Brian Gernhardt wrote:

> The problem is that tac doesn't exist on my OS X system.  I do appear to 
> have a rev command which does the same thing.  Simply posting a patch that 
> does 472s/tac/rev/ would fix the problem on my system, but the fact that 
> this is an issue raises the question of the relative portability of the 
> two commands.  I don't think we have a declared dependancy on GNU's 
> coreutils, which is where Debian lists tac as coming from.

I know the list will be shocked to hear that Solaris has neither.

An easy perl replacement is:

  perl -e 'print reverse <>'

which should work fine for small-ish input (since it puts the whole
thing in memory).

-Peff

[PATCH] Use perl instead of tac

[Brian Gernhardt]

Subject: [PATCH] Use perl instead of tac

tac is part of GNU coreutils and not portable.  Use perl's reverse
function instead, since we already require it.

Signed-off-by: Brian Gernhardt <benji@silverinsanity.com>
---

On Apr 27, 2008, at 2:42 AM, Jeff King wrote:

> I know the list will be shocked to hear that Solaris has neither.

I am shocked and horrified.  But then again, that's mostly just my  
reaction to Solaris in general. ;-D

> An easy perl replacement is:
>
>  perl -e 'print reverse <>'
>
> which should work fine for small-ish input (since it puts the whole
> thing in memory).

Something like this?

And I'm having problems with t3404.13 now (mark :0 invalid).  And it's  
too late for me to track it down.

  git-rebase--interactive.sh |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 1751b08..303b754 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -469,7 +469,7 @@ create_extended_todo_list () {
  	test -n "${last_parent:-}" -a "${last_parent:-}" != $SHORTUPSTREAM  
&& \
  		echo reset $last_parent
  	) | \
-	tac | \
+	perl -e 'print reverse <>' | \
  	while read cmd args
  	do
  		: ${commit_mark_list:=} ${last_commit:=000}
-- 
1.5.5.111.g180d

[PATCH] rebase--interactive: Replace unportable 'tac' by a sed script.

[Johannes Sixt]

From: Johannes Sixt <johannes.sixt@telecom.at>

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
Brian Gernhardt schrieb:
> -    tac | \
> +    perl -e 'print reverse <>' | \

Here's my try, which avoids the perl hammer. ;)

Sorry, I can't test this at the moment due to an unrelated breakage
that I first have to chase down.

-- Hannes

PS: I picked the sed script from this patch by Simon 'corecode' Schubert:

http://article.gmane.org/gmane.comp.version-control.git/37074

 git-rebase--interactive.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 1751b08..a9ac332 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -469,7 +469,7 @@ create_extended_todo_list () {
 	test -n "${last_parent:-}" -a "${last_parent:-}" != $SHORTUPSTREAM && \
 		echo reset $last_parent
 	) | \
-	tac | \
+	sed -ne '1!G;$p;h' | \
 	while read cmd args
 	do
 		: ${commit_mark_list:=} ${last_commit:=000}
-- 
1.5.5.1.930.g66f94.dirty

Re: [PATCH] rebase--interactive: Replace unportable 'tac' by a sed script.

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> From: Johannes Sixt <johannes.sixt@telecom.at>
>
> Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
> ---
> Brian Gernhardt schrieb:
>> -    tac | \
>> +    perl -e 'print reverse <>' | \
>
> Here's my try, which avoids the perl hammer. ;)
> ...
> -	tac | \
> +	sed -ne '1!G;$p;h' | \

Thanks for trying, but frankly, I'd prefer the Perl hammer, as any
advanced sed scripting tend to be far less portable.

Re: [PATCH] rebase--interactive: Replace unportable 'tac' by a sed script.

[Paolo Bonzini]

>> Here's my try, which avoids the perl hammer. ;)
>> ...
>> -	tac | \
>> +	sed -ne '1!G;$p;h' | \
> 
> Thanks for trying, but frankly, I'd prefer the Perl hammer, as any
> advanced sed scripting tend to be far less portable.

In fact this script is not portable. :-(  Not so much because of its 
"advanced-ness", but because it loads the entire file in memory, and 
thus hits the limit on the buffer length of many seds.

Paolo

Re: [PATCH] rebase--interactive: Replace unportable 'tac' by a sed script.

[Jeff King]

On Mon, Apr 28, 2008 at 09:44:48AM +0200, Johannes Sixt wrote:

>> +    perl -e 'print reverse <>' | \
> [...]
> +	sed -ne '1!G;$p;h' | \

Wow, and people complain about perl being unreadable. ;)

-Peff

Re: [PATCH] rebase--interactive: Replace unportable 'tac' by a sed script.

[Andreas Ericsson]

Jeff King wrote:
> On Mon, Apr 28, 2008 at 09:44:48AM +0200, Johannes Sixt wrote:
> 
>>> +    perl -e 'print reverse <>' | \
>> [...]
>> +	sed -ne '1!G;$p;h' | \
> 
> Wow, and people complain about perl being unreadable. ;)
> 

Well, the tmtowtdi dogma has its drawbacks. Here's another sed-script
that does the exact same thing:

	sed '1!G;h;$!d'

That being said, perl borrows most of its regex notation from sed, so
it's not as if perl is easier. This is just the top-end of the scale
that things like "sed s/foo/bar/" is at the bottom of ;-)

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH] Use perl instead of tac

[Jörg Sommer]

Hi,

Brian Gernhardt <benji@silverinsanity.com> wrote:
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 1751b08..303b754 100755
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -469,7 +469,7 @@ create_extended_todo_list () {
>   	test -n "${last_parent:-}" -a "${last_parent:-}" != $SHORTUPSTREAM  
> && \
>   		echo reset $last_parent
>   	) | \
> -	tac | \
> +	perl -e 'print reverse <>' | \

What about using a shell function and a *big* variable or an intermediate
file?

tac()
{
	while read line
	do
		reversed="$line
$reversed"
	done
	echo "${reversed%
}"
}

Bye, Jörg.
-- 
Ich halte ihn zwar für einen Schurken und das was er sagt für
falsch – aber ich bin bereit mein Leben dafür einzusetzen, daß
er seine Meinung sagen kann.		(Voletair)

Re: [PATCH] Use perl instead of tac

[Randal L. Schwartz]

>>>>> "Jörg" == Jörg Sommer <joerg@alea.gnuu.de> writes:

Jörg> What about using a shell function and a *big* variable or an intermediate
Jörg> file?

What makes you think that's any more faster or efficient than calling Perl
at this point?

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.
See PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!

Re: [PATCH] Use perl instead of tac

[David Symonds]

On Mon, Apr 28, 2008 at 10:58 PM, Randal L. Schwartz
<merlyn@stonehenge.com> wrote:
> >>>>> "Jörg" == Jörg Sommer <joerg@alea.gnuu.de> writes:
>
>  Jörg> What about using a shell function and a *big* variable or an intermediate
>  Jörg> file?
>
>  What makes you think that's any more faster or efficient than calling Perl
>  at this point?

I doubt Jörg suggested it for its speed, but it removes the dependency on Perl.

Bit ugly, still.


Dave.

Re: [PATCH] Use perl instead of tac

[Paolo Bonzini]

David Symonds wrote:
> On Mon, Apr 28, 2008 at 10:58 PM, Randal L. Schwartz
> <merlyn@stonehenge.com> wrote:
>>>>>>> "Jörg" == Jörg Sommer <joerg@alea.gnuu.de> writes:
>>  Jörg> What about using a shell function and a *big* variable or an intermediate
>>  Jörg> file?
>>
>>  What makes you think that's any more faster or efficient than calling Perl
>>  at this point?
> 
> I doubt Jörg suggested it for its speed, but it removes the dependency on Perl.
> 
> Bit ugly, still.

And quadratic, unlike Perl (and unlike sed, if it worked portably).

Paolo

Re: [PATCH] Use perl instead of tac

[Brian Gernhardt]

On Apr 28, 2008, at 9:12 AM, David Symonds wrote:

> I doubt Jörg suggested it for its speed, but it removes the  
> dependency on Perl.

We already depend on perl for git-add--interactive.perl in core git,  
and for git-svn, git-cvs*, and git-send-email.  I don't think this one  
line is going to make a big difference in our Perl dependency.

~~ Brian

Re: [PATCH] Use perl instead of tac

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 788 bytes --]

Hi,

On Mon, 28 Apr 2008, Brian Gernhardt wrote:

> On Apr 28, 2008, at 9:12 AM, David Symonds wrote:
> 
> >I doubt Jörg suggested it for its speed, but it removes the dependency 
> >on Perl.
> 
> We already depend on perl for git-add--interactive.perl in core git, and 
> for git-svn, git-cvs*, and git-send-email.  I don't think this one line 
> is going to make a big difference in our Perl dependency.

It does.  It goes the wrong way.  And work on building in 
git-add--interactive.perl has already begun, so that is not a strong 
argument.

As for git-svn and git-cvs* I agree, they will probably remain perl 
scripts.  But I think that they are not that important, since they are not 
strictly _core_ parts of Git, being importers/exporters that not everybody 
needs.

Ciao,
Dscho

Re: [PATCH] Use perl instead of tac

[Jörg Sommer]

Hallo Randal,

Randal L. Schwartz <merlyn@stonehenge.com> wrote:
>>>>>> "Jörg" == Jörg Sommer <joerg@alea.gnuu.de> writes:
>
> Jörg> What about using a shell function and a *big* variable or an intermediate
> Jörg> file?
>
> What makes you think that's any more faster or efficient than calling Perl
> at this point?

Nothing. My intention was not the speed, but the dependency on Perl. But
your are right. Except from the point that my suggestion is broken, it's
*much* slower:

% time dash -c 'while IFS= read -r line; do rev="$line
$rev"; done; printf "%s" "$rev"' < gitk-git/gitk-wish DN
dash -c  < gitk-git/gitk-wish > /dev/null 2> /dev/null  17,89s user 2,68s system 96% cpu 21,352 total

% time tac < gitk-git/gitk-wish DN
tac < gitk-git/gitk-wish > /dev/null 2> /dev/null  0,01s user 0,01s system 91% cpu 0,017 total

% time perl -e 'print reverse <>' < gitk-git/gitk-wish DN
perl -e 'print reverse <>' < gitk-git/gitk-wish > /dev/null 2> /dev/null  0,07s user 0,01s system 59% cpu 0,141 total

But I doubt this hurts, because we don't have such a big input.

% wc -l git.c
390 git.c

% time dash -c 'while IFS= read -r line; do rev="$line
$rev"; done; printf "%s" "$rev"' < git.c DN
dash -c  < git.c > /dev/null 2> /dev/null  0,04s user 0,04s system 62% cpu 0,115 total

And what about something like this: 'tac || rev || perl …'

Bye, Jörg.
-- 
Was der Bauer nicht kennt, das frisst er nicht. Würde der Städter kennen,
was er frisst, er würde umgehend Bauer werden.
                                                       Oliver Hassencamp

Re: [PATCH] Use perl instead of tac

[Avery Pennarun]

On 4/28/08, Jörg Sommer <joerg@alea.gnuu.de> wrote:
>  And what about something like this: 'tac || rev || perl …'

Or we could just add a git-tac.c and use that :)

Have fun,

Avery

Re: [PATCH] Use perl instead of tac

[Matthieu Moy]

Jörg Sommer <joerg@alea.gnuu.de> writes:

> And what about something like this: 'tac || rev || perl …'

Be careful: rev and tac are different. tac reverses lines, while rev
reverses chars inside lines.

My 2 cents,

-- 
Matthieu

Re: [PATCH] Use perl instead of tac

[Brian Gernhardt]

On Apr 28, 2008, at 1:34 PM, Matthieu Moy wrote:

> Jörg Sommer <joerg@alea.gnuu.de> writes:
>
>> And what about something like this: 'tac || rev || perl …'
>
> Be careful: rev and tac are different. tac reverses lines, while rev
> reverses chars inside lines.

Indeed you are correct.  This is a mistake I made in my original  
email, which people have been parroting.  My mistake, sorry.

~~ Brian

Re: [PATCH] Use perl instead of tac

[Brian Gernhardt]

On Apr 28, 2008, at 11:15 AM, Jörg Sommer wrote:

> Nothing. My intention was not the speed, but the dependency on Perl.  
> But
> your are right. Except from the point that my suggestion is broken,  
> it's
> *much* slower:

[snip]

> And what about something like this: 'tac || rev || perl …'

This would actually be spelled something like:

----- 8< -----
tac=""

tac < /dev/null > /dev/null 2>&1
if test $? != 127; then
    tac=tac
fi

if test -z "$tac"; then
    perl < /dev/null > /dev/null 2>&1
    if test $? != 127; then
       tac="perl -e 'print reverse <>'"
    fi
fi

if test -z "$tac"; then
    die "Couldn't find tac or perl."
fi

(subshell) | $tac | while loop
----- 8< -----

Ugly, no?  Plus it adds a dependency on tac OR doesn't solve the perl  
dependency.  I personally think using perl is better than this  
approach.  Even if we're trying to reduce the perl dependency in core,  
it's there right now so this has zero impact now.  The ideal solution  
is to re-write the loop so that it doesn't need to be reversed in the  
first place.  We can use perl until that rewrite is done.

This loop-reverse-loop construct is _extremely_ ugly, and I'm having  
problems following it.  None of what this function is doing is  
immediately obvious.  It's a good candidate for a comment or two.

Looking over it, we should be able to do this with one loop over the  
list of commits, doing this for each of them:

if more than one parent
   other_parents=all but first, comma separated
   print merge command (without marks)
else
   print pick command
fi

if commit is tagged
   print tag command
fi

I also dislike the large lists this is carrying around in shell  
variables.  If I'm reading it correctly, the tag list could be  
replaced by invocations of "git describe --exact-match".  The mark  
list appears to be unavoidable, but significantly smaller than the tag  
list.

Now that I think about it, the generation of marks could be done by a  
second loop over the list.  Notice what commits need to be marked in  
loop 1, then add the mark commands in loop 2.  Both of these loops  
would function in the same direction, removing the need for either tac  
or perl.

~~ Brian

Re: [PATCH] Use perl instead of tac

[Jörg Sommer]

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

Hi Brian,

Brian Gernhardt schrieb am Mon 28. Apr, 15:13 (-0400):
> This loop-reverse-loop construct is _extremely_ ugly, and I'm having  
> problems following it.  None of what this function is doing is  
> immediately obvious.  It's a good candidate for a comment or two.

I write one.

> I also dislike the large lists this is carrying around in shell  
> variables.  If I'm reading it correctly, the tag list could be replaced 
> by invocations of "git describe --exact-match".

Yes. How to get all tags of a commit?

% git tag foo v1.5.5
% git describe --exact-match 9d831805195ba40b62f632acc6bb6e53d3
warning: tag 'v1.5.5' is really 'foo' here
v1.5.5

> Now that I think about it, the generation of marks could be done by a  
> second loop over the list.  Notice what commits need to be marked in  
> loop 1, then add the mark commands in loop 2.  Both of these loops would 
> function in the same direction, removing the need for either tac or perl.

You are right.

Thanks for your comments.

Bye, Jörg.
-- 
Damit das Mögliche entsteht, muß immer wieder das Unmögliche versucht
werden.                                       (Hermann Hesse)

[-- Attachment #2: Digital signature http://en.wikipedia.org/wiki/OpenPGP --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Use perl instead of tac

[Jörg Sommer]

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

Hallo Brian,

Jörg Sommer schrieb am Wed 30. Apr, 11:02 (+0200):
> Brian Gernhardt schrieb am Mon 28. Apr, 15:13 (-0400):
> > Now that I think about it, the generation of marks could be done by a  
> > second loop over the list.  Notice what commits need to be marked in  
> > loop 1, then add the mark commands in loop 2.  Both of these loops would 
> > function in the same direction, removing the need for either tac or perl.
> 
> You are right.

… but I found the problem. I don't know how to pass the list from one
subshell to the other.

% { echo pick; echo merge; a=12 } | { echo +$a+; while read line; do echo $line; done; }
++
pick
merge

The only idea I have is a file. Do you have a better idea?

% { echo pick; echo merge; a=12 } >/tmp/tmp; \
  { echo +$a+; while read line; do echo $line; done; } < /tmp/tmp
+12+
pick
merge

Bye, Jörg.
-- 
Viele Leute glauben, dass sie denken, wenn sie lediglich
ihre Vorurteile neu ordnen.

[-- Attachment #2: Digital signature http://en.wikipedia.org/wiki/OpenPGP --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Use perl instead of tac

[Brian Gernhardt]

On Apr 30, 2008, at 5:39 AM, Jörg Sommer wrote:

> … but I found the problem. I don't know how to pass the list from one
> subshell to the other.
>
> % { echo pick; echo merge; a=12 } | { echo +$a+; while read line; do  
> echo $line; done; }
> ++
> pick
> merge
>
> The only idea I have is a file. Do you have a better idea?
>
> % { echo pick; echo merge; a=12 } >/tmp/tmp; \
>  { echo +$a+; while read line; do echo $line; done; } < /tmp/tmp
> +12+
> pick
> merge

No, I don't.  But I think the resulting code will be easier to read,  
even with the file.

~~ Brian

Re: [PATCH] Use perl instead of tac

[Brian Gernhardt]

On Apr 30, 2008, at 5:02 AM, Jörg Sommer wrote:

>> I also dislike the large lists this is carrying around in shell
>> variables.  If I'm reading it correctly, the tag list could be  
>> replaced
>> by invocations of "git describe --exact-match".
>
> Yes. How to get all tags of a commit?
>
> % git tag foo v1.5.5
> % git describe --exact-match 9d831805195ba40b62f632acc6bb6e53d3
> warning: tag 'v1.5.5' is really 'foo' here
> v1.5.5

I wish I could be clever and say I pointed this out as an obviously  
wrong answer or similar.  But, no, I simply didn't think of that.  The  
long list may be required, despite my concerns about it..  :-(

Those concerns being: overrunning the length of a shell variable, the  
speed of constructing and searching the list, over-complexity of the  
code.  But, of course, if there isn't another way to do it right then  
the list stays.

~~ Brian

Re: [PATCH] Use perl instead of tac

[Jörg Sommer]

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

Hallo Brian,

Brian Gernhardt schrieb am Wed 30. Apr, 11:25 (-0400):
> On Apr 30, 2008, at 5:02 AM, Jörg Sommer wrote:
>
>>> I also dislike the large lists this is carrying around in shell
>>> variables.  If I'm reading it correctly, the tag list could be  
>>> replaced
>>> by invocations of "git describe --exact-match".
>>
>> Yes. How to get all tags of a commit?
>>
>> % git tag foo v1.5.5
>> % git describe --exact-match 9d831805195ba40b62f632acc6bb6e53d3
>> warning: tag 'v1.5.5' is really 'foo' here
>> v1.5.5

And how can I get only tags no annotated tags? We can't recreate
annotated tags with git rebase.

> Those concerns being: overrunning the length of a shell variable,

Are you shure there is any such bounding? I didn't saw anything about the
size of a variables in IEEE 1003.1-2004, but didn't look very carfully.
All my shells (bash, dash, zsh) can handle more than 16777217 characters:

dash -c 'a=a; while :; do a=$a$a; echo $a | wc -c; done'

This would be a new version of create_extended_todo_list() without tac.
What do you think about it? Is it better readable? But there's a bug.

create_extended_todo_list () {
	# The idea of this function is to
	# 1. build a list of tags
	# 2. go through the list and
	#    * issue a reset command to the parent of the commit, if the last
	#      commit was not the parent of the current commit,
	#    * issue a pick command for simple commits,
	#    * issue for each merge commit a merge command with the hashs of
	#      the parent commits,
	#    * register each parent of a merge and issue a mark command
	#      (without an ID) after the commit for each registered commit and
	#    * issue a tag command, if the commit is in the tag list.
	# 3. Then go through the created list and
	#    * add an ID to each mark command and
	#    * replace all occurences of the hash in reset and merge commands
	#      by the mark ID
	test -e "$DOTEST"/cetl.tmp \
		&& die "Someone else uses our filename cetl.tmp." \
			"That's not nice"
	if test t = "${PRESERVE_TAGS:-}"
	then
		tag_list=$(git show-ref --abbrev=7 --tags | \
			(
			while read sha1 tag
			do
				tag=${tag#refs/tags/}
				if test ${last_sha1:-0000} = $sha1
				then
					saved_tags="$saved_tags:$tag"
				else
					printf "%s" "${last_sha1:+ $last_sha1#$saved_tags}"
					last_sha1=$sha1
					saved_tags=$tag
				fi
			done
			echo "${last_sha1:+ $last_sha1:$saved_tags}"
			) )
	else
		tag_list=
	fi
	mark_these_commits=
	while IFS=_ read commit parents subject
	do
		first_parent=${parents%% *}
		if test "${last_commit:-$SHORTUPSTREAM}" != $first_parent
		then
			test "$first_parent" = $SHORTUPSTREAM &&
				first_parent=$SHORTONTO
			echo reset $first_parent
		fi
		unset first_parent
		last_commit=$commit

		case "$parents" in
		*' '*)
			new_parents=
			for p in $parents
			do
				mark_these_commits=$(insert_value_at_key_into_list \
					"$commit" "$p" "$mark_these_commits")
				if test "$p" = $SHORTUPSTREAM
				then
					new_parents="$new_parents $SHORTONTO"
				else
					new_parents="$new_parents $p"
				fi
			done
			unset p
			echo merge $commit ${new_parents# * }
			unset new_parents
			;;
		*)
			echo "pick $commit $subject"
			;;
		esac

		if tmp=$(get_value_from_list $commit "$tag_list")
		then
			for t in $(echo $tmp | tr : ' ')
			do
				echo tag $t
			done
		fi
	done > "$DOTEST"/cetl.tmp
	unset commit parents subject

	commit_mark_list=
	next_mark=0
	last_commit=
	while read cmd args
	do
		case "$cmd" in
		pick)
			this_commit="${args%% *}"
			;;
		reset)
			this_commit=$args
			if tmp=$(get_value_from_list $args "$commit_mark_list")
			then
				args=":$tmp"
			fi
			;;
		merge)
			new_args=
			for i in ${args#* }
			do
				if tmp=$(get_value_from_list $i \
					"$commit_mark_list")
				then
					new_args="$new_args :$tmp"
				else
					new_args="$new_args $i"
				fi
			done
			this_commit="${args%% *}"
			args="$this_commit ${new_args# }"
			;;
		esac

		if tmp=$(get_value_from_list "$last_commit" \
			"$mark_these_commits") && \
			test "${this_commit:-$last_commit}" != $tmp
		then
			if tmp=$(get_value_from_list "$last_commit" \
				"$commit_mark_list")
			then
				test "$last_cmd" = reset -o "$last_cmd" = tag \
					|| echo mark ":$tmp"
			else
				commit_mark_list=$(insert_value_at_key_into_list \
					$next_mark $last_commit "$commit_mark_list")
				echo mark ":$next_mark"
				next_mark=$(($next_mark + 1))
			fi
		fi
		last_commit=${this_commit:-$last_commit}
		unset this_commit

		echo "$cmd $args"
		last_cmd=$cmd
	done < "$DOTEST"/cetl.tmp
	rm "$DOTEST"/cetl.tmp
	unset last_cmd last_commit next_mark cmd args tmp commit_mark_list \
		mark_these_commits
}

The problem is the mark command. If you walk from ONTO to HEAD trough the
list, you must know for a commit, if it is used _later_. But don't create
a mark if it is used immediately, e.g. pick; merge not pick; mark; merge.

If you walk from HEAD to ONTO, this is much easier. I delayed the mark
for the first head of a merge and checked if the next commit is this
commit. This way I keep the todo list clean and don't get something like
this for --first-parent:

pick abc
pick def
mark :0
merge 012 foreign-branch

Bye, Jörg.
-- 
Es ist außerdem ein weit verbreiteter Irrtum das USENET ‚helfen‘ soll.
Tatsächlich wurde USENET nachweislich zur persönlichen Belustigung
seiner Erfinder geschaffen.
Jörg Klemenz <joerg@gmx.net>, <b4ai4o$1u8vmt$2@ID-21915.news.dfncis.de>

[-- Attachment #2: Digital signature http://en.wikipedia.org/wiki/OpenPGP --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Use perl instead of tac

[Brian Gernhardt]

On May 4, 2008, at 6:13 PM, Jörg Sommer wrote:

> Hallo Brian,
>
> Brian Gernhardt schrieb am Wed 30. Apr, 11:25 (-0400):
>> On Apr 30, 2008, at 5:02 AM, Jörg Sommer wrote:
>>> Yes. How to get all tags of a commit?
>>>
>>> % git tag foo v1.5.5
>>> % git describe --exact-match 9d831805195ba40b62f632acc6bb6e53d3
>>> warning: tag 'v1.5.5' is really 'foo' here
>>> v1.5.5
>
> And how can I get only tags no annotated tags? We can't recreate
> annotated tags with git rebase.

Yes, the tag list has to stay.  I had missed several bits of what it  
was trying to do.

>> Those concerns being: overrunning the length of a shell variable,
>
> Are you shure there is any such bounding? I didn't saw anything  
> about the
> size of a variables in IEEE 1003.1-2004, but didn't look very  
> carfully.
> All my shells (bash, dash, zsh) can handle more than 16777217  
> characters:
>
> dash -c 'a=a; while :; do a=$a$a; echo $a | wc -c; done'

My concern is that some shell somewhere (like the ever problematic  
Solaris) would have a limit and it would silently fail.  It may be an  
unfounded concern, but I thought I'd mention it to the list and see if  
anyone agreed.

> This would be a new version of create_extended_todo_list() without  
> tac.
> What do you think about it? Is it better readable? But there's a bug.

I find it more readable and am pleased about the lack of perl and tac.

> create_extended_todo_list () {
> 	# The idea of this function is to
> 	# 1. build a list of tags
> 	# 2. go through the list and
> 	#    * issue a reset command to the parent of the commit, if the last
> 	#      commit was not the parent of the current commit,
> 	#    * issue a pick command for simple commits,
> 	#    * issue for each merge commit a merge command with the hashs of
> 	#      the parent commits,
> 	#    * register each parent of a merge and issue a mark command
> 	#      (without an ID) after the commit for each registered commit  
> and
> 	#    * issue a tag command, if the commit is in the tag list.
> 	# 3. Then go through the created list and
> 	#    * add an ID to each mark command and
> 	#    * replace all occurences of the hash in reset and merge commands
> 	#      by the mark ID

Comment is excellent, although I'd prefer that the three parts were  
simply next to the code they're referring to.

> 	test -e "$DOTEST"/cetl.tmp \
> 		&& die "Someone else uses our filename cetl.tmp." \
> 			"That's not nice"

Is this just to catch a bug where another part of rebase -i uses the  
same file?  Or is there a way this could be triggered by user  
actions?  (I am under the impression that we'd never get this far if  
$DOTEST already existed.)

> 	mark_these_commits=
> 	while IFS=_ read commit parents subject
> 	do
> 		first_parent=${parents%% *}
> 		if test "${last_commit:-$SHORTUPSTREAM}" != $first_parent
> 		then
> 			test "$first_parent" = $SHORTUPSTREAM &&
> 				first_parent=$SHORTONTO
> 			echo reset $first_parent
> 		fi
> 		unset first_parent
> 		last_commit=$commit
>
> 		case "$parents" in
> 		*' '*)
> 			new_parents=
> 			for p in $parents
> 			do

To avoid unneeded marks, you need one more variable and an if  
statement.  Instead of unconditionally marking every parent, keep  
track of the previous commit and add a line like this:

                                if test "$p" != "$prev_commit"
                                then
> 				mark_these_commits=$(insert_value_at_key_into_list \
> 					"$commit" "$p" "$mark_these_commits")
                                fi
>
> 				if test "$p" = $SHORTUPSTREAM
> 				then
> 					new_parents="$new_parents $SHORTONTO"
> 				else
> 					new_parents="$new_parents $p"
> 				fi
> 			done
> 			unset p
> 			echo merge $commit ${new_parents# * }
> 			unset new_parents
> 			;;
> 		*)
> 			echo "pick $commit $subject"
> 			;;
> 		esac
>
> 		if tmp=$(get_value_from_list $commit "$tag_list")
> 		then
> 			for t in $(echo $tmp | tr : ' ')
> 			do
> 				echo tag $t
> 			done
> 		fi
>
> 	done > "$DOTEST"/cetl.tmp
> 	unset commit parents subject
>
> 	commit_mark_list=
> 	next_mark=0
> 	last_commit=
> 	while read cmd args

I'd personally do "cat cetl.tmp | while" instead of "while .... done <  
cetl.tmp", just so that where we're reading from is obvious from the  
start.

> The problem is the mark command. If you walk from ONTO to HEAD  
> trough the
> list, you must know for a commit, if it is used _later_. But don't  
> create
> a mark if it is used immediately, e.g. pick; merge not pick; mark;  
> merge.
>
> If you walk from HEAD to ONTO, this is much easier. I delayed the mark
> for the first head of a merge and checked if the next commit is this
> commit. This way I keep the todo list clean and don't get something  
> like
> this for --first-parent:
>
> pick abc
> pick def
> mark :0
> merge 012 foreign-branch

(See above.)

~~ Brian

Re: [PATCH] Use perl instead of tac

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 816 bytes --]

Hi,

On Mon, 28 Apr 2008, Jörg Sommer wrote:

> Brian Gernhardt <benji@silverinsanity.com> wrote:
> > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> > index 1751b08..303b754 100755
> > --- a/git-rebase--interactive.sh
> > +++ b/git-rebase--interactive.sh
> > @@ -469,7 +469,7 @@ create_extended_todo_list () {
> >   	test -n "${last_parent:-}" -a "${last_parent:-}" != $SHORTUPSTREAM  
> > && \
> >   		echo reset $last_parent
> >   	) | \
> > -	tac | \
> > +	perl -e 'print reverse <>' | \
> 
> What about using a shell function and a *big* variable or an 
> intermediate file?

How about fixing the code to not need tac instead?  We went to great 
lengths to introduce the --reverse option to the rev-list command, in 
order to avoid tac in the original version of rebase -i.

Ciao,
Dscho

Re: [PATCH] Use perl instead of tac

[Brian Gernhardt]

On Apr 28, 2008, at 9:46 AM, Johannes Schindelin wrote:

> How about fixing the code to not need tac instead?  We went to great
> lengths to introduce the --reverse option to the rev-list command, in
> order to avoid tac in the original version of rebase -i.

Because this is reversing the output of a sub-shell (ll.395-471), not  
rev-list.  And making the shell code insert commands before the lines  
the produce it would make the code complex at best.  (And it's not  
simple code either.)

Or at least that's why I didn't do it.  Someone who understands it  
better can feel free to make it work in reverse in the first place.

~~ Brian

Re: [PATCH] Use perl instead of tac

[Johannes Schindelin]

Hi,

On Mon, 28 Apr 2008, Brian Gernhardt wrote:

> On Apr 28, 2008, at 9:46 AM, Johannes Schindelin wrote:
> 
> >How about fixing the code to not need tac instead?  We went to great 
> >lengths to introduce the --reverse option to the rev-list command, in 
> >order to avoid tac in the original version of rebase -i.
> 
> Because this is reversing the output of a sub-shell (ll.395-471), not 
> rev-list.  And making the shell code insert commands before the lines 
> the produce it would make the code complex at best.  (And it's not 
> simple code either.)

That's my _point_: the code is way too complex for what it tries to 
accomplish, namely implement a sane syntax for rebase -i -p.

Ciao,
Dscho

Re: Use of tac in git-rebase--interactive

[しらいしななこ]

Quoting Brian Gernhardt <benji@silverinsanity.com> writes:

> Commit d481bcc9: "Do rebase with preserve merges with advanced TODO
> list" uses the command tac, apparently to reverse the TODO command
> list.  (I don't use rebase -i much, if you can't tell.)

That is very sloppy job on Junio's part.  He usually is very picky about shell portability, and I remember that he earlier rewrote somebody else's commit that originally used tac command.

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

----------------------------------------------------------------------
Get a free email address with REAL anti-spam protection.
http://www.bluebottle.com/tag/1

Re: Use of tac in git-rebase--interactive

[Junio C Hamano]

しらいしななこ  <nanako3@bluebottle.com> writes:

> Quoting Brian Gernhardt <benji@silverinsanity.com> writes:
>
>> Commit d481bcc9: "Do rebase with preserve merges with advanced TODO
>> list" uses the command tac, apparently to reverse the TODO command
>> list.  (I don't use rebase -i much, if you can't tell.)
>
> That is very sloppy job on Junio's part.  He usually is very picky about shell portability, and I remember that he earlier rewrote somebody else's commit that originally used tac command.

Heh, sorry about sloppiness.  I've been swamped lately outside git...