[PATCH] clone: ignore --depth when cloning locally (implicitly --local)

[PATCH] clone: ignore --depth when cloning locally (implicitly --local)

[Johannes Schindelin]

When cloning locally, we default to --local, as it makes the whole
operation fast and efficient.

As the most common intent of cloning with a --depth parameter is to
save space, and --local saves space more than --depth ever can,
warn the user and ignore the --depth parameter when cloning locally.

Should --depth be desired, the user can always force proper cloning
by using a file:// URL.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	Before making git-clone a builtin, we had code to force a clone 
	when a --depth parameter was passed.  However, it is dubitable 
	that a user wants --depth rather than --local, and as in the 
	--local case, we should cater for the common case.

 Documentation/git-clone.txt |    4 ++++
 builtin-clone.c             |    5 ++++-
 t/t5701-clone-local.sh      |    7 +++++++
 3 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 95f08b9..9b8b389 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -138,6 +138,10 @@ then the cloned repository will become corrupt.
 	are only interested in the recent history of a large project
 	with a long history, and would want to send in fixes
 	as patches.
++
+This option is ignored when cloning locally; to force a shallow
+clone even locally, use the `--no-hardlinks` option, or a
+'file://' location.
 
 <repository>::
 	The (possibly remote) repository to clone from.  See the
diff --git a/builtin-clone.c b/builtin-clone.c
index c338910..5831034 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -511,8 +511,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	refspec.src = src_ref_prefix;
 	refspec.dst = branch_top.buf;
 
-	if (path && !is_bundle)
+	if (path && !is_bundle) {
+		if (option_depth)
+			warning("Ignoring --depth for local clone");
 		refs = clone_local(path, git_dir);
+	}
 	else {
 		struct remote *remote = remote_get(argv[0]);
 		transport = transport_get(remote, remote->url[0]);
diff --git a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh
index 3559d17..2938c02 100755
--- a/t/t5701-clone-local.sh
+++ b/t/t5701-clone-local.sh
@@ -132,4 +132,11 @@ test_expect_success 'clone empty repository' '
 	 test $actual = $expected)
 '
 
+test_expect_success 'clone --depth locally ignores --depth' '
+	test_commit meredith chivers &&
+	git clone --depth 1 . depth 2> out.err &&
+	grep "warning: Ignoring --depth for local clone" out.err &&
+	test 1 -lt $(cd depth && git rev-list master | wc -l)
+'
+
 test_done
-- 
1.6.2.rc1.350.g6caf6

Re: [PATCH] clone: ignore --depth when cloning locally (implicitly --local)

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> When cloning locally, we default to --local, as it makes the whole
> operation fast and efficient.
>
> As the most common intent of cloning with a --depth parameter is to
> save space, and --local saves space more than --depth ever can,
> warn the user and ignore the --depth parameter when cloning locally.
>
> Should --depth be desired, the user can always force proper cloning
> by using a file:// URL.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Makes sense.  I'm inclined to apply this as a "fix" before 1.6.2, but I
cannot exactly say what we are fixing.  User's expectations?

Re: [PATCH] clone: ignore --depth when cloning locally (implicitly --local)

[Johannes Schindelin]

Hi,

On Thu, 26 Feb 2009, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > When cloning locally, we default to --local, as it makes the whole
> > operation fast and efficient.
> >
> > As the most common intent of cloning with a --depth parameter is to
> > save space, and --local saves space more than --depth ever can,
> > warn the user and ignore the --depth parameter when cloning locally.
> >
> > Should --depth be desired, the user can always force proper cloning
> > by using a file:// URL.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> Makes sense.  I'm inclined to apply this as a "fix" before 1.6.2, but I
> cannot exactly say what we are fixing.  User's expectations?

It seems somebody mentioned on IRC that --depth with --no-local will not 
work as expected, but was not willing to write to the list, or for that 
matter, do anything else.

