git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: Charvi Mendiratta <charvi077@gmail.com>
Cc: Git List <git@vger.kernel.org>,
	Christian Couder <christian.couder@gmail.com>,
	Phillip Wood <phillip.wood123@gmail.com>,
	Phillip Wood <phillip.wood@dunelm.org.uk>,
	Christian Couder <chriscool@tuxfamily.org>
Subject: Re: [PATCH 6/7] t/t3437: update the tests
Date: Sun, 7 Feb 2021 13:43:14 -0500	[thread overview]
Message-ID: <CAPig+cTDT5Hct7dUTY93nO+P5-US=ZokuGhOQeELPpZwQGzf=w@mail.gmail.com> (raw)
In-Reply-To: <20210207181439.1178-7-charvi077@gmail.com>

On Sun, Feb 7, 2021 at 1:19 PM Charvi Mendiratta <charvi077@gmail.com> wrote:
> Let's do the changes listed below to make tests more easier to follow :
>
> -Remove the dependency of 'expected-message' file from earlier tests to
> make it easier to run tests selectively with '--run' or 'GIT_SKIP_TESTS'.
>
> -Add author timestamp to check that the author date of fixed up commit
> is unchanged.
>
> -Simplify the test_commit_message() and add comments before the
> function.
>
> -Clarify the working of 'fixup -c' with "amend!" in the test-description.
>
> -Remove unnecessary curly braces and use the named commits in the
> tests so that they will still refer to the same commit if the setup
> gets changed in the future whereas 'branch~2' will change which commit
> it points to.

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.)

More below...

> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> ---
> 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.

> @@ -18,36 +20,34 @@ editor to allow the user to edit the message before committing.
> +# test_commit_message <rev> -m <msg>
> +# test_commit_message <rev> <path>
> +# Verify that the commit message of <rev> matches
> +# <msg> or the content of <path>.

Good.

>  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.

  reply	other threads:[~2021-02-07 18:46 UTC|newest]

