| Message ID | ae9e72b73bf2da0de3a5369748ebd358656588d9.1564049474.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase -r: support merge strategies other than recursive | expand |
On 2019-07-25 at 10:11:22, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > The format of the todo list is quite a bit different in the > `--rebase-merges` mode; Let's prepare the fake editor to handle those > todo lists properly, too. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > t/lib-rebase.sh | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh > index 7ea30e5006..662a958575 100644 > --- a/t/lib-rebase.sh > +++ b/t/lib-rebase.sh > @@ -44,10 +44,10 @@ set_fake_editor () { > rm -f "$1" > echo 'rebase -i script before editing:' > cat "$1".tmp > - action=pick > + action=\& So we set action to "&" so we can use it as the result in the sed expression below… > for line in $FAKE_LINES; do > case $line in > - pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d) > + pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|r|merge|m) > action="$line";; > exec_*|x_*|break|b) > echo "$line" | sed 's/_/ /g' >> "$1";; > @@ -61,8 +61,8 @@ set_fake_editor () { > echo "$action XXXXXXX False commit" >> "$1" but then here it doesn't look like "&" is a thing we'd want to use. Is there something I'm missing about this particular case?
Hi brian, On Fri, 26 Jul 2019, brian m. carlson wrote: > On 2019-07-25 at 10:11:22, Johannes Schindelin via GitGitGadget wrote: > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > > > The format of the todo list is quite a bit different in the > > `--rebase-merges` mode; Let's prepare the fake editor to handle those > > todo lists properly, too. > > > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > --- > > t/lib-rebase.sh | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh > > index 7ea30e5006..662a958575 100644 > > --- a/t/lib-rebase.sh > > +++ b/t/lib-rebase.sh > > @@ -44,10 +44,10 @@ set_fake_editor () { > > rm -f "$1" > > echo 'rebase -i script before editing:' > > cat "$1".tmp > > - action=pick > > + action=\& > > So we set action to "&" so we can use it as the result in the sed > expression below… > > > for line in $FAKE_LINES; do > > case $line in > > - pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d) > > + pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|r|merge|m) > > action="$line";; > > exec_*|x_*|break|b) > > echo "$line" | sed 's/_/ /g' >> "$1";; > > @@ -61,8 +61,8 @@ set_fake_editor () { > > echo "$action XXXXXXX False commit" >> "$1" > > but then here it doesn't look like "&" is a thing we'd want to use. Is > there something I'm missing about this particular case? Actually, the part that uses it is not shown in the patch (one of the many, many reasons why I try to discourage patch review and encourage code review instead). The relevant section currently looks somewhat like this: -- snip -- set_fake_editor () { write_script fake-editor.sh <<-\EOF case "$1" in */COMMIT_EDITMSG) test -z "$EXPECT_HEADER_COUNT" || test "$EXPECT_HEADER_COUNT" = "$(sed -n '1s/^# This is a combination of \(.*\) commits\./\1/p' < "$1")" || test "# # GETTEXT POISON #" = "$(sed -n '1p' < "$1")" || exit test -z "$FAKE_COMMIT_MESSAGE" || echo "$FAKE_COMMIT_MESSAGE" > "$1" test -z "$FAKE_COMMIT_AMEND" || echo "$FAKE_COMMIT_AMEND" >> "$1" exit ;; esac test -z "$EXPECT_COUNT" || test "$EXPECT_COUNT" = $(sed -e '/^#/d' -e '/^$/d' < "$1" | wc -l) || exit test -z "$FAKE_LINES" && exit grep -v '^#' < "$1" > "$1".tmp rm -f "$1" echo 'rebase -i script before editing:' cat "$1".tmp action=pick for line in $FAKE_LINES; do case $line in pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d) action="$line";; exec_*|x_*|break|b) echo "$line" | sed 's/_/ /g' >> "$1";; "#") echo '# comment' >> "$1";; ">") echo >> "$1";; bad) action="badcmd";; fakesha) echo "$action XXXXXXX False commit" >> "$1" action=pick;; *) sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1" action=pick;; esac done echo 'rebase -i script after editing:' cat "$1" EOF test_set_editor "$(pwd)/fake-editor.sh" } -- snap -- Most importantly, `action` is used here: sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1" and I changed it to sed -n "${line}s/^[a-z][a-z]*/$action/p" < "$1".tmp >> "$1" In other words, rather than expecting the lines that are used by the fake editor to start with `pick`, after this patch, the tests expect the todo lists to start with a command consisting of lower-case ASCII letters (which catches `pick`, of course, but also `label`, `reset` and `merge`). After this patch, the fake editor also does not try to replace whatever command it finds by `pick`, but it keeps it as-is instead. Ciao, Dscho
On 2019-07-26 at 14:01:03, Johannes Schindelin wrote: > Actually, the part that uses it is not shown in the patch (one of the > many, many reasons why I try to discourage patch review and encourage > code review instead). The relevant section currently looks somewhat like > this: I feel like I may have communicated poorly earlier, so let me retry asking this in a different way. > -- snip -- > set_fake_editor () { > write_script fake-editor.sh <<-\EOF > case "$1" in > */COMMIT_EDITMSG) > test -z "$EXPECT_HEADER_COUNT" || > test "$EXPECT_HEADER_COUNT" = "$(sed -n '1s/^# This is a combination of \(.*\) commits\./\1/p' < "$1")" || > test "# # GETTEXT POISON #" = "$(sed -n '1p' < "$1")" || > exit > test -z "$FAKE_COMMIT_MESSAGE" || echo "$FAKE_COMMIT_MESSAGE" > "$1" > test -z "$FAKE_COMMIT_AMEND" || echo "$FAKE_COMMIT_AMEND" >> "$1" > exit > ;; > esac > test -z "$EXPECT_COUNT" || > test "$EXPECT_COUNT" = $(sed -e '/^#/d' -e '/^$/d' < "$1" | wc -l) || > exit > test -z "$FAKE_LINES" && exit > grep -v '^#' < "$1" > "$1".tmp > rm -f "$1" > echo 'rebase -i script before editing:' > cat "$1".tmp > action=pick I believe you changed this line to "action=\&". > for line in $FAKE_LINES; do > case $line in > pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d) > action="$line";; > exec_*|x_*|break|b) > echo "$line" | sed 's/_/ /g' >> "$1";; > "#") > echo '# comment' >> "$1";; > ">") > echo >> "$1";; > bad) > action="badcmd";; > fakesha) > echo "$action XXXXXXX False commit" >> "$1" And my question was about this line. > action=pick;; > *) > sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1" > action=pick;; > esac > done > echo 'rebase -i script after editing:' > cat "$1" > EOF > > test_set_editor "$(pwd)/fake-editor.sh" > } > -- snap -- > > Most importantly, `action` is used here: > > sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1" > > and I changed it to > > sed -n "${line}s/^[a-z][a-z]*/$action/p" < "$1".tmp >> "$1" > > In other words, rather than expecting the lines that are used by the > fake editor to start with `pick`, after this patch, the tests expect the > todo lists to start with a command consisting of lower-case ASCII > letters (which catches `pick`, of course, but also `label`, `reset` and > `merge`). > > After this patch, the fake editor also does not try to replace whatever > command it finds by `pick`, but it keeps it as-is instead. Right, that's how I read it, and that part I agree with. I think my question is this: in what case do we execute the "fakesha" case? Are we guaranteed that when we do, action isn't "&"? "&" seems fine for the right-hand side of a sed s-statement, but not as the beginning of a typical line in a sequencer file. I ask because if we're testing a failure case, we want it to fail for the right reason (e.g., the commit doesn't exist), and not because we're producing invalid data. If the answer in this case is, "Well, we'll always have something else before it which will set $action properly," then that's fine. This is test code, so it need not be bulletproof, but I did want to ask. If I'm still misunderstanding something, I apologize.
Hi brian, On Fri, 26 Jul 2019, brian m. carlson wrote: > On 2019-07-26 at 14:01:03, Johannes Schindelin wrote: > > Actually, the part that uses it is not shown in the patch (one of the > > many, many reasons why I try to discourage patch review and encourage > > code review instead). The relevant section currently looks somewhat like > > this: > > I feel like I may have communicated poorly earlier, so let me retry > asking this in a different way. Actually, your communication was just fine, the misunderstanding was entirely on my side. My apologies. > > -- snip -- > > set_fake_editor () { > > write_script fake-editor.sh <<-\EOF > > case "$1" in > > */COMMIT_EDITMSG) > > test -z "$EXPECT_HEADER_COUNT" || > > test "$EXPECT_HEADER_COUNT" = "$(sed -n '1s/^# This is a combination of \(.*\) commits\./\1/p' < "$1")" || > > test "# # GETTEXT POISON #" = "$(sed -n '1p' < "$1")" || > > exit > > test -z "$FAKE_COMMIT_MESSAGE" || echo "$FAKE_COMMIT_MESSAGE" > "$1" > > test -z "$FAKE_COMMIT_AMEND" || echo "$FAKE_COMMIT_AMEND" >> "$1" > > exit > > ;; > > esac > > test -z "$EXPECT_COUNT" || > > test "$EXPECT_COUNT" = $(sed -e '/^#/d' -e '/^$/d' < "$1" | wc -l) || > > exit > > test -z "$FAKE_LINES" && exit > > grep -v '^#' < "$1" > "$1".tmp > > rm -f "$1" > > echo 'rebase -i script before editing:' > > cat "$1".tmp > > action=pick > > I believe you changed this line to "action=\&". > > > for line in $FAKE_LINES; do > > case $line in > > pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d) > > action="$line";; > > exec_*|x_*|break|b) > > echo "$line" | sed 's/_/ /g' >> "$1";; > > "#") > > echo '# comment' >> "$1";; > > ">") > > echo >> "$1";; > > bad) > > action="badcmd";; > > fakesha) > > echo "$action XXXXXXX False commit" >> "$1" > > And my question was about this line. Right. It would append `& XXXXXXX False commit`, which is not a valid todo command. So something like - echo "$action XXXXXXX False commit" >> "$1" + test \& = "$action" && c=pick || c=$action + echo "$c XXXXXXX False commit" >>"$1" would be needed. However, what makes me really worried now is that our test suite did not have a fit about this. The CI build passes the test suite on Windows, macOS and Linux: https://github.com/gitgitgadget/git/runs/176651523. > > > action=pick;; > > *) > > sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1" > > action=pick;; > > esac > > done > > echo 'rebase -i script after editing:' > > cat "$1" > > EOF > > > > test_set_editor "$(pwd)/fake-editor.sh" > > } > > -- snap -- > > > > Most importantly, `action` is used here: > > > > sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1" > > > > and I changed it to > > > > sed -n "${line}s/^[a-z][a-z]*/$action/p" < "$1".tmp >> "$1" > > > > In other words, rather than expecting the lines that are used by the > > fake editor to start with `pick`, after this patch, the tests expect the > > todo lists to start with a command consisting of lower-case ASCII > > letters (which catches `pick`, of course, but also `label`, `reset` and > > `merge`). > > > > After this patch, the fake editor also does not try to replace whatever > > command it finds by `pick`, but it keeps it as-is instead. > > Right, that's how I read it, and that part I agree with. I think my > question is this: in what case do we execute the "fakesha" case? Are we > guaranteed that when we do, action isn't "&"? "&" seems fine for the > right-hand side of a sed s-statement, but not as the beginning of a > typical line in a sequencer file. Indeed, the sequencer should throw a real tantrum about this and not even bother to start. But then, the same would hold true for an obviously invalid commit hash. > I ask because if we're testing a failure case, we want it to fail for > the right reason (e.g., the commit doesn't exist), and not because we're > producing invalid data. Indeed. I have even come to the conclusion that our `test_expect_failure` command encourages exactly this type of problem: in the beginning, those test cases might actually be correct, but over time they are prone to fail for all the wrong reasons, because we do not actually test for a specific failure more, we just say that we expect this test case to fail (and that this indicates a bug). > If the answer in this case is, "Well, we'll always have something else > before it which will set $action properly," then that's fine. This is > test code, so it need not be bulletproof, but I did want to ask. I think you are perfectly sane to question this, and to expect me to double check. So, double check I did. Turns out there is a single user of the `fakesha` thing, and it is hidden deep in t3404, prefixed by `test_must_fail`: -- snip -- test_expect_success 'static check of bad SHA-1' ' rebase_setup_and_clean bad-sha && set_fake_editor && test_must_fail env FAKE_LINES="1 2 edit fakesha 3 4 5 #" \ git rebase -i --root 2>actual && test_i18ngrep "edit XXXXXXX False commit" actual && test_i18ngrep "You can fix this with .git rebase --edit-todo.." actual && FAKE_LINES="1 2 4 5 6" git rebase --edit-todo && git rebase --continue && test E = $(git cat-file commit HEAD | sed -ne \$p) ' -- snap -- As you can see, contrary to my expectations it does verify the output. It *also* changes the action to `edit`, which is the reason why there is no `&` ;-) But I think you are correct, I should make sure that the fake editor is still correct with respect to the `pick` command. > If I'm still misunderstanding something, I apologize. I am really impressed and inspired by your gentle language. Thank you for this. Ciao, Dscho
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh index 7ea30e5006..662a958575 100644 --- a/t/lib-rebase.sh +++ b/t/lib-rebase.sh @@ -44,10 +44,10 @@ set_fake_editor () { rm -f "$1" echo 'rebase -i script before editing:' cat "$1".tmp - action=pick + action=\& for line in $FAKE_LINES; do case $line in - pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d) + pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|r|merge|m) action="$line";; exec_*|x_*|break|b) echo "$line" | sed 's/_/ /g' >> "$1";; @@ -61,8 +61,8 @@ set_fake_editor () { echo "$action XXXXXXX False commit" >> "$1" action=pick;; *) - sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1" - action=pick;; + sed -n "${line}s/^[a-z][a-z]*/$action/p" < "$1".tmp >> "$1" + action=\&;; esac done echo 'rebase -i script after editing:'