[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH 2/2] sh-setup: make require_clean_work_tree() work on orphan branches
From:       SZEDER =?utf-8?b?R8OhYm9y?= <szeder () ira ! uka ! de>
Date:       2015-11-30 12:37:35
Message-ID: 20151130133735.Horde.6JXMnrTluxoV71C8eVKrKLi () webmail ! informatik ! kit ! edu
[Download RAW message or body]


Quoting Jeff King <peff@peff.net>:

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


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]