"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 ...
>> 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--am.sh b/git-rebase--am.sh
index df46f4c..4f7b22d 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -4,6 +4,8 @@
# Copyright (c) 2010 Junio C Hamano.
#
+run_specific_rebase_infile() {
+
case "$action" in
continue)
git am --resolved --resolvemsg="$resolvemsg" \
@@ -75,3 +77,4 @@ then
fi
move_to_original_branch
+}
[ Same patch for other git-rebase--*.sh variants]
index 76f7f71..1a150bd 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -186,7 +186,7 @@ run_specific_rebase_internal () {
# run_specific_rebase_internal has the file inclusion as a
# last statement, so POSIX and FreeBSD's return will do the
# same thing.
- . git-rebase--$type
+ run_specific_rebase_infile
}
run_specific_rebase () {
@@ -438,6 +438,8 @@ else
state_dir="$apply_dir"
fi
+. git-rebase--$type
+
if test -z "$rebase_root"
then
case "$#" in
I minimized patch size, so it would obviously need a reidentation, and
would require some cleanup so that run_specific_rebase_internal is
merged back into run_specific_rebase (a bit like your PATCH 2).
I find the result simpler, just using the basic pattern "use '. file' to
import a set of functions, and then use these functions".
The real patch is a bit more tricky though, because we need to run the
". git-rebase--$type" after computing type properly. A patch passing the
tests but requiring cleanup is given below.
> To make the kind of change I think you're proposing would be somewhat
> more invasive than the proposed patch. Each of the git-rebase--$type
> scripts would have to be modified not to do anything other than define
> functions
That's almost what your patch does already: move everything into a
function, and call it. Except, I'd put the function call outside the
file inclusion.
diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index df46f4c..4f7b22d 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -4,6 +4,8 @@
# Copyright (c) 2010 Junio C Hamano.
#
+run_specific_rebase_infile() {
+
case "$action" in
continue)
git am --resolved --resolvemsg="$resolvemsg" \
@@ -75,3 +77,4 @@ then
fi
move_to_original_branch
+}
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
+}
diff --git a/git-rebase.sh b/git-rebase.sh
index 76f7f71..63e0e68 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -186,7 +186,7 @@ run_specific_rebase_internal () {
# run_specific_rebase_internal has the file inclusion as a
# last statement, so POSIX and FreeBSD's return will do the
# same thing.
- . git-rebase--$type
+ run_specific_rebase_infile
}
run_specific_rebase () {
@@ -366,6 +366,29 @@ then
die "$(gettext "The --edit-todo action can only be used during interactive rebase.")"
fi
+if test -n "$rebase_root" && test -z "$onto"
+then
+ test -z "$interactive_rebase" && interactive_rebase=implied
+fi
+
+if test -z "$in_progress"
+then
+ if test -n "$interactive_rebase"
+ then
+ type=interactive
+ state_dir="$merge_dir"
+ elif test -n "$do_merge"
+ then
+ type=merge
+ state_dir="$merge_dir"
+ else
+ type=am
+ state_dir="$apply_dir"
+ fi
+fi
+
+. git-rebase--$type
+
case "$action" in
continue)
# Sanity check
@@ -420,24 +443,6 @@ and run me again. I am stopping in case you still have something
valuable there.')"
fi
-if test -n "$rebase_root" && test -z "$onto"
-then
- test -z "$interactive_rebase" && interactive_rebase=implied
-fi
-
-if test -n "$interactive_rebase"
-then
- type=interactive
- state_dir="$merge_dir"
-elif test -n "$do_merge"
-then
- type=merge
- state_dir="$merge_dir"
-else
- type=am
- state_dir="$apply_dir"
-fi
-
if test -z "$rebase_root"
then
case "$#" in
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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