From: Brian Gernhardt <benji@silverinsanity.com>
Date: Tue, 06 May 2008 04:32:02 GMT
Subject: Re: [PATCH] Use perl instead of tac
Message-ID: <638987EF-5822-4CC6-A895-5650A3237603@silverinsanity.com>
In-Reply-To: <20080504221319.GA14584@alea.gnuu.de>


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