Re: [PATCH 2/2] sh-setup: make require_clean_work_tree() work on orphan branches

From: SZEDER Gábor <szeder@ira.uka.de>
Date: Nov 30, 2015, 12:37 UTC
Message-ID: <20151130133735.Horde.6JXMnrTluxoV71C8eVKrKLi@webmail.informatik.kit.edu>
In-Reply-To: <20151124205036.GF7174@sigill.intra.peff.net>

Quoting Jeff King <peff@peff.net>:

Show 26 quoted lines

> On Tue, Nov 24, 2015 at 03:45:45PM +0100, SZEDER Gábor wrote:
>
>> git-sh-setup's require_clean_work_tree() always exits with error on an
>> orphan branch, even when the index and worktree are both clean.  The
>> reason is that require_clean_work_tree() starts off with verifying
>> HEAD, to make sure that it can safely pass HEAD to 'git diff-index'
>> later when it comes to checking the cleanness of the index, and errors
>> out finding the invalid HEAD of the orphan branch.
>>
>> There are scripts requiring a clean worktree that should work on an
>> orphan branch as well, and those should be able to use this function
>> instead of duplicating its functionality and nice error reporting in a
>> way that handles orphan branches.
>>
>> Fixing this is easy: the index should be compared to the empty tree
>> while on an orphan branch, and to HEAD otherwise.
>>
>> However, just fixing require_clean_work_tree() this way is also
>> dangerous, because scripts must take care to work properly on orphan
>> branches.  Currently a script calling require_clean_work_tree() would
>> exit on a clean orphan branch, but with the simple fix it would
>> continue executing and who knows what the consequences might be if
>> the script is not prepared for orphan branches.
>
> Hmm. I suspect this is not a big deal in practice. Lots of scripts
> (including some of our own, through history) get the orphan case wrong.

Well, to be honest, I thought it's not a big deal, too, but I also thought that the patch will be quickly shot down during review if I don't include some kind of a defense :) I'd be happier to reroll treating the current behavior in the clean orphan case as a bug, marking that test as expect_failure, and then just fixing it without the ORPHAN_OK thing.

> I'm not sure that require_clean_work_tree is necessarily the place to be
> enforcing it, even though it happened to have done so historically.

Yeah, right... It should be checked in the main code path, in git_dir_init(), but then it could hurt scripts that already do the right thing on an orphan branch because they don't set the ORPHAN_OK variable. Though that's probably not a big deal in practice either. Anyway, as it stands the documentation update of this patch is wrong.