threads / patch / 30627

patch, 3 partsclone --local fixes

Subject: [PATCH 0/3] clone --local fixes

## tl;dr

19 messages between May 26, 2012 and May 30, 2012. Diffs are folded; open one to read it.

replies: 18people: 3as markdown or json

Jeff King· May 26, 2012, 03:42 UTC · lore
Emeric mentioned that he was confused by the behavior of
  git clone --local file:///path/to/repo

in that the --local is silently ignored. Looking over the documentation, it is quite unclear whether it is supposed to do anything or not. I don't think it's an especially pressing use case, so one solution could be to simply clarify the current behavior in the documentation.

However, I figured this should just be a two-line fix to give it more sensible behavior, so why not just make it to do the sensible thing? And while my initial version was two lines, it ended up growing to handle all of the corner cases properly. So I'm not super-excited about patch 2, but given that it is written, I think it's a reasonable thing to do (and it is not _too_ complex).

And then while I was on the subject, I did patch 3, which is something that has bugged me for a long time.

  [1/3]: t5701: modernize style
  [2/3]: clone: make --local handle URLs
  [3/3]: clone: allow --no-local to turn off local optimizations
-Peff
Jeff King· May 26, 2012, 03:42 UTC · re: Jeff King · lore

[PATCH 1/3] t5701: modernize style

This test is pretty old and did not follow some of our more modern best practices. In particular:

  1. It chdir'd all over the place, leaving later tests to
     deal with the fallout. Do our chdirs in subshells
     instead.
  2. It did not use test_must_fail.
  3. It did not use test_line_count.
  4. It checked for the non-existence of a ref by looking in the
     .git/refs directory (since we pack refs during clone
     these days, this will always be succeed, making the
     test useless).
     Note that one call to "-e .git/refs/..." remains,
     because it is checking for the existence of a symbolic
     ref, not a ref itself.
Signed-off-by: Jeff King <peff@peff.net>
---
I mostly wanted to pull out the repo_is_hardlinked logic for tests I was
about to add, but once I got cleaning, I couldn't stop. And who can
argue with that diffstat?
 t/t5701-clone-local.sh | 73 ++++++++++++++------------------------------------
 1 file changed, 20 insertions(+), 53 deletions(-)
Show changes to t/t5701-clone-local.sh +20 −53
diff --git a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh
index 6972258..c6feca4 100755
--- a/t/t5701-clone-local.sh
+++ b/t/t5701-clone-local.sh
@@ -3,7 +3,10 @@
 test_description='test local clone'
 . ./test-lib.sh
 
-D=`pwd`
+repo_is_hardlinked() {
+	find "$1/objects" -type f -links 1 >output &&
+	test_line_count = 0 output
+}
 
 test_expect_success 'preparing origin repository' '
 	: >file && git add . && git commit -m1 &&
@@ -19,105 +22,72 @@ test_expect_success 'preparing origin repository' '
 '
 
 test_expect_success 'local clone without .git suffix' '
-	cd "$D" &&
 	git clone -l -s a b &&
-	cd b &&
+	(cd b &&
 	test "$(GIT_CONFIG=.git/config git config --bool core.bare)" = false &&
-	git fetch
+	git fetch)
 '
 
 test_expect_success 'local clone with .git suffix' '
-	cd "$D" &&
 	git clone -l -s a.git c &&
-	cd c &&
-	git fetch
+	(cd c && git fetch)
 '
 
 test_expect_success 'local clone from x' '
-	cd "$D" &&
 	git clone -l -s x y &&
-	cd y &&
-	git fetch
+	(cd y && git fetch)
 '
 
 test_expect_success 'local clone from x.git that does not exist' '
-	cd "$D" &&
-	if git clone -l -s x.git z
-	then
-		echo "Oops, should have failed"
-		false
-	else
-		echo happy
-	fi
+	test_must_fail git clone -l -s x.git z
 '
 
 test_expect_success 'With -no-hardlinks, local will make a copy' '
-	cd "$D" &&
 	git clone --bare --no-hardlinks x w &&
-	cd w &&
-	linked=$(find objects -type f ! -links 1 | wc -l) &&
-	test 0 = $linked
+	! repo_is_hardlinked w
 '
 
 test_expect_success 'Even without -l, local will make a hardlink' '
-	cd "$D" &&
 	rm -fr w &&
 	git clone -l --bare x w &&
-	cd w &&
-	copied=$(find objects -type f -links 1 | wc -l) &&
-	test 0 = $copied
+	repo_is_hardlinked w
 '
 
 test_expect_success 'local clone of repo with nonexistent ref in HEAD' '
-	cd "$D" &&
 	echo "ref: refs/heads/nonexistent" > a.git/HEAD &&
 	git clone a d &&
-	cd d &&
+	(cd d &&
 	git fetch &&
-	test ! -e .git/refs/remotes/origin/HEAD'
+	test ! -e .git/refs/remotes/origin/HEAD)
+'
 
 test_expect_success 'bundle clone without .bundle suffix' '
-	cd "$D" &&
 	git clone dir/b3 &&
-	cd b3 &&
-	git fetch
+	(cd b3 && git fetch)
 '
 
 test_expect_success 'bundle clone with .bundle suffix' '
-	cd "$D" &&
 	git clone b1.bundle &&
