git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Charvi Mendiratta <charvi077@gmail.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Christian Couder <christian.couder@gmail.com>,
	Git List <git@vger.kernel.org>,
	Phillip Wood <phillip.wood123@gmail.com>,
	Christian Couder <chriscool@tuxfamily.org>,
	Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH v4 7/9] t3437: test script for fixup [-C|-c] options in interactive rebase
Date: Thu, 4 Feb 2021 05:31:58 +0530	[thread overview]
Message-ID: <CAPSFM5d=9yV6TrmuZoJX7SMHpKNUdO=n7f-DJN1q4i__hUChSQ@mail.gmail.com> (raw)
In-Reply-To: <CAPig+cSBVG0AdyqXH2mZp6Ohrcb8_ec1Mm_vGbQM4zWT_7yYxQ@mail.gmail.com>

On Wed, 3 Feb 2021 at 11:14, Eric Sunshine <sunshine@sunshineco.com> wrote:

> > > What is "merge_" doing here? It doesn't seem to be used by this patch.
> >
> > Yeah, it's not used, but it might be a good thing to add this for
> > consistency while at it.
>
> It confuses readers (as it did to me), causing them to waste
> brain-cycles trying to figure out why it's present. Thus, it would be
> better to add it when it's actually needed. The waste of brain-cycles
> and time is especially important on a project like Git for which
> reviewers and reviewer time are limited resources.
>

Okay, I will remove "merge_" from this patch series and maybe later will make
separate patch for it and also adding its tests and updating
t3430-rebase-merges.sh

> > > > +# Copyright (c) 2018 Phillip Wood
> > >
> > > Did Phillip write this script? Is this patch based upon an old patch from him?
> >
> > Yeah, it might be a good idea to add a "Based-on-patch-by: Phillip ..."
>
> Agreed.
>
> > > The implementation of test_commit_message() is a bit hard to follow.
> > > It might be simpler to write it more concisely and directly like this:
> > >
> > >     git show --no-patch --pretty=format:%B "$1" >actual &&
> > >     case "$2" in
> > >     -m) echo "$3" >expect && test_cmp expect actual ;;
> >
> > I think we try to avoid many commands on the same line.
>
> For something this minor, it's not likely to matter but, of course, it
> could be split over two lines:
>
>     -m) echo "$3" >expect &&
>         test_cmp expect actual ;;
>
> > >     *) test_cmp "$2" actual ;;
> > >     esac
> >
> > In general I am not sure that using $1, $2, $3 directly makes things
> > easier to understand, but yeah, with the function documentation that
> > you suggest, it might be better to write the function using them
> > directly.
>
> The direct $1, $2, etc. was just an example. It's certainly possible
> to give them names even in the rewritten code I presented. One good
> reason, however, for just using $1, $2, etc. is that $2 is not well
> defined; sometimes it's a switch ("-m") and sometimes its a pathname,
> so it's hard to invent a suitable variable name for it. Also, this
> function becomes so simple (in the rewritten version) that explicit
> variable names don't add a lot of value (the cognitive load is quite
> low because the function is so short).
>

Agree, and will update it.

> > > Style nit: In Git test scripts, the here-doc body and EOF are indented
> > > the same amount as the command which opened the here-doc:
> >
> > I don't think we are very consistent with this and I didn't find
> > anything about this in CodingGuidelines.
> >
> > In t0008 and t0021 for example, the indentation is more like:
> >
> >      cat >message <<-EOF &&
> >           amend! B
> >           ...
> >           body
> >      EOF
> >
> > and I like this style, as it seems clearer than the other styles.
>
> I performed a quick survey of the heredoc styles in the tests. Here
> are the results[1] of my analysis on the 'seen' branch:
>
> total-heredocs=4128
>
> same-indent=3053 (<<EOF & body & EOF share indent)
>
>     cat >expect <<-\EOF
>     body
>     EOF
>
> body-eof-indented=24 (body & EOF indented)
>
>     cat >expect <<-\EOF
>         body
>         EOF
>
> body-indented=735 (body indented; EOF not)
>
>     cat >expect <<-\EOF
>         body
>     EOF
>
> left-margin=316 (<<EOF indented; body & EOF not)
>
>         cat >expect <<\EOF
>     body
>     EOF
>
> So, the indentation recommended in my review -- with 3053 instances
> out of 4128 heredocs -- is by far the most prevalent in the project.
>
> [1]: Note that there is a miniscule amount of inaccuracy in the
> numbers because there are a few cases in which heredocs contain other
> heredocs, and some scripts build heredocs piecemeal when constructing
> other scripts, and I didn't bother making my analysis script handle
> those few cases. The inaccuracy is tiny, thus not meaningful to the
> overall picture.
>

