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

[Junio C Hamano <gitster@pobox.com>]

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

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

> @@ -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)?

> @@ -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?

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