Re: [PATCH v5] Add another option for receive.denyCurrentBranch

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Dec 2, 2014, 08:21 UTC
Message-ID: <alpine.DEB.1.00.1412020909020.13845@s15462909.onlinehome-server.info>
In-Reply-To: <xmqqk32aoowa.fsf@gitster.dls.corp.google.com>

Hi Junio,

On Mon, 1 Dec 2014, Junio C Hamano wrote:

Show 29 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> >
> >> +static const char *update_worktree(unsigned char *sha1)
> >> +{
> >> +...
> >> +	const char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : "..";
> >
> > I overlooked this one, but is there a reason why this has to look at
> > an internal implementatino detail which is git_work_tree_cfg,
> > instead of asking get_git_work_tree()?
> >
> > I am wondering if that reason is a valid rationale to fix whatever
> > makes get_git_work_tree() unsuitable for this purpose.
> >
> > Cc'ing Duy who has been working on the part of the system to
> > determine and set-up work-tree and git-dir.
> 
> Answering myself...
> 
> This is because you know receive-pack runs inside the $GIT_DIR,
> whether it is a bare or non-bare repository, so either core.worktree
> points at a directory that is otherwise unrelated to the $GIT_DIR
> (but is the correct $GIT_WORK_TREE), or the top of the working tree
> must be ".." for a non-bare repository.  I haven't checked the code,
> but we probably are not even doing the repository/working-tree
> discovery to set up the data necessary for get_git_work_tree() to
> give us a correct answer.

Excellent analysis. Indeed, we do not enter the setup_git_directory_gently() path that would interpret core.worktree and call set_git_work_tree() explicitly. Instead, we are using the enter_repo() code path in cmd_receive_pack() because only minimal setup is required for the default operation of git receive-pack.

> Doing an optional set-up to make get_git_work_tree() work would
> involve more damage to the codebase than necessary, I would suspect,
> so let's keep this part of the code as-is.

Indeed, that is the exact reason why I did not want to go down that rabbit hole. There are so many things that are skipped when using enter_repo() instead of setup_git_directory() that the performance impact of switching alone would probably pose a major regression for big hosters like GitHub or Atlassian.

I have to admit also that I never used the work tree feature myself, therefore the support for it is pretty much untested (I *think* that I briefly tested it years ago, when I came up with the original updateInstead patch, but that is *long* ago). We could of course simply go the safe route and error out if we detect that git_work_tree_cfg is set, leaving the work to support it and to add a proper unit test to somebody who actually needs this. Hmm?

Ciao, Johannes