Some issues when trying to set up a shallow git mirror server

Some issues when trying to set up a shallow git mirror server

[Richard Maw]

Hi all.

I've been working on a service that mirrors open source code into git,
(http://git.baserock.org/cgi-bin/cgit.cgi/?q=delta if interested).

Some repositories are too unwieldy to mirror the whole history,
so we're looking at shallow support,
by fetching specified branches with `--depth=1`.

Since we'll be fetching in only a very shallow history,
we found that most of the time git classifies this as non-fast-forward.

I can see why this happens,
since it ends up with non-overlapping ranges of history
it hasn't fetched enough commits to join the dots.

This is inconvenient for us,
as we were explicitly using refspecs which didn't force the fetch,
since we were using the "non fast-forward update" errors
to detect whether upstream force pushed important refs
which could be a sign of tampering.

While the client doesn't have enough information
the server has those commits.
Would it make sense for the server to be able to tell the client
"trust me, that commit is a descendant of the previous one"?

Here's a patch to the test suite that demonstrates the failure,
since I felt I had to put some commands together to demonstrate,
and I may as well put it in a useful format.

diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index a980574..0996d12 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -186,4 +186,24 @@ EOF
 	test_cmp expect actual
 '
 
+test_expect_success 'fetch --depth with discontinuous history' '
+	git init --quiet --bare discontinuous.git &&
+	(
+	cd discontinuous.git &&
+	git fetch --quiet ../.git --depth=1 "refs/*:refs/*"
+	) &&
+	commit 5 &&
+	commit 6 &&
+	(
+	cd discontinuous.git &&
+	git fetch ../.git --depth=1 "refs/*:refs/*" &&
+	git fsck &&
+	git log --format=%s master >actual &&
+	cat <<EOF >expect &&
+6
+EOF
+	test_cmp expect actual
+	)
+'
+
 test_done

I also encountered weird behaviour when trying to push the history,
the general symptom being that the server would only update one ref,
so the client would fail because the server didn't mention all the refs.

diff --git a/t/t5538-push-shallow.sh b/t/t5538-push-shallow.sh
index ceee95b..d0bd538 100755
--- a/t/t5538-push-shallow.sh
+++ b/t/t5538-push-shallow.sh
@@ -120,4 +120,27 @@ EOF
 	git cat-file blob `echo 1|git hash-object --stdin` >/dev/null
 	)
 '
+
+test_expect_success 'push --mirror from shallow clone' '
+	git --git-dir=full/.git tag r0 &&
+	git init --quiet --bare mirror-pusher.git &&
+	git init --quiet --bare mirror.git &&
+	git --git-dir=mirror.git config receive.shallowUpdate true &&
+	(
+	cd mirror-pusher.git &&
+	git remote add --mirror=fetch origin ../full/.git &&
+	git remote add mirror ../mirror.git &&
+	git fetch --quiet origin --depth=1 &&
+	git push --mirror mirror
# The push should work, but does not.
# The following command works for creations, but not deletions.
#git for-each-ref -s --format "git push mirror +%(refname)" | sh
+	) &&
+	git --git-dir=mirror.git for-each-ref --format "%(refname)" --sort refname >actual &&
+	cat <<EOF >expect &&
+refs/heads/master
+refs/remotes/origin/HEAD
+refs/remotes/origin/master
+refs/tags/r0
+EOF
+	test_cmp expect actual
+'
+
 test_done

Re: Some issues when trying to set up a shallow git mirror server

[Junio C Hamano]

Richard Maw <richard.maw@codethink.co.uk> writes:

> This is inconvenient for us,
> as we were explicitly using refspecs which didn't force the fetch,
> since we were using the "non fast-forward update" errors
> to detect whether upstream force pushed important refs
> which could be a sign of tampering.
>
> While the client doesn't have enough information
> the server has those commits.
> Would it make sense for the server to be able to tell the client
> "trust me, that commit is a descendant of the previous one"?

It does not in our security model, as you do not blindly trust the
other side, whether you are a "client" or a "server".

Re: Some issues when trying to set up a shallow git mirror server

[Richard Maw]

