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

[Emily Shaffer <emilyshaffer@google.com>]

Firstly, sorry for the delay, I wasn't working for national holiday from
the 4th til yesterday.

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

After I read your initial comment this is close to what my rewrite
started to look like; I suppose we're only the same page after all :)

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

This writeup is great, and I think your example is very clear and
readable. I'll push a reroll with my similar-looking reformat soon.
Thanks, Johannes.

 - Emily