Re: [PATCH v2 1/2] t3418: add testcase showing problems with rebase -i and strategy options
Elijah Newren writes: > Seems reasonable. Since these tests were essentially copies of other > tests within the same file, just for rebase -i instead of -m, should I > also add another patch to the series fixing up the rebase -m testcase > to also replace the subshell with a single-shot environment > assignment? I personally would think it would be best to leave that to a later day, and take your v3 that properly &&-chains these two assignments. It may be a good clean-up, but is an overkill if done as a preparatory clean-up in the context of these two small fixes.
Re: [PATCH v2 1/2] t3418: add testcase showing problems with rebase -i and strategy options
Johannes Sixt writes: > Pitfalls ahead! Thanks. I said "at least the latter" for this exact reason ;-). > PATH=... git rebase ... > > is OK, but > > PATH=... test_must_fail git rebase ... > > is not; the latter requires the subshell, otherwise the modified PATH > variable survives the command because test_must_fail is a shell > function. Yes, it's silly, but that's how it is. Or you could do test_must_fail env VAR=VAL git rebase ... which I am not particularly fond of, but seems to be used quite often.
Re: [PATCH v2 1/2] t3418: add testcase showing problems with rebase -i and strategy options
On Wed, Jun 27, 2018 at 2:17 PM Johannes Sixt wrote: > Pitfalls ahead! > > PATH=... git rebase ... > > is OK, but > > PATH=... test_must_fail git rebase ... > > is not; the latter requires the subshell, otherwise the modified PATH > variable survives the command because test_must_fail is a shell > function. Yes, it's silly, but that's how it is. As an alternative to the subshell, some test use 'env': test_must_fail env PATH=... git rebase ...
Re: [PATCH v2 1/2] t3418: add testcase showing problems with rebase -i and strategy options
Am 27.06.2018 um 19:27 schrieb Elijah Newren: On Wed, Jun 27, 2018 at 9:54 AM, Junio C Hamano wrote: Having said that, it would be simpler for at least the latter to write it using a single-shot environment assignment, perhaps? I.e. PATH=./test-bin:$PATH git rebase --continue && without running in a subshell? Seems reasonable. Since these tests were essentially copies of other tests within the same file, just for rebase -i instead of -m, should I also add another patch to the series fixing up the rebase -m testcase to also replace the subshell with a single-shot environment assignment? Pitfalls ahead! PATH=... git rebase ... is OK, but PATH=... test_must_fail git rebase ... is not; the latter requires the subshell, otherwise the modified PATH variable survives the command because test_must_fail is a shell function. Yes, it's silly, but that's how it is. -- Hannes
Re: [PATCH v2 1/2] t3418: add testcase showing problems with rebase -i and strategy options
On Wed, Jun 27, 2018 at 9:54 AM, Junio C Hamano wrote: > + ( > + PATH=./test-bin:$PATH > + test_must_fail git rebase -i -s funny -Xopt -Xfoo master topic > + ) && > > + ( > + PATH=./test-bin:$PATH > + git rebase --continue > + ) && > > and it would be reasonable to assume that these variable assignments > would not fail. IOW, there is no BUG in breaking &&-chain at these > two places. It is merely that the automated mechanism introduced by > step 29/29 of that other series does not understand that (which is > expected). It is not wrong to have && at the end of the assignment, > though. > > Having said that, it would be simpler for at least the latter to > write it using a single-shot environment assignment, perhaps? I.e. > > PATH=./test-bin:$PATH git rebase --continue && > > without running in a subshell? Seems reasonable. Since these tests were essentially copies of other tests within the same file, just for rebase -i instead of -m, should I also add another patch to the series fixing up the rebase -m testcase to also replace the subshell with a single-shot environment assignment?
Re: [PATCH v2 1/2] t3418: add testcase showing problems with rebase -i and strategy options
Elijah Newren writes: >>> + chmod +x test-bin/git-merge-funny && >>> + ( >>> + PATH=./test-bin:$PATH >> >> Broken &&-chain (in subshell). >> >>> + test_must_fail git rebase -i -s funny -Xopt -Xfoo master >>> topic >>> + ) && >>> + test -f funny.was.run && >>> + rm funny.was.run && >>> + echo "Resolved" >F2 && >>> + git add F2 && >>> + ( >>> + PATH=./test-bin:$PATH >> >> Ditto. >> > > I'm just trying to prove how important your other patch series is. ;-) Actually, this shows why the other patch series, especially its last step, is problematic a bit. The above assignments are followed by a single command, i.e. + ( + PATH=./test-bin:$PATH + test_must_fail git rebase -i -s funny -Xopt -Xfoo master topic + ) && + ( + PATH=./test-bin:$PATH + git rebase --continue + ) && and it would be reasonable to assume that these variable assignments would not fail. IOW, there is no BUG in breaking &&-chain at these two places. It is merely that the automated mechanism introduced by step 29/29 of that other series does not understand that (which is expected). It is not wrong to have && at the end of the assignment, though. Having said that, it would be simpler for at least the latter to write it using a single-shot environment assignment, perhaps? I.e. PATH=./test-bin:$PATH git rebase --continue && without running in a subshell?
Re: [PATCH v2 1/2] t3418: add testcase showing problems with rebase -i and strategy options
On Wed, Jun 27, 2018 at 12:45 AM, Eric Sunshine wrote: > On Wed, Jun 27, 2018 at 3:36 AM Elijah Newren wrote: >> We are not passing the same args to merge strategies when we are doing an >> --interactive rebase as we do with a --merge rebase. The merge strategy >> should not need to be aware of which type of rebase is in effect. Add a >> testcase which checks for the appropriate args. >> >> Signed-off-by: Elijah Newren >> --- >> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh >> @@ -74,6 +74,38 @@ test_expect_success 'rebase --continue remembers merge >> strategy and options' ' >> +test_expect_failure 'rebase -i --continue handles merge strategy and >> options' ' >> + rm -fr .git/rebase-* && >> + git reset --hard commit-new-file-F2-on-topic-branch && >> + test_commit "commit-new-file-F3-on-topic-branch-for-dash-i" F3 32 && >> + test_when_finished "rm -fr test-bin funny.was.run funny.args" && >> + mkdir test-bin && >> + cat >test-bin/git-merge-funny <<-EOF && >> + #!$SHELL_PATH >> + echo "\$@" >>funny.args >> + case "\$1" in --opt) ;; *) exit 2 ;; esac >> + case "\$2" in --foo) ;; *) exit 2 ;; esac >> + case "\$4" in --) ;; *) exit 2 ;; esac >> + shift 2 && >> + >funny.was.run && >> + exec git merge-recursive "\$@" >> + EOF >> + chmod +x test-bin/git-merge-funny && >> + ( >> + PATH=./test-bin:$PATH > > Broken &&-chain (in subshell). > >> + test_must_fail git rebase -i -s funny -Xopt -Xfoo master >> topic >> + ) && >> + test -f funny.was.run && >> + rm funny.was.run && >> + echo "Resolved" >F2 && >> + git add F2 && >> + ( >> + PATH=./test-bin:$PATH > > Ditto. > I'm just trying to prove how important your other patch series is. ;-) Doh, sorry for the mistake. Again. I'll fix it up.
Re: [PATCH v2 1/2] t3418: add testcase showing problems with rebase -i and strategy options
On Wed, Jun 27, 2018 at 3:36 AM Elijah Newren wrote: > We are not passing the same args to merge strategies when we are doing an > --interactive rebase as we do with a --merge rebase. The merge strategy > should not need to be aware of which type of rebase is in effect. Add a > testcase which checks for the appropriate args. > > Signed-off-by: Elijah Newren > --- > diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh > @@ -74,6 +74,38 @@ test_expect_success 'rebase --continue remembers merge > strategy and options' ' > +test_expect_failure 'rebase -i --continue handles merge strategy and > options' ' > + rm -fr .git/rebase-* && > + git reset --hard commit-new-file-F2-on-topic-branch && > + test_commit "commit-new-file-F3-on-topic-branch-for-dash-i" F3 32 && > + test_when_finished "rm -fr test-bin funny.was.run funny.args" && > + mkdir test-bin && > + cat >test-bin/git-merge-funny <<-EOF && > + #!$SHELL_PATH > + echo "\$@" >>funny.args > + case "\$1" in --opt) ;; *) exit 2 ;; esac > + case "\$2" in --foo) ;; *) exit 2 ;; esac > + case "\$4" in --) ;; *) exit 2 ;; esac > + shift 2 && > + >funny.was.run && > + exec git merge-recursive "\$@" > + EOF > + chmod +x test-bin/git-merge-funny && > + ( > + PATH=./test-bin:$PATH Broken &&-chain (in subshell). > + test_must_fail git rebase -i -s funny -Xopt -Xfoo master topic > + ) && > + test -f funny.was.run && > + rm funny.was.run && > + echo "Resolved" >F2 && > + git add F2 && > + ( > + PATH=./test-bin:$PATH Ditto. > + git rebase --continue > + ) && > + test -f funny.was.run > +'
