[BUG] Colon in remote urls

[BUG] Colon in remote urls

[Klaus Ethgen]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Hello,

I have some repositories where I have a colon in the (local) url for a
remote. That was no problem until now but with 2.11.0, I see the
following problem:
   ~> git push
   Counting objects: 11, done.
   Delta compression using up to 8 threads.
   Compressing objects: 100% (10/10), done.
   Writing objects: 100% (11/11), 1.26 KiB | 0 bytes/s, done.
   Total 11 (delta 7), reused 0 (delta 0)
   remote: error: object directory /home/xxx does not exist; check .git/objects/info/alternates.
   remote: error: object directory yyy.git/objects does not exist; check .git/objects/info/alternates.
   remote: fatal: unresolved deltas left after unpacking
   error: unpack failed: unpack-objects abnormal exit
   To /home/xxx:yyy.git
    ! [remote rejected] master -> master (unpacker error)
   error: failed to push some refs to '/home/xxx:yyy.git'

Prepending the path with "file://" does not help.

It seems that the new git version splits the url at ":" which ends in
two incorrect paths.

Pull seems tho work well currently.

Regards
   Klaus

Ps. I am not subscribet to the mailing list, so please keep me in Cc.
- -- 
Klaus Ethgen                                       http://www.ethgen.ch/
pub  4096R/4E20AF1C 2011-05-16            Klaus Ethgen <Klaus@Ethgen.ch>
Fingerprint: 85D4 CA42 952C 949B 1753  62B3 79D0 B06F 4E20 AF1C
-----BEGIN PGP SIGNATURE-----
Comment: Charset: ISO-8859-1

iQGzBAEBCgAdFiEEMWF28vh4/UMJJLQEpnwKsYAZ9qwFAlhKuV4ACgkQpnwKsYAZ
9qz81Qv6AsgZHlaEHEybERAvGikjZgUvjyC7dzQ2zCmc8iv0eb8kGLiBtVtInsWr
eo/CiMSdX2emoCD5LQq/sxcRIgIoWpF8m2NEXiBMl94d6YLOpvWV1yC1kQ8qK6bt
Pyeo9/LofAnpLcQRn1am8tFwrcCMLZxSM7cxMxjtP6i+RU0MHc/rS1HqhdzptpsH
jvB0x41X7LNoeRLqG5n8lVlyHP1PiGyP0Dl4Aa9rFBHn+hydiJEmLEwdn9w4wgIz
vo2DZmqGm0r4vTaTP1gQRPxoW6fsBZ1uUWKRMjd947R1eaELpyROj4SFt0bWNqwW
cx9izYUuTg+xSe1KTaSgPlmZn1817DlG2yJ/YduKH8v61ZJ2r6B1awO2tz32g7cA
QdjsnzAyz6eVLrrsJ5OrJPRF2Fl7gM22jo9gs3BJrHeJdC9dU6kVIAR3eryoFvUg
fG/Vl2zvfbMRQAaUDGMyxNk5TGVFB6ANw0KS/NczRvmbPA9kukBz012+8ZY8MHzD
aEvmk/yz
=tDwn
-----END PGP SIGNATURE-----

Re: [BUG] Colon in remote urls

[Jeff King]

On Fri, Dec 09, 2016 at 03:02:15PM +0100, Klaus Ethgen wrote:

> I have some repositories where I have a colon in the (local) url for a
> remote. That was no problem until now but with 2.11.0, I see the
> following problem:
>    ~> git push
>    Counting objects: 11, done.
>    Delta compression using up to 8 threads.
>    Compressing objects: 100% (10/10), done.
>    Writing objects: 100% (11/11), 1.26 KiB | 0 bytes/s, done.
>    Total 11 (delta 7), reused 0 (delta 0)
>    remote: error: object directory /home/xxx does not exist; check .git/objects/info/alternates.
>    remote: error: object directory yyy.git/objects does not exist; check .git/objects/info/alternates.
>    remote: fatal: unresolved deltas left after unpacking

Hrm. The problem v2.11's new object-quarantine system. The receiving
side of a push will write into a new temporary object directory, and
refer to the original with the GIT_ALTERNATE_OBJECT_DIRECTORIES
environment variable. But that variable splits its entries on ":", and
has no way of representing a path that includes a ":".

So I think the solution would probably be to introduce some kind of
quoting mechanism to that variable, so that it can pass through
arbitrary paths. Or alternatively use a separate variable, but that does
nothing to help other cases where somebody wants to use xxx:yyy.git as
an alternate.

(One other option is to just declare that the quarantine feature doesn't
work with colons in the pathname, but stop turning it on by default. I'm
not sure I like that, though).

Here's a rough idea of what the quoting solution could look like. It
should make your case work, but I'm not sure what we want to do about
backwards-compatibility, if anything.

---
 sha1_file.c  | 41 ++++++++++++++++++++++++++++++-----------
 tmp-objdir.c | 18 ++++++++++++++++--
 2 files changed, 46 insertions(+), 13 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 9c86d1924..a0b341b5a 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -329,13 +329,35 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,
 	return 0;
 }
 
