Re: [PATCH v2] all: new command used for multi-repo operations

[Junio C Hamano <gitster@pobox.com>]

Lars Hjemli <hjemli@gmail.com> writes:

> +static int walk(struct strbuf *path, int argc, const char **argv)
> +{
> +	DIR *dir;
> +	struct dirent *ent;
> +	struct stat st;
> +	size_t len;
> +
> +	dir = opendir(path->buf);
> +	if (!dir)
> +		return errno;
> +	strbuf_addstr(path, "/");
> +	len = path->len;
> +	while ((ent = readdir(dir))) {
> +		if (!strcmp(ent->d_name, ".") || !strcmp(ent->d_name, ".."))
> +			continue;
> +		if (!strcmp(ent->d_name, ".git")) {
> +			strbuf_addstr(path, ent->d_name);
> +			setenv(GIT_DIR_ENVIRONMENT, path->buf, 1);
> +			strbuf_setlen(path, len - 1);
> +			setenv(GIT_WORK_TREE_ENVIRONMENT, path->buf, 1);
> +			handle_repo(path->buf, argv);
> +			strbuf_addstr(path, "/");
> +			continue;
> +		}
> +		strbuf_setlen(path, len);
> +		strbuf_addstr(path, ent->d_name);
> +		switch (DTYPE(ent)) {
> +		case DT_UNKNOWN:
> +			/* Use stat() instead of lstat(), since we want to
> +			 * know if we can follow this path into another
> +			 * directory - it's  not important if it's actually
> +			 * a symlink which gets us there.
> +			 */

This is wrong if you are on a platform that does have d_type, no?
It may say it is a symbolic link, and until you stat you wouldn't
know if it may lead to a directory.  You can add "case DT_LNK:" that
behaves the same as DT_UNKNOWN, I think.

> +			if (stat(path->buf, &st) || !S_ISDIR(st.st_mode))
> +				break;
> +			/* fallthrough */
> +		case DT_DIR:
> +			walk(path, argc, argv);
> +			break;
> +		}
> +		strbuf_setlen(path, len);
> +	}

But I still do not think this loop is correct.  In a repository that
has a working tree, you would learn that directory $D has $D/.git in
it, feed $D to handle_repo(), and then descend into $D/.git/objects/,
$D/.git/refs, and other random directories to see if you can find
other repositories.  That is just not right.

If this check were doing something like "The directory $D is worth
handing to handle_repo() if it has all of the following: objects/,
refs/ and HEAD that either points inside refs/ or 40-hex.", then it
would make a lot more sense to me, including the part that goes on
to check sibling directories.  As a bonus side effect, it will give
you a support for bare repositories for free.