So given that level of enthusiasm for _supporting_ this feature, I am 
inclined to postpone the patch, or drop it altogether.

I mean, I tried to be a good netizen and not let a patch I started go 
stale, but it is not my itch, and if those whose itch it is do not care, 
maybe I shouldn't, either.

Ciao,
Dscho

[PATCH 0/2] shallow clone stuff

[Johannes Schindelin]

The first patch fixes the current bug that --no-hardlinks is not heeded, 
and the second patch introduces the warning when cloning locally with a 
--depth parameter.

Johannes Schindelin (2):
  clone: do not ignore --no-hardlinks
  clone: ignore --depth when cloning locally (implicitly --local)

 Documentation/git-clone.txt |    4 ++++
 builtin-clone.c             |    5 ++++-
 t/t5701-clone-local.sh      |   14 ++++++++++++++
 3 files changed, 22 insertions(+), 1 deletions(-)

[PATCH 1/2] clone: do not ignore --no-hardlinks

[Johannes Schindelin]

Somehow --no-hardlinks got broken by making clone a builtin.  This
was discovered during my work on --depth being ignored for local
clones.

There will be a test case that tests for --no-hardlinks in conjunction
with --depth, so this patch is not accompanied by a separate test.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin-clone.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index c338910..5540372 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -511,7 +511,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	refspec.src = src_ref_prefix;
 	refspec.dst = branch_top.buf;
 
-	if (path && !is_bundle)
+	if (path && !is_bundle && use_local_hardlinks)
 		refs = clone_local(path, git_dir);
 	else {
 		struct remote *remote = remote_get(argv[0]);
-- 
1.6.2.rc2.404.g8394e

[PATCH 2/2] clone: ignore --depth when cloning locally (implicitly --local)

[Johannes Schindelin]

When cloning locally, we default to --local, as it makes the whole
operation fast and efficient.

As the most common intent of cloning with a --depth parameter is to
save space, and --local saves more space than --depth ever can,
warn the user and ignore the --depth parameter when cloning locally.

Should --depth be desired, the user can always force proper cloning
by passing the --no-hardlinks parameter, or by using a file:// URL.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-clone.txt |    4 ++++
 builtin-clone.c             |    5 ++++-
 t/t5701-clone-local.sh      |   14 ++++++++++++++
 3 files changed, 22 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 95f08b9..9b8b389 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -138,6 +138,10 @@ then the cloned repository will become corrupt.
 	are only interested in the recent history of a large project
 	with a long history, and would want to send in fixes
 	as patches.
++
+This option is ignored when cloning locally; to force a shallow
+clone even locally, use the `--no-hardlinks` option, or a
+'file://' location.
 
 <repository>::
 	The (possibly remote) repository to clone from.  See the
diff --git a/builtin-clone.c b/builtin-clone.c
index 5540372..73d5a76 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -511,8 +511,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	refspec.src = src_ref_prefix;
 	refspec.dst = branch_top.buf;
 
-	if (path && !is_bundle && use_local_hardlinks)
+	if (path && !is_bundle && use_local_hardlinks) {
+		if (option_depth)
+			warning("Ignoring --depth for local clone");
 		refs = clone_local(path, git_dir);
+	}
 	else {
 		struct remote *remote = remote_get(argv[0]);
 		transport = transport_get(remote, remote->url[0]);
diff --git a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh
index 3559d17..8600539 100755
--- a/t/t5701-clone-local.sh
+++ b/t/t5701-clone-local.sh
@@ -132,4 +132,18 @@ test_expect_success 'clone empty repository' '
 	 test $actual = $expected)
 '
 
+test_expect_success 'clone --depth locally ignores --depth' '
+	test_commit meredith chivers && git tag -d meredith &&
+	test_commit lisa diamond && git tag -d lisa &&
+	git clone --depth 1 . depth 2> out.err &&
+	grep "warning: Ignoring --depth for local clone" out.err &&
+	test 2 -lt $(cd depth && git rev-list master | wc -l)
+'
+
+test_expect_success '--depth is not ignored with --no-hardlinks' '
+	git clone --depth 1 --no-hardlinks . depth2 2> out.err &&
+	! grep "warning: Ignoring --depth for local clone" out.err &&
+	test 2 = $(cd depth2 && git rev-list master | wc -l)
+'
+
 test_done
-- 
1.6.2.rc2.404.g8394e

Re: [PATCH 0/2] shallow clone stuff

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> The first patch fixes the current bug that --no-hardlinks is not heeded, 
> and the second patch introduces the warning when cloning locally with a 
> --depth parameter.

Thanks.

Re: [PATCH 1/2] clone: do not ignore --no-hardlinks

[Jeff King]

On Fri, Feb 27, 2009 at 12:31:22AM +0100, Johannes Schindelin wrote:

> Somehow --no-hardlinks got broken by making clone a builtin.  This
> was discovered during my work on --depth being ignored for local
> clones.
> 
> There will be a test case that tests for --no-hardlinks in conjunction
> with --depth, so this patch is not accompanied by a separate test.

Hmm. But --no-hardlinks has an effect later, in copy_or_link_directory,
making it just do a copy. So it _does_ work, just not in the way you
expect.

I think to turn off local shortcuts entirely, --no-local would probably
make more sense. IOW, something like this:

---
diff --git a/builtin-clone.c b/builtin-clone.c
index c338910..7c6f59f 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -35,7 +35,8 @@ static const char * const builtin_clone_usage[] = {
 };
 
 static int option_quiet, option_no_checkout, option_bare, option_mirror;
-static int option_local, option_no_hardlinks, option_shared;
+static int option_local = -1;
+static int option_no_hardlinks, option_shared;
 static char *option_template, *option_reference, *option_depth;
 static char *option_origin = NULL;
 static char *option_upload_pack = "git-upload-pack";
@@ -229,7 +230,7 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest)
 		if (!option_no_hardlinks) {
 			if (!link(src->buf, dest->buf))
 				continue;
-			if (option_local)
+			if (option_local == 1)
 				die("failed to create link %s", dest->buf);
 			option_no_hardlinks = 1;
 		}
