threads / patch / 32549

patchDocumentation on depth option in git clone.

Subject: [PATCH] Documentation on depth option in git clone.

## tl;dr

29 messages between Jan 7, 2013 and Jul 11, 2013. Diffs are folded; open one to read it.

replies: 28people: 5as markdown or json

Stefan Beller· Jan 7, 2013, 18:06 UTC · lore

[PATCH] git clone depth of 0 not possible.

Currently it is not possible to have a shallow depth of just 0, i.e. only one commit in that repository after cloning. The minimum number of commits is 2, caused by depth=1.

I had no good idea how to add this behavior to git clone as the depth variable in git_transport_options struct (file transport.h) uses value 0 for another meaning, so it would have need changes at all places, where the transport options depth is being used (e.g. fetch)

So I documented the current behavior, see attached patch.
Stefan Beller (1):
  Documentation on depth option in git clone.
 Documentation/git-clone.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
-- 
1.8.1.80.g3e293fb.dirty
Stefan Beller· Jan 7, 2013, 18:06 UTC · re: Stefan Beller · lore
---
 Documentation/git-clone.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to Documentation/git-clone.txt +2 −1
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 7fefdb0..e76aa50 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -186,7 +186,8 @@ objects from the source repository into a pack in the cloned repository.
 	it, nor push from nor into it), but is adequate if you
 	are only interested in the recent history of a large project
 	with a long history, and would want to send in fixes
-	as patches.
+	as patches. The depth should be at least 1. If it is 0 or
+	below, the cloned repository will not be shallow.
 
 --single-branch::
 	Clone only the history leading to the tip of a single branch,

-- 
1.8.1.80.g3e293fb.dirty
Jonathan Nieder· Jan 8, 2013, 06:28 UTC · re: Stefan Beller · lore

Re: [PATCH] git clone depth of 0 not possible.

Stefan Beller wrote:
> Currently it is not possible to have a shallow depth of
> just 0, i.e. only one commit in that repository after cloning.
> The minimum number of commits is 2, caused by depth=1.

Sounds buggy. Would anything break if we were to make --depth=1 mean "1 deep, including the tip commit"?

Junio C Hamano· Jan 8, 2013, 06:54 UTC · re: Jonathan Nieder · lore

Re: [PATCH] git clone depth of 0 not possible.

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 8 quoted lines
> Stefan Beller wrote:
>
>> Currently it is not possible to have a shallow depth of
>> just 0, i.e. only one commit in that repository after cloning.
>> The minimum number of commits is 2, caused by depth=1.
>
> Sounds buggy.  Would anything break if we were to make --depth=1 mean
> "1 deep, including the tip commit"?

As long as we do not change the meaning of the "shallow" count going over the wire (i.e. the number we receive from the user will be fudged, so that user's "depth 1" that used to mean "the tip and one behind it" is expressed as "depth 2" at the end-user level, and we send over the wire the number that corresponded to the old "depth 1"), I do not think anything will break, and then --depth=0 may magically start meaning "only the tip; its immediate parents will not be transferred and recorded as the shallow boundary in the receiving repository".

I do not mind carrying such a (technially) backward incompatible change in jn/clone-2.0-depth-off-by-one branch, keep it cooking in 'next' for a while and push it out together with other "2.0" topics in a future release ;-).

Junio C Hamano· Jan 8, 2013, 07:36 UTC · re: Junio C Hamano · lore

Re: [PATCH] git clone depth of 0 not possible.

Junio C Hamano <gitster@pobox.com> writes:
Show 25 quoted lines
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> Stefan Beller wrote:
>>
>>> Currently it is not possible to have a shallow depth of
>>> just 0, i.e. only one commit in that repository after cloning.
>>> The minimum number of commits is 2, caused by depth=1.
>>
>> Sounds buggy.  Would anything break if we were to make --depth=1 mean
>> "1 deep, including the tip commit"?
>
> As long as we do not change the meaning of the "shallow" count going
> over the wire (i.e. the number we receive from the user will be
> fudged, so that user's "depth 1" that used to mean "the tip and one
> behind it" is expressed as "depth 2" at the end-user level, and we
> send over the wire the number that corresponded to the old "depth
> 1"), I do not think anything will break, and then --depth=0 may
> magically start meaning "only the tip; its immediate parents will
> not be transferred and recorded as the shallow boundary in the
> receiving repository".
>
> I do not mind carrying such a (technially) backward incompatible
> change in jn/clone-2.0-depth-off-by-one branch, keep it cooking in
> 'next' for a while and push it out together with other "2.0" topics
> in a future release ;-).

Speaking of --depth, I think in Git 2.0 we should fix the semantics of "deepening" done with "git fetch".

Its "--depth" parameter is used to specify the new depth of the history that you can tangle from the updated tip of remote tracking branches, and it has a rather unpleasant ramifications.

Suppose you start from "git clone --depth=1 $there". You have the today's snapshot, and one parent behind it. You keep working happily with the code and then realize that you want to know a bit more history behind the snapshot you started from.

 (upstream)
  ---o---o---o---A---B
 (you)
                 A---B
So you do:
    $ git fetch --depth=3
 (upstream)
  ---o---o---o---A---B---C---D---E---F---...---W---X---Y---Z
 (you)
                 A---B                         W---X---Y---Z