+const char *parse_alt_odb_entry(const char *string, int sep,
+				struct strbuf *out)
+{
+	const char *p;
+	int literal = 0;
+
+	strbuf_reset(out);
+
+	for (p = string; *p; p++) {
+		if (literal) {
+			strbuf_addch(out, *p);
+			literal = 0;
+		} else {
+			if (*p == '\\')
+				literal = 1;
+			else if (*p == sep)
+				return p + 1;
+			else
+				strbuf_addch(out, *p);
+		}
+	}
+	return p;
+}
+
 static void link_alt_odb_entries(const char *alt, int len, int sep,
 				 const char *relative_base, int depth)
 {
-	struct string_list entries = STRING_LIST_INIT_NODUP;
-	char *alt_copy;
-	int i;
 	struct strbuf objdirbuf = STRBUF_INIT;
+	struct strbuf entry = STRBUF_INIT;
 
 	if (depth > 5) {
 		error("%s: ignoring alternate object stores, nesting too deep.",
@@ -348,16 +370,13 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,
 		die("unable to normalize object directory: %s",
 		    objdirbuf.buf);
 
-	alt_copy = xmemdupz(alt, len);
-	string_list_split_in_place(&entries, alt_copy, sep, -1);
-	for (i = 0; i < entries.nr; i++) {
-		const char *entry = entries.items[i].string;
-		if (entry[0] == '\0' || entry[0] == '#')
+	while (*alt) {
+		alt = parse_alt_odb_entry(alt, sep, &entry);
+		if (!entry.len || entry.buf[0] == '#')
 			continue;
-		link_alt_odb_entry(entry, relative_base, depth, objdirbuf.buf);
+		link_alt_odb_entry(entry.buf, relative_base, depth, objdirbuf.buf);
 	}
-	string_list_clear(&entries, 0);
-	free(alt_copy);
+	strbuf_release(&entry);
 	strbuf_release(&objdirbuf);
 }
 
diff --git a/tmp-objdir.c b/tmp-objdir.c
index 64435f23a..900e15af1 100644
--- a/tmp-objdir.c
+++ b/tmp-objdir.c
@@ -70,6 +70,17 @@ static void remove_tmp_objdir_on_signal(int signo)
 	raise(signo);
 }
 
+static char *backslash_quote(const char *s, int delim)
+{
+	struct strbuf buf = STRBUF_INIT;
+	while (*s) {
+		if (*s == '\\' || *s == delim)
+			strbuf_addch(&buf, '\\');
+		strbuf_addch(&buf, *s++);
+	}
+	return strbuf_detach(&buf, NULL);
+}
+
 /*
  * These env_* functions are for setting up the child environment; the
  * "replace" variant overrides the value of any existing variable with that
@@ -79,12 +90,15 @@ static void remove_tmp_objdir_on_signal(int signo)
  */
 static void env_append(struct argv_array *env, const char *key, const char *val)
 {
+	char *quoted = backslash_quote(val, PATH_SEP);
 	const char *old = getenv(key);
 
 	if (!old)
-		argv_array_pushf(env, "%s=%s", key, val);
+		argv_array_pushf(env, "%s=%s", key, quoted);
 	else
-		argv_array_pushf(env, "%s=%s%c%s", key, old, PATH_SEP, val);
+		argv_array_pushf(env, "%s=%s%c%s", key, old, PATH_SEP, quoted);
+
+	free(quoted);
 }
 
 static void env_replace(struct argv_array *env, const char *key, const char *val)
-- 
2.11.0.201.g51bd297

Re: [BUG] Colon in remote urls

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> (One other option is to just declare that the quarantine feature doesn't
> work with colons in the pathname, but stop turning it on by default. I'm
> not sure I like that, though).

I think we long time ago in 2005 have declared that a colon in a
directory name would not work for Git repositories because of things
like GIT_CEILING_DIRECTORIES, GIT_ALTERNATE_OBJECT_DIRECTORIES; so I
do not think we terribly mind that direction.

> Here's a rough idea of what the quoting solution could look like. It
> should make your case work, but I'm not sure what we want to do about
> backwards-compatibility, if anything.

Yes, obviously it robs from those with backslash in their pathnames
to please those with colons; we've never catered to the latter, so I
am not sure if the trade-off is worth it.

I can see how adding a new environment variable could work, though.
A naive way would be to add GIT_ALT_OBJ_DIRS that uses your quoting
when splitting its elements, give it precedence over the existing
one (or allow to use both and take union as the set of alternate
object stores) and make sure that the codepaths we use internally
uses the new variable.  But if the quarantine codepath will stay to
be the only user of this mechanism (and I strongly suspect that will
be the case), the new environment could just be pointing at a single
directory, i.e. GIT_OBJECT_QUARANTINE_DIRECTORY, whose value is
added without splitting to the list of alternate object stores, and
the quarantine codepath can export that instead.

Re: [BUG] Colon in remote urls

[Johannes Sixt]

Am 09.12.2016 um 16:22 schrieb Jeff King:
> +const char *parse_alt_odb_entry(const char *string, int sep,
> +				struct strbuf *out)
> +{
> +	const char *p;
> +	int literal = 0;
> +
> +	strbuf_reset(out);
> +
> +	for (p = string; *p; p++) {
> +		if (literal) {
> +			strbuf_addch(out, *p);
> +			literal = 0;
> +		} else {
> +			if (*p == '\\')
> +				literal = 1;

There are too many systems out there that use a backslash in path names. 
I don't think it is wise to use it also as the quoting character.

> +			else if (*p == sep)
> +				return p + 1;
> +			else
> +				strbuf_addch(out, *p);
> +		}
> +	}
> +	return p;
> +}

-- Hannes

Re: [BUG] Colon in remote urls

[Jeff King]

On Fri, Dec 09, 2016 at 10:32:48PM +0100, Johannes Sixt wrote:

> > +			if (*p == '\\')
> > +				literal = 1;
> 
> There are too many systems out there that use a backslash in path names. I
> don't think it is wise to use it also as the quoting character.

Yeah, I picked it arbitrarily as the common quoting character, but I
agree it probably makes backwards compatibility (and general usability
when you have to double-backslash each instance) pretty gross on
Windows.

Most of the printable characters are going to suffer from backwards
compatibility issues. Syntactically, we could use any ASCII control code
below 0x20. Certainly our C code would be fine with that, but it seems
pretty nasty.

There's probably some crazy non-printing UTF-8 sequence we could use,
too, but I'm not sure I want to go there.

-Peff

Re: [BUG] Colon in remote urls

[Jeff King]

On Fri, Dec 09, 2016 at 11:07:25AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > (One other option is to just declare that the quarantine feature doesn't
> > work with colons in the pathname, but stop turning it on by default. I'm
> > not sure I like that, though).
> 
> I think we long time ago in 2005 have declared that a colon in a
> directory name would not work for Git repositories because of things
> like GIT_CEILING_DIRECTORIES, GIT_ALTERNATE_OBJECT_DIRECTORIES; so I
> do not think we terribly mind that direction.

I don't mind declaring the feature incompatible. But I'm not sure
whether I like not having it on by default, if only because it means we
have two distinct code paths to care about. They're sufficiently
different that I'd worry about a bug creeping into one and not the
other.

> > Here's a rough idea of what the quoting solution could look like. It
> > should make your case work, but I'm not sure what we want to do about
> > backwards-compatibility, if anything.
> 
> Yes, obviously it robs from those with backslash in their pathnames
> to please those with colons; we've never catered to the latter, so I
> am not sure if the trade-off is worth it.
> 
> I can see how adding a new environment variable could work, though.
> A naive way would be to add GIT_ALT_OBJ_DIRS that uses your quoting
> when splitting its elements, give it precedence over the existing
> one (or allow to use both and take union as the set of alternate
> object stores) and make sure that the codepaths we use internally
> uses the new variable.

Yeah, a new variable solves the backwards-compatibility issue, though if
we continue to use backslash as an escape, then people on Windows will
annoying _have_ to backslash-escape "C:\\" (worse, AIUI the conversion
from "/unix/path" to "C:\unix\path" happens transparently via msys
magic, and it would not know that we need to quote).

I think the least-gross alternative would be to make newline the new
delimiter. It's already the delimiter (and not quotable) in the
objects/info/alternates file, and I have a lot less trouble telling
people with newline in their filenames that they're Doing It Wrong.

> But if the quarantine codepath will stay to
> be the only user of this mechanism (and I strongly suspect that will
> be the case), the new environment could just be pointing at a single
> directory, i.e. GIT_OBJECT_QUARANTINE_DIRECTORY, whose value is
> added without splitting to the list of alternate object stores, and
> the quarantine codepath can export that instead.

Yes, I also considered that approach. The big downside is that it does
not help other users of GIT_ALTERNATE_OBJECT_DIRECTORIES. I doubt that
matters much in practice, though.

My other question is whether we care about compatibility between Git
versions here. If receive-pack sets the variable, we can assume that
index-pack will be run from the same version. But we also run hooks with
the quarantine variables set. The nice thing about the existing scheme
is that older versions of Git (or alternate implementations of Git) will
just work, because it builds on a mechanism that has existed for ages.

And that's the one thing that a quoting scheme has going for it: it only
impacts the variable when there is something to be quoted. So if your
repo path has a colon in it (or semi-colon on Windows) _and_ you are
calling something like jgit from your hook, then you might see a
failure. But either of those by itself is not a problem.

Side note: It makes me wonder if all implementations even support
GIT_ALTERNATE_OBJECT_DIRECTORIES. JGit seems to, looks like libgit2 does
not. The most backwards-compatible thing is not enabling quarantine by
default, and then there's no chance of regression, and you are
responsible for making sure you have a compatible setup before turning
the feature on. But like I said, I'd love to avoid that if we can.

So here's the array of options, as I see it:

  1. Disable quarantine by default; only turn it on if you don't have
     any of the funny corner cases.

  2. Introduce an extra single-item environment variable that gets
     appended to the list of alternates, and side-steps quoting issues.

  3. Introduce a new variable that can hold an alternate list, and
     either teach it reasonable quoting semantics, or give it a
     less-common separator.

  4. Introduce quoting to the existing variable, but make the quoting
     character sufficiently obscure that nobody cares about the lack of
     backwards compatibility.

I actually think (4) is reasonably elegant, except that the resulting
quoting scheme would be vaguely insane to look at. E.g., if we pick
newline as the quote character (not the separator), then you end up
with:

  $ repo=foo:bar.git
  $ GIT_ALTERNATE_OBJECT_DIRECTORIES=$(echo $repo | perl -pe 's/:/\n$&/')
  $ echo "$GIT_ALTERNATE_OBJECT_DIRECTORIES"
  foo
  :bar.git

It's pleasant for machines, but not for humans.

So I dunno. Opinions on those 4 options are welcome.

-Peff

Re: [BUG] Colon in remote urls

[Klaus Ethgen]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Hello,

Am Fr den  9. Dez 2016 um 20:07 schrieb Junio C Hamano:
> Jeff King <peff@peff.net> writes:
> > (One other option is to just declare that the quarantine feature doesn't
> > work with colons in the pathname, but stop turning it on by default. I'm
> > not sure I like that, though).
> 
> I think we long time ago in 2005 have declared that a colon in a
> directory name would not work for Git repositories because of things
> like GIT_CEILING_DIRECTORIES, GIT_ALTERNATE_OBJECT_DIRECTORIES; so I
> do not think we terribly mind that direction.

That is the first I hear and I really wonder about.

A colon a perfectly allowed character in POSIX filesystems.

Moreover, it was no problem before and was introduced as a problem just
in that version. Even more, a pull (and so a clone I believe) of such a
path is absolutely ok. Just the push fails.

> > Here's a rough idea of what the quoting solution could look like. It
> > should make your case work, but I'm not sure what we want to do about
> > backwards-compatibility, if anything.
> 
> Yes, obviously it robs from those with backslash in their pathnames
> to please those with colons; we've never catered to the latter, so I
> am not sure if the trade-off is worth it.

As I quote above, a colon is perfect common in POSIX filesystems. A
backslash is at least uncommon and always needed to quote as it, most
often, has special meaning to os/shell.

I cannot see why a tool (as git is) should decide what characters are
"bad" and what are "good". If the filesystem beneath supports it...

By the way, I didn't find anywhere in git documentation that there are
"bad" chars around.

> I can see how adding a new environment variable could work, though.
> A naive way would be to add GIT_ALT_OBJ_DIRS that uses your quoting
> when splitting its elements, give it precedence over the existing
> one (or allow to use both and take union as the set of alternate
> object stores) and make sure that the codepaths we use internally
> uses the new variable.  But if the quarantine codepath will stay to
> be the only user of this mechanism (and I strongly suspect that will
> be the case), the new environment could just be pointing at a single
> directory, i.e. GIT_OBJECT_QUARANTINE_DIRECTORY, whose value is
> added without splitting to the list of alternate object stores, and
> the quarantine codepath can export that instead.

I didn't get it, why is there a need to split? I mean, it is not
possible to push to two locations at the same time, so why is there
splitting at all?

Regards
   Klaus
- -- 
Klaus Ethgen                                       http://www.ethgen.ch/
pub  4096R/4E20AF1C 2011-05-16            Klaus Ethgen <Klaus@Ethgen.ch>
Fingerprint: 85D4 CA42 952C 949B 1753  62B3 79D0 B06F 4E20 AF1C
-----BEGIN PGP SIGNATURE-----
Comment: Charset: ISO-8859-1

iQGzBAEBCgAdFiEEMWF28vh4/UMJJLQEpnwKsYAZ9qwFAlhLyvIACgkQpnwKsYAZ
9qyBzgv9EzMEWrEgsTd1Z0gjpzpJlhpO8R2I7H4mGvEjlxoTXtUNwjvM+ojAYzaI
F34IBRv9BCha+h7I6ccoaAsfmurz73lA1AKy1IFPrYAoxompYLomC9exY+8+ggdN
G2uVbMTmiL+CxJGo0ItxmiQCcv7himVoyto70Dekc7se+panbzCq/vG4+Rz7pwRn
sWnlKs0tQomi6QXbibo8992v4ECkAXzE2Xc/l5DvosSwNNPsqgdeeiNHEMDTbQq8
jqencfOruCfyMnQ0ejCaTWNbYY5SVUtfWikwta12jB06D1BgHTCRVKZCfhoHJx5+
Ffqj8uuiCJuZGQcopzJWiYU5X+SUHz7Ya+iA3VQOxNEKsGAZgq6QQqDcd0y9fyPt
pzMAYo26GRioDiuQZzZ4CzA5eC0wWozv3oESsKLD5RsbWHV/9ODbpr7lHMW2TmUp
H2vZhre1K/ZX2bODQByJoRNtDUqKvZmI6GsbXrvRAFKF4aCLByFIgcrZprmj++DH
jlb25tjq
=jOGb
-----END PGP SIGNATURE-----

Re: [BUG] Colon in remote urls

[Klaus Ethgen]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Am Fr den  9. Dez 2016 um 22:32 schrieb Johannes Sixt:
> There are too many systems out there that use a backslash in path names. I
> don't think it is wise to use it also as the quoting character.

Well, the minority, I believe. And only the minority where the command
line git is used anywhere.

But for the majority of OS, where the command line git is in use
(instead of graphically third party git tools) is perfect known for
backslash as escaping character. However, don't forget that a backslash
could also be part of the file name.

Regads
   Klaus
- -- 
Klaus Ethgen                                       http://www.ethgen.ch/
pub  4096R/4E20AF1C 2011-05-16            Klaus Ethgen <Klaus@Ethgen.ch>
Fingerprint: 85D4 CA42 952C 949B 1753  62B3 79D0 B06F 4E20 AF1C
-----BEGIN PGP SIGNATURE-----
Comment: Charset: ISO-8859-1

iQGzBAEBCgAdFiEEMWF28vh4/UMJJLQEpnwKsYAZ9qwFAlhLy64ACgkQpnwKsYAZ
9qzYfQwAmVIR+bVOvOcZ+yu7HddC5mwo7st6w+vPcdLKpFWcHIhsG//cHq6he+mm
/Dmfhnc4Yp+dSy7Z99p9DV67hAj0Zxj2koxBo4eCdwWhnKrphCHSST8j2IxIg/0h
Y1axQEBc5hV9nImTYmOks0pa5c9wKkZS36aTjP63PKgIv46A8RDY/QXAm2uO4kjj
gfBEiDVkQA99QlvpP1qbuRCK3QUmfqxrP9ldiAhmuDBbNv2smiBxhoN3E6NgKTJG
fM6WjaZPUqpDUiky5gO0xfOpf6s2c/GMTEO1I5aWom6VKtrOoYUyMlyeMiqALWj7
KfN7TJfDqp3THo0AkLXuukrNMv9gdfgiAimGqYgSfFM9P60aPjGeC7nBt875PSae
jdVjIGyl0tHZzSIbBoSecQqx8h65eZaHLaS1uNf704m+EwIs1X2daI7Re3DcAyzL
EWJxtZyZbG/GooCdA9deywlEAtGNOdIg+p+YkThsmEGiaOir/fAMyviSP/jONTCq
OC5oHOcK
=x9y4
-----END PGP SIGNATURE-----

Re: [BUG] Colon in remote urls

[Klaus Ethgen]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Am Sa den 10. Dez 2016 um  9:26 schrieb Jeff King:
> Yeah, I picked it arbitrarily as the common quoting character, but I
> agree it probably makes backwards compatibility (and general usability
> when you have to double-backslash each instance) pretty gross on
> Windows.

Well, I don't know of many people using the original git on windows.
Most of them using some graphical third-party tools.

The main git suite is most often used on linux where a colon is a valid
character For example using /mnt/c: as mount path for windows file
systems or /bla/foo/dated_repository_2016-12-10_12:00.git for dated and
timed repositories.

My btrfs snapshot dir looks like:
   ~snapshot> l -gGhN
   [...]
   drwxr-x--x 1 296 2016-07-30T13:55 daily_2016-12-10_00:00:01.270213478
   drwxr-x--x 1 296 2016-07-30T13:55 hourly_2016-12-10_05:00:01.372037552
   [...]

Compared to the backslash, although it is a perfect legal character in
POSIX file systems, I do not know any use of it.

Regards
   Klaus
- -- 
Klaus Ethgen                                       http://www.ethgen.ch/
pub  4096R/4E20AF1C 2011-05-16            Klaus Ethgen <Klaus@Ethgen.ch>
Fingerprint: 85D4 CA42 952C 949B 1753  62B3 79D0 B06F 4E20 AF1C
-----BEGIN PGP SIGNATURE-----
Comment: Charset: ISO-8859-1

iQGzBAEBCgAdFiEEMWF28vh4/UMJJLQEpnwKsYAZ9qwFAlhLzc0ACgkQpnwKsYAZ
9qy1jQv/Wcafo8nJuy/dNIpxN5tNaLEENrY6a2dkv379F2miEJYROlWO6UzG86hY
0WIZAm5BKK6SpPVztTMcs2GHPF0iCB4V4RyQFdFa73OhaAgHOJRdy50eaGSz6vt6
lDZkJZsG0FoXcT6Fapdl5xZeoNDXjPcYH/7yFQ7VjMD5HTpLDIs8E5Mb8V1jwehV
JKzQd136vksS2qB96jElAYonXFwImvYfTplH3nELJh/kKRJOT8Mzgj/+X7vxnQcC
NISiLysSxqPm5d9yDsfN1eofMNGn2zgJZStOP6jNV2yqldMgN0fJX4Mt449GpBO8
OSYjN828QsDYXCWdTCKxbLCxjfNxfvQgHHR7ugSlf9xPrro3MjQjg2cMhZ/fCzCm
XcC4X+Iyec2F0wHSQiXqlb7wiOXa1Oup6zmTRe/G5HkhlCap/+R2nOCfkqEEwhkB
moYTqfETqqTJUJiiYVM/U8LBFWGnBBCGWgRPzyNdFna+WnvD93s9JPeg7q9qFm6x
8flMJBm8
=M5IW
-----END PGP SIGNATURE-----

Re: [BUG] Colon in remote urls

[Jeff King]

On Sat, Dec 10, 2016 at 10:29:28AM +0100, Klaus Ethgen wrote:

> > I think we long time ago in 2005 have declared that a colon in a
> > directory name would not work for Git repositories because of things
> > like GIT_CEILING_DIRECTORIES, GIT_ALTERNATE_OBJECT_DIRECTORIES; so I
> > do not think we terribly mind that direction.
> 
> That is the first I hear and I really wonder about.
> 
> A colon a perfectly allowed character in POSIX filesystems.

Sure, it's allowed, but it will cause problems due to other syntactic
conventions.  Try putting "/usr/path:with:colons" into your $PATH
variable, for instance. Try rsyncing "xxx:yyy.git" somewhere.

Git does have heuristics for figuring out the difference between
"host:repo.git" as an SSH remote versus a local path, but they're not
foolproof.

> Moreover, it was no problem before and was introduced as a problem just
> in that version. Even more, a pull (and so a clone I believe) of such a
> path is absolutely ok. Just the push fails.

Sort of. This has always been a problem with the variables Junio
mentioned. The change in v2.11 is that the alternates subsystem is being
used in some cases where it wasn't before, which is surfacing this
limitation in more places.

> > directory, i.e. GIT_OBJECT_QUARANTINE_DIRECTORY, whose value is
> > added without splitting to the list of alternate object stores, and
> > the quarantine codepath can export that instead.
> 
> I didn't get it, why is there a need to split? I mean, it is not
> possible to push to two locations at the same time, so why is there
> splitting at all?

Because the new quarantine feature[1] is built on top of the existing
alternates mechanism, which can have several sources.

I do think we should address this as a regression, but I think repo
names with colons are always going to suffer from some corner cases.

-Peff

[1] See 25ab004c5 and the commits leading up to it for more discussion
    of what the new feature is, if you're curious.

Re: [BUG] Colon in remote urls

[Jeff King]

On Sat, Dec 10, 2016 at 10:41:33AM +0100, Klaus Ethgen wrote:

> Am Sa den 10. Dez 2016 um  9:26 schrieb Jeff King:
> > Yeah, I picked it arbitrarily as the common quoting character, but I
> > agree it probably makes backwards compatibility (and general usability
> > when you have to double-backslash each instance) pretty gross on
> > Windows.
> 
> Well, I don't know of many people using the original git on windows.
> Most of them using some graphical third-party tools.

I laid out options for addressing the problem elsewhere, but I want to
make clear that this line of reasoning is not likely to get any traction
here. Git for Windows is a non-trivial part of the user base, and we do
care about avoiding regressions there.

-Peff

Re: [BUG] Colon in remote urls

[Klaus Ethgen]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Hi,

Am Sa den 10. Dez 2016 um 11:24 schrieb Jeff King:
> > A colon a perfectly allowed character in POSIX filesystems.
> 
> Sure, it's allowed, but it will cause problems due to other syntactic
> conventions.  Try putting "/usr/path:with:colons" into your $PATH
> variable, for instance.

True.

> Try rsyncing "xxx:yyy.git" somewhere.

Only a problem when the part before the colon has no directory limiter
(/) in it. And even then there is ways to work around that limitation.

And in this case, it is pretty good documented. As I told, I never
heared of such limitation in git commands.

> Git does have heuristics for figuring out the difference between
> "host:repo.git" as an SSH remote versus a local path, but they're not
> foolproof.

Well, that is the reason why I first tried to solve it via file://...

Note, I have no problem with it, if that char has to be qouted somehow;
if it is clearly documented. But also then, the handling should be
consistent. In git (in this version) it is not. Pull works without
problems but push dosn't.

> > Moreover, it was no problem before and was introduced as a problem just
> > in that version. Even more, a pull (and so a clone I believe) of such a
> > path is absolutely ok. Just the push fails.
> 
> Sort of. This has always been a problem with the variables Junio
> mentioned. The change in v2.11 is that the alternates subsystem is being
> used in some cases where it wasn't before, which is surfacing this
> limitation in more places.

- -v please. I didn't get it with that alternate stuff in push.

A link to some man page is ok too.

> > > directory, i.e. GIT_OBJECT_QUARANTINE_DIRECTORY, whose value is
> > > added without splitting to the list of alternate object stores, and
> > > the quarantine codepath can export that instead.
> > 
> > I didn't get it, why is there a need to split? I mean, it is not
> > possible to push to two locations at the same time, so why is there
> > splitting at all?
> 
> Because the new quarantine feature[1] is built on top of the existing
> alternates mechanism, which can have several sources.

I'll clone the repo and read about, thanks for the pointer.

I even have to find out about that alternates mechanism and what it has
to do with push but not with pull.

Regards
   Klaus
- -- 
Klaus Ethgen                                       http://www.ethgen.ch/
pub  4096R/4E20AF1C 2011-05-16            Klaus Ethgen <Klaus@Ethgen.ch>
Fingerprint: 85D4 CA42 952C 949B 1753  62B3 79D0 B06F 4E20 AF1C
-----BEGIN PGP SIGNATURE-----
Comment: Charset: ISO-8859-1

iQGzBAEBCgAdFiEEMWF28vh4/UMJJLQEpnwKsYAZ9qwFAlhL3PIACgkQpnwKsYAZ
9qwVXAv/VQPtAfete9ZwVUqjdiALVv6n6Cyy+AyTNyLsj56/83ibl5OIBJyr1qDb
e1QueLKEH6qyln+rC5vgejjq4AHk5aPhAx1IpJaaKJKh298c+IImIMxy+84m8xt5
x39u8Kwuz3oGQNshlTed3/qVUR/zzLzPhbFuvHKk3wXDcIJHjDBmBJ4CWyvmyNOh
OfOnXaqu4ohNJpwlCNsJvojjstdcpl6Uj7UM5BIdmw1JFuZClw0ljWLHDqC1YIma
2QbcJ7eY29E+uR6sJKbXuWLgVDE+RrhbBn/GVBATxP5giBLa2z7+gLSMwxcZjTmv
2gd5nhjBL0aIWrr7IIsgcW9I0P/PTazMGSSBsgupjXqA+/pEBL1ePFoK930F/Hdx
Bqh28jT5OYDoaIQHHDm3z6eZzqysdT2tc+uvBBa+g9NA/D6RzEw1qeRmNvvd4UeS
r8rENu7+nLibUE4JjQqDWKwF6FsZkwVH5xXZZ/VQoZHfFQep73ZnQGz3PcfKXGkl
5BMLh24Y
=FX+B
-----END PGP SIGNATURE-----

Re: [BUG] Colon in remote urls

[Klaus Ethgen]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Am Sa den 10. Dez 2016 um 11:26 schrieb Jeff King:
> On Sat, Dec 10, 2016 at 10:41:33AM +0100, Klaus Ethgen wrote:
> 
> > Am Sa den 10. Dez 2016 um  9:26 schrieb Jeff King:
> > > Yeah, I picked it arbitrarily as the common quoting character, but I
> > > agree it probably makes backwards compatibility (and general usability
> > > when you have to double-backslash each instance) pretty gross on
> > > Windows.
> > 
> > Well, I don't know of many people using the original git on windows.
> > Most of them using some graphical third-party tools.
> 
> I laid out options for addressing the problem elsewhere, but I want to
> make clear that this line of reasoning is not likely to get any traction
> here. Git for Windows is a non-trivial part of the user base, and we do
> care about avoiding regressions there.

I value that.

I just wanted to point out what my observations are and too I don't want
to see that windows stuff (what is the minority in this case) will get
more attention or more priority as stuff on linux (which pretty much is
the majority, I believe). Don't get me wrong, I see them both as
important.

And even on windows there are pathes with colon in it. (read c:\...,
d:\...)

Regards
   Klaus
- -- 
Klaus Ethgen                                       http://www.ethgen.ch/
pub  4096R/4E20AF1C 2011-05-16            Klaus Ethgen <Klaus@Ethgen.ch>
Fingerprint: 85D4 CA42 952C 949B 1753  62B3 79D0 B06F 4E20 AF1C
-----BEGIN PGP SIGNATURE-----
Comment: Charset: ISO-8859-1

iQGzBAEBCgAdFiEEMWF28vh4/UMJJLQEpnwKsYAZ9qwFAlhL3ksACgkQpnwKsYAZ
9qznAgv+IPrgt2yvFE9m/YpZv7viSlV9SsIyr6DjvK1C06N9IZes69z4yywTSjVf
L5x4ecWEg+k4N7ET+qNp1iaFS70juNvWg0hysbteczxElNGOerR8pEWXxKA+8xpN
tcY55ZcbBgihPDlQ+ztHAGlngD7E3wwMbzSC5kGCPYYe7Kw9mhLrXzX/MN5V2H/k
60XXsTkW05kUg01ofFATNppFLJHDTFJhEHiZmytJ5se5fTAUXM0gwFkCYEcPh+72
d6GsodVNRW++i9ojAEqglPbAaEAAFg1MOULa3KN2xsIYhvyZ8P4hNc2DzPcAsdtr
8ngoJb80XQ+gLzwJ80RjUnUNFLE+BFUzLrCRxgYyjk3kD7rewLUFHJkte05WBTRv
t21V/AudDC8i8z7XnQtAFpV8QHjTv00nMqV9e2iaTqmd7QJ8bPvDQmSu86/x3MMM
/2aFUcPmRu1Pp+XGm8KUDOF+kba8IJAi7Zs0UCNdyj6domjZBsE9N3pKkcbxsNBJ
0+5Ln4Vi
=twtR
-----END PGP SIGNATURE-----

Re: [BUG] Colon in remote urls

[Johannes Schindelin]

Hi Klaus,

On Sat, 10 Dec 2016, Klaus Ethgen wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
> 
> Am Fr den  9. Dez 2016 um 22:32 schrieb Johannes Sixt:
> > There are too many systems out there that use a backslash in path names. I
> > don't think it is wise to use it also as the quoting character.
> 
> Well, the minority, I believe. And only the minority where the command
> line git is used anywhere.

Please provide evidence for such bold assumptions.

Ciao,
Johannes

Re: [BUG] Colon in remote urls

[Klaus Ethgen]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Hi,

Am Sa den 10. Dez 2016 um 19:18 schrieb Johannes Schindelin:
> On Sat, 10 Dec 2016, Klaus Ethgen wrote:
> > Am Fr den  9. Dez 2016 um 22:32 schrieb Johannes Sixt:
> > > There are too many systems out there that use a backslash in path names. I
> > > don't think it is wise to use it also as the quoting character.
> > Well, the minority, I believe. And only the minority where the command
> > line git is used anywhere.
> 
> Please provide evidence for such bold assumptions.

How is it "bold" to see that the majority of widows users does not use
or even like command line tools. And as git is command line tool (most
of them), users does use third party tools instead of the original.

I know companies where the "developers" doesn't even know of the
existent of a git command line use. They look with owe when they see
that I use a shell to use git.

Regards
   Klaus
- -- 
Klaus Ethgen                                       http://www.ethgen.ch/
pub  4096R/4E20AF1C 2011-05-16            Klaus Ethgen <Klaus@Ethgen.ch>
Fingerprint: 85D4 CA42 952C 949B 1753  62B3 79D0 B06F 4E20 AF1C
-----BEGIN PGP SIGNATURE-----
Comment: Charset: ISO-8859-1

iQGzBAEBCgAdFiEEMWF28vh4/UMJJLQEpnwKsYAZ9qwFAlhNMioACgkQpnwKsYAZ
9qzPygv/ZZr+qW4y/JoTWP6BVu+qDFhean7mHoj5vCSgXwCPzZHSvWIDLdqcrboR
UR7K3oTz9yMaoMRiNq7pu/QBJlwRSJ8ByqSde8mzXOVTqvEC5kvLugU3Ehc1fs0u
ZpoQvXBy0SrpKcuNApMdTFMO9OmCwRNAt2JecCQqyQi6hs6Ws5xTCReOEry00wb/
RdLKOpXwOn5n3ESRAQcqLWhWGs9aUVrfQRCHR2rIYsjx1s/tt+NVWa0hzTnJZt3T
wcQDlGjgeXsu8gJHPNSxAJv5paiNK4JG5x6UUOUuAzmvIYmwd6kEiyNQctTRd0JM
ZCBEYnmZQhHbvrkyKsVvUYJhE9FT0hKMAJO791ZiLCN696EJR4BCOZ9I+7GePFtY
dxb3RNsI9imCXqAHyaguY5tQImzc7P5eQfvH4CdmI9DOmwMUlirvt7pjT94pLFNQ
pxFphD+gd5tUL6QL5fmoUFQVQacQ9Vfs2riTiHerWnBq8P1Hw4KWVYd6ImFo8u3o
aWvUfXFg
=1Yj3
-----END PGP SIGNATURE-----

Re: [BUG] Colon in remote urls

[Philip Oakley]

From: "Klaus Ethgen" <Klaus@Ethgen.ch>
To: <git@vger.kernel.org>
> Am Sa den 10. Dez 2016 um 19:18 schrieb Johannes Schindelin:
>> On Sat, 10 Dec 2016, Klaus Ethgen wrote:
>> > Am Fr den  9. Dez 2016 um 22:32 schrieb Johannes Sixt:
>> > > There are too many systems out there that use a backslash in path 
>> > > names. I
>> > > don't think it is wise to use it also as the quoting character.
>> > Well, the minority, I believe. And only the minority where the command
>> > line git is used anywhere.
>>
>> Please provide evidence for such bold assumptions.
>
> How is it "bold" to see that the majority of widows users does not use
> or even like command line tools. And as git is command line tool (most
> of them), users does use third party tools instead of the original.
>
> I know companies where the "developers" doesn't even know of the
> existent of a git command line use. They look with owe when they see
> that I use a shell to use git.
>
Hasn't this drifted a little off topic. The issue was typeable characters in 
URLs and how that affects the accessibility of various paths.

We (Git) should be avoiding lock-in to particular systems where possible.

--

Philip

Re: [BUG] Colon in remote urls

[Johannes Schindelin]

Hi Klaus,

On Sun, 11 Dec 2016, Klaus Ethgen wrote:

> Am Sa den 10. Dez 2016 um 19:18 schrieb Johannes Schindelin:
> > On Sat, 10 Dec 2016, Klaus Ethgen wrote:
> > > Am Fr den  9. Dez 2016 um 22:32 schrieb Johannes Sixt:
> > > > There are too many systems out there that use a backslash in path names. I
> > > > don't think it is wise to use it also as the quoting character.
> > > Well, the minority, I believe. And only the minority where the command
> > > line git is used anywhere.
> > 
> > Please provide evidence for such bold assumptions.
> 
> How is it "bold" to see that the majority of widows users does not use
> or even like command line tools.

First of all, it is "Windows users", not "widows users", unless, of
course, you want to discuss things that are completely inappropriate for
this list.

Second, you still did not back up your claim with anything resembling
evidence, instead just reiterating your beliefs. That is not good enough.

Third, my experience contradicts your beliefs rather violently.

So let's try some evidence for a change: the 64-bit installer of Git for
Windows v2.10.2 was downloaded over 1.25 million times. That is not a
negligible number.  If you want to go back to v2.8.1, it is even 3.8
million downloads. That is a tall number to call a minority.

Now let's look at your claim that Windows users do not use the
command-line. The mere existence of posh-git (Powershell bindings for Git)
is already a contradiction to that claim.

Even if that was not enough, the Git for Windows bug tracker is full of
reports of users who clearly use the command-line.

And there is more evidence: When comparing the download numbers of the
different Git for Windows versions, one thing really sticks out: those
versions were downloaded the most (by a factor of more than 2x over the
other versions) which were made available through Visual Studio's
"Download command-line Git tools" feature, e.g. v2.8.1 and v2.9.0. That is
a rather strong indicator that users wanted to use the command-line.

Fourth, even if Windows users were the minority, and even if Windows users
were not using the command-line, which are claims soundly refuted by the
evidence I presented above, the fact alone that you are talking about
putting a group of people at a disadvantage based merely on your belief
that they are in a minority should not inform us, the Git developers, on
any kind of policy decision.

We will not intentionally break Git usage, or make Git usage hard, for
a specific group of Git users, unless there are technical reasons to
do that. Demographic reasons do not count.

For example, we will not make Git hard to use for female programmers,
on the grounds that they currently constitute a minority.

> I know companies where the "developers" doesn't even know of the
> existent of a git command line use. They look with owe when they see
> that I use a shell to use git.

I must have spoken to hundreds of Git for Windows users, and must have
been in communication with many more via email or bug tracker, and I
cannot recall a single one who used Git without using the command-line.

Note: I do not count my personal experience here as evidence, but the
numbers alone are a strong indicator to me that your argument has a pretty
weak foundation.

Ciao,
Johannes

P.S.: Maybe reply-to-all in the future; it is the custom on this here
mailing list.

Re: [BUG] Colon in remote urls

[Klaus Ethgen]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Hi,

Am Mo den 12. Dez 2016 um 12:03 schrieb Johannes Schindelin:
> On Sun, 11 Dec 2016, Klaus Ethgen wrote:
> > Am Sa den 10. Dez 2016 um 19:18 schrieb Johannes Schindelin:
> > > On Sat, 10 Dec 2016, Klaus Ethgen wrote:
> > > > Am Fr den  9. Dez 2016 um 22:32 schrieb Johannes Sixt:
> > > > > There are too many systems out there that use a backslash in path names. I
> > > > > don't think it is wise to use it also as the quoting character.
> > > > Well, the minority, I believe. And only the minority where the command
> > > > line git is used anywhere.
> > > Please provide evidence for such bold assumptions.
> > How is it "bold" to see that the majority of widows users does not use
> > or even like command line tools.
> 
> First of all, it is "Windows users", not "widows users", unless, of
> course, you want to discuss things that are completely inappropriate for
> this list.

Sorry to have a typo in my response.

> Second, you still did not back up your claim with anything resembling
> evidence, instead just reiterating your beliefs. That is not good enough.

Well, my evidence is what I seen with many windows users in the past. I
have no link or something like that. I just shared that observation.

> Third, my experience contradicts your beliefs rather violently.

So we have two different observations. Good. Have no problem with that.

[...]
> Now let's look at your claim that Windows users do not use the
> command-line. The mere existence of posh-git (Powershell bindings for Git)
> is already a contradiction to that claim.

Is that the same git tools or is it a separate implementation?

[Proof of many downloads and other]

Proofs accepted. They do not match my observations but ok.

> Fourth, even if Windows users were the minority, and even if Windows users
> were not using the command-line, which are claims soundly refuted by the
> evidence I presented above, the fact alone that you are talking about
> putting a group of people at a disadvantage based merely on your belief
> that they are in a minority should not inform us, the Git developers, on
> any kind of policy decision.
> 
> We will not intentionally break Git usage, or make Git usage hard, for
> a specific group of Git users, unless there are technical reasons to
> do that. Demographic reasons do not count.

Sorry, but I was posting my answer to the comment of preferring not to
allow ":" in prefer of using "\" for quoting. I never wanted to attack
you. It was just a response... And I do not remember attacking you
personally to style me "bold".

Currently the source of that all was, that push to pathes with ":" did
break in the current version. I did not ask for implementing something
instead of something different. I just ask for fixing of a regression.

Moreover, as you might know, windows users are more affected by that bug
as they have a colon in every absolute path. Nevertheless I had that
problem on linux but with a map to a windows share.

For more, I have to agree with Philip, that this is a bit off topic. So
please stop taking single words on the personal level. Please stay on
the technical level. My english is to bad to go to personal level
discussions. And I don't want to spend time to personal or (as you
tried) feminism discussions.

Regards
   Klaus
- -- 
Klaus Ethgen                                       http://www.ethgen.ch/
pub  4096R/4E20AF1C 2011-05-16            Klaus Ethgen <Klaus@Ethgen.ch>
Fingerprint: 85D4 CA42 952C 949B 1753  62B3 79D0 B06F 4E20 AF1C
-----BEGIN PGP SIGNATURE-----
Comment: Charset: ISO-8859-1

iQGzBAEBCgAdFiEEMWF28vh4/UMJJLQEpnwKsYAZ9qwFAlhOjvwACgkQpnwKsYAZ
9qyb2wwAuUFTRrOQcJ6mDLx5bHPNrQy6e4ify4qWcfcp3VJhOLpA56CdJwwA2bcl
gcllUwOn+SSjjYaRmuFQ6aY0/oKDEtF98Zh01pXLnAgY+Dabg9gfBCMWyWv8LxUf
NWHADPpSkY2eongvm6aXOGTqNvU73wriuQKM6BwG49bJUpdR1q8Fe+DjtufrS8eW
ALeGoEFrbm0aIbd121AvWw53hCz3j9ssDpGdFefkhjeJnRcSSDmzTID5KcuWME9P
L67BjGJb3swYIpw3Sdg7LjWnMK0o9GpzfZVNuJFMsRHBQUaWAQvVG79AHe/5QI9w
pq0K5C6frllP5CgcoD2/H+8F8sMD7BrhyN1MxFRdaa4eCEh/hFxZV2nIfbZnx0SS
5/SNMKcCdly+yodZCgohU4uJuqDBkRIEbr6+OCffsupQMCYkh/Ew/RRa8tMN26bN
45/ferqMK1KEenpllXGNHi3/a9dT5CaqEvLjErdatChhQR6i7QbA5B3KhXKRSsRz
YPbYc7rc
=e8K5
-----END PGP SIGNATURE-----

Re: [BUG] Colon in remote urls

[Johannes Schindelin]

Hi Klaus,

you probably missed the note about reply-to-all being customary on this
list. The reason is that this list is high-volume, and open to
non-subscribers. You may want to get into the habit of hitting
reply-to-all when responding to mails from this list, in case that you
plan to continue communicating on this forum.

On Mon, 12 Dec 2016, Klaus Ethgen wrote:

> Am Mo den 12. Dez 2016 um 12:03 schrieb Johannes Schindelin:
> > On Sun, 11 Dec 2016, Klaus Ethgen wrote:
> > > Am Sa den 10. Dez 2016 um 19:18 schrieb Johannes Schindelin:
> > > > On Sat, 10 Dec 2016, Klaus Ethgen wrote:
> > > > > Am Fr den  9. Dez 2016 um 22:32 schrieb Johannes Sixt:
> > > > > > There are too many systems out there that use a backslash in path names. I
> > > > > > don't think it is wise to use it also as the quoting character.
> > > > > Well, the minority, I believe. And only the minority where the command
> > > > > line git is used anywhere.
> > > > Please provide evidence for such bold assumptions.
> > > How is it "bold" to see that the majority of widows users does not use
> > > or even like command line tools.
> > 
> > Second, you still did not back up your claim with anything resembling
> > evidence, instead just reiterating your beliefs. That is not good enough.
> 
> Well, my evidence is what I seen with many windows users in the past. I
> have no link or something like that. I just shared that observation.

Thank you for confirming that you just shared a personal observation and
did not plan to back that up by scientific evidence.

In light of that, let's continue according to Johannes Sixt's insightful
suggestions.

Ciao,
Johannes

[PATCH 0/2] handling alternates paths with colons

[Jeff King]

On Sat, Dec 10, 2016 at 03:51:33AM -0500, Jeff King wrote:

> So here's the array of options, as I see it:
> 
>   1. Disable quarantine by default; only turn it on if you don't have
>      any of the funny corner cases.
> 
>   2. Introduce an extra single-item environment variable that gets
>      appended to the list of alternates, and side-steps quoting issues.
> 
>   3. Introduce a new variable that can hold an alternate list, and
>      either teach it reasonable quoting semantics, or give it a
>      less-common separator.
> 
>   4. Introduce quoting to the existing variable, but make the quoting
>      character sufficiently obscure that nobody cares about the lack of
>      backwards compatibility.

So I started on (4), but writing the user-facing documentation made me
throw up in my mouth a little. Fortunately that inspired me to come up
with a fifth option: introduce a quoting mechanism that needs a more
obscure signature to kick in, but once kicked-in, uses a less obscure
syntax.

So here are patches that do that. It kicks in only when the first
character of a path is a double-quote, and then expects the usual
C-style quoting. My reasoning is that we wouldn't want to sacrifice
'"' for ':', but it's probably OK if we only care about '"' at the very
beginning of a path. And it's consistent with many other parts of git
which allow optional quoting.

Thoughts?

  [1/2]: alternates: accept double-quoted paths
  [2/2]: tmp-objdir: quote paths we add to alternates

 Documentation/git.txt      |  6 ++++++
 sha1_file.c                | 47 +++++++++++++++++++++++++++++++++++-----------
 t/t5547-push-quarantine.sh | 19 +++++++++++++++++++
 t/t5615-alternate-env.sh   | 18 ++++++++++++++++++
 tmp-objdir.c               | 18 +++++++++++++++++-
 5 files changed, 96 insertions(+), 12 deletions(-)

-Peff

[PATCH 1/2] alternates: accept double-quoted paths

[Jeff King]

We read lists of alternates from objects/info/alternates
files (delimited by newline), as well as from the
GIT_ALTERNATE_OBJECT_DIRECTORIES environment variable
(delimited by colon or semi-colon, depending on the
platform).

There's no mechanism for quoting the delimiters, so it's
impossible to specify an alternate path that contains a
colon in the environment, or one that contains a newline in
a file. We've lived with that restriction for ages because
both alternates and filenames with colons are relatively
rare, and it's only a problem when the two meet. But since
722ff7f87 (receive-pack: quarantine objects until
pre-receive accepts, 2016-10-03), which builds on the
alternates system, every push causes the receiver to set
GIT_ALTERNATE_OBJECT_DIRECTORIES internally.

It would be convenient to have some way to quote the
delimiter so that we can represent arbitrary paths.

The simplest thing would be an escape character before a
quoted delimiter (e.g., "\:" as a literal colon). But that
creates a backwards compatibility problem: any path which
uses that escape character is now broken, and we've just
shifted the problem. We could choose an unlikely escape
character (e.g., something from the non-printable ASCII
range), but that's awkward to use.

Instead, let's treat names as unquoted unless they begin
with a double-quote, in which case they are interpreted via
our usual C-stylke quoting rules. This also breaks
backwards-compatibility, but in a smaller way: it only
matters if your file has a double-quote as the very _first_
character in the path (whereas an escape character is a
problem anywhere in the path).  It's also consistent with
many other parts of git, which accept either a bare pathname
or a double-quoted one, and the sender can choose to quote
or not as required.

Signed-off-by: Jeff King <peff@peff.net>
---
This also lets you specify paths that start with "#", or ones that
contain newlines in an alternates file, though I doubt anybody really
cares much in practice. I didn't even bother to add them to the test
suite, mostly because of the compatibility hassle (though I guess we
could just mark them with !MINGW and be OK).

 Documentation/git.txt    |  6 ++++++
 sha1_file.c              | 47 ++++++++++++++++++++++++++++++++++++-----------
 t/t5615-alternate-env.sh | 18 ++++++++++++++++++
 3 files changed, 60 insertions(+), 11 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index af191c51b..98033302b 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -871,6 +871,12 @@ Git so take care if using a foreign front-end.
 	specifies a ":" separated (on Windows ";" separated) list
 	of Git object directories which can be used to search for Git
 	objects. New objects will not be written to these directories.
++
+	Entries that begin with `"` (double-quote) will be interpreted
+	as C-style quoted paths, removing leading and trailing
+	double-quotes and respecting backslash escapes. E.g., the value
+	`"path-with-\"-and-:-in-it":vanilla-path` has two paths:
+	`path-with-"-and-:-in-it` and `vanilla-path`.
 
 `GIT_DIR`::
 	If the `GIT_DIR` environment variable is set then it
diff --git a/sha1_file.c b/sha1_file.c
index 9c86d1924..117307185 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -26,6 +26,7 @@
 #include "mru.h"
 #include "list.h"
 #include "mergesort.h"
+#include "quote.h"
 
 #ifndef O_NOATIME
 #if defined(__linux__) && (defined(__i386__) || defined(__PPC__))
@@ -329,13 +330,40 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,
 	return 0;
 }
 