Okay, will update the indentation.

[...]
> > >     fixup-*)
> > >         action=$(echo "$line" | sed 's/-/ -/');;
> >
> > I agree that "fixup" arguments are not arbitrary at all, but I think
> > it makes things simpler to just use one way to encode spaces instead
> > of many different ways.
>
> Is that the intention here, though? Is the idea that some day `fixup`
> will accept arbitrary arguments thus needs to encode spaces? If not,
> then mirroring the treatment given to `exec` confuses readers into
> thinking that it will/should accept arbitrary arguments. I brought
> this up in my review specifically because it was confusing to a person
> (me) new to this topic and reading the patches for the first time. The
> more specific and exact the code can be, the less likely it will
> confuse readers in the future.
>

I also agree that fixup will not accept arbitrary arguments, So I think to
go with the method using fixup-*) (as suggested above).

[...]
> > Yeah, perhaps the global "expected-message" could be renamed for
> > example "global-expected-message", and tests which need a specific one
> > could prepare and use a custom "expected-message" (maybe named
> > "custom-expected-message") without ever changing
> > "global-expected-message".
>
> That would be fine, though I wondered while reviewing the patch if a
> global "expect-message" file was even needed since it didn't seem like
> very many tests used it (but I didn't spend a lot of time counting the
> exact number of tests due to the high cognitive load tracing how that
> file might mutate as it passed through each test).
>
> Another really good reason for avoiding having later tests depend upon
> mutations from earlier tests, if possible, is that it makes it easier
> to run tests selectively with --run or GIT_SKIP_TESTS.

Agree, also for this patch series I think to remove all tests for
amend!, change the
test setup and will take care this time to remove the test dependency
(in case of expected-message).

Thanks and Regards,
Charvi

  reply	other threads:[~2021-02-04  0:05 UTC|newest]