But in the meantime, if the upstream accumulated 20+ commits, you end up getting the commit at the updated tip of the upstream, and 3 generations of parents behind it. There will be a 10+ commit worth of gap between the bottom of the new shallow history and the old tip you have been working on, and the history becomes disjoint.

I think we need a protocol update to fix this; instead of sending "Now I want your tips and N commits behind it, please update my shallow bottom accordingly", which creates the above by giving you Z and 3 generations back and updates your cut-off point to W, the receiving end should be able to ask "I have a shallow history that cuts off at these commits. I want to get the history leading up to your tips, and also deepen the history further back from my current cut-off points by N commits", so that you would instead end up with something like this:

 (you)
     o---o---o---A---B---C---D---E---F---...---W---X---Y---Z

That is, truly "deepen my history by 3". We could call that "git fetch --deepen=3" or something.

Junio C Hamano· Jan 8, 2013, 08:19 UTC · re: Junio C Hamano · lore

Re: [PATCH] git clone depth of 0 not possible.

Junio C Hamano <gitster@pobox.com> writes:
Show 15 quoted lines
> I think we need a protocol update to fix this; instead of sending
> "Now I want your tips and N commits behind it, please update my
> shallow bottom accordingly", which creates the above by giving you Z
> and 3 generations back and updates your cut-off point to W, the
> receiving end should be able to ask "I have a shallow history that
> cuts off at these commits. I want to get the history leading up to
> your tips, and also deepen the history further back from my current
> cut-off points by N commits", so that you would instead end up with
> something like this:
>
>  (you)
>      o---o---o---A---B---C---D---E---F---...---W---X---Y---Z
>
> That is, truly "deepen my history by 3".  We could call that "git
> fetch --deepen=3" or something.
I take that back.  If you start from
Show 5 quoted lines
>  (upstream)
>   ---o---o---o---A---B
>
>  (you)
>                  A---B

and you are interested in peeking the history a bit deeper, you should be able to ask "I have a shallow history that cuts off at these commits. I want my history deepened by N commits. I do not care where your current tips are, by the way." with

    git fetch --deepen=3 
and end up with
>  (you)
>      o---o---o---A---B

without getting the new history leading to the updated tip at the upstream. If you want the new history leading to the updated tip, you can just say:

    git fetch
without any --depth nor --deepen option to end up with:
>  (you)
>                  A---B---C---D---E---F---...---W---X---Y---Z
instead.
Duy Nguyen· Jan 8, 2013, 14:28 UTC · re: Junio C Hamano · lore

Re: [PATCH] git clone depth of 0 not possible.

On Tue, Jan 8, 2013 at 2:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Speaking of --depth, I think in Git 2.0 we should fix the semantics
> of "deepening" done with "git fetch".

Speaking of 2.0, we should support depth per ref. Well we don't have to wait until 2.0 because we could just add shallow2 extension to the pack protocol. We should also apply depth to new refs when fetching them the first time.

-- 
Duy
Stefan Beller· Jan 8, 2013, 14:32 UTC · re: Duy Nguyen · lore

Re: [PATCH] git clone depth of 0 not possible.

On 01/08/2013 03:28 PM, Duy Nguyen wrote:
Show 9 quoted lines
> On Tue, Jan 8, 2013 at 2:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Speaking of --depth, I think in Git 2.0 we should fix the semantics
>> of "deepening" done with "git fetch".
> 
> Speaking of 2.0, we should support depth per ref. Well we don't have
> to wait until 2.0 because we could just add shallow2 extension to the
> pack protocol. We should also apply depth to new refs when fetching
> them the first time.
>

Would this mean I could do something along? $ git clone --since v1.8.0 git://github.com/gitster/git.git

So tags would be allowed as anchors?
Duy Nguyen· Jan 8, 2013, 14:45 UTC · re: Stefan Beller · lore

Re: [PATCH] git clone depth of 0 not possible.

On Tue, Jan 8, 2013 at 9:32 PM, Stefan Beller <stefanbeller@googlemail.com> wrote:

Show 15 quoted lines
> On 01/08/2013 03:28 PM, Duy Nguyen wrote:
>> On Tue, Jan 8, 2013 at 2:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Speaking of --depth, I think in Git 2.0 we should fix the semantics
>>> of "deepening" done with "git fetch".
>>
>> Speaking of 2.0, we should support depth per ref. Well we don't have
>> to wait until 2.0 because we could just add shallow2 extension to the
>> pack protocol. We should also apply depth to new refs when fetching
>> them the first time.
>>
>
> Would this mean I could do something along?
> $ git clone --since v1.8.0 git://github.com/gitster/git.git
>
> So tags would be allowed as anchors?
No. This is what I had in mind:
git clone --branch=master --depth=2 git.git # get branch master with depth 2
git fetch --depth=10 origin next            # get branch next with depth 10
                                            # master's depth remains 2
git fetch origin                # get (new) branch 'pu' with default depth 2

But your case is interesting. We could specify --depth=v1.8.0.. or even --depth=v1.8.0~200.. (200 commits before v1.8.0). Somebody may even go crazy and make --depth=v1.6.0..v1.8.0 work. --depth is probably not the right name anymore. Any SHA-1 would be allowed as anchor. But I think we need to wait for reachability bitmap feature to come first so that we can quickly verify the anchor is reachable from the public refs.

-- 
Duy
Junio C Hamano· Jan 8, 2013, 17:24 UTC · re: Stefan Beller · lore

Re: [PATCH] git clone depth of 0 not possible.

