Re: [PATCH] disable grafts during fetch/push/bundle

From: Jeff King <peff@peff.net>
Date: Mar 5, 2014, 00:56 UTC
Message-ID: <20140305005649.GB11509@sigill.intra.peff.net>
In-Reply-To: <xmqqd2i1k7p9.fsf@gitster.dls.corp.google.com>

On Tue, Mar 04, 2014 at 12:52:18PM -0800, Junio C Hamano wrote:

Show 13 quoted lines

> > We already make an attempt to do the right thing in several
> > places by turning off read_replace_refs. However, we missed
> > at least one case (during bundle creation), and we do
> > nothing anywhere to handle grafts.
> 
> "Doing nothing for grafts" has been pretty much a deliberate
> omission.  Because we have no way to transfer how histories are
> grafted together, people cloning from a repository that grafts away
> a commit that records a mistakenly committed sekrit will end up with
> a disjoint history, instead of exposing the sekrit to them, and are
> expected to join the history by recreating grafts (perhaps a README
> of such a project instructs them to do so).  That was deemed far
> better than exposing the hidden history, I think.

I see your point, but I would be tempted to say that the person trying to hide a secret with grafting is simply wrong to do so. You need to cement that history with a rewrite if you want to share with people.

I do not recall any past discussion on this topic, and searching the archive only shows people echoing what I said above. Is this something we've promised to work in the past?

I'm certainly sympathetic to systems failing to a secure default rather than doing something that the user does not expect. But at the same time, if using grafts for security isn't something people reasonably expect, then failing only hurts the non-security cases.

> And "replace tries to do the right thing" was an attempt to rectify
> that misfeature of grafts in that we now do have a way to transfer
> how the history is grafted together, so that project README does not
> have to instruct the fetcher of doing anything special.

Perhaps the right response is "grafts are broken, use git-replace instead". But then should we think about deprecating grafts? Again, this patch was spurred by a real user with a graft trying to push and getting a confusing error message.

Show 7 quoted lines

> It _might_ be a misfeature, however, for the object connectivity
> layer to expose a part of the history replaced away to the party
> that fetches from such a repository.  Ideally, the "right thing"
> ought to be to include history that would be omitted if we did not
> have the replacement (i.e. adding parents the underlying commit does
> not record), while not following the history that replacement wants
> to hide (i.e. excluding the commits replacement commits overlay).

I don't really think it's worth the complexity. It's fairly common knowledge (or at least I think so) that replace refs are a _view_ onto the history. When you share the history graph, you share the true objects. You can _also_ share your views in replace/refs, but it is up to the client to fetch them. If you want to hide things, then you need to rewrite the true objects, end of story.

I dunno. Maybe there are people who have different expectations.

-Peff