On Thu, Jan 07, 2016 at 10:00:07AM -0800, Junio C Hamano wrote:
> Richard Maw <richard.maw@codethink.co.uk> writes:
> > This is inconvenient for us,
> > as we were explicitly using refspecs which didn't force the fetch,
> > since we were using the "non fast-forward update" errors
> > to detect whether upstream force pushed important refs
> > which could be a sign of tampering.
> >
> > While the client doesn't have enough information
> > the server has those commits.
> > Would it make sense for the server to be able to tell the client
> > "trust me, that commit is a descendant of the previous one"?
> 
> It does not in our security model, as you do not blindly trust the
> other side, whether you are a "client" or a "server".

Fair enough.
I didn't know whether Git passed responsibility for that to the transport layer.

Would a mode for fetch to also include the commit chain without the trees fit
the security model?

Re: Some issues when trying to set up a shallow git mirror server

[Duy Nguyen]

On Fri, Jan 8, 2016 at 5:19 PM, Richard Maw <richard.maw@codethink.co.uk> wrote:
> On Thu, Jan 07, 2016 at 10:00:07AM -0800, Junio C Hamano wrote:
>> Richard Maw <richard.maw@codethink.co.uk> writes:
>> > This is inconvenient for us,
>> > as we were explicitly using refspecs which didn't force the fetch,
>> > since we were using the "non fast-forward update" errors
>> > to detect whether upstream force pushed important refs
>> > which could be a sign of tampering.
>> >
>> > While the client doesn't have enough information
>> > the server has those commits.
>> > Would it make sense for the server to be able to tell the client
>> > "trust me, that commit is a descendant of the previous one"?
>>
>> It does not in our security model, as you do not blindly trust the
>> other side, whether you are a "client" or a "server".
>
> Fair enough.
> I didn't know whether Git passed responsibility for that to the transport layer.
>
> Would a mode for fetch to also include the commit chain without the trees fit
> the security model?

It sounds a lot like what I did with narrow clone [2] prototype. A
narrow clone only contains enough objects for certain paths so there's
a chance that we just don't have enough to do a proper merge. A
server-side command was added [1] to retrieve enough objects for the
task. If you fetch commit chain without all necessary trees and
objects, your repo is "broken" from Git point of view and you'll need
to do some extra work to make sure your repo is not actually broken.

[1] http://article.gmane.org/gmane.comp.version-control.git/154371
[2] http://thread.gmane.org/gmane.comp.version-control.git/154343
-- 
Duy

Re: Some issues when trying to set up a shallow git mirror server

[Richard Maw]

On Fri, Jan 08, 2016 at 05:44:30PM +0700, Duy Nguyen wrote:
> It sounds a lot like what I did with narrow clone [2] prototype. A
> narrow clone only contains enough objects for certain paths so there's
> a chance that we just don't have enough to do a proper merge. A
> server-side command was added [1] to retrieve enough objects for the
> task. If you fetch commit chain without all necessary trees and
> objects, your repo is "broken" from Git point of view and you'll need
> to do some extra work to make sure your repo is not actually broken.
> 
> [1] http://article.gmane.org/gmane.comp.version-control.git/154371
> [2] http://thread.gmane.org/gmane.comp.version-control.git/154343

Thanks for the links. I'll have a read to see how it fits together.

Re: Some issues when trying to set up a shallow git mirror server

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Richard Maw <richard.maw@codethink.co.uk> writes:
>
>> This is inconvenient for us,
>> as we were explicitly using refspecs which didn't force the fetch,
>> since we were using the "non fast-forward update" errors
>> to detect whether upstream force pushed important refs
>> which could be a sign of tampering.
>>
>> While the client doesn't have enough information
>> the server has those commits.
>> Would it make sense for the server to be able to tell the client
>> "trust me, that commit is a descendant of the previous one"?
>
> It does not in our security model, as you do not blindly trust the
> other side, whether you are a "client" or a "server".

Thinking about it more, I think my answer was flawed.  

A client may ask the server to give a history, and before it accepts
the result, it must do its own consistency check.  A client may send
its history, and before the server accepts it, the server must do
its own consistency check.  This is the general principle around our
consistency model (it is not limited to "security", but it is more
about "correctness" in general).

