Re: [PATCH] sha1-name.c: for ":/", find detached HEAD commits

From: Jeff King <peff@peff.net>
Date: Jul 10, 2018, 17:54 UTC
Message-ID: <20180710175447.GB25861@sigill.intra.peff.net>
In-Reply-To: <20180710154106.5356-1-wchargin@gmail.com>

On Tue, Jul 10, 2018 at 08:41:06AM -0700, William Chargin wrote:

Show 8 quoted lines

> This patch broadens the set of commits matched by ":/" pathspecs to
> include commits reachable from HEAD but not any named ref. This avoids
> surprising behavior when working with a detached HEAD and trying to
> refer to a commit that was recently created and only exists within the
> detached state.
> 
> Signed-off-by: William Chargin <wchargin@gmail.com>
> Based-on-patch-by: Jeff King <peff@peff.net>

Thanks, this looks perfect!

Just to be clear on licensing, I'll add my:

  Signed-off-by: Jeff King <peff@peff.net>

Show 17 quoted lines

> diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh
> index 62f335b2d..41b9f3eba 100755
> --- a/t/t4208-log-magic-pathspec.sh
> +++ b/t/t4208-log-magic-pathspec.sh
> @@ -25,6 +25,20 @@ test_expect_success '"git log :/a -- " should not be ambiguous' '
>  	git log :/a --
>  '
>  
> +test_expect_success '"git log :/detached -- " should find a commit only in HEAD' '
> +	test_when_finished "git checkout master" &&
> +	git checkout --detach &&
> +	test_tick &&
> +	git commit --allow-empty -m detached &&
> +	test_tick &&
> +	git commit --allow-empty -m something-else &&
> +	git log :/detached --
> +'

At first I wondered if you could just use test_commit here (instead of a manual test_tick and commit). But we care about reachability only from the detached HEAD here, so this must _not_ use test_commit, which creates an extra tag.

It could be worth mentioning that in a comment (since somebody who later refactors to make that change would not realize the problem, as it would simply cause a broken test to return a false success). But...

> +test_expect_success '"git log :/detached -- " should not find an orphaned commit' '
> +	test_must_fail git log :/detached --
> +'

This follow-up test _would_ notice such a breakage. Very nicely done.

-Peff