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

From: Emily Shaffer <emilyshaffer@google.com>
Date: Jul 3, 2019, 20:57 UTC
Message-ID: <20190703205755.GF121233@google.com>
In-Reply-To: <nycvar.QRO.7.76.6.1907032137210.44@tvgsbejvaqbjf.bet>

On Wed, Jul 03, 2019 at 09:41:46PM +0200, Johannes Schindelin wrote:

Show 24 quoted lines

> Hi Emily,
> 
> 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?

Show 8 quoted lines

> 
> So in a sense, I guess I would even suggest to move as much of the setup
> for your test cases outside, preferably into an initial `setup` test case
> that initializes the minimal scenario required by the regression test
> case.
> 
> Thanks,
> Dscho