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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 23, 2013, 06:44 UTC
Message-ID: <7vy5fkiek0.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CAJDDKr6exXh14m08HTihxREjSFgyPT0bN1cF8eUryXJHOgFL1A@mail.gmail.com>

David Aguilar <davvid@gmail.com> writes:

Show 25 quoted lines

>> +static int walk(struct strbuf *path, int argc, const char **argv)
>> +{
>> +       DIR *dir;
>> +       struct dirent *ent;
>> +       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_setlen(path, len - 1);
>> +                       chdir(path->buf);
>> +                       handle_repo(path->buf, argv);
>> +                       chdir(root);
>> +                       strbuf_addstr(path, "/");
>> +                       continue;
>> +               }
>
> Does this section above properly handle .git files (where .git is a
> file, not a directory)?

This scans a directory $D to ask "is there '.git' in you?" and if the answer is "yes", then hands $D (not "$D/.git") to handle_repo(). That logic will not miss a gitfile that points at the real $GIT_DIR elsewhere.

There is a recursive call to walk() later in the same loop when the found entry ent turns out to be a directory, and "$D/" + ent->d_name is given to this function.

But I do not think the loop structure of this function is right. If $D has ".git" in it, should it even try to feed other subdirectories of $D (say "$D/a") to itself in recursion to see if $D/a/.git exists?

I think it should be more like

	walk(struct strbuf *path)
        {
		size_t dirlen = path->len;
		int has_git;

		strbuf_addstr(path, "/.git");
		has_git = !lstat(path->buf);
		strbuf_setlen(path, dirlen);

                if (has_git) {
                        handle_repo(path->buf);
			return;
		}
                dir = opendir(path->buf);
		while ((ent = readdir(dir))) {
			... skip . and .. ...
			strbuf_addstr(path, ent->d_name);
			walk(path);
			strbuf_setlen(path, dirlen);
                }
	}

The determination of has_git can be a bit fancier than a simple !lstat() as you mentioned.