While that consistency principle must hold everywhere in our system,
it does not mean a client cannot ask a server to do something whose
result it has to trust, at least to some degree, because there is
fundamentally no way to independenty verify the result.

I think what you were hinting falls into that category.  The client
cannot verify that the new tip is a descendant of the old one
without having the full history between these two points, but
transfering that history would defeat the whole point of using a
shallowed history.

So in that sense I do think that such a protocol extension does make
sense.  But it would involve some extra pieces of information that
need to be sent between the client and the server, not just "I'll
trust you".

In the current "git fetch" protocol, the sender gives the receiver a
list of refs, and for each of these refs, the object name it points
at.  Then the receiver asks the sender to give it the objects that
appear in the history leading to a set of objects (aka "want"), and
tells the sender what objects it completely has already (aka
"have").  The idea is that the sender can exclude objects that are
reachable from "have"s when it enumerates the objects that need to
be sent.  When the receiver is shallow, it also tells the sender
that its current history is truncated at such and such commits,
lacking things before them.  After that exchange, the sender just
gives the receiver a packfile that contains the objects requested.
The receiver verifies that the packfile makes sense, e.g. it has all
the "want" objects it asked for, and the objects that they refer to
(recursively) are available, before updating its refs with (some of)
the "want" objects.

In the simplest case, for example, where you might have a single
refs/heads/master that currently points at O and the other side has
N at its refs/heads/master, the sender would say "I have N at
refs/heads/master" and the receiver would say "I want N, and I have
O".  A shallow receiver may also say "I do not have history behind
O" and "I only want a history 1 commit deep".  And the sender would
send objects that is necessary to satisfy the request, e.g. commit N
itself and the trees and blobs in commit N that are not common with
commit O.

Notice that nowhere in this exchange the receiver tells the sender
what it intends to do with the "want" objects?

Because the receiver does not say "I want N and I intend to replace
O I currently have refs/heads/master", there is no way for the
sender to say "trust me, N is a descendant of O".  It simply does
not know if the receiver _cares_ how N and O are related with each
other.

So if you want to do this, a new protocol extension needs to allow
your updated sender (upload-pack) and receiver (fetch-pack) to work
more like this:

 * The sender would advertise "I support that extension", while
   giving the usual "here are my refs and its current values".

 * The receiver would say "I want to use that extension", and to
   each of its "want" (which usually consists of "want" followed by
   an object name and nothing else), it optionally adds names of the
   objects it wants to verify ancestry relationship with.

   E.g. if you have O at the tip of the master branch and P at the
   tip of the maint branch, the sender has N at both of these two
   branches, and if you are updating your master and maint with
   their master and maint, you would say something like "want N O P"
   to tell the sender that you want history leading to N, and you
   want to see if N is a descendant of O and if N is a descendant of
   P.

 * The receiver and the sender then does the usual "have"/"ack"
   exchange, which does not have to change any behaviour with this
   extension.

 * Finally, when the sender sends out the resulting packfile, it
   also has to tell the receiver which of the object pairs the
   receiver asked it to check the ancestry relationship violate the
   fast-forward rule.  In the earlier example of fast-forwarding O
   and P with N, where the receiver asked "want N O P", the receiver
   asked to check object pairs <N, O> and <N, P>.  If P fast-forwards
   to N but O does not, then the sender would tell the receiver the
   fact that "O does not fast forward to N".

With such an extension, your updated receiver can receive the
necessary objects to update your history to "N", but notice that it
would result in non-ff update to update master (that used to be O)
with the new commit N.

Re: Some issues when trying to set up a shallow git mirror server

[Richard Maw]

On Fri, Jan 08, 2016 at 01:37:02PM -0800, Junio C Hamano wrote:

Thanks for the write-up.
I knew most of how the protocol worked, but not all,
and I appreciate the write-up of how the extension could work.

If I have time I can see if I can implement this protocol extension myself.

<snip>
> While that consistency principle must hold everywhere in our system,
> it does not mean a client cannot ask a server to do something whose
> result it has to trust, at least to some degree, because there is
> fundamentally no way to independenty verify the result.

