From: SZEDER Gábor Date: Mon, 30 Nov 2015 12:37:35 GMT Subject: Re: [PATCH 2/2] sh-setup: make require_clean_work_tree() work on orphan branches Message-ID: <20151130133735.Horde.6JXMnrTluxoV71C8eVKrKLi@webmail.informatik.kit.edu> In-Reply-To: <20151124205036.GF7174@sigill.intra.peff.net> Quoting Jeff King : > 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.