Re: [PATCH 5/7] rebase -i: return control to the caller, for housekeeping

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 23, 2013, 16:57 UTC
Message-ID: <7vzjwpi3rp.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1366725724-1016-6-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Show 14 quoted lines

> On a successful interactive rebase, git-rebase--interactive.sh
> currently cleans up and exits on its own.  Instead of doing these
> two things ourselves:
>
>     rm -fr "$dotest"
>     git gc --auto
>
> let us return control to the caller (git-rebase.sh), to do the
> needful.  The advantage of doing this is that the caller can implement
> a generic cleanup routine (and possibly other things) independent of
> specific rebases.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---

And this answers the question in my review for [4/7]. It would make sense to have these two patch subseries asn three patches (prepare git-rebase.sh, and then convert each backends separately), or a single patch; two patches like this does not make much sense to me.

Show 35 quoted lines

>  git-rebase--interactive.sh | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index cc3a9a7..9514e31 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -597,7 +597,7 @@ do_next () {
>  		fi
>  		;;
>  	esac
> -	test -s "$todo" && return
> +	test -s "$todo" && return 1
>  
>  	comment_for_reflog finish &&
>  	newhead=$(git rev-parse HEAD) &&
> @@ -623,17 +623,15 @@ do_next () {
>  		"$GIT_DIR"/hooks/post-rewrite rebase < "$rewritten_list"
>  		true # we don't care if this hook failed
>  	fi &&
> -	rm -rf "$state_dir" &&
> -	git gc --auto &&
>  	warn "Successfully rebased and updated $head_name."
>  
> -	exit
> +	return 0
>  }
>  
>  do_rest () {
>  	while :
>  	do
> -		do_next
> +		do_next && break
>  	done
>  }

This is somewhat suspicious. We used to die when do_next failed, or let do_next exit with success.

But now you let do_rest return (what does it return???)...

Show 7 quoted lines

>  
> @@ -799,12 +797,12 @@ first and then run 'git rebase --continue' again."
>  	record_in_rewritten "$(cat "$state_dir"/stopped-sha)"
>  
>  	require_clean_work_tree "rebase"
> -	do_rest
> +	do_rest && return 0

... and its caller reports success here only when it succeeds. What happens do_rest returns a failure?

Show 9 quoted lines

>  	;;
>  skip)
>  	git rerere clear
>  
> -	do_rest
> +	do_rest && return 0
>  	;;
>  edit-todo)
>  	git stripspace --strip-comments <"$todo" >"$todo".new