On Apr 12, 2014, at 10:07, Matthieu Moy wrote:
> "Kyle J. McKay" <mackyle@xxxxxxxxx> writes:
>
>> 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
>
> You're confusing function calls and function nesting. do_rest calls
> do_next, but the definition of do_next is not nested within do_rest.
>
> When I talk about nested function, I mean
>
> f() {
> g() {
> ...
> }
> }
>
> Obviously, having functions call each other is not an issue. That's
> what
> functions are meant to be.
>
> Now, having run_specific_rebase_internal include a file which defines
> functions which contain nested functions _is_ something I find
> weird. It
> both stresses the shell in a buggy area and makes the code harder to
> understand.
I meant: "nested functions" = "nested function calls"
You meant: "nested functions" = "nested function definitions"
Okay. But nested function definitions is not something new to the
rebase code.
>> 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.
>
> Function definitions, and variables assignments. Is it so much of an
> issue?
>
> What's the difference between a function definition or variable
> assignment within git-rebase--*.sh and within git-rebase.sh?
As I said, this is the issue:
On Apr 11, 2014, at 16:08, Kyle J. McKay wrote:
> 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.
Let me rephrase.
Patch 1/3 does not change the order in which individual statements are
executed in the rebase code. Nor does it change the logic. It simply
introduces one additional function callstack level, but the same
individual statements are executed in the same order for all control
flows. No additional statements other than the introduced callstack
level. Nothing's executed in a different order. No control paths
execute additional statements they did not before.
The changes you propose introduce exactly the same additional function
callstack level. Then they proceed to alter the order in which
statements are executed. Statements that did not execute in some
control paths before are now executed in those control paths. Other
statements are moved around to execute earlier/later than they did
before.
My point is not that this is wrong. It's nice, really, to move the
"dot" include to the top level. The point is that's not necessary to
fix the problem and moving statements around and adding statements to
some control paths increases the risk of introducing an uncaught bug
when it's not necessary to fix the problem.
I would like to get a fix in so that rebase works out-of-the-box when
Git version 1.8.4 or later is built on FreeBSD.
So I'm not going to belabor the point anymore.
There's a follow-up patch 4/3 attached to the end of this message that
makes the changes you have proposed in your earlier email on top of
patches 1/3 and 2/3. The log message and all changes are taken from
your emails and so this patch is assigned to you and has a
'Needs-Signed-off-by:' line.
On Apr 11, 2014, at 10:30, Matthieu Moy wrote:
> 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.
Unfortunately, after applying the below patch, some of the rebase
tests (3403, 3407, 3418, 3420, 3421) start failing for me on systems
where they did not fail previously. Even though some of the previously
failing tests on FreeBSD now pass with the patch, 3421 now fails on
FreeBSD where it did not before.
Here's a test summary of the t34xx*.sh tests:
NOTE: at the time of these tests, maint and v1.9.2 were at the same
commit.
______________SYSTEM____t34xx*.sh_results________________
maint FreeBSD FAIL: 3403,3404,3407,3409,3410,3412,3418,3419,3420
maint Darwin PASS
maint Linux PASS
maint+1-3/3 FreeBSD PASS
maint+1-3/3 Darwin PASS
maint+1-3/3 Linux PASS
maint+1-4/3 FreeBSD FAIL[*]: 3403,3407,3418,3420,3421
maint+1-4/3 Darwin FAIL[*]: 3403,3407,3418,3420,3421
maint+1-4/3 Linux FAIL[*]: 3403,3407,3418,3420,3421
[*]: The failing test reports for all three systems are identical for
the "maint+1-4/3" run (except for the wallclock secs part):
Test Summary Report
-------------------
t3403-rebase-skip.sh (Wstat: 256 Tests: 10 Failed: 3)
Failed tests: 8-10
Non-zero exit status: 1
t3407-rebase-abort.sh (Wstat: 256 Tests: 11 Failed: 3)
Failed tests: 7-9
Non-zero exit status: 1
t3418-rebase-continue.sh (Wstat: 256 Tests: 6 Failed: 2)
Failed tests: 5-6
Non-zero exit status: 1
t3420-rebase-autostash.sh (Wstat: 256 Tests: 24 Failed: 11)
Failed tests: 14-24
Non-zero exit status: 1
t3421-rebase-topology-linear.sh (Wstat: 256 Tests: 76 Failed: 16)
Failed tests: 51-53, 55, 57, 59-63, 65, 67, 73-76
Non-zero exit status: 1
Files=22, Tests=374, 354 wallclock secs ( 0.31 usr 0.13 sys + 79.57 cusr 233.99 csys = 314.00 CPU)
Result: FAIL
make: *** [prove] Error 1
So I suggest that in the interest of fixing rebase on FreeBSD in an
expeditious fashion, patches 1/3 and 2/3 get picked up (see note
below) now and that the follow-on patch below, after being enhanced to
pass all tests, be submitted separately at some future point.
--Kyle
P.S. Note to JCH: the below patch requires the previous 1/3 and 2/3
be applied first. As per SubmittingPatches for bug fixes those are
based on maint. Because of the whitespace change 1/3 introduces it
does not apply cleanly to master, next or pu. :( Please let me know
if you would like me to resend the initial series (1 & 2 -- 3 has
already been picked up) based on a different branch instead of maint.
---- 8< ----
From: Matthieu Moy <Matthieu.Moy@xxxxxxxxxxxxxxx>
Subject: [PATCH 4/3] rebase: stop using . within function
Move the whole run_specific_rebase_internal function to
git-rebase--$type.
The .-ed script defines the complete function, and then the
function is used from the toplevel script.
The goal is to avoid using tricky features that may trigger
bugs on some shells.
The result is simpler, just using the basic pattern:
1. use '. file' to import a set of functions
2. then use these functions
Needs-Signed-off-by: Matthieu Moy <Matthieu.Moy@xxxxxxxxxxxxxxx>
---
git-rebase--am.sh | 3 +--
git-rebase--interactive.sh | 3 +--
git-rebase--merge.sh | 3 +--
git-rebase.sh | 40 +++++++++++++++++++++-------------------
4 files changed, 24 insertions(+), 25 deletions(-)
diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index 2d3f6d55..b48b3e90 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -4,7 +4,7 @@
# Copyright (c) 2010 Junio C Hamano.
#
-git_rebase__am() {
+run_specific_rebase_infile() {
case "$action" in
continue)
git am --resolved --resolvemsg="$resolvemsg" &&
@@ -75,4 +75,3 @@ git_rebase__am() {
move_to_original_branch
}
-git_rebase__am
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 42164f11..a7670eb0 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -810,7 +810,7 @@ add_exec_commands () {
mv "$1.new" "$1"
}
-git_rebase__interactive() {
+run_specific_rebase_infile() {
case "$action" in
continue)
# do we have anything to commit?
@@ -1044,4 +1044,3 @@ EOF
git update-ref ORIG_HEAD $orig_head
do_rest
}
-git_rebase__interactive
diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
index b5f05bf5..9550e656 100644
--- a/git-rebase--merge.sh
+++ b/git-rebase--merge.sh
@@ -101,7 +101,7 @@ finish_rb_merge () {
say All done.
}
-git_rebase__merge() {
+run_specific_rebase_infile() {
case "$action" in
continue)
read_state
@@ -153,4 +153,3 @@ git_rebase__merge() {
finish_rb_merge
}
-git_rebase__merge
diff --git a/git-rebase.sh b/git-rebase.sh
index 07e2bd48..9e105626 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -175,7 +175,7 @@ run_specific_rebase () {
export GIT_EDITOR
autosquash=
fi
- . git-rebase--$type
+ run_specific_rebase_infile
ret=$?
if test $ret -eq 0
then
@@ -353,6 +353,26 @@ 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 -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
+
+. git-rebase--$type
+
case "$action" in
continue)
# Sanity check
@@ -407,24 +427,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
--
1.8.5
--
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