+static const char *parse_alt_odb_entry(const char *string,
+				       int sep,
+				       struct strbuf *out)
+{
+	const char *end;
+
+	strbuf_reset(out);
+
+	if (*string == '#') {
+		/* comment; consume up to next separator */
+		end = strchrnul(string, sep);
+	} else if (*string == '"' && !unquote_c_style(out, string, &end)) {
+		/*
+		 * quoted path; unquote_c_style has copied the
+		 * data for us and set "end". Broken quoting (e.g.,
+		 * an entry that doesn't end with a quote) falls
+		 * back to the unquoted case below.
+		 */
+	} else {
+		/* normal, unquoted path */
+		end = strchrnul(string, sep);
+		strbuf_add(out, string, end - string);
+	}
+
+	if (*end)
+		end++;
+	return end;
+}
+
 static void link_alt_odb_entries(const char *alt, int len, int sep,
 				 const char *relative_base, int depth)
 {
-	struct string_list entries = STRING_LIST_INIT_NODUP;
-	char *alt_copy;
-	int i;
 	struct strbuf objdirbuf = STRBUF_INIT;
+	struct strbuf entry = STRBUF_INIT;
 
 	if (depth > 5) {
 		error("%s: ignoring alternate object stores, nesting too deep.",
@@ -348,16 +376,13 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,
 		die("unable to normalize object directory: %s",
 		    objdirbuf.buf);
 
-	alt_copy = xmemdupz(alt, len);
-	string_list_split_in_place(&entries, alt_copy, sep, -1);
-	for (i = 0; i < entries.nr; i++) {
-		const char *entry = entries.items[i].string;
-		if (entry[0] == '\0' || entry[0] == '#')
+	while (*alt) {
+		alt = parse_alt_odb_entry(alt, sep, &entry);
+		if (!entry.len)
 			continue;
-		link_alt_odb_entry(entry, relative_base, depth, objdirbuf.buf);
+		link_alt_odb_entry(entry.buf, relative_base, depth, objdirbuf.buf);
 	}
-	string_list_clear(&entries, 0);
-	free(alt_copy);
+	strbuf_release(&entry);
 	strbuf_release(&objdirbuf);
 }
 
diff --git a/t/t5615-alternate-env.sh b/t/t5615-alternate-env.sh
index eec4137ca..69fd8f891 100755
--- a/t/t5615-alternate-env.sh
+++ b/t/t5615-alternate-env.sh
@@ -68,4 +68,22 @@ test_expect_success 'access alternate via relative path (subdir)' '
 	EOF
 '
 
+# set variables outside test to avoid quote insanity; the \057 is '/',
+# which doesn't need quoting, but just confirms that de-quoting
+# is working.
+quoted='"one.git\057objects"'
+unquoted='two.git/objects'
+test_expect_success 'mix of quoted and unquoted alternates' '
+	check_obj "$quoted:$unquoted" <<-EOF
+	$one blob
+	$two blob
+'
+
+test_expect_success 'broken quoting falls back to interpreting raw' '
+	mv one.git \"one.git &&
+	check_obj \"one.git/objects <<-EOF
+	$one blob
+	EOF
+'
+
 test_done
-- 
2.11.0.203.g6657065

[PATCH 2/2] tmp-objdir: quote paths we add to alternates

[Jeff King]

Commit 722ff7f87 (receive-pack: quarantine objects until
pre-receive accepts, 2016-10-03) regressed pushes to
repositories with colon (or semi-colon in Windows in them)
because it adds the repository's main object directory to
GIT_ALTERNATE_OBJECT_DIRECTORIES. The receiver interprets
the colon as a delimiter, not as part of the path, and
index-pack is unable to find objects which it needs to
resolve deltas.

The previous commit introduced a quoting mechanism for the
alternates list; let's use it here to cover this case. We'll
avoid quoting when we can, though. This alternate setup is
also used when calling hooks, so it's possible that the user
may call older git implementations which don't understand
the quoting mechanism. By quoting only when necessary, this
setup will continue to work unless the user _also_ has a
repository whose path contains the delimiter.

Signed-off-by: Jeff King <peff@peff.net>
---
Johannes, please let me know if I am wrong about skipping the test on
!MINGW. The appropriate check there would be ";" anyway, but I am not
sure _that_ is allowed in paths, either.

 t/t5547-push-quarantine.sh | 19 +++++++++++++++++++
 tmp-objdir.c               | 18 +++++++++++++++++-
 2 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh
index 1e5d32d06..6275ec807 100755
--- a/t/t5547-push-quarantine.sh
+++ b/t/t5547-push-quarantine.sh
@@ -33,4 +33,23 @@ test_expect_success 'rejected objects are removed' '
 	test_cmp expect actual
 '
 
+# MINGW does not allow colons in pathnames in the first place
+test_expect_success !MINGW 'push to repo path with colon' '
+	# The interesting failure case here is when the
+	# receiving end cannot access its original object directory,
+	# so make it likely for us to generate a delta by having
+	# a non-trivial file with multiple versions.
+
+	test-genrandom foo 4096 >file.bin &&
+	git add file.bin &&
+	git commit -m bin &&
+	git clone --bare . xxx:yyy.git &&
+
+	echo change >>file.bin &&
+	git commit -am change &&
+	# Note that we have to use the full path here, or it gets confused
+	# with the ssh host:path syntax.
+	git push "$PWD/xxx:yyy.git" HEAD
+'
+
 test_done
diff --git a/tmp-objdir.c b/tmp-objdir.c
index 64435f23a..b2d9280f1 100644
--- a/tmp-objdir.c
+++ b/tmp-objdir.c
@@ -5,6 +5,7 @@
 #include "string-list.h"
 #include "strbuf.h"
 #include "argv-array.h"
+#include "quote.h"
 
 struct tmp_objdir {
 	struct strbuf path;
@@ -79,12 +80,27 @@ static void remove_tmp_objdir_on_signal(int signo)
  */
 static void env_append(struct argv_array *env, const char *key, const char *val)
 {
-	const char *old = getenv(key);
+	struct strbuf quoted = STRBUF_INIT;
+	const char *old;
 
+	/*
+	 * Avoid quoting if it's not necessary, for maximum compatibility
+	 * with older parsers which don't understand the quoting.
+	 */
+	if (*val == '"' || strchr(val, PATH_SEP)) {
+		strbuf_addch(&quoted, '"');
+		quote_c_style(val, &quoted, NULL, 1);
+		strbuf_addch(&quoted, '"');
+		val = quoted.buf;
+	}
+
+	old = getenv(key);
 	if (!old)
 		argv_array_pushf(env, "%s=%s", key, val);
 	else
 		argv_array_pushf(env, "%s=%s%c%s", key, old, PATH_SEP, val);
+
+	strbuf_release(&quoted);
 }
 
 static void env_replace(struct argv_array *env, const char *key, const char *val)
-- 
2.11.0.203.g6657065

Re: [PATCH 2/2] tmp-objdir: quote paths we add to alternates

[Johannes Sixt]

Am 12.12.2016 um 20:53 schrieb Jeff King:
> Johannes, please let me know if I am wrong about skipping the test on
> !MINGW.

Thanks for being considerate. The !MINGW prerequisite is required for 
the test as written.

> The appropriate check there would be ";" anyway, but I am not
> sure _that_ is allowed in paths, either.

That was also my line of thought. I tested earlier today whether a file 
name can have ";", and the OS did not reject it bluntly. Let me see 
whether I can adjust the test case for Windows.

-- Hannes

Re: [PATCH 0/2] handling alternates paths with colons

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So here are patches that do that. It kicks in only when the first
> character of a path is a double-quote, and then expects the usual
> C-style quoting.

The quote being per delimited component is what makes this fifth
approach a huge win.  

All sane components on a list-valued environment are still honored
and an insane component that has either a colon in the middle or
begins with a double-quote gets quoted.  As long as nobody used a
path that begins with a double-quote as an element in such a
list-valued environment (and they cannot be, as using a non-absolute
path as an element does not make much sense), this will be safe, and
a path with a colon didn't work with Git unaware of the new quoting
rule anyway.  Nice.

Re: [PATCH 1/2] alternates: accept double-quoted paths

[Duy Nguyen]

On Tue, Dec 13, 2016 at 2:52 AM, Jeff King <peff@peff.net> wrote:
> Instead, let's treat names as unquoted unless they begin
> with a double-quote, in which case they are interpreted via
> our usual C-stylke quoting rules. This also breaks
> backwards-compatibility, but in a smaller way: it only
> matters if your file has a double-quote as the very _first_
> character in the path (whereas an escape character is a
> problem anywhere in the path).  It's also consistent with
> many other parts of git, which accept either a bare pathname
> or a double-quoted one, and the sender can choose to quote
> or not as required.

At least attr has the same problem and is going the same direction
[1]. Cool. (I actually thought the patch was in and evidence that this
kind of backward compatibility breaking was ok, turns out the patch
has stayed around for years)

[1] http://public-inbox.org/git/%3C20161110203428.30512-18-sbeller@google.com%3E/
-- 
Duy

Re: [PATCH 2/2] tmp-objdir: quote paths we add to alternates

[Jeff King]

On Mon, Dec 12, 2016 at 09:53:11PM +0100, Johannes Sixt wrote:

> > The appropriate check there would be ";" anyway, but I am not
> > sure _that_ is allowed in paths, either.
> 
> That was also my line of thought. I tested earlier today whether a file name
> can have ";", and the OS did not reject it bluntly. Let me see whether I can
> adjust the test case for Windows.

The naive conversion is just:

diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh
index 6275ec807..e195e168b 100755
--- a/t/t5547-push-quarantine.sh
+++ b/t/t5547-push-quarantine.sh
@@ -33,8 +33,13 @@ test_expect_success 'rejected objects are removed' '
 	test_cmp expect actual
 '
 
-# MINGW does not allow colons in pathnames in the first place
-test_expect_success !MINGW 'push to repo path with colon' '
+if test_have_prereq MINGW
+then
+	path_sep=';'
+else
+	path_sep=':'
+fi
+test_expect_success 'push to repo path with (semi-)colon separator' '
 	# The interesting failure case here is when the
 	# receiving end cannot access its original object directory,
 	# so make it likely for us to generate a delta by having
@@ -43,13 +48,13 @@ test_expect_success !MINGW 'push to repo path with colon' '
 	test-genrandom foo 4096 >file.bin &&
 	git add file.bin &&
 	git commit -m bin &&
-	git clone --bare . xxx:yyy.git &&
+	git clone --bare . xxx${path_sep}yyy.git &&
 
 	echo change >>file.bin &&
 	git commit -am change &&
 	# Note that we have to use the full path here, or it gets confused
 	# with the ssh host:path syntax.
-	git push "$PWD/xxx:yyy.git" HEAD
+	git push "$PWD/xxx${path_sep}yyy.git" HEAD
 '
 
 test_done

Does that work?

-Peff

Re: [PATCH 0/2] handling alternates paths with colons

[Jeff King]

On Mon, Dec 12, 2016 at 02:37:08PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > So here are patches that do that. It kicks in only when the first
> > character of a path is a double-quote, and then expects the usual
> > C-style quoting.
> 
> The quote being per delimited component is what makes this fifth
> approach a huge win.  
> 
> All sane components on a list-valued environment are still honored
> and an insane component that has either a colon in the middle or
> begins with a double-quote gets quoted.  As long as nobody used a
> path that begins with a double-quote as an element in such a
> list-valued environment (and they cannot be, as using a non-absolute
> path as an element does not make much sense), this will be safe, and
> a path with a colon didn't work with Git unaware of the new quoting
> rule anyway.  Nice.

We do support non-absolute paths, both in alternates files and
environment variables. It's a nice feature if you want to have a
relocatable family of shared repositories. I'd imagine that most cases
start with "../", though.

-Peff

Re: [PATCH 1/2] alternates: accept double-quoted paths

[Jeff King]

On Tue, Dec 13, 2016 at 06:30:15PM +0700, Duy Nguyen wrote:

> On Tue, Dec 13, 2016 at 2:52 AM, Jeff King <peff@peff.net> wrote:
> > Instead, let's treat names as unquoted unless they begin
> > with a double-quote, in which case they are interpreted via
> > our usual C-stylke quoting rules. This also breaks
> > backwards-compatibility, but in a smaller way: it only
> > matters if your file has a double-quote as the very _first_
> > character in the path (whereas an escape character is a
> > problem anywhere in the path).  It's also consistent with
> > many other parts of git, which accept either a bare pathname
> > or a double-quoted one, and the sender can choose to quote
> > or not as required.
> 
> At least attr has the same problem and is going the same direction
> [1]. Cool. (I actually thought the patch was in and evidence that this
> kind of backward compatibility breaking was ok, turns out the patch
> has stayed around for years)
> 
> [1] http://public-inbox.org/git/%3C20161110203428.30512-18-sbeller@google.com%3E/

Thanks for digging that up. As soon as I came up with the idea[1], I
wanted to use the attr code as an example of a similar problem and
solution, but I couldn't find it in the code. Which makes sense if it
wasn't merged.

I do think it's a pretty reasonable approach in general, and would be OK
for the attributes code.

-Peff

[1] One could argue that I did not come up with the idea at all, but
    rather just remembered that somebody else had done so. :)

Re: [PATCH 1/2] alternates: accept double-quoted paths

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> At least attr has the same problem and is going the same direction
> [1]. Cool. (I actually thought the patch was in and evidence that this
> kind of backward compatibility breaking was ok, turns out the patch
> has stayed around for years)
>
> [1] http://public-inbox.org/git/%3C20161110203428.30512-18-sbeller@google.com%3E/

Seeing that again, I see this in the proposed log message:

    Full pattern must be quoted. So 'pat"t"ern attr' will give exactly
    'pat"t"ern', not 'pattern'.

I cannot tell what it is trying to say.  The code suggests something
quite different, i.e. a line like this

	"pat\"t\"ern" attr

would tell us that a path that matches the pattern

	pat"t"ern

is given 'attr'.  

The log message may need to be cleaned up.  The update by that
commit to the documentation looks alright, thoguh.

Re: [PATCH 2/2] tmp-objdir: quote paths we add to alternates

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The naive conversion is just:
> ...
> -# MINGW does not allow colons in pathnames in the first place
> -test_expect_success !MINGW 'push to repo path with colon' '
> +if test_have_prereq MINGW
> +then
> +	path_sep=';'
> +else
> +	path_sep=':'
> +fi
> ...
> -	git clone --bare . xxx:yyy.git &&
> +	git clone --bare . xxx${path_sep}yyy.git &&

Don't you want to dq the whole thing to prevent the shell from
splitting this into two commands at ';'?  The other one below is OK.

>  
>  	echo change >>file.bin &&
>  	git commit -am change &&
>  	# Note that we have to use the full path here, or it gets confused
>  	# with the ssh host:path syntax.
> -	git push "$PWD/xxx:yyy.git" HEAD
> +	git push "$PWD/xxx${path_sep}yyy.git" HEAD
>  '
>  
>  test_done
>
> Does that work?
>
> -Peff

Re: [PATCH 0/2] handling alternates paths with colons

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Dec 12, 2016 at 02:37:08PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > So here are patches that do that. It kicks in only when the first
>> > character of a path is a double-quote, and then expects the usual
>> > C-style quoting.
>> 
>> The quote being per delimited component is what makes this fifth
>> approach a huge win.  
>> 
>> All sane components on a list-valued environment are still honored
>> and an insane component that has either a colon in the middle or
>> begins with a double-quote gets quoted.  As long as nobody used a
>> path that begins with a double-quote as an element in such a
>> list-valued environment (and they cannot be, as using a non-absolute
>> path as an element does not make much sense), this will be safe, and
>> a path with a colon didn't work with Git unaware of the new quoting
>> rule anyway.  Nice.
>
> We do support non-absolute paths, both in alternates files and
> environment variables. It's a nice feature if you want to have a
> relocatable family of shared repositories. I'd imagine that most cases
> start with "../", though.

Yes.  I was only talking about the environment variable, as you can
tell from my mention of "colon" there.

Re: [PATCH 2/2] tmp-objdir: quote paths we add to alternates

[Jeff King]

On Tue, Dec 13, 2016 at 10:10:04AM -0800, Junio C Hamano wrote:

> > -	git clone --bare . xxx:yyy.git &&
> > +	git clone --bare . xxx${path_sep}yyy.git &&
> 
> Don't you want to dq the whole thing to prevent the shell from
> splitting this into two commands at ';'?  The other one below is OK.

After expansion, I don't think the shell will do any further processing
except for whitespace splitting. E.g.:

  $ debug() { for i in "$@"; do echo "got $i"; done; }
  $ sep=';'
  $ space=' '
  $ debug foo${sep}bar
  got foo;bar
  $ debug foo${space}bar
  got foo
  got bar

I don't mind quoting it to make it more obvious, though.

-Peff

Re: [PATCH 0/2] handling alternates paths with colons

[Jeff King]

On Tue, Dec 13, 2016 at 10:10:54AM -0800, Junio C Hamano wrote:

> > We do support non-absolute paths, both in alternates files and
> > environment variables. It's a nice feature if you want to have a
> > relocatable family of shared repositories. I'd imagine that most cases
> > start with "../", though.
> 
> Yes.  I was only talking about the environment variable, as you can
> tell from my mention of "colon" there.

Right, but we also support relative paths via environment variables. I
don't think that changes the conclusion though. I'm not convinced
relative paths via the environment aren't slightly insane in the first
place, given the discussion in 37a95862c (alternates: re-allow relative
paths from environment, 2016-11-07). And they'd probably start with
"../" as well.

-Peff

Re: [PATCH 0/2] handling alternates paths with colons

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Right, but we also support relative paths via environment variables. I
> don't think that changes the conclusion though. I'm not convinced
> relative paths via the environment aren't slightly insane in the first
> place,

Sorry, a triple negation is above my head.  "relative paths in env
aren't insane" == "using relative paths is OK" and you are not
convinced that it is the case, so you are saying that you think
relative paths in env is not something that is sensible?

> given the discussion in 37a95862c (alternates: re-allow relative
> paths from environment, 2016-11-07). And they'd probably start with
> "../" as well.

Yeah.  In any case, there is a potential regression but the chance
is miniscule---the user has to have been using a relative path that
begins with a double-quote in the environment or in alternates file.

Re: [PATCH 0/2] handling alternates paths with colons

[Jeff King]

On Tue, Dec 13, 2016 at 10:42:39AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Right, but we also support relative paths via environment variables. I
> > don't think that changes the conclusion though. I'm not convinced
> > relative paths via the environment aren't slightly insane in the first
> > place,
> 
> Sorry, a triple negation is above my head.  "relative paths in env
> aren't insane" == "using relative paths is OK" and you are not
> convinced that it is the case, so you are saying that you think
> relative paths in env is not something that is sensible?

I think relative paths in env have very flaky semantics which makes them
inadvisable to use in general. That being said, when we broke even those
flaky semantics somebody complained. So I consider a feature we _do_
support, but not one I would recommend to people.

-Peff

[PATCH 3/2] t5547-push-quarantine: run the path separator test on Windows, too

[Johannes Sixt]

To perform the test case on Windows in a way that corresponds to the
POSIX version, inject the semicolon in a directory name.

Typically, an absolute POSIX style path, such as the one in $PWD, is
translated into a Windows style path by bash when it invokes git.exe.
However, the presence of the semicolon suppresses this translation;
but the untranslated POSIX style path is useless for git.exe.
Therefore, instead of $PWD pass the Windows style path that $(pwd)
produces.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
Am 12.12.2016 um 20:53 schrieb Jeff King:
> Johannes, please let me know if I am wrong about skipping the test on
> !MINGW. The appropriate check there would be ";" anyway, but I am not
> sure _that_ is allowed in paths, either.

Here is a version for Windows. I'd prefer this patch on top instead
of squashing it into yours to keep the $PWD vs. $(pwd) explanation.

The result is the same as yours in all practical matters; but this
version I have already tested.

 t/t5547-push-quarantine.sh | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh
index 6275ec807b..af9fcd833a 100755
--- a/t/t5547-push-quarantine.sh
+++ b/t/t5547-push-quarantine.sh
@@ -33,8 +33,7 @@ test_expect_success 'rejected objects are removed' '
 	test_cmp expect actual
 '
 
-# MINGW does not allow colons in pathnames in the first place
-test_expect_success !MINGW 'push to repo path with colon' '
+test_expect_success 'push to repo path with path separator (colon)' '
 	# The interesting failure case here is when the
 	# receiving end cannot access its original object directory,
 	# so make it likely for us to generate a delta by having
@@ -43,13 +42,20 @@ test_expect_success !MINGW 'push to repo path with colon' '
 	test-genrandom foo 4096 >file.bin &&
 	git add file.bin &&
 	git commit -m bin &&
-	git clone --bare . xxx:yyy.git &&
+
+	if test_have_prereq MINGW
+	then
+		pathsep=";"
+	else
+		pathsep=":"
+	fi &&
+	git clone --bare . "xxx${pathsep}yyy.git" &&
 
 	echo change >>file.bin &&
 	git commit -am change &&
 	# Note that we have to use the full path here, or it gets confused
 	# with the ssh host:path syntax.
-	git push "$PWD/xxx:yyy.git" HEAD
+	git push "$(pwd)/xxx${pathsep}yyy.git" HEAD
 '
 
 test_done
-- 
2.11.0.55.g6a4dbb1

Re: [PATCH 3/2] t5547-push-quarantine: run the path separator test on Windows, too

[Jeff King]

On Tue, Dec 13, 2016 at 08:09:31PM +0100, Johannes Sixt wrote:

> Am 12.12.2016 um 20:53 schrieb Jeff King:
> > Johannes, please let me know if I am wrong about skipping the test on
> > !MINGW. The appropriate check there would be ";" anyway, but I am not
> > sure _that_ is allowed in paths, either.
> 
> Here is a version for Windows. I'd prefer this patch on top instead
> of squashing it into yours to keep the $PWD vs. $(pwd) explanation.
> 
> The result is the same as yours in all practical matters; but this
> version I have already tested.

Yeah, I'm happy to have this on top. The patch itself looks obviously
correct. Thanks!

-Peff

Re: [PATCH 3/2] t5547-push-quarantine: run the path separator test on Windows, too

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> To perform the test case on Windows in a way that corresponds to the
> POSIX version, inject the semicolon in a directory name.
>
> Typically, an absolute POSIX style path, such as the one in $PWD, is
> translated into a Windows style path by bash when it invokes git.exe.
> However, the presence of the semicolon suppresses this translation;
> but the untranslated POSIX style path is useless for git.exe.
> Therefore, instead of $PWD pass the Windows style path that $(pwd)
> produces.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> Am 12.12.2016 um 20:53 schrieb Jeff King:
>> Johannes, please let me know if I am wrong about skipping the test on
>> !MINGW. The appropriate check there would be ";" anyway, but I am not
>> sure _that_ is allowed in paths, either.
>
> Here is a version for Windows. I'd prefer this patch on top instead
> of squashing it into yours to keep the $PWD vs. $(pwd) explanation.
>
> The result is the same as yours in all practical matters; but this
> version I have already tested.

Will queue (I would wait for peff@ to say "OK", but I suspect he
would be OK in this case).

Thanks.


>  t/t5547-push-quarantine.sh | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh
> index 6275ec807b..af9fcd833a 100755
> --- a/t/t5547-push-quarantine.sh
> +++ b/t/t5547-push-quarantine.sh
> @@ -33,8 +33,7 @@ test_expect_success 'rejected objects are removed' '
>  	test_cmp expect actual
>  '
>  
> -# MINGW does not allow colons in pathnames in the first place
> -test_expect_success !MINGW 'push to repo path with colon' '
> +test_expect_success 'push to repo path with path separator (colon)' '
>  	# The interesting failure case here is when the
>  	# receiving end cannot access its original object directory,
>  	# so make it likely for us to generate a delta by having
> @@ -43,13 +42,20 @@ test_expect_success !MINGW 'push to repo path with colon' '
>  	test-genrandom foo 4096 >file.bin &&
>  	git add file.bin &&
>  	git commit -m bin &&
> -	git clone --bare . xxx:yyy.git &&
> +
> +	if test_have_prereq MINGW
> +	then
> +		pathsep=";"
> +	else
> +		pathsep=":"
> +	fi &&
> +	git clone --bare . "xxx${pathsep}yyy.git" &&
>  
>  	echo change >>file.bin &&
>  	git commit -am change &&
>  	# Note that we have to use the full path here, or it gets confused
>  	# with the ssh host:path syntax.
> -	git push "$PWD/xxx:yyy.git" HEAD
> +	git push "$(pwd)/xxx${pathsep}yyy.git" HEAD
>  '
>  
>  test_done

Re: [PATCH 2/2] tmp-objdir: quote paths we add to alternates

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Dec 13, 2016 at 10:10:04AM -0800, Junio C Hamano wrote:
>
>> > -	git clone --bare . xxx:yyy.git &&
>> > +	git clone --bare . xxx${path_sep}yyy.git &&
>> 
>> Don't you want to dq the whole thing to prevent the shell from
>> splitting this into two commands at ';'?  The other one below is OK.
>
> After expansion, I don't think the shell will do any further processing
> except for whitespace splitting. E.g.:

Ah, my mistake.  Staring at too many `eval`s does it to me.

Thanks.

Re: [PATCH 1/2] alternates: accept double-quoted paths

[Duy Nguyen]

On Wed, Dec 14, 2016 at 1:08 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> At least attr has the same problem and is going the same direction
>> [1]. Cool. (I actually thought the patch was in and evidence that this
>> kind of backward compatibility breaking was ok, turns out the patch
>> has stayed around for years)
>>
>> [1] http://public-inbox.org/git/%3C20161110203428.30512-18-sbeller@google.com%3E/
>
> Seeing that again, I see this in the proposed log message:
>
>     Full pattern must be quoted. So 'pat"t"ern attr' will give exactly
>     'pat"t"ern', not 'pattern'.
>
> I cannot tell what it is trying to say.

It's another (bad) way of saying that quoting must start at the
beginning (so the closing quote must be at the end of pattern, i.e.
the whole pattern is quoted).

> The log message may need to be cleaned up.

Yeah. Next time I'll write more or less "Look at Jeff's commit, this
is basically it" when Stephan re-rolls sb/attr
-- 
Duy

[PATCH 4/2] t5615-alternate-env: double-quotes in file names do not work on Windows

[Johannes Sixt]

Protect a recently added test case with !MINGW.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 I don't remember why I did not notice this failure sooner.
 Perhaps I did, but then ran out of time to debug it...

 The patch should go on top of jk/quote-env-path-list-component.

 t/t5615-alternate-env.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5615-alternate-env.sh b/t/t5615-alternate-env.sh
index 69fd8f8911..26ebb0375d 100755
--- a/t/t5615-alternate-env.sh
+++ b/t/t5615-alternate-env.sh
@@ -79,7 +79,7 @@ test_expect_success 'mix of quoted and unquoted alternates' '
 	$two blob
 '
 
-test_expect_success 'broken quoting falls back to interpreting raw' '
+test_expect_success !MINGW 'broken quoting falls back to interpreting raw' '
 	mv one.git \"one.git &&
 	check_obj \"one.git/objects <<-EOF
 	$one blob
-- 
2.11.0.79.gf6b77ca

Re: [PATCH 4/2] t5615-alternate-env: double-quotes in file names do not work on Windows

[Jeff King]

On Wed, Dec 21, 2016 at 10:33:43PM +0100, Johannes Sixt wrote:

> Protect a recently added test case with !MINGW.
> 
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  I don't remember why I did not notice this failure sooner.
>  Perhaps I did, but then ran out of time to debug it...
> 
>  The patch should go on top of jk/quote-env-path-list-component.
> [...]
> -test_expect_success 'broken quoting falls back to interpreting raw' '
> +test_expect_success !MINGW 'broken quoting falls back to interpreting raw' '
>  	mv one.git \"one.git &&
>  	check_obj \"one.git/objects <<-EOF
>  	$one blob

Hmph. I explicitly avoided a colon in the filename so that it would run
on MINGW. Is a double-quote also not allowed?

-Peff

Re: [PATCH 4/2] t5615-alternate-env: double-quotes in file names do not work on Windows

[Johannes Sixt]

Am 21.12.2016 um 23:42 schrieb Jeff King:
> On Wed, Dec 21, 2016 at 10:33:43PM +0100, Johannes Sixt wrote:
>
>> Protect a recently added test case with !MINGW.
>>
>> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
>> ---
>>  I don't remember why I did not notice this failure sooner.
>>  Perhaps I did, but then ran out of time to debug it...
>>
>>  The patch should go on top of jk/quote-env-path-list-component.
>> [...]
>> -test_expect_success 'broken quoting falls back to interpreting raw' '
>> +test_expect_success !MINGW 'broken quoting falls back to interpreting raw' '
>>  	mv one.git \"one.git &&
>>  	check_obj \"one.git/objects <<-EOF
>>  	$one blob
>
> Hmph. I explicitly avoided a colon in the filename so that it would run
> on MINGW. Is a double-quote also not allowed?

It is not allowed; that was my conclusion. But now that you ask, I'll 
double-check.

-- Hannes

Re: [PATCH 4/2] t5615-alternate-env: double-quotes in file names do not work on Windows

[Johannes Sixt]

Am 22.12.2016 um 07:13 schrieb Johannes Sixt:
> Am 21.12.2016 um 23:42 schrieb Jeff King:
>> Hmph. I explicitly avoided a colon in the filename so that it would run
>> on MINGW. Is a double-quote also not allowed?
>
> It is not allowed; that was my conclusion. But now that you ask, I'll
> double-check.

Ok, the point is that I get this error:

mv: cannot move `one.git' to `"one.git'

I don't feel inclined to find out which of 'mv' or the Windows API or 
the NTFS driver prevents the double-quotes and come up with a 
work-around just to get the test to run in some form. Let's leave it at 
the !MINGW prerequisite.

-- Hannes

Re: [PATCH 4/2] t5615-alternate-env: double-quotes in file names do not work on Windows

[Johannes Schindelin]

Hi Hannes,

On Thu, 22 Dec 2016, Johannes Sixt wrote:

> Am 22.12.2016 um 07:13 schrieb Johannes Sixt:
> > Am 21.12.2016 um 23:42 schrieb Jeff King:
> > > Hmph. I explicitly avoided a colon in the filename so that it would
> > > run on MINGW. Is a double-quote also not allowed?
> >
> > It is not allowed; that was my conclusion. But now that you ask, I'll
> > double-check.
> 
> Ok, the point is that I get this error:
> 
> mv: cannot move `one.git' to `"one.git'
> 
> I don't feel inclined to find out which of 'mv' or the Windows API or
> the NTFS driver prevents the double-quotes and come up with a
> work-around just to get the test to run in some form. Let's leave it at
> the !MINGW prerequisite.

Double-quotes are not allowed in file names in Windows. NTFS would allow
them just fine.

Ciao,
Dscho

Re: [PATCH 4/2] t5615-alternate-env: double-quotes in file names do not work on Windows

[Jeff King]

On Thu, Dec 22, 2016 at 08:06:02PM +0100, Johannes Sixt wrote:

> Am 22.12.2016 um 07:13 schrieb Johannes Sixt:
> > Am 21.12.2016 um 23:42 schrieb Jeff King:
> > > Hmph. I explicitly avoided a colon in the filename so that it would run
> > > on MINGW. Is a double-quote also not allowed?
> > 
> > It is not allowed; that was my conclusion. But now that you ask, I'll
> > double-check.
> 
> Ok, the point is that I get this error:
> 
> mv: cannot move `one.git' to `"one.git'
> 
> I don't feel inclined to find out which of 'mv' or the Windows API or the
> NTFS driver prevents the double-quotes and come up with a work-around just
> to get the test to run in some form. Let's leave it at the !MINGW
> prerequisite.

Thank you for double-checking (and sorry if my initial question was
confusing; I somehow missed the mention of dq in your subject line, and
just read the message body).

Your patch seems like the best solution.

-Peff