Thread overview: 110+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-08  9:23 [RFC PATCH 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
2021-01-08  9:23 ` [RFC PATCH 1/9] rebase -i: only write fixup-message when it's needed Charvi Mendiratta
2021-01-13 18:43   ` Taylor Blau
2021-01-14  8:12     ` Charvi Mendiratta
2021-01-14 10:46       ` Phillip Wood
2021-01-15  8:38         ` Charvi Mendiratta
2021-01-15 17:22           ` Junio C Hamano
2021-01-16  4:49             ` Charvi Mendiratta
2021-01-08  9:23 ` [RFC PATCH 2/9] sequencer: factor out code to append squash message Charvi Mendiratta
2021-01-08  9:23 ` [RFC PATCH 3/9] rebase -i: comment out squash!/fixup! subjects from " Charvi Mendiratta
2021-01-13 19:01   ` Taylor Blau
2021-01-14  8:27     ` Charvi Mendiratta
2021-01-14 10:29       ` Phillip Wood
2021-01-15  8:35         ` Charvi Mendiratta
2021-01-15  8:44           ` Christian Couder
2021-01-15 11:12             ` Charvi Mendiratta
2021-01-17  3:39         ` Charvi Mendiratta
2021-01-18 18:29           ` Phillip Wood
2021-01-19  4:08             ` Charvi Mendiratta
2021-01-08  9:23 ` [RFC PATCH 4/9] sequencer: pass todo_item to do_pick_commit() Charvi Mendiratta
2021-01-08  9:23 ` [RFC PATCH 5/9] sequencer: use const variable for commit message comments Charvi Mendiratta
2021-01-13 19:14   ` Taylor Blau
2021-01-13 20:37     ` Junio C Hamano
2021-01-14  7:40       ` Christian Couder
2021-01-14  8:57     ` Charvi Mendiratta
2021-01-08  9:23 ` [RFC PATCH 6/9] rebase -i: add fixup [-C | -c] command Charvi Mendiratta
2021-01-14  9:23   ` Christian Couder
2021-01-14  9:45     ` Charvi Mendiratta
2021-01-08  9:23 ` [RFC PATCH 7/9] t3437: test script for fixup [-C|-c] options in interactive rebase Charvi Mendiratta
2021-01-08  9:23 ` [RFC PATCH 8/9] rebase -i: teach --autosquash to work with amend! Charvi Mendiratta
2021-01-08  9:23 ` [RFC PATCH 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options Charvi Mendiratta
2021-01-19  7:40 ` [PATCH v2 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
2021-01-24 17:03   ` [PATCH v3 " Charvi Mendiratta
2021-01-24 17:03     ` [PATCH v3 1/9] rebase -i: only write fixup-message when it's needed Charvi Mendiratta
2021-01-24 17:04     ` [PATCH v3 2/9] sequencer: factor out code to append squash message Charvi Mendiratta
2021-01-24 17:04     ` [PATCH v3 3/9] rebase -i: comment out squash!/fixup! subjects from " Charvi Mendiratta
2021-01-24 17:04     ` [PATCH v3 4/9] sequencer: pass todo_item to do_pick_commit() Charvi Mendiratta
2021-01-24 17:04     ` [PATCH v3 5/9] sequencer: use const variable for commit message comments Charvi Mendiratta
2021-01-24 17:04     ` [PATCH v3 6/9] rebase -i: add fixup [-C | -c] command Charvi Mendiratta
2021-01-24 17:04     ` [PATCH v3 7/9] t3437: test script for fixup [-C|-c] options in interactive rebase Charvi Mendiratta
2021-01-24 17:04     ` [PATCH v3 8/9] rebase -i: teach --autosquash to work with amend! Charvi Mendiratta
2021-01-24 17:04     ` [PATCH v3 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options Charvi Mendiratta
2021-01-29 18:20     ` [PATCH v4 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
2021-01-29 18:20       ` [PATCH v4 1/9] rebase -i: only write fixup-message when it's needed Charvi Mendiratta
2021-01-29 18:20       ` [PATCH v4 2/9] sequencer: factor out code to append squash message Charvi Mendiratta
2021-01-29 18:20       ` [PATCH v4 3/9] rebase -i: comment out squash!/fixup! subjects from " Charvi Mendiratta
2021-01-29 18:20       ` [PATCH v4 4/9] sequencer: pass todo_item to do_pick_commit() Charvi Mendiratta
2021-01-29 18:20       ` [PATCH v4 5/9] sequencer: use const variable for commit message comments Charvi Mendiratta
2021-01-29 18:20       ` [PATCH v4 6/9] rebase -i: add fixup [-C | -c] command Charvi Mendiratta
2021-02-02  0:47         ` Eric Sunshine
2021-02-02 15:29           ` Charvi Mendiratta
2021-02-03  5:05             ` Eric Sunshine
2021-02-04  0:00               ` Charvi Mendiratta
2021-02-04  0:14                 ` Eric Sunshine
2021-01-29 18:20       ` [PATCH v4 7/9] t3437: test script for fixup [-C|-c] options in interactive rebase Charvi Mendiratta
2021-02-02  2:01         ` Eric Sunshine
2021-02-02 10:02           ` Christian Couder
2021-02-02 15:31             ` Charvi Mendiratta
2021-02-03  5:44             ` Eric Sunshine
2021-02-04  0:01               ` Charvi Mendiratta [this message]
2021-02-04 10:46           ` Phillip Wood
2021-02-04 16:14             ` Eric Sunshine
2021-02-04 19:12           ` Charvi Mendiratta
2021-01-29 18:20       ` [PATCH v4 8/9] rebase -i: teach --autosquash to work with amend! Charvi Mendiratta
2021-02-02  3:20         ` Eric Sunshine
2021-02-02 15:29           ` Charvi Mendiratta
2021-01-29 18:20       ` [PATCH v4 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options Charvi Mendiratta
2021-02-02  3:23         ` Eric Sunshine
2021-02-02 14:12           ` Marc Branchaud
2021-02-02 15:30           ` Charvi Mendiratta
2021-02-04 19:04       ` [PATCH v5 0/8][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
2021-02-04 19:05         ` [PATCH v5 1/8] rebase -i: only write fixup-message when it's needed Charvi Mendiratta
2021-02-04 19:05         ` [PATCH v5 2/8] sequencer: factor out code to append squash message Charvi Mendiratta
2021-02-04 19:05         ` [PATCH v5 3/8] rebase -i: comment out squash!/fixup! subjects from " Charvi Mendiratta
2021-02-04 19:05         ` [PATCH v5 4/8] sequencer: pass todo_item to do_pick_commit() Charvi Mendiratta
2021-02-04 19:05         ` [PATCH v5 5/8] sequencer: use const variable for commit message comments Charvi Mendiratta
2021-02-04 19:05         ` [PATCH v5 6/8] rebase -i: add fixup [-C | -c] command Charvi Mendiratta
2021-02-04 19:05         ` [PATCH v5 7/8] t3437: test script for fixup [-C|-c] options in interactive rebase Charvi Mendiratta
2021-02-04 19:05         ` [PATCH v5 8/8] doc/git-rebase: add documentation for fixup [-C|-c] options Charvi Mendiratta
2021-02-05  7:30         ` [PATCH v5 0/8][Outreachy] rebase -i: add options to fixup command Eric Sunshine
2021-02-05  9:42           ` Charvi Mendiratta
2021-02-05 18:25             ` Christian Couder
2021-02-05 18:56               ` Eric Sunshine
2021-02-06  5:36                 ` Charvi Mendiratta
2021-02-05 19:13               ` Junio C Hamano
2021-02-06  5:37                 ` Charvi Mendiratta
2021-01-19  7:40 ` [PATCH v2 1/9] rebase -i: only write fixup-message when it's needed Charvi Mendiratta
2021-01-19  7:40 ` [PATCH v2 2/9] sequencer: factor out code to append squash message Charvi Mendiratta
2021-01-19  7:40 ` [PATCH v2 3/9] rebase -i: comment out squash!/fixup! subjects from " Charvi Mendiratta
2021-01-21  1:38   ` Junio C Hamano
2021-01-21 14:02     ` Charvi Mendiratta
2021-01-21 15:21       ` Christian Couder
2021-01-21 16:58         ` Phillip Wood
2021-01-21 20:56         ` Junio C Hamano
2021-01-22 19:41           ` Charvi Mendiratta
2021-01-22 19:41         ` Charvi Mendiratta
2021-01-19  7:40 ` [PATCH v2 4/9] sequencer: pass todo_item to do_pick_commit() Charvi Mendiratta
2021-01-19  7:41 ` [PATCH v2 5/9] sequencer: use const variable for commit message comments Charvi Mendiratta
2021-01-19  7:41 ` [PATCH v2 6/9] rebase -i: add fixup [-C | -c] command Charvi Mendiratta
2021-01-19  7:41 ` [PATCH v2 7/9] t3437: test script for fixup [-C|-c] options in interactive rebase Charvi Mendiratta
2021-01-19  7:41 ` [PATCH v2 8/9] rebase -i: teach --autosquash to work with amend! Charvi Mendiratta
2021-01-19  7:41 ` [PATCH v2 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options Charvi Mendiratta
2021-01-19 14:37   ` Marc Branchaud
2021-01-19 17:13     ` Charvi Mendiratta
2021-01-19 22:05       ` Marc Branchaud
2021-01-20  7:10         ` Charvi Mendiratta
2021-01-20 11:04       ` Phillip Wood
2021-01-20 12:31         ` Charvi Mendiratta
2021-01-20 14:29           ` Phillip Wood
2021-01-20 16:09             ` 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='CAPSFM5d=9yV6TrmuZoJX7SMHpKNUdO=n7f-DJN1q4i__hUChSQ@mail.gmail.com' \
    --to=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 \
    --cc=sunshine@sunshineco.com \
    /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.
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).