patchclone: ignore --depth when cloning locally (implicitly --local)

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

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

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?

Hi,

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

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

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

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

Thanks.

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

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

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:

---

Jeff King <peff@peff.net> writes:

Yup, I agree.

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.

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

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.

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

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.

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

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*

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

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

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

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