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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 2, 2015, 23:07 UTC
Message-ID: <xmqqwpsw4fhi.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20151124205036.GF7174@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

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.

The state of the repository this topic wants to deal with better would be the same as the state you would get after "tar xf ... && git init && git add .", no? In the "orphan" case, unlike such a plain vanilla "created a new repository" case where the index could be empty for a long time before the initial 'git add', the index is almost always populated, so most likely this change will not make much difference in that require_clean_work_tree() would stop operation until "rm --cached ." empties the index.

And if the user did "rm --cached ." to empty the index, it is fine for us to happily consider all these files as untracked, even when HEAD is not pointing at an existing ref. So it is likely that the added ORPHAN_OK knob would be unnecessary for "orphan" case.

However, I suspect that this would affect the users in the "I am a new user, I just initialized my first Git repository and I haven't done a 'git add' yet" status much more. Do we have corner cases where everyday commands do not work well while on an unborn branch immediately after 'git init'? That is the kind of bug that we need to protect users from, either by keeping the "No HEAD yet, no operation that requires clean working tree" logic, or by fixing such bugs.

> 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.

It is unclear to me what you meant by "it" in "enforcing it".

> Still, it may be prudent to err on the side of caution. I'm on the
> fence.