From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: "brian m. carlson" <sandals@crustytoothpaste.net>
Cc: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 10/12] t/lib-rebase: prepare for testing `git rebase --rebase-merges`
Date: Wed, 31 Jul 2019 13:25:42 +0200 (CEST) [thread overview]
Message-ID: <nycvar.QRO.7.76.6.1907311306360.21907@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20190726210820.GF9319@genre.crustytoothpaste.net>
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
next prev parent reply other threads:[~2019-07-31 11:25 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-25 10:11 [PATCH 00/12] rebase -r: support merge strategies other than recursive Johannes Schindelin via GitGitGadget
2019-07-25 10:11 ` [PATCH 01/12] t3427: add a clarifying comment Johannes Schindelin via GitGitGadget
2019-07-25 10:11 ` [PATCH 02/12] t3427: simplify the `setup` test case significantly Johannes Schindelin via GitGitGadget
2019-07-25 10:11 ` [PATCH 03/12] t3427: move the `filter-branch` invocation into the `setup` case Johannes Schindelin via GitGitGadget
2019-07-25 10:11 ` [PATCH 05/12] t3427: fix erroneous assumption Johannes Schindelin via GitGitGadget
2019-07-25 10:11 ` [PATCH 04/12] t3427: condense the unnecessarily repetitive test cases into three Johannes Schindelin via GitGitGadget
2019-07-25 10:11 ` [PATCH 06/12] t3427: accommodate for the `rebase --merge` backend having been replaced Johannes Schindelin via GitGitGadget
2019-07-25 10:11 ` [PATCH 08/12] t3427: mark two test cases as requiring support for `git rebase -p` Johannes Schindelin via GitGitGadget
2019-07-25 10:11 ` [PATCH 07/12] t3427: fix another incorrect assumption Johannes Schindelin via GitGitGadget
2019-07-25 10:11 ` [PATCH 09/12] rebase -r: support merge strategies other than `recursive` Johannes Schindelin via GitGitGadget
2019-07-25 10:11 ` [PATCH 10/12] t/lib-rebase: prepare for testing `git rebase --rebase-merges` Johannes Schindelin via GitGitGadget
2019-07-26 7:43 ` brian m. carlson
2019-07-26 14:01 ` Johannes Schindelin
2019-07-26 21:08 ` brian m. carlson
2019-07-31 11:25 ` Johannes Schindelin [this message]
2019-07-25 10:11 ` [PATCH 11/12] t3418: test `rebase -r` with merge strategies Johannes Schindelin via GitGitGadget
2019-07-25 10:11 ` [PATCH 12/12] rebase -r: do not (re-)generate root commits with `--root` *and* `--onto` Johannes Schindelin via GitGitGadget
2019-07-25 17:10 ` [PATCH 00/12] rebase -r: support merge strategies other than recursive Junio C Hamano
2019-07-26 7:52 ` brian m. carlson
2019-07-31 15:18 ` [PATCH v2 00/16] " Johannes Schindelin via GitGitGadget
2019-07-31 15:18 ` [PATCH v2 01/16] Drop unused git-rebase--am.sh Johannes Schindelin via GitGitGadget
2019-07-31 15:18 ` [PATCH v2 02/16] t3400: stop referring to the scripted rebase Johannes Schindelin via GitGitGadget
2019-07-31 15:18 ` [PATCH v2 03/16] .gitignore: there is no longer a built-in `git-rebase--interactive` Johannes Schindelin via GitGitGadget
2019-07-31 15:18 ` [PATCH v2 04/16] sequencer: the `am` and `rebase--interactive` scripts are gone Johannes Schindelin via GitGitGadget
2019-07-31 15:18 ` [PATCH v2 05/16] rebase: fold git-rebase--common into the -p backend Johannes Schindelin via GitGitGadget
2019-07-31 15:18 ` [PATCH v2 06/16] t3427: add a clarifying comment Johannes Schindelin via GitGitGadget
2019-07-31 15:18 ` [PATCH v2 07/16] t3427: simplify the `setup` test case significantly Johannes Schindelin via GitGitGadget
2019-07-31 15:18 ` [PATCH v2 08/16] t3427: move the `filter-branch` invocation into the `setup` case Johannes Schindelin via GitGitGadget
2019-07-31 15:18 ` [PATCH v2 09/16] t3427: condense the unnecessarily repetitive test cases into three Johannes Schindelin via GitGitGadget
2019-07-31 15:18 ` [PATCH v2 10/16] t3427: fix erroneous assumption Johannes Schindelin via GitGitGadget
2019-07-31 15:18 ` [PATCH v2 11/16] t3427: accommodate for the `rebase --merge` backend having been replaced Johannes Schindelin via GitGitGadget
2019-07-31 15:18 ` [PATCH v2 12/16] t3427: fix another incorrect assumption Johannes Schindelin via GitGitGadget
2019-07-31 15:18 ` [PATCH v2 13/16] rebase -r: support merge strategies other than `recursive` Johannes Schindelin via GitGitGadget
2019-07-31 15:18 ` [PATCH v2 14/16] t/lib-rebase: prepare for testing `git rebase --rebase-merges` Johannes Schindelin via GitGitGadget
2019-07-31 15:18 ` [PATCH v2 16/16] rebase -r: do not (re-)generate root commits with `--root` *and* `--onto` Johannes Schindelin via GitGitGadget
2019-07-31 15:18 ` [PATCH v2 15/16] t3418: test `rebase -r` with merge strategies Johannes Schindelin via GitGitGadget
2019-09-04 21:40 ` [PATCH v2 17/16] t3427: accelerate this test by using fast-export and fast-import Elijah Newren
2019-09-09 20:29 ` Johannes Schindelin
2019-09-09 21:06 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=nycvar.QRO.7.76.6.1907311306360.21907@tvgsbejvaqbjf.bet \
--to=johannes.schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=sandals@crustytoothpaste.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).