Re: [PATCH 2/5] name-rev: extend --refs to accept multiple patterns

On Thu, Jan 12, 2017 at 1:56 AM, Johannes Schindelin
[off-list ref] wrote:

Hi Jake,

On Wed, 11 Jan 2017, Jacob Keller wrote:

quoted

diff --git a/t/t6007-rev-list-cherry-pick-file.sh b/t/t6007-rev-list-cherry-pick-file.sh
index 1408b608eb03..d072ec43b016 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh

@@ -99,6 +99,36 @@ test_expect_success '--cherry-pick bar does not come up empty (II)' '
      test_cmp actual.named expect
 '

+test_expect_success 'name-rev multiple --refs combine inclusive' '
+     git rev-list --left-right --cherry-pick F...E -- bar > actual &&

Our current coding style seems to skip the space between `>` and `actual`
(this applies to all redirections added in this patch).

Right that's easy to fix.

quoted

+     git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" \
+             < actual > actual.named &&
+     test_cmp actual.named expect
+'
+
+cat >expect <<EOF
+<tags/F
+$(git rev-list --left-right --right-only --cherry-pick F...E -- bar)
+EOF

In the current revision of t6007, we seem to list the expected output
explicitly, i.e. *not* generating it dynamically.

If you *do* insist to generate the `expect` file dynamically, a better way
would be to include that generation in the `test_expect_success` code so
that errors in the call can be caught, too:

The main reason I don't like static expecations is that often other
tests inserted before or after my test now have to know what to put
here. Specifically, the problem is that we expect the output not to be
named, but actually to be sha1 hex output. This seems really brittle
for a test.

test_expect_success 'name-rev --refs excludes non-matched patterns' '
        echo "<tags/F" >expect &&
        git rev-list --left-right --right-only --cherry-pick F...E -- \
                bar >>expect &&
        [...]

However, if I was asked for my preference, I would suggest to specify the
`expect` contents explicitly, to document the expectation as of time of
writing. The reason: I debugged my share of test breakages and these
dynamically-generated `expect` files are the worst. When things break, you
have to dig *real* deep to figure out what is going wrong, as sometimes
the *generation of the `expect` file* regresses.

Do you have a better suggestion for how to check the expect vs actual
output ignoring the raw hex data that would be there otherwise? What I
want to avoid is a brittle test that depends precisely on actions of
prior tests in generating commits and tags. Specifically in this case,
we're going to end up with the sha1 hex ID of the commit in the
expected value.. hard-coding that feels wrong.

Thanks,
Jake

Ciao,
Dscho