Re: [PATCH 7/7] rebase: implement --[no-]autostash and rebase.autostash

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 23, 2013, 17:34 UTC
Message-ID: <7vip3di21r.fsf@alter.siamese.dyndns.org>
In-Reply-To: <7vr4i1i3a9.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 30 quoted lines

> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> +apply_autostash () {
>> +	if test -f "$state_dir/autostash"
>> +	then
>> +		stash_sha1=$(cat "$state_dir/autostash")
>> +		git stash apply $stash_sha1 2>&1 >/dev/null ||
>> +		die "
>> +$(eval_gettext 'Applying autostash resulted in conflicts.
>> +Either fix the conflicts now, or run
>> +	git reset --hard
>> +and apply the stash on your desired branch:
>> +	git stash apply $stash_sha1
>> +at any time.')" &&
>> +		echo "Applied autostash"
>
> That && looks funny.  What does it even mean for die to succeed and
> give control back to you for a chance to say "Applied"?
>
> 	stash apply || die
> 	echo applied
>
> would be far more logical.
>
>> +	fi
>> +	git gc --auto &&
>> +	rm -rf "$state_dir"
>> +}
>
> This is somewhat worrisome.

One more thing on this function. [4/7] (and [5/7]) justified nicely why it is a good idea to have a central place to do the clean-up tasks. apply_autostash is a poor name for that "clean-up" function. The central clean-up may happen to involve applying a stash in this version, but applying stash will not stay the entirety of the clean-up work forever. The entirety of the clean-up work used to be just 'git gc --auto && rm -fr "$state_dir"' for eternity, and this series is adding something else. It is not hard to imagine somebody else would want to add other kinds of clean-up tasks in here.

Perhaps "finish_rebase" or something?