[PATCH 0/4] Pushing into a repository with alternates

[PATCH 0/4] Pushing into a repository with alternates

[Junio C Hamano]

Here is a re-rolled series of the topic we discussed earlier.

Junio C Hamano (4):
  is_directory(): a generic helper function
  receive-pack: make it a builtin
  push: prepare sender to receive extended ref information from the receiver
  push: receiver end advertises refs from alternate repositories

 Makefile                                 |    2 +-
 abspath.c                                |   13 ++++++++-
 builtin-clone.c                          |    9 +-----
 builtin-fetch-pack.c                     |    2 +-
 receive-pack.c => builtin-receive-pack.c |   44 +++++++++++++++++++++++++++--
 builtin-send-pack.c                      |   21 +++++++++++---
 builtin.h                                |    1 +
 cache.h                                  |    9 +++++-
 connect.c                                |   17 +++++++++++-
 daemon.c                                 |   10 ++-----
 git.c                                    |    1 +
 rerere.c                                 |    3 +-
 sha1_file.c                              |   13 +++++++-
 transport.c                              |    4 +-
 14 files changed, 115 insertions(+), 34 deletions(-)
 rename receive-pack.c => builtin-receive-pack.c (91%)

[PATCH 1/4] is_directory(): a generic helper function

[Junio C Hamano]

A simple "grep -e stat --and -e S_ISDIR" revealed there are many
open-coded implementations of this function.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 abspath.c       |   13 ++++++++++++-
 builtin-clone.c |    9 +--------
 cache.h         |    1 +
 daemon.c        |   10 +++-------
 rerere.c        |    3 +--
 sha1_file.c     |    3 +--
 6 files changed, 19 insertions(+), 20 deletions(-)

diff --git a/abspath.c b/abspath.c
index 0d56124..8c06782 100644
--- a/abspath.c
+++ b/abspath.c
@@ -1,5 +1,16 @@
 #include "cache.h"
 
+/*
+ * Do not use this for inspecting *tracked* content.  When path is a
+ * symlink to a directory, we do not want to say it is a directory.
+ * Worth yet, leading components in path could contain symbolic links.
+ */
+int is_directory(const char *path)
+{
+	struct stat st;
+	return (!stat(path, &st) && S_ISDIR(st.st_mode));
+}
+
 /* We allow "recursive" symbolic links. Only within reason, though. */
 #define MAXDEPTH 5
 
