Re: [PATCH 1/2] rebase tests: test linear branch topology

From: Jeff King <peff@peff.net>
Date: Feb 22, 2019, 14:53 UTC
Message-ID: <20190222145344.GA5090@sigill.intra.peff.net>
In-Reply-To: <20190221214059.9195-2-avarab@gmail.com>

On Thu, Feb 21, 2019 at 10:40:58PM +0100, Ævar Arnfjörð Bjarmason wrote:

> Add tests rebasing a linear branch topology to linear rebase tests
> added in 2aad7cace2 ("add simple tests of consistency across rebase
> types", 2013-06-06).

I had trouble parsing this. Did you mean s/topology to/topology, similar to/?

Show 5 quoted lines

> These tests are duplicates of two surrounding tests that do the same
> with tags pointing to the same objects. Right now there's no change in
> behavior being introduced, but as we'll see in a subsequent change
> rebase can have different behaviors when working implicitly with
> remote tracking branches.

It took me a while to figure out what was new in these tests. Maybe:

  These tests are duplicates of two surrounding tests, but with one
  change: the existing tests refer to objects by their tag names, but
  here we'll use branches (pointing at the same objects).

But then I'm left wondering why that's important.

> While I'm at it add a --fork-point test, strictly speaking this is
> redundant to the existing '' test, as no argument to rebase implies
> --fork-point. But now it's easier to grep for tests that explicitly
> stress --fork-point.

That makes sense.

Show 6 quoted lines

> +test_expect_success 'setup branches and remote tracking' '
> +	git tag -l >tags &&
> +	for tag in $(cat tags)
> +	do
> +		git branch branch-$tag $tag || return 1
> +	done &&

I don't think we need this extra tmpfile and cat, do we? I.e.,

  for tag in $(git tag -l)

would work. We should probably avoid depending on the exact output of the porcelain "tag", though. Maybe:

  git for-each-ref \
    --format='create refs/heads/branch-%(refname:strip=2) %(objectname)' \
    refs/tags |
  git update-ref --stdin

which has the added bonus of using a constant number of processes.

-Peff