So there'll be a greater level of trust required in the transport protocol,
so you wouldn't necessarily want to allow this protocol over git:// or http://.

<snip>
> So if you want to do this, a new protocol extension needs to allow
> your updated sender (upload-pack) and receiver (fetch-pack) to work
> more like this:
> 
>  * The sender would advertise "I support that extension", while
>    giving the usual "here are my refs and its current values".
> 
>  * The receiver would say "I want to use that extension", and to
>    each of its "want" (which usually consists of "want" followed by
>    an object name and nothing else), it optionally adds names of the
>    objects it wants to verify ancestry relationship with.
> 
>    E.g. if you have O at the tip of the master branch and P at the
>    tip of the maint branch, the sender has N at both of these two
>    branches, and if you are updating your master and maint with
>    their master and maint, you would say something like "want N O P"
>    to tell the sender that you want history leading to N, and you
>    want to see if N is a descendant of O and if N is a descendant of
>    P.
> 
>  * The receiver and the sender then does the usual "have"/"ack"
>    exchange, which does not have to change any behaviour with this
>    extension.
> 
>  * Finally, when the sender sends out the resulting packfile, it
>    also has to tell the receiver which of the object pairs the
>    receiver asked it to check the ancestry relationship violate the
>    fast-forward rule.  In the earlier example of fast-forwarding O
>    and P with N, where the receiver asked "want N O P", the receiver
>    asked to check object pairs <N, O> and <N, P>.  If P fast-forwards
>    to N but O does not, then the sender would tell the receiver the
>    fact that "O does not fast forward to N".

So this would be another step after the receiver communicates "done",
but before the pack file itself gets sent,
so the sender can determine that it doesn't need to tell the reciever
the relationship between two commits,
since it knows you can work it out yourself?

Otherwise if it can probably communicate the relationships before the have list
is sent, and extend the shallow-update part of the protocol instead, which
might be simpler.

> With such an extension, your updated receiver can receive the
> necessary objects to update your history to "N", but notice that it
> would result in non-ff update to update master (that used to be O)
> with the new commit N.

Code-wise, does the following make sense?

1.  Add a field to `struct ref` to flag a "trusted fast-forward".
2.  Change `find_common()` in `fetch-pack.c`
    and `receive_needs()` in `upload-pack.c`
    to communicate the relationships we're interested in as above,
    and set the "trusted fast-forward" flag.
3.  Change `update_local_ref()` in `builtin/fetch.c`
    to check `ref->trusted_fast_forward || in_merge_bases(current, updated)`.

Re: Some issues when trying to set up a shallow git mirror server

[Junio C Hamano]

Richard Maw <richard.maw@codethink.co.uk> writes:

> On Fri, Jan 08, 2016 at 01:37:02PM -0800, Junio C Hamano wrote:
> ...
>> So if you want to do this, a new protocol extension needs to allow
>> your updated sender (upload-pack) and receiver (fetch-pack) to work
>> more like this:
>> ...
>>  * Finally, when the sender sends out the resulting packfile, it
>>    also has to tell the receiver which of the object pairs the
>>    receiver asked it to check the ancestry relationship violate the
>>    fast-forward rule.  In the earlier example of fast-forwarding O
>>    and P with N, where the receiver asked "want N O P", the receiver
>>    asked to check object pairs <N, O> and <N, P>.  If P fast-forwards
>>    to N but O does not, then the sender would tell the receiver the
>>    fact that "O does not fast forward to N".
>
> So this would be another step after the receiver communicates "done",
> but before the pack file itself gets sent,
> so the sender can determine that it doesn't need to tell the reciever
> the relationship between two commits,
> since it knows you can work it out yourself?
>
> Otherwise if it can probably communicate the relationships before the have list
> is sent, and extend the shallow-update part of the protocol instead, which
> might be simpler.

I left the detail as vague ;-).

The new request does not have to piggyback on existing "want"
message.  And thinking about it again, it probably is cleaner if it
didn't.  After the use of the protocol extension "ancestry-check" is
negotiated the usual way between the sender and the receiver, the
receiver would send "check-ff N O" and "check-ff N P" after it sends
all of its "want" messages but before it sends the "flush" to go
into the "have"/"ack" common ancestry discovery.

