On Apr 11, 2014, at 10:30, Matthieu Moy wrote:
"Kyle J. McKay" <mackyle@xxxxxxxxx> writes:
There are already nested functions with file inclusion between both
levels of nesting in git-rebase--interactive.sh and git-rebase--
merge.sh now, so it's not introducing anything new.
OK, so it's less serious than I thought. But still, we're
introducing a
function with 3 levels of nesting, split accross files, in an area
where
we know that at least one shell is buggy ...
Currently in maint:
The current code in maint does this:
git-rebase.sh: top-level
git-rebase.sh: run_specific_rebase()
git-rebase.sh: run_specific_rebase_internal() -- contains "dot"
git-rebase--interactive.sh: top-level (using --continue or --
skip)
git-rebase--interactive.sh: do_rest
git-rebase--interactive.sh: do_next
git-rebase--interactive.sh: record_in_rewritten
git-rebase--interactive.sh: flush_rewritten_pending
So I really do not see the additional level of nesting as an issue
since we've already got much more than 3 levels of nesting going on
now. If nesting was going to be a problem, something would have
broken already. In fact, since the follow on patch removes the
run_specific_rebase_internal function what we would have after the
originally proposed first two patches is:
git-rebase.sh: top-level
git-rebase.sh: run_specific_rebase() -- contains "dot"
git-rebase--interactive.sh: top-level (using --continue or --skip)
git-rebase--interactive.sh: git_rebase__interactive
git-rebase--interactive.sh: do_rest
git-rebase--interactive.sh: do_next
git-rebase--interactive.sh: record_in_rewritten
git-rebase--interactive.sh: flush_rewritten_pending
Which has exactly the same nesting depth albeit the "dot" has moved up
one level.
IOW, why not move the whole run_specific_rebase_internal function to
git-rebase--$type?
So what change are you proposing exactly?
Something along the lines of this:
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 6046778..5dfbf14 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -820,6 +820,7 @@ add_exec_commands () {
mv "$1.new" "$1"
}
+run_specific_rebase_infile() {
case "$action" in
continue)
# do we have anything to commit?
@@ -1055,3 +1056,4 @@ GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION:
checkout $onto_name"
output git checkout $onto || die_abort "could not detach HEAD"
git update-ref ORIG_HEAD $orig_head
do_rest
+}
diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
index d84f412..907aa46 100644
--- a/git-rebase--merge.sh
+++ b/git-rebase--merge.sh
@@ -99,6 +99,7 @@ finish_rb_merge () {
say All done.
}
+run_specific_rebase_infile () {
case "$action" in
continue)
read_state
@@ -149,3 +150,4 @@ do
done
finish_rb_merge
+}
The problem with these changes, particularly the git-rebase--
interactive.sh one is that a bunch of code is still run when the file
is "dot" included. With the changes to git-rebase.sh, that code will
now run regardless of the action and it will run before it would have
now. So if any of the variables it sets affect the functions like
read_basic_state or finish_rebase (they don't currently appear to),
then there's a potential for new bugs. That initial code would not
previously have run in the --abort case at all.
But, you say the tests pass with those changes, so the changes are
probably okay. However, they create a potential situation where some
code is added to the top of one of the git-rebase--$type.sh scripts
and suddenly git rebase --abort stops working right because that code
is being run when it shouldn't or the operation of read_basic_state
and/or finish_rebase is adversely affected. Hopefully the rebase
tests would catch any such issue right away though.
So, in light of the fact that function nesting seems to be a non-issue
here, and it seems to me the originally proposed changes have much
less potential to introduce breakage either now or in the future, I
still prefer them.
--Kyle
--
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