threads / patch / 8063

patch, 3 partsMove remote parsing into a library file out of builtin-push.

Subject: [PATCH 1/3] Move remote parsing into a library file out of builtin-push.

## tl;dr

10 messages between May 10, 2007 and May 10, 2007. Diffs are folded; open one to read it.

replies: 9people: 2as markdown or json

Daniel Barkalow· May 10, 2007, 02:04 UTC · lore

The new parser is different from the one in builtin-push in two ways: the default is to use the current branch's remote, if there is one, before "origin"; and config is used in preference to remotes.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
 Makefile       |    5 +-
 builtin-push.c |  190 ++++++-----------------------------------------------
 remote.c       |  201 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 remote.h       |   18 +++++
 4 files changed, 242 insertions(+), 172 deletions(-)
 create mode 100644 remote.c
 create mode 100644 remote.h
Show changes to 4 files +242 −172

Makefile, builtin-push.c, remote.c, remote.h

diff --git a/Makefile b/Makefile
index e0a1308..dd64b7d 100644
--- a/Makefile
+++ b/Makefile
@@ -288,7 +288,8 @@ LIB_H = \
 	diff.h object.h pack.h pkt-line.h quote.h refs.h list-objects.h sideband.h \
 	run-command.h strbuf.h tag.h tree.h git-compat-util.h revision.h \
 	tree-walk.h log-tree.h dir.h path-list.h unpack-trees.h builtin.h \
-	utf8.h reflog-walk.h patch-ids.h attr.h decorate.h progress.h mailmap.h
+	utf8.h reflog-walk.h patch-ids.h attr.h decorate.h progress.h \
+	mailmap.h remote.h
 
 DIFF_OBJS = \
 	diff.o diff-lib.o diffcore-break.o diffcore-order.o \
@@ -310,7 +311,7 @@ LIB_OBJS = \
 	write_or_die.o trace.o list-objects.o grep.o match-trees.o \
 	alloc.o merge-file.o path-list.o help.o unpack-trees.o $(DIFF_OBJS) \
 	color.o wt-status.o archive-zip.o archive-tar.o shallow.o utf8.o \
-	convert.o attr.o decorate.o progress.o mailmap.o
+	convert.o attr.o decorate.o progress.o mailmap.o remote.o
 
 BUILTIN_OBJS = \
 	builtin-add.o \
diff --git a/builtin-push.c b/builtin-push.c
index cb78401..0e602f3 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -5,17 +5,13 @@
 #include "refs.h"
 #include "run-command.h"
 #include "builtin.h"
-
-#define MAX_URI (16)
+#include "remote.h"
 
 static const char push_usage[] = "git-push [--all] [--tags] [--receive-pack=<git-receive-pack>] [--repo=all] [-f | --force] [-v] [<repository> <refspec>...]";
 
 static int all, tags, force, thin = 1, verbose;
 static const char *receivepack;
 
-#define BUF_SIZE (2084)
-static char buffer[BUF_SIZE];
-
 static const char **refspec;
 static int refspec_nr;
 
@@ -137,175 +133,29 @@ static void set_refspecs(const char **refs, int nr)
 	expand_refspecs();
 }
 
