Re: [PATCH v2 2/2] t3200: verify "branch --list" sanity when rebasing from detached HEAD

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Apr 3, 2018 at 12:31 AM, Kaartic Sivaraam
<kaartic.sivaraam@gmail.com> wrote:
> From: Eric Sunshine <sunshine@sunshineco.com>
>
> "git branch --list" shows an in-progress rebase as:
>
>   * (no branch, rebasing <branch>)
>     master
>     ...
>
> However, if the rebase is started from a detached HEAD, then there is no
> <branch>, and it would attempt to print a NULL pointer. The previous
> commit fixed this problem, so add a test to verify that the output is
> sane in this situation.
>
> Note that the "detached HEAD" test case might actually fail in some cases
> as the actual output of "git branch --list" might contain remote branch
> names which is not considered by the test case as it is rare to happen
> in the test environment.

This paragraph was not in the original patch[1]. I _think_ what you
are saying (which took a while to decipher) is that if a command such
as "git checkout origin/next" ever gets inserted into the script
before the test, the test will be fooled since "git branch --list"
will show "detached HEAD origin/next" rather than "detached HEAD
d3adb33f", the latter of which is what the test is expecting.

Unfortunately, this paragraph makes it sound as if the test can fail
randomly (which, I believe, is not the case), and nobody would want a
test added which is unreliable, thus this paragraph is not helping to
sell this patch (in fact, it's actively hurting it). Ideally, the test
should be entirely deterministic so that it can't be fooled like this.
Rather than including this (harmful) paragraph in the commit message,
let's ensure that the test is deterministic (see below).

[1]: https://public-inbox.org/git/20180325054824.GA56795@flurp.local/

> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> ---
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> @@ -1246,6 +1247,29 @@ test_expect_success '--merged is incompatible with --no-merged' '
> +test_expect_success '--list during rebase from detached HEAD' '
> +       test_when_finished "reset_rebase && git checkout master" &&
> +       git checkout HEAD^0 &&

This is the line which I think is causing concern for you. If someone
inserted a new test before this one which invoked "git checkout
origin/next" (or something), then even after "git checkout HEAD^0",
"git branch --list" would still report the unexpected "detached HEAD
origin/next". Let's fix this, and make the test deterministic, by
doing this instead:

    git checkout master^0 &&

Thanks.

> +       oid=$(git rev-parse --short HEAD) &&
> +       FAKE_LINES="1 edit 2" &&
> +       export FAKE_LINES &&
> +       set_fake_editor &&
> +       git rebase -i HEAD~2 &&
> +       git branch --list >actual &&
> +       test_i18ngrep "rebasing detached HEAD $oid" actual
> +'