From: Eric Sunshine <sunshine@sunshineco.com>
To: Chris Torek <chris.torek@gmail.com>
Cc: Chris Torek via GitGitGadget <gitgitgadget@gmail.com>,
Git List <git@vger.kernel.org>
Subject: Re: [PATCH] git-mv: improve error message for conflicted file
Date: Sat, 18 Jul 2020 02:55:37 -0400 [thread overview]
Message-ID: <CAPig+cQpUu2UO-+jWn1nTaDykWnxwuEitzVB7PnW2SS_b7V8Hg@mail.gmail.com> (raw)
In-Reply-To: <CAPx1GvduDZw5pmfZHACDGZsMR5YYDowLw6+az+oL6oWLvDyCFA@mail.gmail.com>
On Fri, Jul 17, 2020 at 9:35 PM Chris Torek <chris.torek@gmail.com> wrote:
> On Fri, Jul 17, 2020 at 4:47 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > ... use literal TABs and let the here-doc provide the newlines.
>
> I personally hate depending on literal tabs, as they're really
> hard to see. If q_to_tab() is OK I'll use that.
q_to_tab() is a good choice.
> > I realize that this test script is already filled with this sort of
> > thing where actions are performed outside of tests, however, these
> > days we frown upon that, and there really isn't a good reason to avoid
> > taking care of this clean up via the modern idiom of using
> > test_when_finished(), which you would call immediately after creating
> > the file in the test. So:
>
> Indeed, that's where I copied it from.
>
> Should I clean up other instances of separated-out `rm -f`s
> in this file in a separate commit?
In general, as a reviewer, I don't mind seeing a patch or two cleaning
up style and other violations, however, the magnitude of the fixes
this script needs is quite significant and would end up requiring a
fair number of patches. As such, I'm not particularly eager to see the
improvement made by this patch -- which is nicely standalone --
weighed down by a lengthy series of patches which aren't really
related to it.
If cleaning up the t7001 test script is something you might be
interested in doing, then making it a separate patch series would be
more palatable. A scan of the script reveals the following problems,
though there may be others:
* old style:
test_expect_success \
'title' \
'body line 1 &&
body line 2'
should become:
test_expect_success 'title' '
body line 1 &&
body line 2
'
* test bodies should be indented with TAB, not spaces
* some tests use a deprecated style in which there are unnecessary
blank lines after the opening quote of the test body and before the
closing quote; these blanks lines should be removed
* style for `cd` in subshell is:
(
cd foo &&
...
) &&
not:
(cd foo &&
...
) &&
* there should be no whitespace after redirect operators, so:
foo > actual &&
should become:
foo >actual &&
* tests 'cd' around and expect other tests to know the current
directory and 'cd' relative to that; instead, any test which uses
'cd' should do so in a subshell to ensure the current directory is
restored by the time the test ends:
test_expect_success 'title' '
something &&
(
cd somewhere &&
something-else
)
'
Alternately, it may be possible to take advantage of `-C` if
`something-else` is a `git` command:
test_expect_success 'title' '
something &&
git -C somewhere foo
'
* use `>` rather than `touch` to create an empty file when the
timestamp isn't relevant to the test
* cleanup code outside of tests should be moved into the test and
scheduled for execution via test_when_finished()
* there are several standalone "clean up" tests which invoke `git
reset --hard` which should be folded into the tests for which they
are cleaning up
* multiple commands on one line:
mkdir foo && >foo/bar && git add foo/bar &&
should be split across multiple lines:
mkdir foo &&
>foo/bar &&
git add foo/bar &&
* at least one test incorrectly uses single quotes within the body of
the test which itself is contained within single quotes; when
quoting is needed inside a test body, it should be using double
quotes instead; however, in this case, the quotes aren't even
needed, so:
git commit -m 'initial' &&
can just become:
git commit -m initial &&
* take advantage of here-docs, so:
{ echo other/a.txt; echo other/b.txt; } >expect &&
can be expressed more cleanly as:
cat >expect <<-\EOF &&
other/a.txt
other/b.txt
EOF
* use `test` rather than `[`
* optional: rename the setup test 'prepare reference tree' to 'setup'
* optional modernization: use test_path_exists() and cousins instead
of `test -f`, etc.
* optional: avoid `git` command upstream of a pipe since the pipe will
swallow its exit code, thus a crash won't necessarily be noticed
* optional: it's unusual for tests to blast the test's ".git"
directory and recreate it with `git init`, however, a number of
tests in this script do so; for tests which really require a new
repository, the more common approach is to use test_create_repo() to
create a new repository into which the test can `cd` (in a subshell)
without disturbing the repository used by the other tests in the
script
A few of the above fixes can probably be combined into a single patch,
in particular, the style fixes in the first four bullet points. Each
remaining bullet point, however, probably deserves its own patch
(including the one about removing whitespace after a redirect
operator).
next prev parent reply other threads:[~2020-07-18 6:55 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-17 23:24 [PATCH] git-mv: improve error message for conflicted file Chris Torek via GitGitGadget
2020-07-17 23:47 ` Eric Sunshine
2020-07-18 1:35 ` Chris Torek
2020-07-18 6:55 ` Eric Sunshine [this message]
2020-07-18 0:07 ` Junio C Hamano
2020-07-18 2:00 ` Elijah Newren
2020-07-18 17:46 ` Junio C Hamano
2020-07-19 1:48 ` Elijah Newren
2020-07-19 6:16 ` Junio C Hamano
2020-07-20 6:17 ` [PATCH v2] " Chris Torek via GitGitGadget
2020-07-20 18:28 ` 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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAPig+cQpUu2UO-+jWn1nTaDykWnxwuEitzVB7PnW2SS_b7V8Hg@mail.gmail.com \
--to=sunshine@sunshineco.com \
--cc=chris.torek@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.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).