Stefan Beller <stefanbeller@googlemail.com> writes:
Show 14 quoted lines
> On 01/08/2013 03:28 PM, Duy Nguyen wrote:
>> On Tue, Jan 8, 2013 at 2:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Speaking of --depth, I think in Git 2.0 we should fix the semantics
>>> of "deepening" done with "git fetch".
>> 
>> Speaking of 2.0, we should support depth per ref. Well we don't have
>> to wait until 2.0 because we could just add shallow2 extension to the
>> pack protocol. We should also apply depth to new refs when fetching
>> them the first time.
>
> Would this mean I could do something along?
> $ git clone --since v1.8.0 git://github.com/gitster/git.git
>
> So tags would be allowed as anchors?

As the end-user facing UI, I think it would be much easier to use for users who want to get only the recent part of history that is relevant to their development if you allowed them to ask "starting from this one, I do not care anything older than that" with such an interface. The current "count how many more generations you want" interface is crazy in that it forces you to count what you have not even seen; I suspect the only reason it was done in such a hacky manner was implementation expediency.

At the syntax level, however, I do not think we can use --since there, because the keyword has a very different meaning already.

I personally do not think "depth per ref" deserves "it would be nice to support in 2.0", let alone "2.0 *should* support", label. Some may find it an interesting mental exercise to think about corner cases it will introduce and have to deal with (e.g. you ask 100 from master and 2 from maint, but maint is behind master by less than 100---what should happen?), but I do not particularly see any practical use cases, and I highly doubt that there is much value in bringing in extra complexity such a "feature" requires to do it right.

Duy Nguyen· Jan 8, 2013, 07:38 UTC · re: Junio C Hamano · lore

Re: [PATCH] git clone depth of 0 not possible.

