[PATCH] git clone depth of 0 not possible.

[PATCH] git clone depth of 0 not possible.

[Stefan Beller]

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

[PATCH] Documentation on depth option in git clone.

[Stefan Beller]

---
 Documentation/git-clone.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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

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

[Jonathan Nieder]

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"?

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

[Junio C Hamano]

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 ;-).

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

[Duy Nguyen]

On Tue, Jan 8, 2013 at 1:06 AM, Stefan Beller
<stefanbeller@googlemail.com> 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.
>
> 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

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

[Junio C Hamano]

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

> 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.

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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> 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.

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

[Duy Nguyen]

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.

[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

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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> 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.

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

[Junio C Hamano]

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

> 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

>  (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.

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

[Duy Nguyen]

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

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

[Stefan Beller]

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?

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

[Duy Nguyen]

On Tue, Jan 8, 2013 at 9:32 PM, Stefan Beller
<stefanbeller@googlemail.com> wrote:
> 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

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

[Junio C Hamano]

Stefan Beller <stefanbeller@googlemail.com> writes:

> 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.

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

[Matthijs Kooijman]

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.

> >>> 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

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

[Jonathan Nieder]

Matthijs Kooijman wrote:

> 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

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

[Jonathan Nieder]

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

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

[Matthijs Kooijman]

Hi Jonathan,

> > 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

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

[Jonathan Nieder]

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.

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

[Junio C Hamano]

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:

 - 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).

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

[Matthijs Kooijman]

Hi Junio,

On Tue, May 28, 2013 at 10:04:46AM -0700, Junio C Hamano wrote:
> 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)?

>  - 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?

>  - 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

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

[Junio C Hamano]

Matthijs Kooijman <matthijs@stdin.nl> writes:

>> 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.

>>  - 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.


>> 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"?

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

[Matthijs Kooijman]

Hi Junio,

> 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

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

[Matthijs Kooijman]

Hi Junio,

> 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

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

[Matthijs Kooijman]

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(-)

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

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

[Matthijs Kooijman]

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(-)

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

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

[Matthijs Kooijman]

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(-)

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

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

[Duy Nguyen]

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>
> ---
>  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

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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> 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-(.

>> ---
>>  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