@@ -511,7 +512,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	refspec.src = src_ref_prefix;
 	refspec.dst = branch_top.buf;
 
-	if (path && !is_bundle)
+	if (option_local != 0 && path && !is_bundle)
 		refs = clone_local(path, git_dir);
 	else {
 		struct remote *remote = remote_get(argv[0]);

Re: [PATCH 1/2] clone: do not ignore --no-hardlinks

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Feb 27, 2009 at 12:31:22AM +0100, Johannes Schindelin wrote:
>
>> Somehow --no-hardlinks got broken by making clone a builtin.  This
>> was discovered during my work on --depth being ignored for local
>> clones.
>> 
>> There will be a test case that tests for --no-hardlinks in conjunction
>> with --depth, so this patch is not accompanied by a separate test.
>
> Hmm. But --no-hardlinks has an effect later, in copy_or_link_directory,
> making it just do a copy. So it _does_ work, just not in the way you
> expect.

Yup, I agree.

> I think to turn off local shortcuts entirely, --no-local would probably
> make more sense. IOW, something like this:

So across filesystems:

 - "git clone /p/a/t/h" falls back to copying;

 - "git clone --local /p/a/t/h" should fail without falling back to
   copying; and

 - "git clone --no-local /p/a/t/h" should work as if file:///p/a/t/h
   was given.

That is much more sensible than making "git clone --no-hardlinks /p/a/t/h"
imply more than what the option really means: we are making a local copy
but do not cheat with hardlinking.

Re: [PATCH 1/2] clone: do not ignore --no-hardlinks

[Junio C Hamano]

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

> Jeff King <peff@peff.net> writes:
>
> So across filesystems:
>
>  - "git clone /p/a/t/h" falls back to copying;
>
>  - "git clone --local /p/a/t/h" should fail without falling back to
>    copying; and
>
>  - "git clone --no-local /p/a/t/h" should work as if file:///p/a/t/h
>    was given.
>
> That is much more sensible than making "git clone --no-hardlinks /p/a/t/h"
> imply more than what the option really means: we are making a local copy
> but do not cheat with hardlinking.

I liked it so much that I wrote a commit message for it:

    commit c255d7bbf4dd567ca60a8991c3ac052c9b2d1593
    Author: Jeff King <peff@peff.net>
    Date:   Thu Feb 26 23:31:49 2009 -0800

        clone: do not ignore --no-local option

        With a variable initialized to false, we couldn't tell if the user didn't
        give --local or if the user explicitly told us --no-local after we get control
        back from parse_options().

        This fixes and makes "git clone --no-local /p/a/t/h" always use non-local
        transport (aka "git native protocol over pipe").

        Signed-off-by: Junio C Hamano <gitster@pobox.com>

Except that it does not work.

Initializing option_local to -1 and then feeding it to parse_options() via
OPT_BOOLEAN() is a cute idea, but when parse_options() sees a command line
like this:

	$ git clone -l /p/a/t/h

It triggers this codepath in get_value():

	case OPTION_BOOLEAN:
		*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
		return 0;

and ends up incrementing it to zero.

I wonder what would break if we simply change this to:

	case OPTION_BOOLEAN:
		*(int *)opt->value = !unset;
		return 0;

Damn it, it is called BOOLEAN, and naïvely I would think --option would
set it to 1, --no-option would set it to 0, and not having --option nor
--no-option would leave the associated variable alone, but apparently that
is not what is happening.

Pierre, do you remember why this code is implemented this way?  The
"increment if we have --option" semantics seems to date back to 4a59fd1
(Add a simple option parser., 2007-10-15) which is the day one of the
history of parse-options.

Re: [PATCH 1/2] clone: do not ignore --no-hardlinks

[Junio C Hamano]

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

> It triggers this codepath in get_value():
>
> 	case OPTION_BOOLEAN:
> 		*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
> 		return 0;
>
> and ends up incrementing it to zero.
>
> I wonder what would break if we simply change this to:
>
> 	case OPTION_BOOLEAN:
> 		*(int *)opt->value = !unset;
> 		return 0;
>
> Damn it, it is called BOOLEAN, and naïvely I would think --option would
> set it to 1, --no-option would set it to 0, and not having --option nor
> --no-option would leave the associated variable alone, but apparently that
> is not what is happening.
>
> Pierre, do you remember why this code is implemented this way?  The
> "increment if we have --option" semantics seems to date back to 4a59fd1
> (Add a simple option parser., 2007-10-15) which is the day one of the
> history of parse-options.

I think that this came from a misguided attempt to do:

	-v
        -v -v
        -v -v -v

to cumulatively record the desired verbosity levels.

Originally we did not have OPT__VERBOSITY() nor OPT_VERBOSE() so it is
understandable that OPT_VERBOSE() was implemented in terms of this
OPT_BOOLEAN() construct.

I think all parse_options() users that do support the verbosity level is
either using OPT__VERBOSE() or OPT_VERBOSITY() these days, and we could
probably fix this by doing something like:

 (1) Introduce OPTION_CUMULATIVE_LEVEL whose behaviour is "--no-option to
     reset to zero, --option to raise by one", and fix OPTION_BOOLEAN to
     "--no-option to zero, --option to one":

	case OPTION_BOOLEAN:
 		*(int *)opt->value = !unset;
		return 0;
	case OPTION_CUMULATIVE_LEVEL:
 		*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
 		return 0;

 (2) Use OPT_CUMULATIVE_LEVEL() instead of OPT_BOOLEAN() to implement
     OPT__VERBOSE() and OPT__QUIET().

But I am inclined to postpone such a change til after 1.6.2.

Re: [PATCH 1/2] clone: do not ignore --no-hardlinks

[Jeff King]

On Thu, Feb 26, 2009 at 11:59:14PM -0800, Junio C Hamano wrote:

> Except that it does not work.
> 
> Initializing option_local to -1 and then feeding it to parse_options() via
> OPT_BOOLEAN() is a cute idea, but when parse_options() sees a command line
> like this:

Ah, sorry. I didn't really test it thoroughly (though I did try
--no-local, I obviously did not test --local with a hardlink failure).

