[PATCH 0/3] clone --local fixes

[PATCH 0/3] clone --local fixes

[Jeff King]

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

[PATCH 1/3] t5701: modernize style

[Jeff King]

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

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

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

[Jeff King]

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

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

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

[Jeff King]

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

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

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

[Jeff King]

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

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

[Jeff King]

On Sat, May 26, 2012 at 11:32:44PM -0700, Junio C Hamano wrote:

> 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

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

[Johannes Sixt]

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. But what
does this mean w.r.t. parsing the URL in the strict sense? Is "c:" the
host part?

-- Hannes

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

[Jeff King]

On Mon, May 28, 2012 at 08:31:09PM +0200, Johannes Sixt wrote:

> 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

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

[Junio C Hamano]

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.

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

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

[Jeff King]

On Tue, May 29, 2012 at 10:43:32AM -0700, Junio C Hamano wrote:

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

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

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

[Jeff King]

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

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

[Jeff King]

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

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

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

[Jeff King]

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

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

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

[Junio C Hamano]

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.

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

[Jeff King]

On Wed, May 30, 2012 at 10:20:19AM -0700, Junio C Hamano wrote:

> 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

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

[Junio C Hamano]

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

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

[Jeff King]

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.

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

;-)