git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
May 10, 2013, 15:32 UTC
Message-ID
<7vvc6q3l6t.fsf@alter.siamese.dyndns.org>
In-Reply-To
<1368196005-5354-5-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 37 quoted lines
> 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@gmail.com>
> ---
>  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.

Previous: Junio C HamanoNext: Ramkumar Ramachandra
Message 11 of 18 in “rebase.autostash completed”
  1. 0/8 rebase.autostash completedRamkumar Ramachandra, May 10, 2013
  2. 1/8 am: suppress error output from a conditionalRamkumar Ramachandra, May 10, 2013
  3. Junio C HamanoMay 10, 2013
  4. Ramkumar RamachandraMay 11, 2013
  5. Junio C HamanoMay 12, 2013
  6. 2/8 rebase -i: don't error out if $state_dir already existsRamkumar Ramachandra, May 10, 2013
  7. 3/8 am: tighten a conditional that checks for $dotestRamkumar Ramachandra, May 10, 2013
  8. 4/8 rebase: prepare to do generic housekeepingRamkumar Ramachandra, May 10, 2013
  9. Eric SunshineMay 10, 2013
  10. Junio C HamanoMay 10, 2013
  11. Junio C HamanoMay 10, 2013
  12. Ramkumar RamachandraMay 10, 2013
  13. 5/8 am: return control to caller, for housekeepingRamkumar Ramachandra, May 10, 2013
  14. 6/8 rebase -i: return control to caller, for housekeepingRamkumar Ramachandra, May 10, 2013
  15. 7/8 rebase --merge: return control to caller, for housekeepingRamkumar Ramachandra, May 10, 2013
  16. 8/8 rebase: implement --[no-]autostash and rebase.autostashRamkumar Ramachandra, May 10, 2013
  17. Eric SunshineMay 10, 2013
  18. Junio C HamanoMay 10, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.