On Tue, Jan 8, 2013 at 1:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
>> Sounds buggy.  Would anything break if we were to make --depth=1 mean
>> "1 deep, including the tip commit"?
>
> As long as we do not change the meaning of the "shallow" count going
> over the wire (i.e. the number we receive from the user will be
> fudged, so that user's "depth 1" that used to mean "the tip and one
> behind it" is expressed as "depth 2" at the end-user level, and we
> send over the wire the number that corresponded to the old "depth
> 1"), I do not think anything will break, and then --depth=0 may
> magically start meaning "only the tip; its immediate parents will
> not be transferred and recorded as the shallow boundary in the
> receiving repository".

I'd rather we reserve 0 for unlimited fetch, something we haven't done so far [1]. And because "unlimited clone" with --depth does not make sense, --depth=0 should be rejected by git-clone.

[1] If we don't want to break the protocol, we could make depth 0xffffffff a special value as "unlimited" for newer git. Older git works most of the time, until some project exceeds 4G commit depth history.

-- 
Duy
Junio C Hamano· Jan 8, 2013, 08:05 UTC · re: Duy Nguyen · lore

Re: [PATCH] git clone depth of 0 not possible.

Duy Nguyen <pclouds@gmail.com> writes:
Show 17 quoted lines
> On Tue, Jan 8, 2013 at 1:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Sounds buggy.  Would anything break if we were to make --depth=1 mean
>>> "1 deep, including the tip commit"?
>>
>> As long as we do not change the meaning of the "shallow" count going
>> over the wire (i.e. the number we receive from the user will be
>> fudged, so that user's "depth 1" that used to mean "the tip and one
>> behind it" is expressed as "depth 2" at the end-user level, and we
>> send over the wire the number that corresponded to the old "depth
>> 1"), I do not think anything will break, and then --depth=0 may
>> magically start meaning "only the tip; its immediate parents will
>> not be transferred and recorded as the shallow boundary in the
>> receiving repository".
>
> I'd rather we reserve 0 for unlimited fetch, something we haven't done
> so far [1]. And because "unlimited clone" with --depth does not make
> sense, --depth=0 should be rejected by git-clone.

I actually was thinking about changing --depth=1 to mean "the tip, with zero commits behind it" (and that was consistent with my description of "fudging"), but ended up saying "--depth=0" by mistake. I too think "--depth=0" or "--depth<0" does not make sense, so we are in agreement.

Thanks for a sanity check.
Matthijs Kooijman· May 28, 2013, 09:18 UTC · re: Junio C Hamano · lore

Re: [PATCH] git clone depth of 0 not possible.

Hi Junio,

I'm interested in getting a fetch tip commit only feature into git, I'll probably look into creating a patch for this.

Show 22 quoted lines
> >>> Sounds buggy.  Would anything break if we were to make --depth=1 mean
> >>> "1 deep, including the tip commit"?
> >>
> >> As long as we do not change the meaning of the "shallow" count going
> >> over the wire (i.e. the number we receive from the user will be
> >> fudged, so that user's "depth 1" that used to mean "the tip and one
> >> behind it" is expressed as "depth 2" at the end-user level, and we
> >> send over the wire the number that corresponded to the old "depth
> >> 1"), I do not think anything will break, and then --depth=0 may
> >> magically start meaning "only the tip; its immediate parents will
> >> not be transferred and recorded as the shallow boundary in the
> >> receiving repository".
> >
> > I'd rather we reserve 0 for unlimited fetch, something we haven't done
> > so far [1]. And because "unlimited clone" with --depth does not make
> > sense, --depth=0 should be rejected by git-clone.
> 
> I actually was thinking about changing --depth=1 to mean "the tip,
> with zero commits behind it" (and that was consistent with my
> description of "fudging"), but ended up saying "--depth=0" by
> mistake.  I too think "--depth=0" or "--depth<0" does not make
> sense, so we are in agreement.
Did you consider how to implement this? Looking at the code, it seems
the "deepen" parameter in the wire protocol now means:
 - 0: Do not change anything about the shallowness (i.e., fetch
   everything from the shallow root to the tip).
 - > 0: Create new shallow commits at depth commits below the tip (so
   depth == 1 means tip and one below).
 - INFINITE_DEPTH (0x7fffffff): Remove all shallowness and fetch
   complete history.

Given this, I'm not sure how one can express "fetch the tip and nothing below that", since depth == 0 already has a different meaning.

Of course, one could using depth == 1 in this case to receive two commits and then drop one, but this would seem a bit pointless to me (especially if the commit below the tip is very different from the tip leading to a lot of useless data transfer).

Or did I misunderstand something here?
Gr.
Matthijs
Jonathan Nieder· May 28, 2013, 16:28 UTC · re: Matthijs Kooijman · lore

Re: [PATCH] git clone depth of 0 not possible.

Matthijs Kooijman wrote:
Show 11 quoted lines
> Did you consider how to implement this? Looking at the code, it seems
> the "deepen" parameter in the wire protocol now means:
>  - 0: Do not change anything about the shallowness (i.e., fetch
>    everything from the shallow root to the tip).
>  - > 0: Create new shallow commits at depth commits below the tip (so
>    depth == 1 means tip and one below).
>  - INFINITE_DEPTH (0x7fffffff): Remove all shallowness and fetch
>    complete history.
>
> Given this, I'm not sure how one can express "fetch the tip and nothing
> below that", since depth == 0 already has a different meaning.

If I remember correctly, what we discussed is just changing the protocol to "5 means a depth of 5". The client already trusts what the server provides.

Thanks and hope that helps, Jonathan

Jonathan Nieder· May 28, 2013, 16:31 UTC · re: Jonathan Nieder · lore

Re: [PATCH] git clone depth of 0 not possible.

Jonathan Nieder wrote:
> If I remember correctly, what we discussed is just changing the
> protocol to "5 means a depth of 5". The client already trusts what the
> server provides.

(Or tweaking the protocol by adding a new capability, if unpredictable behavior based on the version of the server won't fly. :))

Jonathan
Matthijs Kooijman· May 28, 2013, 16:34 UTC · re: Jonathan Nieder · lore

Re: [PATCH] git clone depth of 0 not possible.

Hi Jonathan,
Show 14 quoted lines
> > Did you consider how to implement this? Looking at the code, it seems
> > the "deepen" parameter in the wire protocol now means:
> >  - 0: Do not change anything about the shallowness (i.e., fetch
> >    everything from the shallow root to the tip).
> >  - > 0: Create new shallow commits at depth commits below the tip (so
> >    depth == 1 means tip and one below).
> >  - INFINITE_DEPTH (0x7fffffff): Remove all shallowness and fetch
> >    complete history.
> >
> > Given this, I'm not sure how one can express "fetch the tip and nothing
> > below that", since depth == 0 already has a different meaning.
> 
> If I remember correctly, what we discussed is just changing the
> protocol to "5 means a depth of 5".
The mail from Junio I replied to said:
> >> As long as we do not change the meaning of the "shallow" count
> >> going over the wire

Which seems to conflict with your suggestion. Or are the "shallow count" and the "depth" different things?

> The client already trusts what the server provides.

In other words: we won't break existing clients if we suddenly send back one less commit than before, since the client just sends over what it wants and then assumes that whatever it gets back is really what it wanted?

Gr.
Matthijs
Jonathan Nieder· May 28, 2013, 16:58 UTC · re: Matthijs Kooijman · lore

Re: [PATCH] git clone depth of 0 not possible.

Matthijs Kooijman wrote:
> In other words: we won't break existing clients if we suddenly send back
> one less commit than before, since the client just sends over what it
> wants and then assumes that whatever it gets back is really what it
> wanted?
Yes, depending on your definition of "break".

An advantage of that approach is that old clients would get the new, intuitive behavior without upgrading. A disadvantage is that it is a confusing world where the same command produces different effects when contacting different servers.

Junio C Hamano· May 28, 2013, 17:04 UTC · re: Matthijs Kooijman · lore

Re: [PATCH] git clone depth of 0 not possible.

Matthijs Kooijman <matthijs@stdin.nl> writes:
Show 11 quoted lines
> Did you consider how to implement this? Looking at the code, it seems
> the "deepen" parameter in the wire protocol now means:
>  - 0: Do not change anything about the shallowness (i.e., fetch
>    everything from the shallow root to the tip).
>  - > 0: Create new shallow commits at depth commits below the tip (so
>    depth == 1 means tip and one below).
>  - INFINITE_DEPTH (0x7fffffff): Remove all shallowness and fetch
>    complete history.
>
> Given this, I'm not sure how one can express "fetch the tip and nothing
> below that", since depth == 0 already has a different meaning.
Doing it "correctly" (in the shorter term) would involve:
 - adding a capability on the sending side "fixed-off-by-one-depth"
   to the protocol, and teaching the sending side to advertise the
   capability;
   
 - teaching the requestor that got --depth=N from the end user to
   pay attention to the new capability in such a way that:
   - when talking to an old sender (i.e. without the off-by-one
     fix), send N-1 for N greater than 1.  Punt on N==1;
   - when talking to a fixed sender, ask to enable the capability,
     and send N as is (including N==1).
 - teaching the sending side to see if the new behaviour to fix
   off-by-one is asked by the requestor, and stop at the correct
   number of commits, not oversending one more.  Otherwise retain
   the old behaviour.

In the longer term, I think we should introduce a better deepening mechanism. Cf.

  http://thread.gmane.org/gmane.comp.version-control.git/212912/focus=212940
> Of course, one could using depth == 1 in this case to receive two
> commits and then drop one, but this would seem a bit pointless to me
> (especially if the commit below the tip is very different from the tip
> leading to a lot of useless data transfer).
Matthijs Kooijman· May 30, 2013, 08:23 UTC · re: Junio C Hamano · lore

Re: [PATCH] git clone depth of 0 not possible.

Hi Junio,
On Tue, May 28, 2013 at 10:04:46AM -0700, Junio C Hamano wrote:
Show 15 quoted lines
> Matthijs Kooijman <matthijs@stdin.nl> writes:
> 
> > Did you consider how to implement this? Looking at the code, it seems
> > the "deepen" parameter in the wire protocol now means:
> >  - 0: Do not change anything about the shallowness (i.e., fetch
> >    everything from the shallow root to the tip).
> >  - > 0: Create new shallow commits at depth commits below the tip (so
> >    depth == 1 means tip and one below).
> >  - INFINITE_DEPTH (0x7fffffff): Remove all shallowness and fetch
> >    complete history.
> >
> > Given this, I'm not sure how one can express "fetch the tip and nothing
> > below that", since depth == 0 already has a different meaning.
> 
> Doing it "correctly" (in the shorter term) would involve:

Given below suggestion, I take it you don't like what Jonathan proposed (changing the meaning of the deepen parameter in the protocol so that the server effectively decides how to interpret --depth)?

Show 8 quoted lines
>  - adding a capability on the sending side "fixed-off-by-one-depth"
>    to the protocol, and teaching the sending side to advertise the
>    capability;
>    
>  - teaching the sending side to see if the new behaviour to fix
>    off-by-one is asked by the requestor, and stop at the correct
>    number of commits, not oversending one more.  Otherwise retain
>    the old behaviour.

We can implement these two in current git already, since they only add to the protocol, not break it in an incompatible manner, right?

Show 8 quoted lines
>  - teaching the requestor that got --depth=N from the end user to
>    pay attention to the new capability in such a way that:
> 
>    - when talking to an old sender (i.e. without the off-by-one
>      fix), send N-1 for N greater than 1.  Punt on N==1;
> 
>    - when talking to a fixed sender, ask to enable the capability,
>      and send N as is (including N==1).

And these should wait for git2, since they change the meaning of the --depth parameter? Or is this change ok for current git as well?

What do you mean by "punt" exactly? Show an error to the user, saying only depth >= 2 is supported?

> In the longer term, I think we should introduce a better deepening
> mechanism.  Cf.

Even when there will be a better deepening mechanism, the above is still useful (passing --depth=1 serves to get just a single commit without history, which is a distinct usecase from deepening the history of an existing shallow repository). In other words, I think the "improved deepening" and "fixed depth" should be complementary features.

Gr.
Matthijs
Junio C Hamano· Jun 2, 2013, 19:14 UTC · re: Matthijs Kooijman · lore

Re: [PATCH] git clone depth of 0 not possible.

Matthijs Kooijman <matthijs@stdin.nl> writes:
Show 5 quoted lines
>> Doing it "correctly" (in the shorter term) would involve:
>
> Given below suggestion, I take it you don't like what Jonathan proposed
> (changing the meaning of the deepen parameter in the protocol so that
> the server effectively decides how to interpret --depth)?
Correct.
> We can implement these two in current git already, since they only
> add to the protocol, not break it in an incompatible manner, right?
Correct.
Show 10 quoted lines
>>  - teaching the requestor that got --depth=N from the end user to
>>    pay attention to the new capability in such a way that:
>> 
>>    - when talking to an old sender (i.e. without the off-by-one
>>      fix), send N-1 for N greater than 1.  Punt on N==1;
>> 
>>    - when talking to a fixed sender, ask to enable the capability,
>>      and send N as is (including N==1).
> And these should wait for git2, since they change the meaning of the
> --depth parameter? Or is this change ok for current git as well?

My suggestion was based on the understanding that everybody agreed that the current behaviour of --depth=1 to have one extra commit behind the shallow "snapshot" aka "poor-man's tarball", is a *bug* to be fixed, so I didn't mean it as a "backward incompatible change" at all.

> What do you mean by "punt" exactly?

As old senders can only send a history with 2 or more commits deep, it would be sensible for the receiver to warn the user that we are buggily asking for one more than the user asked for to the sender, and fetch history with two commits. It would be a regression to error it out.

Show 6 quoted lines
>> In the longer term, I think we should introduce a better deepening
>> mechanism.  Cf.
> Even when there will be a better deepening mechanism, the above is still
> useful (passing --depth=1 serves to get just a single commit without
> history, which is a distinct usecase from deepening the history of an
> existing shallow repository).

Correct. That is why I said "in the longer term, we should introduce". Did I say "introduce and replace with it"?

Matthijs Kooijman· Jul 9, 2013, 13:35 UTC · re: Junio C Hamano · lore

Re: [PATCH] git clone depth of 0 not possible.

Hi Junio,
Show 19 quoted lines
> Doing it "correctly" (in the shorter term) would involve:
> 
>  - adding a capability on the sending side "fixed-off-by-one-depth"
>    to the protocol, and teaching the sending side to advertise the
>    capability;
>    
>  - teaching the requestor that got --depth=N from the end user to
>    pay attention to the new capability in such a way that:
> 
>    - when talking to an old sender (i.e. without the off-by-one
>      fix), send N-1 for N greater than 1.  Punt on N==1;
> 
>    - when talking to a fixed sender, ask to enable the capability,
>      and send N as is (including N==1).
> 
>  - teaching the sending side to see if the new behaviour to fix
>    off-by-one is asked by the requestor, and stop at the correct
>    number of commits, not oversending one more.  Otherwise retain
>    the old behaviour.

While implementing the above, I noticed my fix now introduced an off-by-one error the other way. When investigating, I found this commit:

	commit 682c7d2f1a2d1a5443777237450505738af2ff1a
	Author: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
	Date:   Fri Jan 11 16:05:47 2013 +0700
	    upload-pack: fix off-by-one depth calculation in shallow clone
	    get_shallow_commits() is used to determine the cut points at a given
	    depth (i.e. the number of commits in a chain that the user likes to
	    get). However we count current depth up to the commit "commit" but we
	    do the cutting at its parents (i.e. current depth + 1). This makes
	    upload-pack always return one commit more than requested. This patch
	    fixes it.
	    Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
	    Signed-off-by: Junio C Hamano <gitster@pobox.com>

Which actually seems to fix the off-by-one bug that is described in this thread, but without going through the hoops of preserving current behaviour for older git versions (that is, it makes behaviour dependent on server version instead of client version).

Does this mean the discussion in this thread is meaningless, or is that commit not intended to be the final fix?

In any case, IIUC that particular patch makes a piece of the existing code dead, which needs to be removed.

Gr.
Matthijs
Matthijs Kooijman· Jul 11, 2013, 10:57 UTC · re: Matthijs Kooijman · lore

Re: [PATCH] git clone depth of 0 not possible.

Hi Junio,
Show 26 quoted lines
> While implementing the above, I noticed my fix now introduced an
> off-by-one error the other way. When investigating, I found this commit:
> 
> 	commit 682c7d2f1a2d1a5443777237450505738af2ff1a
> 	Author: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> 	Date:   Fri Jan 11 16:05:47 2013 +0700
> 
> 	    upload-pack: fix off-by-one depth calculation in shallow clone
> 
> 	    get_shallow_commits() is used to determine the cut points at a given
> 	    depth (i.e. the number of commits in a chain that the user likes to
> 	    get). However we count current depth up to the commit "commit" but we
> 	    do the cutting at its parents (i.e. current depth + 1). This makes
> 	    upload-pack always return one commit more than requested. This patch
> 	    fixes it.
> 
> 	    Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> 	    Signed-off-by: Junio C Hamano <gitster@pobox.com>
> 
> Which actually seems to fix the off-by-one bug that is described in this
> thread, but without going through the hoops of preserving current
> behaviour for older git versions (that is, it makes behaviour dependent
> on server version instead of client version).
> 
> Does this mean the discussion in this thread is meaningless, or is that
> commit not intended to be the final fix?

Looking more closely, I also see that the above change is already released in 1.8.2 versions. Given that, I don't think it makes sense to to still try to provide this capability to get backward compatible behaviour, since this would cause a off-by-one error the other way when talking to 1.8.2.x servers...

However, since I pretty much finished the code for this, I'll send over the patches and let you decide wether to include them or not. If you want to include them but they need to be changed in some way, just let me know.

The first patch of the series should be merged regardless.
Gr.
Matthijs
Matthijs Kooijman· Jul 11, 2013, 11:25 UTC · re: Matthijs Kooijman · lore

[PATCH 1/3] upload-pack: Remove a piece of dead code

Commit 682c7d2 (upload-pack: fix off-by-one depth calculation in shallow clone) introduced a new check in get_shallow_commits to decide when to stop traversing the history and mark the current commit as a shallow root.

With this new check in place, the old check can no longer be true, since the first check always fires first. This commit removes that check, making the code a bit more simple again.

Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
---
 shallow.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)
Show changes to shallow.c +6 −11
diff --git a/shallow.c b/shallow.c
index cbe2526..8a9c96d 100644
--- a/shallow.c
+++ b/shallow.c
@@ -110,17 +110,12 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
 					continue;
 				*pointer = cur_depth;
 			}
-			if (cur_depth < depth) {
-				if (p->next)
-					add_object_array(&p->item->object,
-							NULL, &stack);
-				else {
-					commit = p->item;
-					cur_depth = *(int *)commit->util;
-				}
-			} else {
-				commit_list_insert(p->item, &result);
-				p->item->object.flags |= shallow_flag;
+			if (p->next)
+				add_object_array(&p->item->object,
+						NULL, &stack);
+			else {
+				commit = p->item;
+				cur_depth = *(int *)commit->util;
 			}
 		}
 	}