-	cd b1 &&
-	git fetch
+	(cd b1 && git fetch)
 '
 
 test_expect_success 'bundle clone from b4' '
-	cd "$D" &&
 	git clone b4 bdl &&
-	cd bdl &&
-	git fetch
+	(cd bdl && git fetch)
 '
 
 test_expect_success 'bundle clone from b4.bundle that does not exist' '
-	cd "$D" &&
-	if git clone b4.bundle bb
-	then
-		echo "Oops, should have failed"
-		false
-	else
-		echo happy
-	fi
+	test_must_fail git clone b4.bundle bb
 '
 
 test_expect_success 'bundle clone with nonexistent HEAD' '
-	cd "$D" &&
 	git clone b2.bundle b2 &&
-	cd b2 &&
+	(cd b2 &&
 	git fetch &&
-	test ! -e .git/refs/heads/master
+	test_must_fail git rev-parse --verify refs/heads/master)
 '
 
 test_expect_success 'clone empty repository' '
-	cd "$D" &&
 	mkdir empty &&
 	(cd empty &&
 	 git init &&
@@ -135,7 +105,6 @@ test_expect_success 'clone empty repository' '
 '
 
 test_expect_success 'clone empty repository, and then push should not segfault.' '
-	cd "$D" &&
 	rm -fr empty/ empty-clone/ &&
 	mkdir empty &&
 	(cd empty && git init) &&
@@ -145,13 +114,11 @@ test_expect_success 'clone empty repository, and then push should not segfault.'
 '
 
 test_expect_success 'cloning non-existent directory fails' '
-	cd "$D" &&
 	rm -rf does-not-exist &&
 	test_must_fail git clone does-not-exist
 '
 
 test_expect_success 'cloning non-git directory fails' '
-	cd "$D" &&
 	rm -rf not-a-git-repo not-a-git-repo-clone &&
 	mkdir not-a-git-repo &&
 	test_must_fail git clone not-a-git-repo not-a-git-repo-clone

-- 
1.7.10.1.21.g62fda49.dirty
Jeff King· May 26, 2012, 03:45 UTC · re: Jeff King · lore

[PATCH 2/3] clone: make --local handle URLs

Once upon a time, the --local flag was used to inform git that it should perform some optimizations when cloning a local repository. These days, we perform those optimizations automatically, and --local is largely a no-op. The one exception is that we will complain when "--local" is given if hardlinking fails, whereas the default behavior is to simply fallback to a copy.

Using --local with anything except a local path is also a no-op, because we never bother to look at the flag. So this:

  git clone --local file:///path/to/repo

silently ignores the local flag, when a more sensible behavior would be to treat it the same as:

  git clone --local /path/to/repo
Likewise, this:
  git clone --local http://example.com/repo.git
is nonsense, but git silently accepts it.

This patch causes --local to treat file:// URLs as local, and to flag an error when nonsensical combinations are requested.

Signed-off-by: Jeff King <peff@peff.net>
---
I had hoped this could just be:
  if (opt_local && !prefixcmp(repo_name, "file://"))
          repo_name += 7;

but we have to handle URL decoding. And did you know that file:// URLs can have a hostname in them? How useless. I made sure this behaves the same as the parser in connect.c, though I couldn't find a good way to share the code without making both sides less readable.

 Documentation/git-clone.txt | 17 ++++++++++-------
 builtin/clone.c             | 26 ++++++++++++++++++++++++++
 t/t5550-http-fetch.sh       |  4 ++++
 t/t5701-clone-local.sh      | 15 +++++++++++++++
 4 files changed, 55 insertions(+), 7 deletions(-)
Show changes to 4 files +55 −7

Documentation/git-clone.txt, builtin/clone.c, t/t5550-http-fetch.sh, t/t5701-clone-local.sh

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 6e22522..589f3ce 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -46,13 +46,16 @@ OPTIONS
 	mechanism and clones the repository by making a copy of
 	HEAD and everything under objects and refs directories.
 	The files under `.git/objects/` directory are hardlinked
-	to save space when possible.  This is now the default when
-	the source repository is specified with `/path/to/repo`
-	syntax, so it essentially is a no-op option.  To force
-	copying instead of hardlinking (which may be desirable
-	if you are trying to make a back-up of your repository),
-	but still avoid the usual "git aware" transport
-	mechanism, `--no-hardlinks` can be used.
+	to save space when possible.
++
+This is the default when the source repository is specified with
+`/path/to/repo`, but not with a `file://` URL; specifying `--local` with
+a file URL will bypass the regular transport mechanism as if a direct
+path had been provided.
++
+To force copying instead of hardlinking (which may be desirable if you
+are trying to make a back-up of your repository), but still avoid the
+usual "git aware" transport mechanism, `--no-hardlinks` can be used.
 
 --no-hardlinks::
 	Optimize the cloning process from a repository on a
diff --git a/builtin/clone.c b/builtin/clone.c
index a4d8d25..05944db 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -23,6 +23,7 @@
 #include "branch.h"
 #include "remote.h"
 #include "run-command.h"
+#include "url.h"
 
 /*
  * Overall FIXMEs:
@@ -607,6 +608,22 @@ static void write_config(struct string_list *config)
 	}
 }
 
+static const char *url_to_local_path(const char *url)
+{
+	const char *decoded = url_decode(url);
+	const char *p;
+
+	decoded = skip_prefix(decoded, "file://");
+	if (!decoded)
+		return NULL;
+
+	if (has_dos_drive_prefix(decoded))
+		return decoded;
+
+	p = strchr(decoded, '/');
+	return p ? p : decoded;
+}
+
 int cmd_clone(int argc, const char **argv, const char *prefix)
 {
 	int is_bundle = 0, is_local;
@@ -666,6 +683,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		repo = xstrdup(absolute_path(repo_name));
 	else if (!strchr(repo_name, ':'))
 		die(_("repository '%s' does not exist"), repo_name);
+	else if (option_local) {
+		const char *url_path = url_to_local_path(repo_name);
+		if (!url_path)
+			die(_("cannot use --local with a non-local URL"));
+		path = get_repo_path(url_path, &is_bundle);
+		if (!path)
+			die(_("repository '%s' does not exist"), url_path);
+		repo = xstrdup(absolute_path(url_path));
+	}
 	else
 		repo = repo_name;
 	is_local = path && !is_bundle;
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index b06f817..b8d77b6 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -40,6 +40,10 @@ test_expect_success 'clone http repository' '
 	test_cmp file clone/file
 '
 
+test_expect_success 'cloning http with --local fails' '
+	test_must_fail git clone --local $HTTPD_URL/dumb/repo.git local-clone
+'
+
 test_expect_success 'create password-protected repository' '
 	mkdir "$HTTPD_DOCUMENT_ROOT_PATH/auth/" &&
 	cp -Rf "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
diff --git a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh
index c6feca4..d2e0165 100755
--- a/t/t5701-clone-local.sh
+++ b/t/t5701-clone-local.sh
@@ -124,4 +124,19 @@ test_expect_success 'cloning non-git directory fails' '
 	test_must_fail git clone not-a-git-repo not-a-git-repo-clone
 '
 
+test_expect_success 'cloning file:// turns off local optimizations' '
+	git clone --bare file://"$PWD"/a non-local &&
+	! repo_is_hardlinked non-local
+'
+
+test_expect_success 'cloning file:// with --local uses hardlinks' '
+	git clone --bare --local file://"$PWD"/a force-local &&
+	repo_is_hardlinked force-local
+'
+
+test_expect_success 'cloning file:// with --local parses URL properly' '
+	git clone --bare --local file://host/"$PWD"/%61 force-local-odd &&
+	repo_is_hardlinked force-local-odd
+'
+
 test_done

-- 
1.7.10.1.21.g62fda49.dirty
Johannes Sixt· May 28, 2012, 18:31 UTC · re: Jeff King · lore

Re: [PATCH 2/3] clone: make --local handle URLs

Am 26.05.2012 05:45, schrieb Jeff King:
> And did you know that file:// URLs
> can have a hostname in them? How useless.
...
Show 14 quoted lines
> +test_expect_success 'cloning file:// turns off local optimizations' '
> +	git clone --bare file://"$PWD"/a non-local &&
> +	! repo_is_hardlinked non-local
> +'
> +
> +test_expect_success 'cloning file:// with --local uses hardlinks' '
> +	git clone --bare --local file://"$PWD"/a force-local &&
> +	repo_is_hardlinked force-local
> +'
> +
> +test_expect_success 'cloning file:// with --local parses URL properly' '
> +	git clone --bare --local file://host/"$PWD"/%61 force-local-odd &&
> +	repo_is_hardlinked force-local-odd
> +'

I'm pretty certain that we must use file://c:/path/to/repo on Windows to make these work. This means we need $(pwd) rather than $PWD. But what does this mean w.r.t. parsing the URL in the strict sense? Is "c:" the host part?

-- Hannes
Jeff King· May 28, 2012, 19:10 UTC · re: Johannes Sixt · lore

Re: [PATCH 2/3] clone: make --local handle URLs

On Mon, May 28, 2012 at 08:31:09PM +0200, Johannes Sixt wrote:
Show 21 quoted lines
> Am 26.05.2012 05:45, schrieb Jeff King:
> > And did you know that file:// URLs
> > can have a hostname in them? How useless.
> ...
> > +test_expect_success 'cloning file:// turns off local optimizations' '
> > +	git clone --bare file://"$PWD"/a non-local &&
> > +	! repo_is_hardlinked non-local
> > +'
> > +
> > +test_expect_success 'cloning file:// with --local uses hardlinks' '
> > +	git clone --bare --local file://"$PWD"/a force-local &&
> > +	repo_is_hardlinked force-local
> > +'
> > +
> > +test_expect_success 'cloning file:// with --local parses URL properly' '
> > +	git clone --bare --local file://host/"$PWD"/%61 force-local-odd &&
> > +	repo_is_hardlinked force-local-odd
> > +'
> 
> I'm pretty certain that we must use file://c:/path/to/repo on Windows to
> make these work. This means we need $(pwd) rather than $PWD.
Ah, sorry, I always forget your pwd woes.
> But what does this mean w.r.t. parsing the URL in the strict sense? Is
> "c:" the host part?

Technically, yes. But there is magic in connect.c to handle file://c:/path/to/repo, and include "c:/" as part of the path. And I replicated that logic in my patch (it's the call to has_dos_drive_prefix in url_to_local_path).

-Peff
Jeff King· May 26, 2012, 03:45 UTC · re: Jeff King · lore

[PATCH 3/3] clone: allow --no-local to turn off local optimizations

This is basically the same as using "file://", but is a little less subtle for the end user.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-clone.txt |  4 +++-
 builtin/clone.c             | 12 ++++++------
 t/t5701-clone-local.sh      |  5 +++++
 3 files changed, 14 insertions(+), 7 deletions(-)
Show changes to 3 files +14 −7

Documentation/git-clone.txt, builtin/clone.c, t/t5701-clone-local.sh

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 589f3ce..f388374 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -51,7 +51,9 @@ OPTIONS
 This is the default when the source repository is specified with
 `/path/to/repo`, but not with a `file://` URL; specifying `--local` with
 a file URL will bypass the regular transport mechanism as if a direct
-path had been provided.
+path had been provided. Specifying `--no-local` will cancel any previous
+`--local`, and will override the default when `/path/to/repo` is given,
+using the regular git transport instead.
 +
 To force copying instead of hardlinking (which may be desirable if you
 are trying to make a back-up of your repository), but still avoid the
diff --git a/builtin/clone.c b/builtin/clone.c
index 05944db..d004abb 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -39,7 +39,7 @@ static const char * const builtin_clone_usage[] = {
 };
 
 static int option_no_checkout, option_bare, option_mirror, option_single_branch = -1;
-static int option_local, option_no_hardlinks, option_shared, option_recursive;
+static int option_local = -1, option_no_hardlinks, option_shared, option_recursive;
 static char *option_template, *option_depth;
 static char *option_origin = NULL;
 static char *option_branch = NULL;
@@ -71,8 +71,8 @@ static struct option builtin_clone_options[] = {
 		PARSE_OPT_NOARG | PARSE_OPT_HIDDEN },
 	OPT_BOOLEAN(0, "mirror", &option_mirror,
 		    "create a mirror repository (implies bare)"),
-	OPT_BOOLEAN('l', "local", &option_local,
-		    "to clone from a local repository"),
+	OPT_BOOL('l', "local", &option_local,
+		"to clone from a local repository"),
 	OPT_BOOLEAN(0, "no-hardlinks", &option_no_hardlinks,
 		    "don't use local hardlinks, always copy"),
 	OPT_BOOLEAN('s', "shared", &option_shared,
@@ -343,7 +343,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 > 0)
 				die_errno(_("failed to create link '%s'"), dest->buf);
 			option_no_hardlinks = 1;
 		}
@@ -683,7 +683,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		repo = xstrdup(absolute_path(repo_name));
 	else if (!strchr(repo_name, ':'))
 		die(_("repository '%s' does not exist"), repo_name);
-	else if (option_local) {
+	else if (option_local > 0) {
 		const char *url_path = url_to_local_path(repo_name);
 		if (!url_path)
 			die(_("cannot use --local with a non-local URL"));
@@ -694,7 +694,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	}
 	else
 		repo = repo_name;
-	is_local = path && !is_bundle;
+	is_local = option_local != 0 && path && !is_bundle;
 	if (is_local && option_depth)
 		warning(_("--depth is ignored in local clones; use file:// instead."));
 
diff --git a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh
index d2e0165..64faff1 100755
--- a/t/t5701-clone-local.sh
+++ b/t/t5701-clone-local.sh
@@ -139,4 +139,9 @@ test_expect_success 'cloning file:// with --local parses URL properly' '
 	repo_is_hardlinked force-local-odd
 '
 
+test_expect_success 'cloning a local path with --no-local does not hardlink' '
+	git clone --bare --no-local a force-nonlocal &&
+	! repo_is_hardlinked force-nonlocal
+'
+
 test_done

-- 
1.7.10.1.21.g62fda49.dirty
Jeff King· May 26, 2012, 04:11 UTC · re: Jeff King · lore

[PATCH 4/3] clone: send diagnostic messages to stderr

Putting messages like "Cloning into.." and "done" on stdout is un-Unix and uselessly clutters the stdout channel. Send them to stderr.

We have to tweak two tests to accommodate this:
  1. t5601 checks for doubled output due to forking, and
     doesn't actually care where the output goes; adjust it
     to check stderr.
  2. t5702 is trying to test whether progress output was
     sent to stderr, but naively does so by checking
     whether stderr produced any output. Instead, have it
     look for "%", a token found in progress output but not
     elsewhere (and which lets us avoid hard-coding the
     progress text in the test).
Signed-off-by: Jeff King <peff@peff.net>
---
This one isn't really related to the other patches in the series, but
while we're on the subject of extremely minor git-clone annoyances, I
thought I'd throw it in as a bonus round.

Arguably the test in t5601 should just go away entirely. stderr tends to be line-buffered anyway, so the thing it is testing for wouldn't happen. Not to mention that according to 2c3766f, which introduced it, the problem was due to start_async() not flushing output before forking. But we long ago switched start_async to use pthreads, so the bug it is testing for wouldn't even be detectable on any modern platform.

 builtin/clone.c          | 6 +++---
 t/t5601-clone.sh         | 2 +-
 t/t5702-clone-options.sh | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)
Show changes to 3 files +6 −6

builtin/clone.c, t/t5601-clone.sh, t/t5702-clone-options.sh

diff --git a/builtin/clone.c b/builtin/clone.c
index d004abb..08470ed 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -371,7 +371,7 @@ static void clone_local(const char *src_repo, const char *dest_repo)
 	}
 
 	if (0 <= option_verbosity)
-		printf(_("done.\n"));
+		fprintf(stderr, _("done.\n"));
 }
 
 static const char *junk_work_tree;
@@ -751,9 +751,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 
 	if (0 <= option_verbosity) {
 		if (option_bare)
-			printf(_("Cloning into bare repository '%s'...\n"), dir);
+			fprintf(stderr, _("Cloning into bare repository '%s'...\n"), dir);
 		else
-			printf(_("Cloning into '%s'...\n"), dir);
+			fprintf(stderr, _("Cloning into '%s'...\n"), dir);
 	}
 	init_db(option_template, INIT_DB_QUIET);
 	write_config(&option_config);
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 67869b4..aa9f991 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -36,7 +36,7 @@ test_expect_success 'clone with excess parameters (2)' '
 
 test_expect_success C_LOCALE_OUTPUT 'output from clone' '
 	rm -fr dst &&
-	git clone -n "file://$(pwd)/src" dst >output &&
+	git clone -n "file://$(pwd)/src" dst >output 2>&1 &&
 	test $(grep Clon output | wc -l) = 1
 '
 
diff --git a/t/t5702-clone-options.sh b/t/t5702-clone-options.sh
index 02cb024..67e170e 100755
--- a/t/t5702-clone-options.sh
+++ b/t/t5702-clone-options.sh
@@ -22,14 +22,14 @@ test_expect_success 'clone -o' '
 test_expect_success 'redirected clone' '
 
 	git clone "file://$(pwd)/parent" clone-redirected >out 2>err &&
-	test ! -s err
+	! grep % err
 
 '
 test_expect_success 'redirected clone -v' '
 
 	git clone --progress "file://$(pwd)/parent" clone-redirected-progress \
 		>out 2>err &&
-	test -s err
+	grep % err
 
 '
 

-- 
1.7.10.1.21.g62fda49.dirty
Junio C Hamano· May 27, 2012, 06:32 UTC · re: Jeff King · lore

Re: [PATCH 0/3] clone --local fixes

Jeff King <peff@peff.net> writes:
Show 6 quoted lines
> Emeric mentioned that he was confused by the behavior of
>
>   git clone --local file:///path/to/repo
>
> in that the --local is silently ignored. Looking over the documentation,
> it is quite unclear whether it is supposed to do anything or not.

According to 23d5335 (git clone: do not issue warning while cloning locally across filesystems, 2007-08-20) and 3d5c418 (git-clone: aggressively optimize local clone behaviour., 2007-08-01), in the very early days we only used the local optimization when the end user explicitly asked for it with the '--local' option.

After 3d5c418f, the option has been made a no-op, because the local-ness of the $repository parameter, where the definition of the local-ness is "does 'cd $repository' succeed?", is all that is needed to trigger the codepath.

The logic to choose the local optimization has since been "is it a path to a local directory---if so, use the optimizaiton. Otherwise do the usual URL based thing". And that is why we tell people when they want to try cloning without local optimization to add "file://" in front of the path.

So your example "file:///" should *not* work even if --local is given, unless you happen to have a directory called "file:" in your current directory and it has path/to/repo subdirectory, which is a git repository. Specifically, the repository at /path/to/repo is not what the command line is naming.

I think we probably should stop advertising --local in the documentation and help text.

Jeff King· May 28, 2012, 05:36 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/3] clone --local fixes

On Sat, May 26, 2012 at 11:32:44PM -0700, Junio C Hamano wrote:
Show 5 quoted lines
> So your example "file:///" should *not* work even if --local is given,
> unless you happen to have a directory called "file:" in your current
> directory and it has path/to/repo subdirectory, which is a git repository.
> Specifically, the repository at /path/to/repo is not what the command line
> is naming.

I think it depends on the definition of "--local". If it means "when we are cloning without a URL, turn on the local optimizations", then yes, "file://" should not work. If it means "turn on local optimizations if this destination supports it", then it should.

The current behavior is ambiguous as to whether it is the first case, or whether it is the second, and it was simply buggy. The history you gave argues that the original intent was the former. But to me that is much less important than what is useful and least surprising to users.

There are basically three sane behaviors for "git clone --local file://":

  1. --local is silently ignored, because it means "if we are local,
     turn on optimizations" (though it has a horrible name, in that case)
  2. it's an error; you should not use --local with a URL
  3. it should use local optimizations (because file:// is local)

Is there a compelling reason not to do (3)? It seems to be the friendliest and least surprising to me.

I'll admit I don't care too much about this use case. People don't tend to type "file://..." when they could type the simpler thing, so I doubt anyone is hurting. I just don't see a reason not to make it work, besides the usual "it is a non-zero amount of code".

> I think we probably should stop advertising --local in the documentation
> and help text.

I kind of agree, and that was what I was going to do originally (instead of making it work). But I do think that "--no-local" (from my patch 3) is actually useful in practice, so I'd rather not drop the option from the documentation entirely.

-Peff
Junio C Hamano· May 29, 2012, 17:43 UTC · re: Jeff King · lore

Re: [PATCH 0/3] clone --local fixes

Jeff King <peff@peff.net> writes:
> I think it depends on the definition of "--local". If it means "when we
> are cloning without a URL, turn on the local optimizations", then yes,
> "file://" should not work. If it means "turn on local optimizations if
> this destination supports it", then it should.

It has meant the former since the day --local was introduced, and the semi-deprecation at 3d5c418 (git-clone: aggressively optimize local clone behaviour., 2007-08-01) didn't change it, either.

> The current behavior is ambiguous as to whether it is the first case, or
> whether it is the second, and it was simply buggy. The history you gave
> argues that the original intent was the former. But to me that is much
> less important than what is useful and least surprising to users.

Changing it would make it even more confusing to people who started using Git before mid 2007, though. That is why I am for deprecating (and eventually removing) "--local".

It is not like we do not have a way to force "local" behaviour. Just give a local pathname and "local" behaviour will kick in; if you do not want "local" behaviour, give network URL or file:// URL. If there were no old scripts, there is no reason to have "--local" in the supported options list of the command.

Having said all that, because the conclusion above is "we need to keep --local for now for old scripts, and its behaviour for sane case should not change", there are some glitches we may want to address for people who try to use "--local" without knowing that it no longer is necessary to use it if you live in modern age. For example:

 $ git clone --local $URL

does not error out, when $URL does not name a repository in the local filesystem. I think that *can* be changed without breaking old scripts. We usually probe to see if $URL is a pathname of a local repository, and then fall back to transports, but we should error out when --local is explicitly asked for, instead of falling back.

Show 5 quoted lines
> There are basically three sane behaviors for "git clone --local
> file://":
>
>   1. --local is silently ignored, because it means "if we are local,
>      turn on optimizations" (though it has a horrible name, in that case)

That is my first preference; "--local" started as an opt-in experiment until it turns out to be useful and became no-op because of 3d5c418, and for the purpose of that experiment, "--local" was perfectly a good name. It is a no-op now.

>   2. it's an error; you should not use --local with a URL

And this is my first preference for a longer time deprecation plan (see above).

>   3. it should use local optimizations (because file:// is local)

Hrm. "file://" has always been a way to say "do not use local optimization". Doesn't it feel simply insane to invent "--local file://" as a way to say "even though the second word on this command line tells you not to use the local optimization, by adding this newly redefined --local option, I am telling you to use the local codepath after all"? Instead, I would prefer to see us just finish the deprecation that started long time ago.

Jeff King· May 30, 2012, 11:03 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/3] clone --local fixes

On Tue, May 29, 2012 at 10:43:32AM -0700, Junio C Hamano wrote:
Show 10 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > I think it depends on the definition of "--local". If it means "when we
> > are cloning without a URL, turn on the local optimizations", then yes,
> > "file://" should not work. If it means "turn on local optimizations if
> > this destination supports it", then it should.
> 
> It has meant the former since the day --local was introduced, and
> the semi-deprecation at 3d5c418 (git-clone: aggressively optimize
> local clone behaviour., 2007-08-01) didn't change it, either.

Right, but my argument was that since nobody probably ever cared about the distinction, it is more important to do the least surprising thing.

Show 8 quoted lines
> > The current behavior is ambiguous as to whether it is the first case, or
> > whether it is the second, and it was simply buggy. The history you gave
> > argues that the original intent was the former. But to me that is much
> > less important than what is useful and least surprising to users.
> 
> Changing it would make it even more confusing to people who started
> using Git before mid 2007, though.  That is why I am for deprecating
> (and eventually removing) "--local".

Yes, it would technically be a regression. But I highly doubt that somebody is relying on the fact that "--local" with file:// is a silent no-op. And other than that, the behavior remains the same (note that my patch explicitly did not try to turn on --local when it sees file://).

Anyway, I do not really care about this part of the series. It was not "this is a needed feature" but rather "this is less surprising and easy to do, so why not?". We can drop it and replace it with a documentation update.

I would still like to keep the --no-local patch, for two reasons:
  1. The fact that using file:// overrides the local optimizations is
     somewhat non-obvious if you do not already know that it is the
     case.
  2. File URLs require absolute paths[1]. You can't do the equivalent of
     "git clone --no-local foo.git" without resorting to $PWD.
So here's an updated series (I see you took the test cleanups already).
  [1/2]: docs/clone: mention that --local may be ignored
  [2/2]: clone: allow --no-local to turn off local optimizations

I didn't bother with deprecation or erroring out on URLs. So far we have had exactly one report on this, and I think the improved documentation would have solved this particular case.

-Peff
Jeff King· May 30, 2012, 11:08 UTC · re: Jeff King · lore

Re: [PATCH 0/3] clone --local fixes

On Wed, May 30, 2012 at 07:03:05AM -0400, Jeff King wrote:
>   2. File URLs require absolute paths[1]. You can't do the equivalent of
>      "git clone --no-local foo.git" without resorting to $PWD.
I forgot to include my footnote, which was:
  You can actually do "git clone file://foo" and we will treat that as
  the path "foo". Which does not match the definition of the "file"
  scheme, which would interpret that as "host 'foo', no path", but does
  match what some other software does. However, if there _is_ a slash,
  then we correctly throw away the host bit, so that "file://junk/foo"
  _also_ points to the path "foo". Which means there is no equivalent
  for:
    git clone --no-local subdir/foo.git
-Peff
Jeff King· May 30, 2012, 11:09 UTC · re: Jeff King · lore

[PATCH 1/2] docs/clone: mention that --local may be ignored

The --local flag is not "treat this like a local repository", but rather "if we are local, turn on optimizations". Therefore it does nothing in the case of:

  git clone --local file:///path/to/repo
Let's make that more clear in the documentation.
Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-clone.txt | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)
Show changes to Documentation/git-clone.txt +10 −7
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 6e22522..1d267f4 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -46,13 +46,16 @@ OPTIONS
 	mechanism and clones the repository by making a copy of
 	HEAD and everything under objects and refs directories.
 	The files under `.git/objects/` directory are hardlinked
-	to save space when possible.  This is now the default when
-	the source repository is specified with `/path/to/repo`
-	syntax, so it essentially is a no-op option.  To force
-	copying instead of hardlinking (which may be desirable
-	if you are trying to make a back-up of your repository),
-	but still avoid the usual "git aware" transport
-	mechanism, `--no-hardlinks` can be used.
+	to save space when possible.
++
+If the repository is specified as a local path (e.g., `/path/to/repo`),
+this is the default, and --local is essentially a no-op.  If the
+repository is specified as a URL, then this flag is ignored (and we
+never use the local optimizations).
++
+To force copying instead of hardlinking (which may be desirable if you
+are trying to make a back-up of your repository), but still avoid the
+usual "git aware" transport mechanism, `--no-hardlinks` can be used.
 
 --no-hardlinks::
 	Optimize the cloning process from a repository on a

-- 
1.7.11.rc0.12.g6048c92
Jeff King· May 30, 2012, 11:10 UTC · re: Jeff King · lore

[PATCH 2/2] clone: allow --no-local to turn off local optimizations

This is basically the same as using "file://", but is a little less subtle for the end user. It also allows relative paths to be specified.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-clone.txt |  4 +++-
 builtin/clone.c             | 10 +++++-----
 t/t5701-clone-local.sh      | 10 ++++++++++
 3 files changed, 18 insertions(+), 6 deletions(-)
Show changes to 3 files +18 −6

Documentation/git-clone.txt, builtin/clone.c, t/t5701-clone-local.sh

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 1d267f4..c1ddd4c 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -51,7 +51,9 @@ OPTIONS
 If the repository is specified as a local path (e.g., `/path/to/repo`),
 this is the default, and --local is essentially a no-op.  If the
 repository is specified as a URL, then this flag is ignored (and we
-never use the local optimizations).
+never use the local optimizations).  Specifying `--no-local` will
+override the default when `/path/to/repo` is given, using the regular
+git transport instead.
 +
 To force copying instead of hardlinking (which may be desirable if you
 are trying to make a back-up of your repository), but still avoid the
diff --git a/builtin/clone.c b/builtin/clone.c
index a4d8d25..7f3b982 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -38,7 +38,7 @@ static const char * const builtin_clone_usage[] = {
 };
 
 static int option_no_checkout, option_bare, option_mirror, option_single_branch = -1;
-static int option_local, option_no_hardlinks, option_shared, option_recursive;
+static int option_local = -1, option_no_hardlinks, option_shared, option_recursive;
 static char *option_template, *option_depth;
 static char *option_origin = NULL;
 static char *option_branch = NULL;
@@ -70,8 +70,8 @@ static struct option builtin_clone_options[] = {
 		PARSE_OPT_NOARG | PARSE_OPT_HIDDEN },
 	OPT_BOOLEAN(0, "mirror", &option_mirror,
 		    "create a mirror repository (implies bare)"),
-	OPT_BOOLEAN('l', "local", &option_local,
-		    "to clone from a local repository"),
+	OPT_BOOL('l', "local", &option_local,
+		"to clone from a local repository"),
 	OPT_BOOLEAN(0, "no-hardlinks", &option_no_hardlinks,
 		    "don't use local hardlinks, always copy"),
 	OPT_BOOLEAN('s', "shared", &option_shared,
@@ -342,7 +342,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 > 0)
 				die_errno(_("failed to create link '%s'"), dest->buf);
 			option_no_hardlinks = 1;
 		}
@@ -668,7 +668,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		die(_("repository '%s' does not exist"), repo_name);
 	else
 		repo = repo_name;
-	is_local = path && !is_bundle;
+	is_local = option_local != 0 && path && !is_bundle;
 	if (is_local && option_depth)
 		warning(_("--depth is ignored in local clones; use file:// instead."));
 
diff --git a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh
index c6feca4..2d6121f 100755
--- a/t/t5701-clone-local.sh
+++ b/t/t5701-clone-local.sh
@@ -124,4 +124,14 @@ test_expect_success 'cloning non-git directory fails' '
 	test_must_fail git clone not-a-git-repo not-a-git-repo-clone
 '
 
+test_expect_success 'cloning file:// does not hardlink' '
+	git clone --bare file://"$(pwd)"/a non-local &&
+	! repo_is_hardlinked non-local
+'
+
+test_expect_success 'cloning a local path with --no-local does not hardlink' '
+	git clone --bare --no-local a force-nonlocal &&
+	! repo_is_hardlinked force-nonlocal
+'
+
 test_done

-- 
1.7.11.rc0.12.g6048c92
Junio C Hamano· May 30, 2012, 17:20 UTC · re: Jeff King · lore

Re: [PATCH 2/2] clone: allow --no-local to turn off local optimizations

Makes sense; thanks.
By the way, a somewhat funny exercise I found last night is this:
	$ cd /var/tmp
        $ mkdir -p foo.xz:pub/git.git
        $ cd foo.xz:pub/git.git
        $ git init
        $ git commit --allow-empty -m initial
        $ cd /var/tmp
        $ git clone foo.xz:pub/git.git x
This does not work as the transfer phase wants to run ssh on it.
Jeff King· May 30, 2012, 21:59 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] clone: allow --no-local to turn off local optimizations

On Wed, May 30, 2012 at 10:20:19AM -0700, Junio C Hamano wrote:
Show 11 quoted lines
> By the way, a somewhat funny exercise I found last night is this:
> 
> 	$ cd /var/tmp
>         $ mkdir -p foo.xz:pub/git.git
>         $ cd foo.xz:pub/git.git
>         $ git init
>         $ git commit --allow-empty -m initial
>         $ cd /var/tmp
>         $ git clone foo.xz:pub/git.git x
> 
> This does not work as the transfer phase wants to run ssh on it.

Yeah, you would have to do file://$PWD/foo.xz:pub/git.git, I think. Of course, there would be no way to specify "--local" while doing so... :)

Similarly, I find it a little odd that "git clone file:///foo.git" will actually find a file named "file:/foo.git" before checking the URL (IOW, the argument is a path first, and then fallback to URL). I suspect nobody actually cares about either, as they are very unlikely corner cases.

-Peff
Junio C Hamano· May 30, 2012, 22:10 UTC · re: Jeff King · lore

Re: [PATCH 2/2] clone: allow --no-local to turn off local optimizations

Jeff King <peff@peff.net> writes:
Show 5 quoted lines
> Similarly, I find it a little odd that "git clone file:///foo.git" will
> actually find a file named "file:/foo.git" before checking the URL (IOW,
> the argument is a path first, and then fallback to URL). I suspect
> nobody actually cares about either, as they are very unlikely corner
> cases.

Yeah, if anything, I would have expected --no-local to mean "I might have a local file that happens to be the same as this URL, but I am not cloning from there; just go straight to the URL using transports".

Jeff King· May 30, 2012, 23:21 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] clone: allow --no-local to turn off local optimizations

On Wed, May 30, 2012 at 03:10:37PM -0700, Junio C Hamano wrote:
Show 11 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > Similarly, I find it a little odd that "git clone file:///foo.git" will
> > actually find a file named "file:/foo.git" before checking the URL (IOW,
> > the argument is a path first, and then fallback to URL). I suspect
> > nobody actually cares about either, as they are very unlikely corner
> > cases.
> 
> Yeah, if anything, I would have expected --no-local to mean "I might
> have a local file that happens to be the same as this URL, but I am
> not cloning from there; just go straight to the URL using transports".

But that would not be the opposite of "--local", which you have just argued is not about interpreting the URL syntax at all, but is about turning off the local optimization code path when the origin repo is local.

Interestingly, it seems that we don't handle this case well at all, anyway. We notice that "file:///foo.git" is resolvable as a path, set is_local, and set the path to "$PWD/file:///foo.git". But we still feed that to the transport code to get the list of refs (just not to fetch them). And the transport code barfs and says "I don't understand the URL scheme $PWD/file".

So I'm not sure what the actual rules were meant to be, and if we actually follow them. Or whether there are even intentional rules, and it is not "what happens to work". Again, these are such silly corner cases that I suspect it is simply the case that nobody has run into them or cared.

-Peff
Junio C Hamano· May 30, 2012, 23:33 UTC · re: Jeff King · lore

Re: [PATCH 2/2] clone: allow --no-local to turn off local optimizations

Jeff King <peff@peff.net> writes:
Show 18 quoted lines
> On Wed, May 30, 2012 at 03:10:37PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > Similarly, I find it a little odd that "git clone file:///foo.git" will
>> > actually find a file named "file:/foo.git" before checking the URL (IOW,
>> > the argument is a path first, and then fallback to URL). I suspect
>> > nobody actually cares about either, as they are very unlikely corner
>> > cases.
>> 
>> Yeah, if anything, I would have expected --no-local to mean "I might
>> have a local file that happens to be the same as this URL, but I am
>> not cloning from there; just go straight to the URL using transports".
>
> But that would not be the opposite of "--local", which you have just
> argued is not about interpreting the URL syntax at all, but is about
> turning off the local optimization code path when the origin repo is
> local.

What I meant to say was that promoting "--local" like your original series did and giving it a new meaning did not make much sense in the context of the current semantics (i.e. if the path exists, it is a path and you do not have to tell "--local" about it), but the semantics _instead_ needs "--no-local" to be complete; without "--no-local" that is defined as such, the funny corner case that a path with the same as $URL prevents you from going to where you want to go.

> Interestingly, it seems that we don't handle this case well at all,

Yes, isn't it interesting? It is not just we feed it to transport, but we also store it in the config so later "git fetch" will also do something inconsistent. "<scheme>://", primarily because it has doubled slashes, I wouldn't worry too much about them, but I would not be surprised if somebody saw scp-style <host>:<path> conflict with a local path and wished we handled such a case a bit more sanely.

> ... Again, these are such silly corner
> cases that I suspect it is simply the case that nobody has run into them
> or cared.
;-)

← back to recent threads