Re: [PATCH] Use perl instead of tac

From: Brian Gernhardt <benji@silverinsanity.com>
Date: May 6, 2008, 04:32 UTC
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:

Show 13 quoted lines

> 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.

Show 10 quoted lines

>> 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.

Show 17 quoted lines

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

Show 18 quoted lines

> 	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

Show 32 quoted lines

>
> 				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.

Show 17 quoted lines

> 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