-- 
1.8.3.rc1
Matthijs Kooijman· Jul 11, 2013, 11:25 UTC · re: Matthijs Kooijman · lore

[PATCH 2/3] upload-pack: Introduce new "fixed-off-by-one-depth" server feature

Commit 682c7d2 (upload-pack: fix off-by-one depth calculation in shallow clone) changed the meaning of the fetch depth sent over the wire to mean the total number of commits to return, instead of the number of commits beyond the first. However, when this change is deployed on some servers but not others, this can cause a client to behave differently based on the server version, which is unexpected.

To prevent this, the new, fixed, depth behaviour is advertised as a server feature and the old behaviour is restored when the feature is not requested by the client.

Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
---
 upload-pack.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
Show changes to upload-pack.c +9 −2
diff --git a/upload-pack.c b/upload-pack.c
index 127e59a..59f43d1 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -46,6 +46,7 @@ static unsigned int timeout;
 static int use_sideband;
 static int advertise_refs;
 static int stateless_rpc;
+static int fixed_depth;
 
 static void reset_timeout(void)
 {
@@ -633,6 +634,8 @@ static void receive_needs(void)
 			no_progress = 1;
 		if (parse_feature_request(features, "include-tag"))
 			use_include_tag = 1;
+		if (parse_feature_request(features, "fixed-off-by-one-depth"))
+			fixed_depth = 1;
 
 		o = parse_object(sha1_buf);
 		if (!o)
@@ -669,10 +672,14 @@ static void receive_needs(void)
 				struct object *object = shallows.objects[i].item;
 				object->flags |= NOT_SHALLOW;
 			}
