Re: [PATCH 6/7] t/t3437: update the tests

Hi Eric,

On Mon, 8 Feb 2021 at 00:13, Eric Sunshine [off-list ref] wrote:

[...]

Typically, if you find yourself enumerating a list of distinct changes
like this in a commit message, it's a good indication that it should
be split into multiple patches, each taking care of one item from the
list. A good reason for splitting it up like this is that it's
difficult for reviewers to keep the entire list in mind while
reviewing the patch, however, it's easy to keep in mind a single
stated goal while reading the changes.

Having said that, I'm not sure it's worth a re-roll or the extra work
of actually splitting it up since you've already been dragged deeper
into this than planned, and these are relatively minor issues.

(Returning to this after reading the remainder of the patch, I did
find it reasonably confusing trying to figure out which changes
related to each other and to items from the list above. It would have
been easier to reason about the changes had they been done in separate
patches. Still, though, I'm not sure it's worth the time and effort to
split them up -- but I wouldn't complain if you did.)

Agree, I will split this patch.

More below...

quoted

Signed-off-by: Charvi Mendiratta <redacted>
---

diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh

@@ -8,8 +8,10 @@ test_description='git rebase interactive fixup options
 This test checks the "fixup [-C|-c]" command of rebase interactive.
 In addition to amending the contents of the commit, "fixup -C"
 replaces the original commit message with the message of the fixup
-commit. "fixup -c" also replaces the original message, but opens the
-editor to allow the user to edit the message before committing.
+commit and similar to "fixup" command that works with "fixup!", "fixup -C"
+works with "amend!" upon --autosquash. "fixup -c" also replaces the original
+message, but opens the editor to allow the user to edit the message before
+committing.
 '

I had trouble digesting this run-on sentence due, I think, to the
mixing of thoughts. It might be easier to understand if you first talk
only about the options to `fixup` (-c/-C), and then, as a separate
sentence, talk about how `amend!` is transformed into `fixup -C` (like
`fixup!` is transformed into `fixup`). However, as this is just minor
descriptive text in a test file, not user-facing documentation, I'm
not sure it matters enough to warrant a re-roll.

Okay, will change it.

quoted

 test_commit_message () {
+       git show --no-patch --pretty=format:%B "$1" >actual &&
+    case "$2" in
+    -m) echo "$3" >expect &&
+           test_cmp expect actual ;;
+    *) test_cmp "$2" actual ;;
+    esac
 }

The funky indentation here is due to a mix of tabs and spaces. It
should use tabs exclusively.

Oh, thanks I will correct it.