@@ -17,7 +28,7 @@ const char *make_absolute_path(const char *path)
 		die ("Too long path: %.*s", 60, path);
 
 	while (depth--) {
-		if (stat(buf, &st) || !S_ISDIR(st.st_mode)) {
+		if (!is_directory(buf)) {
 			char *last_slash = strrchr(buf, '/');
 			if (last_slash) {
 				*last_slash = '\0';
diff --git a/builtin-clone.c b/builtin-clone.c
index c843529..a4b8790 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -77,7 +77,7 @@ static char *get_repo_path(const char *repo, int *is_bundle)
 	for (i = 0; i < ARRAY_SIZE(suffix); i++) {
 		const char *path;
 		path = mkpath("%s%s", repo, suffix[i]);
-		if (!stat(path, &st) && S_ISDIR(st.st_mode)) {
+		if (is_directory(path)) {
 			*is_bundle = 0;
 			return xstrdup(make_nonrelative_path(path));
 		}
@@ -140,13 +140,6 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 	return xstrndup(start, end - start);
 }
 
-static int is_directory(const char *path)
-{
-	struct stat buf;
-
-	return !stat(path, &buf) && S_ISDIR(buf.st_mode);
-}
-
 static void strip_trailing_slashes(char *dir)
 {
 	char *end = dir + strlen(dir);
diff --git a/cache.h b/cache.h
index de8c2b6..ce1f630 100644
--- a/cache.h
+++ b/cache.h
@@ -533,6 +533,7 @@ static inline int is_absolute_path(const char *path)
 {
 	return path[0] == '/' || has_dos_drive_prefix(path);
 }
+int is_directory(const char *);
 const char *make_absolute_path(const char *path);
 const char *make_nonrelative_path(const char *path);
 const char *make_relative_path(const char *abs, const char *base);
diff --git a/daemon.c b/daemon.c
index c315932..ab7a273 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1115,13 +1115,9 @@ int main(int argc, char **argv)
 	if (strict_paths && (!ok_paths || !*ok_paths))
 		die("option --strict-paths requires a whitelist");
 
-	if (base_path) {
-		struct stat st;
-
-		if (stat(base_path, &st) || !S_ISDIR(st.st_mode))
-			die("base-path '%s' does not exist or "
-			    "is not a directory", base_path);
-	}
+	if (base_path && !is_directory(base_path))
+		die("base-path '%s' does not exist or is not a directory",
+		    base_path);
 
 	if (inetd_mode) {
 		struct sockaddr_storage ss;
diff --git a/rerere.c b/rerere.c
index 323e493..c38886b 100644
--- a/rerere.c
+++ b/rerere.c
@@ -319,7 +319,6 @@ static int git_rerere_config(const char *var, const char *value, void *cb)
 
 static int is_rerere_enabled(void)
 {
-	struct stat st;
 	const char *rr_cache;
 	int rr_cache_exists;
 
@@ -327,7 +326,7 @@ static int is_rerere_enabled(void)
 		return 0;
 
 	rr_cache = git_path("rr-cache");
-	rr_cache_exists = !stat(rr_cache, &st) && S_ISDIR(st.st_mode);
+	rr_cache_exists = is_directory(rr_cache);
 	if (rerere_enabled < 0)
 		return rr_cache_exists;
 
diff --git a/sha1_file.c b/sha1_file.c
index 9ee1ed1..ae550e8 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -250,7 +250,6 @@ static void read_info_alternates(const char * alternates, int depth);
  */
 static int link_alt_odb_entry(const char * entry, int len, const char * relative_base, int depth)
 {
-	struct stat st;
 	const char *objdir = get_object_directory();
 	struct alternate_object_database *ent;
 	struct alternate_object_database *alt;
@@ -281,7 +280,7 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative
 	ent->base[pfxlen] = ent->base[entlen-1] = 0;
 
 	/* Detect cases where alternate disappeared */
-	if (stat(ent->base, &st) || !S_ISDIR(st.st_mode)) {
+	if (!is_directory(ent->base)) {
 		error("object directory %s does not exist; "
 		      "check .git/objects/info/alternates.",
 		      ent->base);
-- 
1.6.0.1.415.g7bb82

[PATCH 2/4] receive-pack: make it a builtin

[Junio C Hamano]

It is a good thing to do in general, but more importantly, transport
routines can only be used by built-ins, which is what I'll be adding next.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Makefile                                 |    2 +-
 receive-pack.c => builtin-receive-pack.c |    6 +++---
 builtin.h                                |    1 +
 git.c                                    |    1 +
 4 files changed, 6 insertions(+), 4 deletions(-)
 rename receive-pack.c => builtin-receive-pack.c (99%)

diff --git a/Makefile b/Makefile
index f4c31c8..f9c54ff 100644
--- a/Makefile
+++ b/Makefile
@@ -294,7 +294,6 @@ PROGRAMS += git-mktag$X
 PROGRAMS += git-mktree$X
 PROGRAMS += git-pack-redundant$X
 PROGRAMS += git-patch-id$X
-PROGRAMS += git-receive-pack$X
 PROGRAMS += git-send-pack$X
 PROGRAMS += git-shell$X
 PROGRAMS += git-show-index$X
@@ -546,6 +545,7 @@ BUILTIN_OBJS += builtin-prune-packed.o
 BUILTIN_OBJS += builtin-prune.o
 BUILTIN_OBJS += builtin-push.o
 BUILTIN_OBJS += builtin-read-tree.o
+BUILTIN_OBJS += builtin-receive-pack.o
 BUILTIN_OBJS += builtin-reflog.o
 BUILTIN_OBJS += builtin-remote.o
 BUILTIN_OBJS += builtin-rerere.o
diff --git a/receive-pack.c b/builtin-receive-pack.c
similarity index 99%
rename from receive-pack.c
rename to builtin-receive-pack.c
index b81678a..6d6027e 100644
--- a/receive-pack.c
+++ b/builtin-receive-pack.c
@@ -462,14 +462,14 @@ static int delete_only(struct command *cmd)
 	return 1;
 }
 
-int main(int argc, char **argv)
+int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 {
 	int i;
 	char *dir = NULL;
 
 	argv++;
 	for (i = 1; i < argc; i++) {
-		char *arg = *argv++;
+		const char *arg = *argv++;
 
 		if (*arg == '-') {
 			/* Do flag handling here */
@@ -477,7 +477,7 @@ int main(int argc, char **argv)
 		}
 		if (dir)
 			usage(receive_pack_usage);
-		dir = arg;
+		dir = xstrdup(arg);
 	}
 	if (!dir)
 		usage(receive_pack_usage);
diff --git a/builtin.h b/builtin.h
index e67cb20..5d7cdca 100644
--- a/builtin.h
+++ b/builtin.h
@@ -78,6 +78,7 @@ extern int cmd_prune(int argc, const char **argv, const char *prefix);
 extern int cmd_prune_packed(int argc, const char **argv, const char *prefix);
 extern int cmd_push(int argc, const char **argv, const char *prefix);
 extern int cmd_read_tree(int argc, const char **argv, const char *prefix);
+extern int cmd_receive_pack(int argc, const char **argv, const char *prefix);
 extern int cmd_reflog(int argc, const char **argv, const char *prefix);
 extern int cmd_remote(int argc, const char **argv, const char *prefix);
 extern int cmd_config(int argc, const char **argv, const char *prefix);
diff --git a/git.c b/git.c
index adf7352..2f5b4d7 100644
--- a/git.c
+++ b/git.c
@@ -328,6 +328,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "prune-packed", cmd_prune_packed, RUN_SETUP },
 		{ "push", cmd_push, RUN_SETUP },
 		{ "read-tree", cmd_read_tree, RUN_SETUP },
+		{ "receive-pack", cmd_receive_pack },
 		{ "reflog", cmd_reflog, RUN_SETUP },
 		{ "remote", cmd_remote, RUN_SETUP },
 		{ "repo-config", cmd_config },
-- 
1.6.0.1.415.g7bb82

[PATCH 3/4] push: prepare sender to receive extended ref information from the receiver

[Junio C Hamano]

"git push" enhancement allows the receiving end to report not only its own
refs but refs in repositories it borrows from via the alternate object
store mechanism.  By telling the sender that objects reachable from these
extra refs are already complete in the receiving end, the number of
objects that need to be transfered can be cut down.

These entries are sent over the wire with string ".have", instead of the
actual names of the refs.  This string was chosen so that they are ignored
by older programs at the sending end.  If we sent some random but valid
looking refnames for these entries, "matching refs" rule (triggered when
running "git push" without explicit refspecs, where the sender learns what
refs the receiver has, and updates only the ones with the names of the
refs the sender also has) and "delete missing" rule (triggered when "git
push --mirror" is used, where the sender tells the receiver to delete the
refs it itself does not have) would try to update/delete them, which is
not what we want.

This prepares the send-pack (and "push" that runs native protocol) to
accept extended existing ref information and make use of it.  The ".have"
entries are excluded from ref matching rules, and are exempt from deletion
rule while pushing with --mirror option, but are still used for pack
generation purposes by providing more "bottom" range commits.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-fetch-pack.c |    2 +-
 builtin-send-pack.c  |   21 ++++++++++++++++-----
 cache.h              |    6 +++++-
 connect.c            |   17 ++++++++++++++++-
 transport.c          |    4 ++--
 5 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index 459c6f0..e0c2561 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -735,7 +735,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	conn = git_connect(fd, (char *)dest, args.uploadpack,
 			   args.verbose ? CONNECT_VERBOSE : 0);
 	if (conn) {
-		get_remote_heads(fd[0], &ref, 0, NULL, 0);
+		get_remote_heads(fd[0], &ref, 0, NULL, 0, NULL);
 
 		ref = fetch_pack(&args, fd, conn, ref, dest, nr_heads, heads, NULL);
 		close(fd[0]);
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 7588d22..b3c22f6 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -18,7 +18,7 @@ static struct send_pack_args args = {
 /*
  * Make a pack stream and spit it out into file descriptor fd
  */
-static int pack_objects(int fd, struct ref *refs)
+static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *extra)
 {
 	/*
 	 * The child becomes pack-objects --revs; we feed
@@ -34,6 +34,8 @@ static int pack_objects(int fd, struct ref *refs)
 		NULL,
 	};
 	struct child_process po;
+	int i;
+	char buf[42];
 
 	if (args.use_thin_pack)
 		argv[4] = "--thin";
@@ -49,9 +51,15 @@ static int pack_objects(int fd, struct ref *refs)
 	 * We feed the pack-objects we just spawned with revision
 	 * parameters by writing to the pipe.
 	 */
-	while (refs) {
-		char buf[42];
+	for (i = 0; i < extra->nr; i++) {
+		memcpy(buf + 1, sha1_to_hex(&extra->array[i][0]), 40);
+		buf[0] = '^';
+		buf[41] = '\n';
+		if (!write_or_whine(po.in, buf, 42, "send-pack: send refs"))
+			break;
+	}
 
+	while (refs) {
 		if (!is_null_sha1(refs->old_sha1) &&
 		    has_sha1_file(refs->old_sha1)) {
 			memcpy(buf + 1, sha1_to_hex(refs->old_sha1), 40);
@@ -381,14 +389,17 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 	int expect_status_report = 0;
 	int flags = MATCH_REFS_NONE;
 	int ret;
+	struct extra_have_objects extra_have;
 
+	memset(&extra_have, 0, sizeof(extra_have));
 	if (args.send_all)
 		flags |= MATCH_REFS_ALL;
 	if (args.send_mirror)
 		flags |= MATCH_REFS_MIRROR;
 
 	/* No funny business with the matcher */
-	remote_tail = get_remote_heads(in, &remote_refs, 0, NULL, REF_NORMAL);
+	remote_tail = get_remote_heads(in, &remote_refs, 0, NULL, REF_NORMAL,
+				       &extra_have);
 	get_local_heads();
 
 	/* Does the other end support the reporting? */
@@ -496,7 +507,7 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 
 	packet_flush(out);
 	if (new_refs && !args.dry_run) {
-		if (pack_objects(out, remote_refs) < 0)
+		if (pack_objects(out, remote_refs, &extra_have) < 0)
 			return -1;
 	}
 	else
diff --git a/cache.h b/cache.h
index ce1f630..98a7421 100644
--- a/cache.h
+++ b/cache.h
@@ -709,7 +709,11 @@ extern struct child_process *git_connect(int fd[2], const char *url, const char
 extern int finish_connect(struct child_process *conn);
 extern int path_match(const char *path, int nr, char **match);
 extern int get_ack(int fd, unsigned char *result_sha1);
-extern struct ref **get_remote_heads(int in, struct ref **list, int nr_match, char **match, unsigned int flags);
+struct extra_have_objects {
+	int nr, alloc;
+	unsigned char (*array)[20];
+};
+extern struct ref **get_remote_heads(int in, struct ref **list, int nr_match, char **match, unsigned int flags, struct extra_have_objects *);
 extern int server_supports(const char *feature);
 
 extern struct packed_git *parse_pack_index(unsigned char *sha1);
diff --git a/connect.c b/connect.c
index dd96f8e..071e355 100644
--- a/connect.c
+++ b/connect.c
@@ -41,12 +41,21 @@ int check_ref_type(const struct ref *ref, int flags)
 	return check_ref(ref->name, strlen(ref->name), flags);
 }
 
+static void add_extra_have(struct extra_have_objects *extra, unsigned char *sha1)
+{
+	if (extra->alloc <= extra->nr)
+		ALLOC_GROW(extra->array, extra->nr + 1, extra->alloc);
+	hashcpy(&(extra->array[extra->nr][0]), sha1);
+	extra->nr++;
+}
+
 /*
  * Read all the refs from the other end
  */
 struct ref **get_remote_heads(int in, struct ref **list,
 			      int nr_match, char **match,
-			      unsigned int flags)
+			      unsigned int flags,
+			      struct extra_have_objects *extra_have)
 {
 	*list = NULL;
 	for (;;) {
@@ -72,6 +81,12 @@ struct ref **get_remote_heads(int in, struct ref **list,
 			server_capabilities = xstrdup(name + name_len + 1);
 		}
 
+		if (extra_have &&
+		    name_len == 5 && !memcmp(".have", name, 5)) {
+			add_extra_have(extra_have, old_sha1);
+			continue;
+		}
+
 		if (!check_ref(name, name_len, flags))
 			continue;
 		if (nr_match && !path_match(name, nr_match, match))
diff --git a/transport.c b/transport.c
index 71433d9..f7db5d9 100644
--- a/transport.c
+++ b/transport.c
@@ -619,7 +619,7 @@ static struct ref *get_refs_via_connect(struct transport *transport)
 	struct ref *refs;
 
 	connect_setup(transport);
-	get_remote_heads(data->fd[0], &refs, 0, NULL, 0);
+	get_remote_heads(data->fd[0], &refs, 0, NULL, 0, NULL);
 
 	return refs;
 }
@@ -652,7 +652,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 
 	if (!data->conn) {
 		connect_setup(transport);
-		get_remote_heads(data->fd[0], &refs_tmp, 0, NULL, 0);
+		get_remote_heads(data->fd[0], &refs_tmp, 0, NULL, 0, NULL);
 	}
 
 	refs = fetch_pack(&args, data->fd, data->conn,
-- 
1.6.0.1.415.g7bb82

[PATCH 4/4] push: receiver end advertises refs from alternate repositories

[Junio C Hamano]

Earlier, when pushing into a repository that borrows from alternate object
stores, we followed the longstanding design decision not to trust refs in
the alternate repository that houses the object store we are borrowing
from.  If your public repository is borrowing from Linus's public
repository, you pushed into it long time ago, and now when you try to push
your updated history that is in sync with more recent history from Linus,
you will end up sending not just your own development, but also the
changes you acquired through Linus's tree, even though the objects needed
for the latter already exists at the receiving end.  This is because the
receiving end does not advertise that the objects only reachable from the
borrowed repository (i.e. Linus's) are already available there.

This solves the issue by making the receiving end advertise refs from
borrowed repositories.  They are not sent with their true names but with a
phoney name ".have" to make sure that the old senders will safely ignore
them (otherwise, the old senders will misbehave, trying to push matching
refs, and mirror push that deletes refs that only exist at the receiving
end).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-receive-pack.c |   38 ++++++++++++++++++++++++++++++++++++++
 cache.h                |    2 ++
 sha1_file.c            |   10 ++++++++++
 3 files changed, 50 insertions(+), 0 deletions(-)

diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index 6d6027e..45e3cd9 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -6,6 +6,8 @@
 #include "exec_cmd.h"
 #include "commit.h"
 #include "object.h"
+#include "remote.h"
+#include "transport.h"
 
 static const char receive_pack_usage[] = "git-receive-pack <git-dir>";
 
@@ -462,6 +464,40 @@ static int delete_only(struct command *cmd)
 	return 1;
 }
 
+static int add_refs_from_alternate(struct alternate_object_database *e, void *unused)
+{
+	char *other = xstrdup(make_absolute_path(e->base));
+	size_t len = strlen(other);
+	struct remote *remote;
+	struct transport *transport;
+	const struct ref *extra;
+
+	while (other[len-1] == '/')
+		other[--len] = '\0';
+	if (len < 8 || memcmp(other + len - 8, "/objects", 8))
+		return 0;
+	/* Is this a git repository with refs? */
+	memcpy(other + len - 8, "/refs", 6);
+	if (!is_directory(other))
+		return 0;
+	other[len - 8] = '\0';
+	remote = remote_get(other);
+	transport = transport_get(remote, other);
+	for (extra = transport_get_remote_refs(transport);
+	     extra;
+	     extra = extra->next) {
+		add_extra_ref(".have", extra->old_sha1, 0);
+	}
+	transport_disconnect(transport);
+	free(other);
+	return 0;
+}
+
+static void add_alternate_refs(void)
+{
+	foreach_alt_odb(add_refs_from_alternate, NULL);
+}
+
 int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 {
 	int i;
@@ -497,7 +533,9 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 	else if (0 <= receive_unpack_limit)
 		unpack_limit = receive_unpack_limit;
 
+	add_alternate_refs();
 	write_head_info();
+	clear_extra_refs();
 
 	/* EOF */
 	packet_flush(1);
diff --git a/cache.h b/cache.h
index 98a7421..99af83a 100644
--- a/cache.h
+++ b/cache.h
@@ -641,6 +641,8 @@ extern struct alternate_object_database {
 } *alt_odb_list;
 extern void prepare_alt_odb(void);
 extern void add_to_alternates_file(const char *reference);
+typedef int alt_odb_fn(struct alternate_object_database *, void *);
+extern void foreach_alt_odb(alt_odb_fn, void*);
 
 struct pack_window {
 	struct pack_window *next;
diff --git a/sha1_file.c b/sha1_file.c
index ae550e8..12be17b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -393,6 +393,16 @@ void add_to_alternates_file(const char *reference)
 		link_alt_odb_entries(alt, alt + strlen(alt), '\n', NULL, 0);
 }
 
+void foreach_alt_odb(alt_odb_fn fn, void *cb)
+{
+	struct alternate_object_database *ent;
+
+	prepare_alt_odb();
+	for (ent = alt_odb_list; ent; ent = ent->next)
+		if (fn(ent, cb))
+			return;
+}
+
 void prepare_alt_odb(void)
 {
 	const char *alt;
-- 
1.6.0.1.415.g7bb82

Re: [PATCH 2/4] receive-pack: make it a builtin

[Paolo Bonzini]

Junio C Hamano wrote:
> It is a good thing to do in general, but more importantly, transport
> routines can only be used by built-ins, which is what I'll be adding next.

So how do I

        receive-pack = /home/bonzinip/bin/git-receive-pack

now?  Using libexec or ".../bin/git receive-pack" is fine, I just would
like to know it and see it in the release notes.  Or maybe
git-receive-pack should be installed in $(bindir) even if it is a builtin?

Paolo

Re: [PATCH 1/4] is_directory(): a generic helper function

[Johannes Sixt]

Junio C Hamano schrieb:
> +/*
> + * Do not use this for inspecting *tracked* content.  When path is a
> + * symlink to a directory, we do not want to say it is a directory.

I though stat(2) checks the thing that a symlink points to. Then either
this comment is not correct or you want to use lstat(2), no?

> + * Worth yet, leading components in path could contain symbolic links.
> + */
> +int is_directory(const char *path)
> +{
> +	struct stat st;
> +	return (!stat(path, &st) && S_ISDIR(st.st_mode));
> +}

-- Hannes

Re: [PATCH 1/4] is_directory(): a generic helper function

[Jeff King]

On Tue, Sep 09, 2008 at 12:31:39PM +0200, Johannes Sixt wrote:

> Junio C Hamano schrieb:
> > +/*
> > + * Do not use this for inspecting *tracked* content.  When path is a
> > + * symlink to a directory, we do not want to say it is a directory.
> 
> I though stat(2) checks the thing that a symlink points to. Then either
> this comment is not correct or you want to use lstat(2), no?

I assume he meant "do not use this for tracked content, because in that
case...". But I had to read it twice to make sense.

> > + * Worth yet, leading components in path could contain symbolic links.

And this didn't help. I assume it should be "worse yet" and Junio
developed some sort of typing lisp?

-Peff

Re: [PATCH 3/4] push: prepare sender to receive extended ref information from the receiver

[Jeff King]

On Tue, Sep 09, 2008 at 01:27:09AM -0700, Junio C Hamano wrote:

> +struct extra_have_objects {
> +	int nr, alloc;
> +	unsigned char (*array)[20];
> +};

This declaration made me double-take. I think it is correct, and you are
saving an extra pointer over a malloc. But the syntax ends up a little
confusing:

        hashcpy(&(extra->array[extra->nr][0]), sha1);

Using a struct probably would have been a little easier to read. I don't
know if this is worth changing.

> +	if (extra->alloc <= extra->nr)
> +		ALLOC_GROW(extra->array, extra->nr + 1, extra->alloc);

Isn't the conditional superfluous (ALLOC_GROW already makes this check)?

-Peff

Re: [PATCH 2/4] receive-pack: make it a builtin

[Junio C Hamano]

Paolo Bonzini <bonzini@gnu.org> writes:

> Junio C Hamano wrote:
>> It is a good thing to do in general, but more importantly, transport
>> routines can only be used by built-ins, which is what I'll be adding next.
>
> So how do I
>
>         receive-pack = /home/bonzinip/bin/git-receive-pack
>
> now?  Using libexec or ".../bin/git receive-pack" is fine, I just would
> like to know it and see it in the release notes.

Please check your Makefile (I'm too lazy to double check); don't we
install these server side programs in $(bindir)?

Re: [PATCH 2/4] receive-pack: make it a builtin

[Paolo Bonzini]

>> So how do I
>>
>>         receive-pack = /home/bonzinip/bin/git-receive-pack
>>
>> now?  Using libexec or ".../bin/git receive-pack" is fine, I just would
>> like to know it and see it in the release notes.
> 
> Please check your Makefile (I'm too lazy to double check); don't we
> install these server side programs in $(bindir)?

Yes, I interpreted the obsolete comment in Makefile

# ... and all the rest that could be moved out of bindir to gitexecdir

as s/could be/were/, but there is a special rule in install to place
these in $(bindir).

Paolo

Re: [PATCH 1/4] is_directory(): a generic helper function

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Sep 09, 2008 at 12:31:39PM +0200, Johannes Sixt wrote:
>
>> Junio C Hamano schrieb:
>> > +/*
>> > + * Do not use this for inspecting *tracked* content.  When path is a
>> > + * symlink to a directory, we do not want to say it is a directory.
>> 
>> I though stat(2) checks the thing that a symlink points to. Then either
>> this comment is not correct or you want to use lstat(2), no?
>
> I assume he meant "do not use this for tracked content, because in that
> case...". But I had to read it twice to make sense.

Sorry, I can't type, but you assumed correctly.  When we are tracking a
path "a/b" where "b" is a submodule, we want to notice the change made by
the user to rmdir "a/b" and replace it with a symbolic link that points
elsewhere, so we shouldn't be using is_directory() in such a codepath.
Worse yet, we also want to catch the case where you "rm -fr a" and make a
symlink that points at a directory that may (or may not) have a
subdirectory "b".  If we were tracking "a/b", then we have to say that the
directory "a/b" does not exist anymore.  It shows that is_directory() is a
wrong function to use while inspecting the tracked content (aka worktree).