Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Emily,

On Wed, 3 Jul 2019, Emily Shaffer wrote:

> On Wed, Jul 03, 2019 at 09:41:46PM +0200, Johannes Schindelin wrote:
>
> > On Wed, 3 Jul 2019, Emily Shaffer wrote:
> >
> > > > > +	up="$HTTPD_URL"/smart/atomic-branches.git &&
> > > > > +	test_commit atomic1 &&
> > > > > +	test_commit atomic2 &&
> > > > > +	git push "$up" master &&
> > > >
> > > > It would be more succinct to do a `git clone --bare . "$d"` here,
> > > > instead of a `git init --bare` and a `git push` no?
> > >
> > > I'm not sure I would say "more succinct." This leaves the test with the
> > > same number of lines,
> >
> > No, it does not, as `git clone --bare . "$d"` does _both_ the initializing
> > and the object transfer.
> >
> > It only saves one line, of course, but do keep in mind that anybody
> > running into any kind of regression with your test case needs to
> > understand what it does. And from experience I can tell you that reading
> > any test case longer than 5 lines is quite annoying when you actually
> > only care about fixing the regression, and not so much about the wonderful
> > story the test case tells.
>
> I suppose I'm confused, then, as I understood you were asking me to
> combine my three test cases into one, which naturally makes the test
> itself more complex and longer to read. Which do you prefer?

If I were tasked with developing this further, I would try to move as much
of the setup into the initial test case (if there is already a `setup`
test case; otherwise I would create one). In fact, I would try to reuse as
much of the existing setup test case as possible, and only add commands if
really necessary. Then, I would try to combine the three test cases in the
patch into a single one, structuring it by white space and using comments
to clarify what is happening.

In my mind, even just adding an empty line before the comments like "Make
master incompatible with up/master" would make it much easier for me to
read, were I to analyze a test breakage.

I have to admit that I have a hard time understanding what the intention
of those three test cases is because I get confused: where does the set-up
end, where is the code that is expected to fail, where are the
expectations tested?

Also, I get confused by how similar the test cases look, and have a hard
time discerning what the differences are (i.e. what are the interesting
bits, where the entropy comes from).

I could imagine that I would have had an easier time reading something
like this:

test_expect_success 'push --atomic' '
	: set up two branches, one which will require a force push &&
	git checkout -b fast-forwarding master &&
	test_commit push-atomic &&
	git checkout -b non-fast-forwarding &&

	: now, initialize the bare repository to push to &&
	d=$HTTPD_DOCUMENT_ROOT_PATH/atomic.git &&
	git clone --bare . $d &&

	: modify the two branches and create a third one &&
	git reset --hard HEAD^ &&
	git checkout fast-forwarding &&
	test_commit no-need-to-force &&
	git branch new-branch &&

	: now the atomic push must fail &&
	test_must_fail git push --atomic "$HTTPD_URL"/smart/atomic.git \
		fast-forwarding non-fast-forwarding new-branch 2>err &&

	: verify that new-branch was not pushed &&
	test_must_fail git -C $d rev-parse --verify refs/heads/new-branch &&

	: fast-forwarding should be mentioned even if it would have been OK &&
	grep fast-forwarding err
'

Of course, everybody has their preferences, and their personal style. I do
not want you to imitate my style just to "pacify" me. That's not the point
of this example.

The point is that I need some structure to walk along, especially when I
am a bit annoyed at a test case that shows that I introduced a regression
and all I want is to understand as quickly as possible what I did wrong
again so that I can fix it and move on.

The point is that I want a regression test case to _not_ distract me from
the essential part, ideally I should be able to ignore all the set-up code
and deduce from the command-line of the failing command what is going on.

For example, if the test case fails in the line `grep fast-forwarding err`
above, that command-line alone does not tell me anything, it just
frustrates me. If there is a comment above that line (which is ideally
part of the `-x` trace, that's why I used `:` instead of `#`) that states
the intention in what I consider to be clear, simple English, it is a lot
easier to figure out what the heck is going wrong.

Of course, it would be even better if we had a function, say,
`must_contain` that runs that `grep` and shows the file contents and the
regular expression if that `grep` failed. That's of course outside of the
concern of your patch. But this idea illustrates what I want in a
regression test case: I want it to _help_ me figure out things when it
breaks.

Ciao,
Dscho

P.S.: Please note that the many test cases _I_ introduced into Git's test
suite mostly do not conform to what I wrote above. I learned _quite_ a
lot of how I want regression test cases to look like in the past six
months, not from writing them, but from analyzing literally hundreds of
Azure Pipelines builds. And your question forced me to think about my
learnings to formulate the above (hopefully clear) explanation of what I
want in a regression test, so: thank you.