> Damn it, it is called BOOLEAN, and naïvely I would think --option would
> set it to 1, --no-option would set it to 0, and not having --option nor
> --no-option would leave the associated variable alone, but apparently that
> is not what is happening.

That is exactly what I expected as a caller (and it sounds like you did,
too), which perhaps argues in favor of changing it.

Upon seeing this, I guessed (as you did in the next email) that it was
about bumping up levels of a boolean for things like verbosity. And I
agree that sould not be OPT_BOOLEAN, but rather OPT_INCREMENT or
whatever.

However, I did some grepping and it doesn't look like anybody is
actually making use of the incrementing behavior (I was just looking for
"verbose", though, so it's possible some unrelated boolean is using it).
So it may be that the behavior can be changed now while no callers care,
and we can get away without OPT_INCREMENT until somebody actually needs
it.

-Peff

Re: [PATCH 1/2] clone: do not ignore --no-hardlinks

[Daniel Barkalow]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2330 bytes --]

On Fri, 27 Feb 2009, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > It triggers this codepath in get_value():
> >
> > 	case OPTION_BOOLEAN:
> > 		*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
> > 		return 0;
> >
> > and ends up incrementing it to zero.
> >
> > I wonder what would break if we simply change this to:
> >
> > 	case OPTION_BOOLEAN:
> > 		*(int *)opt->value = !unset;
> > 		return 0;
> >
> > Damn it, it is called BOOLEAN, and naïvely I would think --option would
> > set it to 1, --no-option would set it to 0, and not having --option nor
> > --no-option would leave the associated variable alone, but apparently that
> > is not what is happening.
> >
> > Pierre, do you remember why this code is implemented this way?  The
> > "increment if we have --option" semantics seems to date back to 4a59fd1
> > (Add a simple option parser., 2007-10-15) which is the day one of the
> > history of parse-options.
> 
> I think that this came from a misguided attempt to do:
> 
> 	-v
>         -v -v
>         -v -v -v
> 
> to cumulatively record the desired verbosity levels.
> 
> Originally we did not have OPT__VERBOSITY() nor OPT_VERBOSE() so it is
> understandable that OPT_VERBOSE() was implemented in terms of this
> OPT_BOOLEAN() construct.
> 
> I think all parse_options() users that do support the verbosity level is
> either using OPT__VERBOSE() or OPT_VERBOSITY() these days, and we could
> probably fix this by doing something like:
> 
>  (1) Introduce OPTION_CUMULATIVE_LEVEL whose behaviour is "--no-option to
>      reset to zero, --option to raise by one", and fix OPTION_BOOLEAN to
>      "--no-option to zero, --option to one":
> 
> 	case OPTION_BOOLEAN:
>  		*(int *)opt->value = !unset;
> 		return 0;
> 	case OPTION_CUMULATIVE_LEVEL:
>  		*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
>  		return 0;
> 
>  (2) Use OPT_CUMULATIVE_LEVEL() instead of OPT_BOOLEAN() to implement
>      OPT__VERBOSE() and OPT__QUIET().

I think there are a few other options that work like that, such as -C for 
diff and the like; I don't know if there are any others that go through 
parse_options, but I wouldn't count on there not being any. So I think the 
conversion of OPT_BOOLEAN needs to go with an audit of current users.

	-Daniel
*This .sig left intentionally blank*