Re: [PATCH 02/31] rebase: refactor reading of state

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 28, 2010, 23:08 UTC
Message-ID: <7v1v51v6l2.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1293528648-21873-3-git-send-email-martin.von.zweigbergk@gmail.com>

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:

Show 27 quoted lines

> +read_state () {
> +	if test -d "$merge_dir"
> +	then
> +		state_dir="$merge_dir"
> +		prev_head=$(cat "$merge_dir"/prev_head) &&
> +		end=$(cat "$merge_dir"/end) &&
> +		msgnum=$(cat "$merge_dir"/msgnum)
> +	else
> +		state_dir="$apply_dir"
> +	fi &&
> +	head_name=$(cat "$state_dir"/head-name) &&
> +	onto=$(cat "$state_dir"/onto) &&
> +	orig_head=$(cat "$state_dir"/orig-head) &&
> +	GIT_QUIET=$(cat "$state_dir"/quiet)
> +}
> +
>  continue_merge () {
>  	test -n "$prev_head" || die "prev_head must be defined"
>  	test -d "$merge_dir" || die "$merge_dir directory does not exist"
> @@ -138,10 +154,6 @@ call_merge () {
>  }
>  
>  move_to_original_branch () {
> -	test -z "$head_name" &&
> -		head_name="$(cat "$merge_dir"/head-name)" &&
> -		onto="$(cat "$merge_dir"/onto)" &&
> -		orig_head="$(cat "$merge_dir"/orig-head)"

It used to be safe to call this without head_name and friends set, because the function took care of reading these itself. Nobody calls this without head_name set anymore?

I am not complaining nor suggesting to add an unnecessary "read_state" here only to slow things down---I am making sure that you removed this because you know it is unnecessary.

Show 12 quoted lines

> @@ -220,13 +232,9 @@ do
>  			echo "mark them as resolved using git add"
>  			exit 1
>  		}
> +		read_state
>  		if test -d "$merge_dir"
>  		then
> -			prev_head=$(cat "$merge_dir/prev_head")
> -			end=$(cat "$merge_dir/end")
> -			msgnum=$(cat "$merge_dir/msgnum")
> -			onto=$(cat "$merge_dir/onto")
> -			GIT_QUIET=$(cat "$merge_dir/quiet")

Even though this patch may make the code shorter, it starts to read head_name and orig_head that we previously did not open and change the values of variables with what are read from them. Does this change affect the behaviour in any way (either in performance or in correctness)?

Show 10 quoted lines

> @@ -236,10 +244,6 @@ do
>  			finish_rb_merge
>  			exit
>  		fi
> -		head_name=$(cat "$apply_dir"/head-name) &&
> -		onto=$(cat "$apply_dir"/onto) &&
> -		orig_head=$(cat "$apply_dir"/orig-head) &&
> -		GIT_QUIET=$(cat "$apply_dir"/quiet)
>  		git am --resolved --3way --resolvemsg="$RESOLVEMSG" &&
>  		move_to_original_branch

Earlier move-to-original-branch was Ok to be called without head_name, and the old code here read from the file anyway, so it didn't matter, but now it seems that the first check and assignment you removed from the function may matter because this caller does not even read from head_name. Are you sure about this change?

Show 6 quoted lines

> @@ -279,18 +275,15 @@ do
>  			die "No rebase in progress?"
>  
>  		git rerere clear
> -
> -		test -d "$merge_dir" || merge_dir="$apply_dir"

My heartbeat skipped when I first saw this. Thanks to the previous commit, it was exposed that somebody reused $dotest that was only to be used when using merge machinery because the things left to be done in this codepath are common between the merge and apply, which is kind of sloppy, but that sloppiness is now gone ;-).

Are there places that read from individual files for states left after this patch, or read_state is the only interface to get to the states? If the latter that would be a great news, and also would suggest that we may want to have a corresponding write_state function (and we may even want to make the state into a single file to reduce I/O---but that is a separate issue that can be done at the very end of the series if it turns out to be beneficial).

Of course it is fine if introduction of read_state is an attempt to catch most common cases, but it would reduce chances of mistake if the coverage were 100% (as opposed to 99.9%) hence this question.