Git Mailing List Archive on
 help / color / Atom feed
From: Eric Sunshine <>
To: Chris Torek <>
Cc: Chris Torek via GitGitGadget <>,
	Git List <>
Subject: Re: [PATCH] git-mv: improve error message for conflicted file
Date: Sat, 18 Jul 2020 02:55:37 -0400
Message-ID: <> (raw)
In-Reply-To: <>

On Fri, Jul 17, 2020 at 9:35 PM Chris Torek <> wrote:
> On Fri, Jul 17, 2020 at 4:47 PM Eric Sunshine <> 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 &&
    ) &&


    (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 &&

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

* 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

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

  reply index

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-17 23:24 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:

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

  git send-email \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Mailing List Archive on

Archives are clonable:
	git clone --mirror git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ \
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:

AGPL code for this site: git clone