Re: [PATCH 4/4] t: branch: improve test rollback

On Sat, Sep 7, 2013 at 11:56 PM, Jeff King [off-list ref] wrote:

On Sat, Sep 07, 2013 at 10:02:59PM -0500, Felipe Contreras wrote:

quoted

'git checkout -' works perfectly fine, and it's the closest we have to
my suggested test_checkout() which I think would be ideal, but I'm not
going to work on.

I do not think it works perfectly fine if the checkout itself fails. For
example, after your patch, one of the tests reads:

  test_when_finished "git checkout -" &&
  git checkout HEAD^{} &&
  test_must_fail git branch --set-upstream-to master

The cleanup is undoing what happened on the second line. So if we fail
on the third line, it does what we want. If we fail on the second line,
then what branch are switching to?

I think you would at least want to reverse the order as:

  git checkout HEAD^{} &&
  test_when_finished "git checkout -" &&
  test_must_fail git branch --set-upstream-to master

so that you know you are actually going to the pre-test state.

Yeah, that would be better and easily achievable.

-- 
Felipe Contreras