Thread overview: 58+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-07 18:14 [PATCH 0/7][Outreachy] Improve the 'fixup [-C | -c]' in interactive rebase Charvi Mendiratta
2021-02-07 18:14 ` [PATCH 1/7] sequencer: fixup the datatype of the 'flag' argument Charvi Mendiratta
2021-02-07 18:14 ` [PATCH 2/7] sequencer: rename a few functions Charvi Mendiratta
2021-02-07 18:14 ` [PATCH 3/7] rebase -i: clarify and fix 'fixup -c' rebase-todo help Charvi Mendiratta
2021-02-07 18:49   ` Eric Sunshine
2021-02-08  4:30     ` Charvi Mendiratta
2021-02-07 18:14 ` [PATCH 4/7] t/lib-rebase: change the implementation of commands with options Charvi Mendiratta
2021-02-07 18:14 ` [PATCH 5/7] t3437: fix indendation of the here-doc Charvi Mendiratta
2021-02-07 18:54   ` Eric Sunshine
2021-02-08  4:30     ` Charvi Mendiratta
2021-02-08 10:37     ` Phillip Wood
2021-02-07 18:14 ` [PATCH 6/7] t/t3437: update the tests Charvi Mendiratta
2021-02-07 18:43   ` Eric Sunshine [this message]
2021-02-08  4:30     ` Charvi Mendiratta
2021-02-07 18:14 ` [PATCH 7/7] doc/rebase -i: fix typo in the documentation of 'fixup' command Charvi Mendiratta
2021-02-07 18:57 ` [PATCH 0/7][Outreachy] Improve the 'fixup [-C | -c]' in interactive rebase Eric Sunshine
2021-02-08  4:31   ` Charvi Mendiratta
2021-02-08 19:25 ` [PATCH v2 00/11][Outreachy] " Charvi Mendiratta
2021-02-08 21:57   ` Junio C Hamano
2021-02-09  7:19     ` Charvi Mendiratta
2021-02-08 19:25 ` [PATCH v2 01/11] sequencer: fixup the datatype of the 'flag' argument Charvi Mendiratta
2021-02-08 19:25 ` [PATCH v2 02/11] sequencer: rename a few functions Charvi Mendiratta
2021-02-08 19:25 ` [PATCH v2 03/11] rebase -i: clarify and fix 'fixup -c' rebase-todo help Charvi Mendiratta
2021-02-08 21:24   ` Junio C Hamano
2021-02-09  7:13     ` Charvi Mendiratta
2021-02-09  8:33       ` Eric Sunshine
2021-02-09 19:08         ` Junio C Hamano
2021-02-09 19:13           ` Eric Sunshine
2021-02-10  5:43             ` Charvi Mendiratta
2021-02-08 19:25 ` [PATCH v2 04/11] t/lib-rebase: change the implementation of commands with options Charvi Mendiratta
2021-02-08 21:36   ` Junio C Hamano
2021-02-08 23:19     ` Christian Couder
2021-02-09  7:19       ` Charvi Mendiratta
2021-02-08 19:25 ` [PATCH v2 05/11] t/t3437: fix indentation of the here-doc Charvi Mendiratta
2021-02-08 19:25 ` [PATCH v2 06/11] t/t3437: remove the dependency of 'expected-message' file from tests Charvi Mendiratta
2021-02-08 19:25 ` [PATCH v2 07/11] t/t3437: check author date of the fixed up commit Charvi Mendiratta
2021-02-08 19:25 ` [PATCH v2 08/11] t/t3437: simplify and document the test helpers Charvi Mendiratta
2021-02-08 19:25 ` [PATCH v2 09/11] t/t3437: cleanup the 'setup' test and use named commits in the tests Charvi Mendiratta
2021-02-08 21:41   ` Junio C Hamano
2021-02-09  7:13     ` Charvi Mendiratta
2021-02-08 19:25 ` [PATCH v2 10/11] t/t3437: fixup the test 'multiple fixup -c opens editor once' Charvi Mendiratta
2021-02-08 19:25 ` [PATCH v2 11/11] doc/rebase -i: fix typo in the documentation of 'fixup' command Charvi Mendiratta
2021-02-10 11:36 ` [PATCH v3 00/11][Outreachy] Improve the 'fixup [-C | -c]' in interactive rebase Charvi Mendiratta
2021-02-11 17:19   ` Junio C Hamano
2021-02-11 22:26     ` Charvi Mendiratta
2021-02-11 22:44       ` Junio C Hamano
2021-02-12  0:19         ` Charvi Mendiratta
2021-02-10 11:36 ` [PATCH v3 01/11] sequencer: fixup the datatype of the 'flag' argument Charvi Mendiratta
2021-02-10 11:36 ` [PATCH v3 02/11] sequencer: rename a few functions Charvi Mendiratta
2021-02-10 11:36 ` [PATCH v3 03/11] rebase -i: clarify and fix 'fixup -c' rebase-todo help Charvi Mendiratta
2021-02-10 11:36 ` [PATCH v3 04/11] t/lib-rebase: update the documentation of FAKE_LINES Charvi Mendiratta
2021-02-10 11:36 ` [PATCH v3 05/11] t/t3437: fixup here-docs in the 'setup' test Charvi Mendiratta
2021-02-10 11:36 ` [PATCH v3 06/11] t/t3437: remove the dependency of 'expected-message' file from tests Charvi Mendiratta
2021-02-10 11:36 ` [PATCH v3 07/11] t/t3437: check the author date of fixed up commit Charvi Mendiratta
2021-02-10 11:36 ` [PATCH v3 08/11] t/t3437: simplify and document the test helpers Charvi Mendiratta
2021-02-10 11:36 ` [PATCH v3 09/11] t/t3437: use named commits in the tests Charvi Mendiratta
2021-02-10 11:36 ` [PATCH v3 10/11] t/t3437: fixup the test 'multiple fixup -c opens editor once' Charvi Mendiratta
2021-02-10 11:36 ` [PATCH v3 11/11] doc/rebase -i: fix typo in the documentation of 'fixup' command Charvi Mendiratta

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

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAPig+cTDT5Hct7dUTY93nO+P5-US=ZokuGhOQeELPpZwQGzf=w@mail.gmail.com' \
    --to=sunshine@sunshineco.com \
    --cc=charvi077@gmail.com \
    --cc=chriscool@tuxfamily.org \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=phillip.wood123@gmail.com \
    --cc=phillip.wood@dunelm.org.uk \
    --subject='Re: [PATCH 6/7] t/t3437: update the tests' \
    /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

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 NNTP newsgroup(s).