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

2013-05-10 Thread Ramkumar Ramachandra 
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 artag...@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
 }
 
 run_pre_rebase_hook () {
-- 
1.8.3.rc1.52.gc14258d

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

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

2013-05-10 Thread Eric Sunshine 
On Fri, May 10, 2013 at 10:26 AM, Ramkumar Ramachandra
artag...@gmail.com wrote:
 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 artag...@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

For numeric comparison, use '-eq' rather than '=', which is for strings.

 +   then
 +   git gc --auto 
 +   rm -rf $state_dir
 +   fi
 +   exit $ret
  }

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

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

2013-05-10 Thread Junio C Hamano 
Eric Sunshine sunsh...@sunshineco.com writes:

 On Fri, May 10, 2013 at 10:26 AM, Ramkumar Ramachandra
 artag...@gmail.com wrote:
 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 artag...@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

 For numeric comparison, use '-eq' rather than '=', which is for strings.

Do not listen to this.

We know the condition we want is to have $?  with a value that
stringifies to 0 and the above reads much easier.


 +   then
 +   git gc --auto 
 +   rm -rf $state_dir
 +   fi
 +   exit $ret
  }

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

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

2013-05-10 Thread Junio C Hamano 
Ramkumar Ramachandra artag...@gmail.com 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 artag...@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.


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

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

2013-05-10 Thread Ramkumar Ramachandra 
Junio C Hamano wrote:
 So it is either (1) the added code is unreachable and unexercised at
 this point in the series, or

Yeah, it's (1).

 Perhaps this should come a bit later in the series?

When exactly?  I picked up on your suggestion to separate out the
preparation-for-$backend-to-return step.  The next three patches
convert each of the $backend scripts to return.

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

Okay, I can update the commit message.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html