-		else
+		else {
+			/* Emulate off-by-one bug in older versions */
+			if (!fixed_depth)
+				depth++;
 			backup = result =
 				get_shallow_commits(&want_obj, depth,
 						    SHALLOW, NOT_SHALLOW);
+		}
 		while (result) {
 			struct object *object = &result->item->object;
 			if (!(object->flags & (CLIENT_SHALLOW|NOT_SHALLOW))) {
@@ -738,7 +745,7 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
 {
 	static const char *capabilities = "multi_ack thin-pack side-band"
 		" side-band-64k ofs-delta shallow no-progress"
-		" include-tag multi_ack_detailed";
+		" include-tag multi_ack_detailed fixed-off-by-one-depth";
 	const char *refname_nons = strip_namespace(refname);
 	unsigned char peeled[20];
 

-- 
1.8.3.rc1
Matthijs Kooijman· Jul 11, 2013, 11:25 UTC · re: Matthijs Kooijman · lore

[PATCH 3/3] fetch-pack: Request fixed-off-by-one-depth when available

This server feature changes the meaning of the fetch depth, allowing fetching only a single revision instead of at least two as before. To make sure the behaviour only depends on the client version, the depth value sent over the wire is corrected depending on wether the server has the fix.

There is one corner case: A server without the fix cannot send less than 2 commmits, so when --depth=1 is specified a warning is shown and 2 commits are fetched instead of 1.

Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
---
 fetch-pack.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)
Show changes to fetch-pack.c +24 −2
diff --git a/fetch-pack.c b/fetch-pack.c
index abe5ffb..799b2c1 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -39,6 +39,7 @@ static int marked;
 
 static struct commit_list *rev_list;
 static int non_common_revs, multi_ack, use_sideband, allow_tip_sha1_in_want;
+static int fixed_depth;
 
 static void rev_list_push(struct commit *commit, int mark)
 {
@@ -327,6 +328,7 @@ static int find_common(struct fetch_pack_args *args,
 			if (prefer_ofs_delta)   strbuf_addstr(&c, " ofs-delta");
 			if (agent_supported)    strbuf_addf(&c, " agent=%s",
 							    git_user_agent_sanitized());
+			if (fixed_depth)        strbuf_addstr(&c, " fixed-off-by-one-depth");
 			packet_buf_write(&req_buf, "want %s%s\n", remote_hex, c.buf);
 			strbuf_release(&c);
 		} else
@@ -342,8 +344,23 @@ static int find_common(struct fetch_pack_args *args,
 
 	if (is_repository_shallow())
 		write_shallow_commits(&req_buf, 1);
-	if (args->depth > 0)
-		packet_buf_write(&req_buf, "deepen %d", args->depth);
+	if (args->depth > 0) {
+		if (!fixed_depth && args->depth == 1)
+			warning("Server does not support depth=1, using depth=2 instead");
+		if (!fixed_depth && args->depth > 1) {
+			/* Old server that interprets "deepen 1" as
+			   "give me tip + 1 extra commit" */
+			packet_buf_write(&req_buf, "deepen %d", args->depth - 1);
+		} else if (!fixed_depth && args->depth == 1) {
+			/* Old servers cannot handle depth=1 (deepen=0
+			   means don't change depth / full depth). */
+			packet_buf_write(&req_buf, "deepen 1");
+		} else {
+			/* New server, send depth as-is */
+			packet_buf_write(&req_buf, "deepen %d", args->depth);
+		}
+	}
+
 	packet_buf_flush(&req_buf);
 	state_len = req_buf.len;
 
@@ -874,6 +891,11 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 			fprintf(stderr, "Server supports ofs-delta\n");
 	} else
 		prefer_ofs_delta = 0;
+	if (server_supports("fixed-off-by-one-depth")) {
+		if (args->verbose)
+			fprintf(stderr, "Server has fixed meaning of depth value\n");
+		fixed_depth = 1;
+	}
 
 	if ((agent_feature = server_feature_value("agent", &agent_len))) {
 		agent_supported = 1;

-- 
1.8.3.rc1
Duy Nguyen· Jul 11, 2013, 12:08 UTC · re: Matthijs Kooijman · lore

Re: [PATCH 1/3] upload-pack: Remove a piece of dead code

On Thu, Jul 11, 2013 at 6:25 PM, Matthijs Kooijman <matthijs@stdin.nl> wrote:
Show 8 quoted lines
> Commit 682c7d2 (upload-pack: fix off-by-one depth calculation in shallow
> clone) introduced a new check in get_shallow_commits to decide when to
> stop traversing the history and mark the current commit as a shallow
> root.
>
> With this new check in place, the old check can no longer be true, since
> the first check always fires first. This commit removes that check,
> making the code a bit more simple again.
True. Ack-by: me.
Show 36 quoted lines
> Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
> ---
>  shallow.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/shallow.c b/shallow.c
> index cbe2526..8a9c96d 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -110,17 +110,12 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
>                                         continue;
>                                 *pointer = cur_depth;
>                         }
> -                       if (cur_depth < depth) {
> -                               if (p->next)
> -                                       add_object_array(&p->item->object,
> -                                                       NULL, &stack);
> -                               else {
> -                                       commit = p->item;
> -                                       cur_depth = *(int *)commit->util;
> -                               }
> -                       } else {
> -                               commit_list_insert(p->item, &result);
> -                               p->item->object.flags |= shallow_flag;
> +                       if (p->next)
> +                               add_object_array(&p->item->object,
> +                                               NULL, &stack);
> +                       else {
> +                               commit = p->item;
> +                               cur_depth = *(int *)commit->util;
>                         }
>                 }
>         }
> --
> 1.8.3.rc1
>

-- Duy

Junio C Hamano· Jul 11, 2013, 15:49 UTC · re: Duy Nguyen · lore

Re: [PATCH 1/3] upload-pack: Remove a piece of dead code

Duy Nguyen <pclouds@gmail.com> writes:
Show 13 quoted lines
> On Thu, Jul 11, 2013 at 6:25 PM, Matthijs Kooijman <matthijs@stdin.nl> wrote:
>> Commit 682c7d2 (upload-pack: fix off-by-one depth calculation in shallow
>> clone) introduced a new check in get_shallow_commits to decide when to
>> stop traversing the history and mark the current commit as a shallow
>> root.
>>
>> With this new check in place, the old check can no longer be true, since
>> the first check always fires first. This commit removes that check,
>> making the code a bit more simple again.
>
> True. Ack-by: me.
>
>> Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>

Yeah, thanks both. I tend to agree that 2 and 3 are the right change that came too late after the ship sailed X-(.

Show 37 quoted lines
>> ---
>>  shallow.c | 17 ++++++-----------
>>  1 file changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/shallow.c b/shallow.c
>> index cbe2526..8a9c96d 100644
>> --- a/shallow.c
>> +++ b/shallow.c
>> @@ -110,17 +110,12 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
>>                                         continue;
>>                                 *pointer = cur_depth;
>>                         }
>> -                       if (cur_depth < depth) {
>> -                               if (p->next)
>> -                                       add_object_array(&p->item->object,
>> -                                                       NULL, &stack);
>> -                               else {
>> -                                       commit = p->item;
>> -                                       cur_depth = *(int *)commit->util;
>> -                               }
>> -                       } else {
>> -                               commit_list_insert(p->item, &result);
>> -                               p->item->object.flags |= shallow_flag;
>> +                       if (p->next)
>> +                               add_object_array(&p->item->object,
>> +                                               NULL, &stack);
>> +                       else {
>> +                               commit = p->item;
>> +                               cur_depth = *(int *)commit->util;
>>                         }
>>                 }
>>         }
>> --
>> 1.8.3.rc1
>>
> --
> Duy
Duy Nguyen· Jan 8, 2013, 07:33 UTC · re: Stefan Beller · lore

Re: [PATCH] git clone depth of 0 not possible.

On Tue, Jan 8, 2013 at 1:06 AM, Stefan Beller <stefanbeller@googlemail.com> wrote:

Show 11 quoted lines
> Currently it is not possible to have a shallow depth of
> just 0, i.e. only one commit in that repository after cloning.
> The minimum number of commits is 2, caused by depth=1.
>
> I had no good idea how to add this behavior to git clone as
> the depth variable in git_transport_options struct (file transport.h)
> uses value 0 for another meaning, so it would have need changes at
> all places, where the transport options depth is being used
> (e.g. fetch)
>
> So I documented the current behavior, see attached patch.

If we choose not to do the off-by-one topic Junio suggested elsewhere in the same thread, I think this document patch should be turned into code instead. Just reject --depth=0 with an explanation. Users who are hit by this will be caught without the need to read through the document.

-- 
Duy
Junio C Hamano· Jan 8, 2013, 07:37 UTC · re: Duy Nguyen · lore

Re: [PATCH] git clone depth of 0 not possible.

Duy Nguyen <pclouds@gmail.com> writes:
Show 5 quoted lines
> If we choose not to do the off-by-one topic Junio suggested elsewhere
> in the same thread, I think this document patch should be turned into
> code instead. Just reject --depth=0 with an explanation. Users who are
> hit by this will be caught without the need to read through the
> document.

I thought --depth=0 was a way to explicitly say "I do not want any shallow history" so far, so we would need to be a bit more careful, though.

← back to recent threads