-static int get_remotes_uri(const char *repo, const char *uri[MAX_URI])
-{
-	int n = 0;
-	FILE *f = fopen(git_path("remotes/%s", repo), "r");
-	int has_explicit_refspec = refspec_nr || all || tags;
-
-	if (!f)
-		return -1;
-	while (fgets(buffer, BUF_SIZE, f)) {
-		int is_refspec;
-		char *s, *p;
-
-		if (!prefixcmp(buffer, "URL:")) {
-			is_refspec = 0;
-			s = buffer + 4;
-		} else if (!prefixcmp(buffer, "Push:")) {
-			is_refspec = 1;
-			s = buffer + 5;
-		} else
-			continue;
-
-		/* Remove whitespace at the head.. */
-		while (isspace(*s))
-			s++;
-		if (!*s)
-			continue;
-
-		/* ..and at the end */
-		p = s + strlen(s);
-		while (isspace(p[-1]))
-			*--p = 0;
-
-		if (!is_refspec) {
-			if (n < MAX_URI)
-				uri[n++] = xstrdup(s);
-			else
-				error("more than %d URL's specified, ignoring the rest", MAX_URI);
-		}
-		else if (is_refspec && !has_explicit_refspec) {
-			if (!wildcard_ref(s))
-				add_refspec(xstrdup(s));
-		}
-	}
-	fclose(f);
-	if (!n)
-		die("remote '%s' has no URL", repo);
-	return n;
-}
-
-static const char **config_uri;
-static const char *config_repo;
-static int config_repo_len;
-static int config_current_uri;
-static int config_get_refspecs;
-static int config_get_receivepack;
-
-static int get_remote_config(const char* key, const char* value)
-{
-	if (!prefixcmp(key, "remote.") &&
-	    !strncmp(key + 7, config_repo, config_repo_len)) {
-		if (!strcmp(key + 7 + config_repo_len, ".url")) {
-			if (config_current_uri < MAX_URI)
-				config_uri[config_current_uri++] = xstrdup(value);
-			else
-				error("more than %d URL's specified, ignoring the rest", MAX_URI);
-		}
-		else if (config_get_refspecs &&
-			 !strcmp(key + 7 + config_repo_len, ".push")) {
-			if (!wildcard_ref(value))
-				add_refspec(xstrdup(value));
-		}
-		else if (config_get_receivepack &&
-			 !strcmp(key + 7 + config_repo_len, ".receivepack")) {
-			if (!receivepack) {
-				char *rp = xmalloc(strlen(value) + 16);
-				sprintf(rp, "--receive-pack=%s", value);
-				receivepack = rp;
-			} else
-				error("more than one receivepack given, using the first");
-		}
-	}
-	return 0;
-}
-
-static int get_config_remotes_uri(const char *repo, const char *uri[MAX_URI])
-{
-	config_repo_len = strlen(repo);
-	config_repo = repo;
-	config_current_uri = 0;
-	config_uri = uri;
-	config_get_refspecs = !(refspec_nr || all || tags);
-	config_get_receivepack = (receivepack == NULL);
-
-	git_config(get_remote_config);
-	return config_current_uri;
-}
-
-static int get_branches_uri(const char *repo, const char *uri[MAX_URI])
-{
-	const char *slash = strchr(repo, '/');
-	int n = slash ? slash - repo : 1000;
-	FILE *f = fopen(git_path("branches/%.*s", n, repo), "r");
-	char *s, *p;
-	int len;
-
-	if (!f)
-		return 0;
-	s = fgets(buffer, BUF_SIZE, f);
-	fclose(f);
-	if (!s)
-		return 0;
-	while (isspace(*s))
-		s++;
-	if (!*s)
-		return 0;
-	p = s + strlen(s);
-	while (isspace(p[-1]))
-		*--p = 0;
-	len = p - s;
-	if (slash)
-		len += strlen(slash);
-	p = xmalloc(len + 1);
-	strcpy(p, s);
-	if (slash)
-		strcat(p, slash);
-	uri[0] = p;
-	return 1;
-}
-
-/*
- * Read remotes and branches file, fill the push target URI
- * list.  If there is no command line refspecs, read Push: lines
- * to set up the *refspec list as well.
- * return the number of push target URIs
- */
-static int read_config(const char *repo, const char *uri[MAX_URI])
-{
-	int n;
-
-	if (*repo != '/') {
-		n = get_remotes_uri(repo, uri);
-		if (n > 0)
-			return n;
-
-		n = get_config_remotes_uri(repo, uri);
-		if (n > 0)
-			return n;
-
-		n = get_branches_uri(repo, uri);
-		if (n > 0)
-			return n;
-	}
-
-	uri[0] = repo;
-	return 1;
-}
-
 static int do_push(const char *repo)
 {
-	const char *uri[MAX_URI];
-	int i, n, errs;
+	int i, errs;
 	int common_argc;
 	const char **argv;
 	int argc;
+	struct remote *remote = remote_get(repo);
 
-	n = read_config(repo, uri);
-	if (n <= 0)
+	if (!remote)
 		die("bad repository '%s'", repo);
 
+	if (remote->receivepack) {
+		char *rp = xmalloc(strlen(remote->receivepack) + 16);
+		sprintf(rp, "--receive-pack=%s", remote->receivepack);
+		receivepack = rp;
+	}
+	if (!refspec && !all && !tags && remote->push_refspec_nr) {
+		for (i = 0; i < remote->push_refspec_nr; i++) {
+			if (!wildcard_ref(remote->push_refspec[i]))
+				add_refspec(remote->push_refspec[i]);
+		}
+	}
+
 	argv = xmalloc((refspec_nr + 10) * sizeof(char *));
 	argv[0] = "dummy-send-pack";
 	argc = 1;
@@ -318,12 +168,12 @@ static int do_push(const char *repo)
 	common_argc = argc;
 
 	errs = 0;
-	for (i = 0; i < n; i++) {
+	for (i = 0; i < remote->uri_nr; i++) {
 		int err;
 		int dest_argc = common_argc;
 		int dest_refspec_nr = refspec_nr;
 		const char **dest_refspec = refspec;
-		const char *dest = uri[i];
+		const char *dest = remote->uri[i];
 		const char *sender = "send-pack";
 		if (!prefixcmp(dest, "http://") ||
 		    !prefixcmp(dest, "https://"))
@@ -341,7 +191,7 @@ static int do_push(const char *repo)
 		if (!err)
 			continue;
 
-		error("failed to push to '%s'", uri[i]);
+		error("failed to push to '%s'", remote->uri[i]);
 		switch (err) {
 		case -ERR_RUN_COMMAND_FORK:
 			error("unable to fork for %s", sender);
@@ -362,7 +212,7 @@ static int do_push(const char *repo)
 int cmd_push(int argc, const char **argv, const char *prefix)
 {
 	int i;
-	const char *repo = "origin";	/* default repository */
+	const char *repo = NULL;	/* default repository */
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
diff --git a/remote.c b/remote.c
new file mode 100644
index 0000000..32a0acf
--- /dev/null
+++ b/remote.c
@@ -0,0 +1,201 @@
+#include "cache.h"
+#include "remote.h"
+#include "refs.h"
+
+static struct remote **remotes;
+static int allocated_remotes;
+
+#define BUF_SIZE (2084)
+static char buffer[BUF_SIZE];
+
+static void add_push_refspec(struct remote *remote, const char *ref)
+{
+	int nr = remote->push_refspec_nr + 1;
+	remote->push_refspec =
+		xrealloc(remote->push_refspec, nr * sizeof(char *));
+	remote->push_refspec[nr-1] = ref;
+	remote->push_refspec_nr = nr;
+}
+
+static void add_uri(struct remote *remote, const char *uri)
+{
+	int nr = remote->uri_nr + 1;
+	remote->uri =
+		xrealloc(remote->uri, nr * sizeof(char *));
+	remote->uri[nr-1] = uri;
+	remote->uri_nr = nr;
+}
+
+static struct remote *make_remote(const char *name, int len)
+{
+	int i, empty = -1;
+
+	for (i = 0; i < allocated_remotes; i++) {
+		if (!remotes[i]) {
+			if (empty < 0)
+				empty = i;
+		} else {
+			if (len ? (!strncmp(name, remotes[i]->name, len) &&
+				   !remotes[i]->name[len]) :
+			    !strcmp(name, remotes[i]->name))
+				return remotes[i];
+		}
+	}
+
+	if (empty < 0) {
+		empty = allocated_remotes;
+		allocated_remotes += allocated_remotes ? allocated_remotes : 1;
+		remotes = xrealloc(remotes,
+				   sizeof(*remotes) * allocated_remotes);
+		memset(remotes + empty, 0,
+		       (allocated_remotes - empty) * sizeof(*remotes));
+	}
+	remotes[empty] = xcalloc(1, sizeof(struct remote));
+	if (len)
+		remotes[empty]->name = xstrndup(name, len);
+	else
+		remotes[empty]->name = xstrdup(name);
+	return remotes[empty];
+}
+
+static void read_remotes_file(struct remote *remote)
+{
+	FILE *f = fopen(git_path("remotes/%s", remote->name), "r");
+
+	if (!f)
+		return;
+	while (fgets(buffer, BUF_SIZE, f)) {
+		int value_list;
+		char *s, *p;
+
+		if (!prefixcmp(buffer, "URL:")) {
+			value_list = 0;
+			s = buffer + 4;
+		} else if (!prefixcmp(buffer, "Push:")) {
+			value_list = 1;
+			s = buffer + 5;
+		} else
+			continue;
+
+		while (isspace(*s))
+			s++;
+		if (!*s)
+			continue;
+
+		p = s + strlen(s);
+		while (isspace(p[-1]))
+			*--p = 0;
+
+		switch (value_list) {
+		case 0:
+			add_uri(remote, xstrdup(s));
+			break;
+		case 1:
+			add_push_refspec(remote, xstrdup(s));
+			break;
+		}
+	}
+}
+
+static void read_branches_file(struct remote *remote)
+{
+	const char *slash = strchr(remote->name, '/');
+	int n = slash ? slash - remote->name : 1000;
+	FILE *f = fopen(git_path("branches/%.*s", n, remote->name), "r");
+	char *s, *p;
+	int len;
+
+	if (!f)
+		return;
+	s = fgets(buffer, BUF_SIZE, f);
+	fclose(f);
+	if (!s)
+		return;
+	while (isspace(*s))
+		s++;
+	if (!*s)
+		return;
+	p = s + strlen(s);
+	while (isspace(p[-1]))
+		*--p = 0;
+	len = p - s;
+	if (slash)
+		len += strlen(slash);
+	p = xmalloc(len + 1);
+	strcpy(p, s);
+	if (slash)
+		strcat(p, slash);
+	add_uri(remote, p);
+}
+
+static char *default_remote_name = NULL;
+static const char *current_branch = NULL;
+static int current_branch_len = 0;
+
+static int handle_config(const char *key, const char *value)
+{
+	const char *name;
+	const char *subkey;
+	struct remote *remote;
+	if (!prefixcmp(key, "branch.") && current_branch &&
+	    !strncmp(key + 7, current_branch, current_branch_len) &&
+	    !strcmp(key + 7 + current_branch_len, ".remote")) {
+		free(default_remote_name);
+		default_remote_name = xstrdup(value);
+	}
+	if (prefixcmp(key,  "remote."))
+		return 0;
+	name = key + 7;
+	subkey = strrchr(name, '.');
+	if (!subkey)
+		return error("Config with no key for remote %s", name);
+	remote = make_remote(name, subkey - name);
+	if (!strcmp(subkey, ".url")) {
+		add_uri(remote, xstrdup(value));
+	} else if (!strcmp(subkey, ".push")) {
+		add_push_refspec(remote, xstrdup(value));
+	} else if (!strcmp(subkey, ".receivepack")) {
+		if (!remote->receivepack)
+			remote->receivepack = xstrdup(value);
+		else
+			error("more than one receivepack given, using the first");
+	}
+	return 0;
+}
+
+static void read_config(void)
+{
+	unsigned char sha1[20];
+	const char *head_ref;
+	int flag;
+	if (default_remote_name) // did this already
+		return;
+	default_remote_name = xstrdup("origin");
+	current_branch = NULL;
+	head_ref = resolve_ref("HEAD", sha1, 0, &flag);
+	if (head_ref && (flag & REF_ISSYMREF) &&
+	    !prefixcmp(head_ref, "refs/heads/")) {
+		current_branch = head_ref + strlen("refs/heads/");
+		current_branch_len = strlen(current_branch);
+	}
+	git_config(handle_config);
+}
+
+struct remote *remote_get(const char *name)
+{
+	struct remote *ret;
+
+	read_config();
+	if (!name)
+		name = default_remote_name;
+	ret = make_remote(name, 0);
+	if (*name == '/')
+		add_uri(ret, name);
+	if (!ret->uri)
+		read_remotes_file(ret);
+	if (!ret->uri)
+		read_branches_file(ret);
+	if (!ret->uri)
+		return NULL;
+	return ret;
+}
diff --git a/remote.h b/remote.h
new file mode 100644
index 0000000..73747a8
--- /dev/null
+++ b/remote.h
@@ -0,0 +1,18 @@
+#ifndef REMOTE_H
+#define REMOTE_H
+
+struct remote {
+	const char *name;
+
+	const char **uri;
+	int uri_nr;
+
+	const char **push_refspec;
+	int push_refspec_nr;
+
+	const char *receivepack;
+};
+
+struct remote *remote_get(const char *name);
+
+#endif
-- 
1.5.2.rc2.22.g9bca
Junio C Hamano· May 10, 2007, 07:07 UTC · re: Daniel Barkalow · lore

Re: [PATCH 1/3] Move remote parsing into a library file out of builtin-push.

This seems to break t9400, with "fatal: bad repository 'gitcvs.git", upon "git push".

: gitster t/db/remote; sh t9400-git-cvsserver-server.sh -i -v
* expecting success: cvs -Q co -d cvswork master &&
   test "$(echo $(grep -v ^D cvswork/CVS/Entries|cut -d/ -f2,3,5))" = "empty/1.1/"
cvs checkout: Updating cvswork
U cvswork/empty
*   ok 1: basic checkout
* expecting success: echo testfile1 >testfile1 &&
   git add testfile1 &&
   git commit -q -m "Add testfile1" &&
   git push gitcvs.git >/dev/null &&
   cd cvswork &&
   cvs -Q update &&
   test "$(echo $(grep testfile1 CVS/Entries|cut -d/ -f2,3,5))" = "testfile1/1.1/" &&
   diff -q testfile1 ../testfile1
fatal: bad repository 'gitcvs.git'
* FAIL 2: cvs update (create new file)
	echo testfile1 >testfile1 &&
	   git add testfile1 &&
	   git commit -q -m "Add testfile1" &&
	   git push gitcvs.git >/dev/null &&
	   cd cvswork &&
	   cvs -Q update &&
	   test "$(echo $(grep testfile1 CVS/Entries|cut -d/ -f2,3,5))" = "testfile1/1.1/" &&
	   diff -q testfile1 ../testfile1
: gitster t/db/remote; 
Daniel Barkalow· May 10, 2007, 07:45 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/3] Move remote parsing into a library file out of builtin-push.

On Thu, 10 May 2007, Junio C Hamano wrote:
Show 14 quoted lines
> This seems to break t9400, with "fatal: bad repository 'gitcvs.git",
> upon "git push".
> 
> : gitster t/db/remote; sh t9400-git-cvsserver-server.sh -i -v
> * expecting success: cvs -Q co -d cvswork master &&
>    test "$(echo $(grep -v ^D cvswork/CVS/Entries|cut -d/ -f2,3,5))" = "empty/1.1/"
> cvs checkout: Updating cvswork
> U cvswork/empty
> *   ok 1: basic checkout
> 
> * expecting success: echo testfile1 >testfile1 &&
>    git add testfile1 &&
>    git commit -q -m "Add testfile1" &&
>    git push gitcvs.git >/dev/null &&

The man page doesn't think this is valid, since it only claims absolute paths to work for local repositories. (Current builtin-push seems to be sloppy; it has the die() that triggers in my version, but its read_config() always returns a value that doesn't trigger it, suggesting that there's some sort of bug.)

Following patch fixes t9400 to use absolute paths (assuming that's what it expects "git push gitcvs.git" to mean).

=== cut here === Local repository names for git-push are only documented with absolute paths, even though current git-push is lenient. Use an absolute path in t9400-git-cvsserver-server.sh.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
Show changes to t/t9400-git-cvsserver-server.sh +6 −6
diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
index f17be6b..178b9b4 100755
--- a/t/t9400-git-cvsserver-server.sh
+++ b/t/t9400-git-cvsserver-server.sh
@@ -45,7 +45,7 @@ test_expect_success 'cvs update (create new file)' \
   'echo testfile1 >testfile1 &&
    git add testfile1 &&
    git commit -q -m "Add testfile1" &&
-   git push gitcvs.git >/dev/null &&
+   git push $(pwd)/gitcvs.git >/dev/null &&
    cd cvswork &&
    cvs -Q update &&
    test "$(echo $(grep testfile1 CVS/Entries|cut -d/ -f2,3,5))" = "testfile1/1.1/" &&
@@ -56,7 +56,7 @@ test_expect_success 'cvs update (update existing file)' \
   'echo line 2 >>testfile1 &&
    git add testfile1 &&
    git commit -q -m "Append to testfile1" &&
-   git push gitcvs.git >/dev/null &&
+   git push $(pwd)/gitcvs.git >/dev/null &&
    cd cvswork &&
    cvs -Q update &&
    test "$(echo $(grep testfile1 CVS/Entries|cut -d/ -f2,3,5))" = "testfile1/1.2/" &&
@@ -69,7 +69,7 @@ test_expect_failure "cvs update w/o -d doesn't create subdir (TODO)" \
    echo >test/empty &&
    git add test &&
    git commit -q -m "Single Subdirectory" &&
-   git push gitcvs.git >/dev/null &&
+   git push $(pwd)/gitcvs.git >/dev/null &&
    cd cvswork &&
    cvs -Q update &&
    test ! -d test'
@@ -82,7 +82,7 @@ test_expect_success 'cvs update (subdirectories)' \
       git add $dir;
    done) &&
    git commit -q -m "deep sub directory structure" &&
-   git push gitcvs.git >/dev/null &&
+   git push $(pwd)/gitcvs.git >/dev/null &&
    cd cvswork &&
    cvs -Q update -d &&
    (for dir in A A/B A/B/C A/D E; do
@@ -100,7 +100,7 @@ cd "$WORKDIR"
 test_expect_success 'cvs update (delete file)' \
   'git rm testfile1 &&
    git commit -q -m "Remove testfile1" &&
-   git push gitcvs.git >/dev/null &&
+   git push $(pwd)/gitcvs.git >/dev/null &&
    cd cvswork &&
    cvs -Q update &&
    test -z "$(grep testfile1 CVS/Entries)" &&
@@ -111,7 +111,7 @@ test_expect_success 'cvs update (re-add deleted file)' \
   'echo readded testfile >testfile1 &&
    git add testfile1 &&
    git commit -q -m "Re-Add testfile1" &&
-   git push gitcvs.git >/dev/null &&
+   git push $(pwd)/gitcvs.git >/dev/null &&
    cd cvswork &&
    cvs -Q update &&
    test "$(echo $(grep testfile1 CVS/Entries|cut -d/ -f2,3,5))" = "testfile1/1.4/" &&
Junio C Hamano· May 10, 2007, 07:52 UTC · re: Daniel Barkalow · lore

Re: [PATCH 1/3] Move remote parsing into a library file out of builtin-push.

Daniel Barkalow <barkalow@iabervon.org> writes:
Show 19 quoted lines
> On Thu, 10 May 2007, Junio C Hamano wrote:
>
>> This seems to break t9400, with "fatal: bad repository 'gitcvs.git",
>> upon "git push".
>> 
>> : gitster t/db/remote; sh t9400-git-cvsserver-server.sh -i -v
>> * expecting success: cvs -Q co -d cvswork master &&
>>    test "$(echo $(grep -v ^D cvswork/CVS/Entries|cut -d/ -f2,3,5))" = "empty/1.1/"
>> cvs checkout: Updating cvswork
>> U cvswork/empty
>> *   ok 1: basic checkout
>> 
>> * expecting success: echo testfile1 >testfile1 &&
>>    git add testfile1 &&
>>    git commit -q -m "Add testfile1" &&
>>    git push gitcvs.git >/dev/null &&
>
> The man page doesn't think this is valid, since it only claims absolute 
> paths to work for local repositories.

Does it? I suspect we need to fix the manpage then, as it is fairly common to do

	$ git fetch ../next-door-neighbour
and expect the opposite to work as well.
And I think it does today.
Daniel Barkalow· May 10, 2007, 08:04 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/3] Move remote parsing into a library file out of builtin-push.

On Thu, 10 May 2007, Junio C Hamano wrote:
Show 30 quoted lines
> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > On Thu, 10 May 2007, Junio C Hamano wrote:
> >
> >> This seems to break t9400, with "fatal: bad repository 'gitcvs.git",
> >> upon "git push".
> >> 
> >> : gitster t/db/remote; sh t9400-git-cvsserver-server.sh -i -v
> >> * expecting success: cvs -Q co -d cvswork master &&
> >>    test "$(echo $(grep -v ^D cvswork/CVS/Entries|cut -d/ -f2,3,5))" = "empty/1.1/"
> >> cvs checkout: Updating cvswork
> >> U cvswork/empty
> >> *   ok 1: basic checkout
> >> 
> >> * expecting success: echo testfile1 >testfile1 &&
> >>    git add testfile1 &&
> >>    git commit -q -m "Add testfile1" &&
> >>    git push gitcvs.git >/dev/null &&
> >
> > The man page doesn't think this is valid, since it only claims absolute 
> > paths to work for local repositories.
> 
> Does it?  I suspect we need to fix the manpage then, as it is
> fairly common to do 
> 
> 	$ git fetch ../next-door-neighbour
> 
> and expect the opposite to work as well.
> 
> And I think it does today.

Hmm, and I guess URIs on the command line work the same way. How about requiring a '/' somewhere in a repository argument in order to treat it as a repository instead of a remote name? Then "../next-door-neighbour" would work, "./gitcvs.git" would work (in the odd case where you actually have a bare repository sitting in your working directory), but we'd avoid the current default of pushing to a bare repository in "./origin/" if nothing at all is configured.

	-Daniel
*This .sig left intentionally blank*
Junio C Hamano· May 10, 2007, 08:21 UTC · re: Daniel Barkalow · lore

Re: [PATCH 1/3] Move remote parsing into a library file out of builtin-push.

Daniel Barkalow <barkalow@iabervon.org> writes:
Show 9 quoted lines
>> And I think it does today.
>
> Hmm, and I guess URIs on the command line work the same way. How about 
> requiring a '/' somewhere in a repository argument in order to treat it as 
> a repository instead of a remote name? Then "../next-door-neighbour" would 
> work, "./gitcvs.git" would work (in the odd case where you actually have a 
> bare repository sitting in your working directory), but we'd avoid the 
> current default of pushing to a bare repository in "./origin/" if nothing 
> at all is configured.

When I wrote the message you are responding to, I thought this was a regression from the current behaviour, which (IIRC--it's getting late and I am tired to double check) essentially says if the token is a name of the directory, the target repository is a local one, but "we'd avoid..." part seems to suggest that you actually did this deliberately as a fix to some problem in the current behaviour. I am not however sure what it exactly is. Could you care to elaborate the part after "we'd avoid..." to clarify what the problem is, please?

Daniel Barkalow· May 10, 2007, 08:33 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/3] Move remote parsing into a library file out of builtin-push.

On Thu, 10 May 2007, Junio C Hamano wrote:
Show 21 quoted lines
> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> >> And I think it does today.
> >
> > Hmm, and I guess URIs on the command line work the same way. How about 
> > requiring a '/' somewhere in a repository argument in order to treat it as 
> > a repository instead of a remote name? Then "../next-door-neighbour" would 
> > work, "./gitcvs.git" would work (in the odd case where you actually have a 
> > bare repository sitting in your working directory), but we'd avoid the 
> > current default of pushing to a bare repository in "./origin/" if nothing 
> > at all is configured.
> 
> When I wrote the message you are responding to, I thought this
> was a regression from the current behaviour, which (IIRC--it's
> getting late and I am tired to double check) essentially says if
> the token is a name of the directory, the target repository is a
> local one, but "we'd avoid..." part seems to suggest that you
> actually did this deliberately as a fix to some problem in the
> current behaviour.  I am not however sure what it exactly is.
> Could you care to elaborate the part after "we'd avoid..." to
> clarify what the problem is, please?

The problem, in general, is that, if the remote name you specify (or "origin" if you don't specify any) is not configured as a remote, it is treated as a filename in the current directory for a local push. E.g.:

$ git init $ git push fatal: 'origin': unable to chdir or not a git archive fatal: The remote end hung up unexpectedly

It's actually trying to push to ./origin/, which is totally nuts as a default repository to push to. Similarly, if you typo an actual remote name. Furthermore, builtin-push.c has an error message for the situation where the repository specification is wrong, suggesting that there is some invalid repository specification, but it isn't reachable. And it carefully prevents remote names from starting with a '/', suggestion that that is the distinguishing characteristic between directly-specified repository URIs and configured remotes (which can't really be right, of course).

I think the right answer is to say that configured remotes cannot contain slashes, and directly-specified URIs must contain slashes, and it'll all be clear.

	-Daniel
*This .sig left intentionally blank*
Junio C Hamano· May 10, 2007, 08:43 UTC · re: Daniel Barkalow · lore

Re: [PATCH 1/3] Move remote parsing into a library file out of builtin-push.

Daniel Barkalow <barkalow@iabervon.org> writes:
Show 32 quoted lines
> On Thu, 10 May 2007, Junio C Hamano wrote:
>
>> Daniel Barkalow <barkalow@iabervon.org> writes:
>> 
>> >> And I think it does today.
>> >
>> > Hmm, and I guess URIs on the command line work the same way. How about 
>> > requiring a '/' somewhere in a repository argument in order to treat it as 
>> > a repository instead of a remote name? Then "../next-door-neighbour" would 
>> > work, "./gitcvs.git" would work (in the odd case where you actually have a 
>> > bare repository sitting in your working directory), but we'd avoid the 
>> > current default of pushing to a bare repository in "./origin/" if nothing 
>> > at all is configured.
>> 
>> When I wrote the message you are responding to, I thought this
>> was a regression from the current behaviour, which (IIRC--it's
>> getting late and I am tired to double check) essentially says if
>> the token is a name of the directory, the target repository is a
>> local one, but "we'd avoid..." part seems to suggest that you
>> actually did this deliberately as a fix to some problem in the
>> current behaviour.  I am not however sure what it exactly is.
>> Could you care to elaborate the part after "we'd avoid..." to
>> clarify what the problem is, please?
>
> The problem, in general, is that, if the remote name you specify (or 
> "origin" if you don't specify any) is not configured as a remote, it is 
> treated as a filename in the current directory for a local push. E.g.:
>
> $ git init
> $ git push
> fatal: 'origin': unable to chdir or not a git archive
> fatal: The remote end hung up unexpectedly
Ahh.  You were trying to give it a better error message.

I think I lied in the previous message. I said we try to see if it is a local directory name before using that name, but we do not do it, and leave the error detection to the lower level on the other side (push spawns send-pack which in turn spawns receive-pack) instead.

Perhaps an alternative is to see if the name is configured as a remote (if so, we obviously use it), and if not do stat() to see if it is a directory (if so, use it as a local repository). Then we do not have to impose new restriction of slash at all, although it might complicate the code a bit more.

Daniel Barkalow· May 10, 2007, 16:40 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/3] Move remote parsing into a library file out of builtin-push.

On Thu, 10 May 2007, Junio C Hamano wrote:
Show 48 quoted lines
> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > On Thu, 10 May 2007, Junio C Hamano wrote:
> >
> >> Daniel Barkalow <barkalow@iabervon.org> writes:
> >> 
> >> >> And I think it does today.
> >> >
> >> > Hmm, and I guess URIs on the command line work the same way. How about 
> >> > requiring a '/' somewhere in a repository argument in order to treat it as 
> >> > a repository instead of a remote name? Then "../next-door-neighbour" would 
> >> > work, "./gitcvs.git" would work (in the odd case where you actually have a 
> >> > bare repository sitting in your working directory), but we'd avoid the 
> >> > current default of pushing to a bare repository in "./origin/" if nothing 
> >> > at all is configured.
> >> 
> >> When I wrote the message you are responding to, I thought this
> >> was a regression from the current behaviour, which (IIRC--it's
> >> getting late and I am tired to double check) essentially says if
> >> the token is a name of the directory, the target repository is a
> >> local one, but "we'd avoid..." part seems to suggest that you
> >> actually did this deliberately as a fix to some problem in the
> >> current behaviour.  I am not however sure what it exactly is.
> >> Could you care to elaborate the part after "we'd avoid..." to
> >> clarify what the problem is, please?
> >
> > The problem, in general, is that, if the remote name you specify (or 
> > "origin" if you don't specify any) is not configured as a remote, it is 
> > treated as a filename in the current directory for a local push. E.g.:
> >
> > $ git init
> > $ git push
> > fatal: 'origin': unable to chdir or not a git archive
> > fatal: The remote end hung up unexpectedly
> 
> Ahh.  You were trying to give it a better error message.
> 
> I think I lied in the previous message.  I said we try to see if
> it is a local directory name before using that name, but we do
> not do it, and leave the error detection to the lower level on
> the other side (push spawns send-pack which in turn spawns
> receive-pack) instead.
> 
> Perhaps an alternative is to see if the name is configured as a
> remote (if so, we obviously use it), and if not do stat() to see
> if it is a directory (if so, use it as a local repository).
> Then we do not have to impose new restriction of slash at all,
> although it might complicate the code a bit more.

The problem I see with allowing paths without slashes is if you've got a subproject with a name similar to a remote name, and type the wrong one (particularly due to tab-completion), or if you've got a remote name you use in other projects that matches a subproject in a project where you aren't using that remote name. I think that a repository in your working directory is unlikely to be something you actually want to push to or fetch from (and if this is actually what you want, ./directory is the usual unixy thing for saying, no really, I want a relative path in the current directory, and would work here; and it would be good practice anyway, so that you don't get tripped up if you create a remote configuration with that name later). Obviously, ../something has a slash; it should also take ':' to mean a URI (which I didn't realize last night), so that "person@machine:directory.git" is a URI.

I think, as a general rule, that it would be cleanest to distinguish lexically between repository names that indicate configured remotes and repository names that are URIs, particularly if we only break usage that people only do for writing git regression tests.

	-Daniel
*This .sig left intentionally blank*
Junio C Hamano· May 10, 2007, 08:35 UTC · re: Daniel Barkalow · lore

Re: [PATCH 1/3] Move remote parsing into a library file out of builtin-push.

Daniel Barkalow <barkalow@iabervon.org> writes:
> ..., "./gitcvs.git" would work (in the odd case where you actually have a 
> bare repository sitting in your working directory), ...

This is a bit of tangent, because it does not change the issue we are discussing, and I suspect you already know this.

But to avoid future confusion by people on the list who read this in the archive...

	"foo.git" does _NOT_ mean the directory is a bare
	repository.  It is perfectly normal to have "foo.git/"
	that has a working tree whose repository data lives in
	"foo.git/.git".

I _think_ (meaning, I haven't polled the userbase) requiring '/' may not break people's existing setup too badly, while it _is_ a regression in the sense that we suddenly start disallowing something we allowed for a long time, perhaps without no apparent good reason.

I hope you'll correct me about "without no apparent good reason" part, which was what I was asking in my previous message. With a good rationale, it is much easier to sell a change that is a regression in the strictest sense but is unlikely to hurt people in practice.

← back to recent threads