I do not have a strong opinion on where the sender should reply with
"not-ff N O" in the protocol.  Immediately after the receiver says
"I've done with my 'want's (and now 'check-ff's)" by flushing may be
a good place to do so.

>> With such an extension, your updated receiver can receive the
>> necessary objects to update your history to "N", but notice that it
>> would result in non-ff update to update master (that used to be O)
>> with the new commit N.
>
> Code-wise, does the following make sense?
>
> 1.  Add a field to `struct ref` to flag a "trusted fast-forward".
> 2.  Change `find_common()` in `fetch-pack.c`
>     and `receive_needs()` in `upload-pack.c`
>     to communicate the relationships we're interested in as above,
>     and set the "trusted fast-forward" flag.
> 3.  Change `update_local_ref()` in `builtin/fetch.c`
>     to check `ref->trusted_fast_forward || in_merge_bases(current, updated)`.

Re: Some issues when trying to set up a shallow git mirror server

[Richard Maw]

On Tue, Jan 12, 2016 at 10:29:27AM -0800, Junio C Hamano wrote:
> I left the detail as vague ;-).
> 
> The new request does not have to piggyback on existing "want"
> message.  And thinking about it again, it probably is cleaner if it
> didn't.  After the use of the protocol extension "ancestry-check" is
> negotiated the usual way between the sender and the receiver, the
> receiver would send "check-ff N O" and "check-ff N P" after it sends
> all of its "want" messages but before it sends the "flush" to go
> into the "have"/"ack" common ancestry discovery.
> 
> I do not have a strong opinion on where the sender should reply with
> "not-ff N O" in the protocol.  Immediately after the receiver says
> "I've done with my 'want's (and now 'check-ff's)" by flushing may be
> a good place to do so.

In that case, I can think of two other useful times to do it:

1.  Before any "want" requests.

    This would also let you extend ls-remote to let it display ancestry.

    This is complicated by the fact that normally the client responds
    with which features it supports in the first "want",
    so the sender would have to support "check-ff N O <FEATURES>"
    if it advertised "ancestry-check".

2.  After the pack is transferred.

    Then the receiver can check ancestry with the objects,
    and only request ancestry if it's missing history.

    I'm not sure whether there's any value in not requesting it,
    since while it would reduce the amount of work the sender needs to do,
    it still means the sender has to wait for the client to do the check,
    rather than hanging up and allowing it to process another connection.

Re: Some issues when trying to set up a shallow git mirror server

[Junio C Hamano]

Richard Maw <richard.maw@codethink.co.uk> writes:

> 1.  Before any "want" requests.
>
>     This would also let you extend ls-remote to let it display ancestry.
>
>     This is complicated by the fact that normally the client responds
>     with which features it supports in the first "want",
>     so the sender would have to support "check-ff N O <FEATURES>"
>     if it advertised "ancestry-check".

Yes, that sounds like a good thing.  Actually, ls-remote is a
degenerated case of fetch that sends 0 "want" requests, so the above
may be identical to what I suggested in the message you are
responding to.

> 2.  After the pack is transferred.

I do not think the protocol is structured to allow you to do this
easily.  The reading side is expected to read thru til the end of
the stream, not stopping at the end of the packdata, IIRC.

Re: Some issues when trying to set up a shallow git mirror server

[Richard Maw]

On Wed, Jan 13, 2016 at 09:14:10AM -0800, Junio C Hamano wrote:
> Richard Maw <richard.maw@codethink.co.uk> writes:
> 
> > 1.  Before any "want" requests.
> >
> >     This would also let you extend ls-remote to let it display ancestry.
> >
> >     This is complicated by the fact that normally the client responds
> >     with which features it supports in the first "want",
> >     so the sender would have to support "check-ff N O <FEATURES>"
> >     if it advertised "ancestry-check".
> 
> Yes, that sounds like a good thing.  Actually, ls-remote is a
> degenerated case of fetch that sends 0 "want" requests, so the above
> may be identical to what I suggested in the message you are
> responding to.

Thanks. I've got a good idea of what I'd need to do now.