Re: [PATCH 4/8] rebase: prepare to do generic housekeeping

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Ramkumar Ramachandra <artagnon@xxxxxxxxx> writes:

> On successful completion of a rebase in git-rebase--$backend.sh, the
> $backend script cleans up on its own and exits.  The cleanup routine
> is however, independent of the $backend, and each $backend script
> unnecessarily duplicates this work:
>
>     rm -rf "$state_dir"
>     git gc --auto
>
> Prepare git-rebase.sh for later patches that return control from each
> $backend script back to us, for performing this generic cleanup
> routine.
>
> Another advantage is that git-rebase.sh can implement a generic
> finish_rebase() to possibly do additional tasks in addition to the
> cleanup.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@xxxxxxxxx>
> ---
>  git-rebase.sh | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/git-rebase.sh b/git-rebase.sh
> index 2c692c3..84dc7b0 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -150,6 +150,13 @@ run_specific_rebase () {
>  		autosquash=
>  	fi
>  	. git-rebase--$type
> +	ret=$?
> +	if test $ret = 0
> +	then
> +		git gc --auto &&
> +		rm -rf "$state_dir"
> +	fi
> +	exit $ret
>  }

Doesn't this exit look suspicious?  The existing callsites of this
shell function has a lot of code after them (e.g. when "continue"
$action is given, run_specific_rebase is run) but there is no exit
after the call returns, so they may already be expecting the
function not to return, exiting by itself.  But then the last step
of this function in the original code, ". git-rebase--$type", would
be the one that is causing us to exit, no?

So it is either (1) the added code is unreachable and unexercised at
this point in the series, or (2) my analysis above is incorrect and
". git-rebase--$type" does return to let the caller proceed, but
this patch changes the behaviour and breaks the caller.  I think it
is the former but then the organization of the series does not make
sense.

Perhaps this should come a bit later in the series?

At least the log message should mention that this is adding an
unreachable cruft at this step, if the order is to be kept.


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]