Re: [PATCH v4 1/2] for-each-repo: new command used for multi-repo operations

On Sun, Jan 27, 2013 at 8:04 PM, Junio C Hamano [off-list ref] wrote:

Lars Hjemli [off-list ref] writes:

quoted

The command also honours the option '--clean' which restricts the set of
repos to those which '--dirty' would skip, and '-x' which is used to
execute non-git commands.

It might make sense to internally use RUN_GIT_CMD flag when the
first word of the command line is 'git' as an optimization, but
I am not sure it is a good idea to force the end users to think
when to use -x and when not to is a good idea.

In other words, I think

     git for-each-repo -d diff --name-only
     git for-each-repo -d -x ls '*.c'

is less nice than letting the user say

     git for-each-repo -d git diff --name-only
     git for-each-repo -d ls '*.c'

The 'git-for-each-repo' command was made to allow any git command to
be executed in all discovered repositories, and I've used it that way
for two years (in the form of a shell-script called 'git-all'). During
this time, I've occasionally thought about forking non-git commands
but the itch hasn't been strong enough for me to scratch. The point
I'm trying to make is that to me, this command acts as a modifier for
other git commands[1]. Having the possibility to execute non-git
commands would be nice, but it is not the main objective of this
command.

[1] The 'git -a' rewrite patch shows how I think about this command -
it's just an option to the 'git' command, modifying the way any
subcommand is invoked (btw: I don't expect that patch to be applied
since 'git-all' was deemed to generic, so I'll just carry the patch in
my own tree).

quoted

Finally, the command to execute within each repo is optional. If none is
given, git-for-each-repo will just print the path to each repo found. And
since the command supports -z, this can be used for more advanced scripting
needs.

It amounts to the same thing, but I would rather describe it as:

    To allow scripts to handle paths with shell-unsafe characters,
    support "-z" to show paths with NUL termination.  Otherwise,
    such paths are shown with the usual c-quoting.

Much better, thanks.

One more thing that nobody brought up during the previous reviews is
if we want to support subset of repositories by allowing the
standard pathspec match mechanism.  For example,

        git for-each-repo -d git diff --name-only -- foo/ bar/b\*z

might be a way to ask "please find repositories match the given
pathspecs (i.e. foo/ bar/b\*z) and run the command in the ones that
are dirty".  We would need to think about how to mark the end of the
command though---we could borrow \; from find(1), even though find
is not the best example of the UI design.  I.e.

        git for-each-repo -d git diff --name-only \; [--] foo/ bar/b\*z

with or without "--".

I don't think this would be very nice to end users, and would prefer
--include and --exclude options (the latter is actually already a part
of git-all, added by one of my coworkers).

quoted

+NOTES
+-----
+
+For the purpose of `git-for-each-repo`, a dirty worktree is defined as a
+worktree with uncommitted changes.

Is it a definition that is different from usual?  If so why does it
need to be inconsistent with the rest of the system?

I just wanted to clarify what condition --dirty and --clean will
check. In particular, the lack of checking for untracked files (which
could be added as yet another option).

quoted

+static void print_repo_path(const char *path, unsigned pretty)
+{
+     if (path[0] == '.' && path[1] == '/')
+             path += 2;
+     if (pretty)
+             color_fprintf_ln(stdout, color, "[%s]", path);

This is shown before running a command in that repository.  I am of
two minds.  It certainly is nice to be able to tell which repository
each block of output lines comes from, and not requiring the command
to do this themselves is a good default.  However, I wonder if people
would want to do something like this:

        git for-each-repo sh -c '
                git diff --name-only |
                sed -e "s|^|$path/|"
        '

to get a consolidated view, in a way similar to how "submodule
foreach" can be used.  This unconditional output will get in the way
for such a use case.

I guess -q/--quiet could be useful.

quoted

+static int walk(struct strbuf *path, int argc, const char **argv)
+{
+     DIR *dir;
+     struct dirent *ent;
+     struct stat st;
+     size_t len;
+     int has_dotgit = 0;
+     struct string_list list = STRING_LIST_INIT_DUP;
+     struct string_list_item *item;
+
+     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")) {
+                     has_dotgit = 1;
+                     continue;
+             }
+             switch (DTYPE(ent)) {
+             case DT_UNKNOWN:
+             case DT_LNK:
+                     /* Use stat() to figure out if this path leads
+                      * to a directory - it's  not important if it's
+                      * a symlink which gets us there.
+                      */
+                     strbuf_setlen(path, len);
+                     strbuf_addstr(path, ent->d_name);
+                     if (stat(path->buf, &st) || !S_ISDIR(st.st_mode))
+                             break;
+                     /* fallthrough */
+             case DT_DIR:
+                     string_list_append(&list, ent->d_name);
+                     break;
+             }
+     }
+     closedir(dir);
+     strbuf_setlen(path, len);
+     if (has_dotgit)
+             handle_repo(path, argv);
+     sort_string_list(&list);
+     for_each_string_list_item(item, &list) {
+             strbuf_setlen(path, len);
+             strbuf_addstr(path, item->string);
+             walk(path, argc, argv);
+     }
+     string_list_clear(&list, 0);
+     return 0;
+}

Is the "collect-first-and-then-sort" done so that the repositories
are shown in a stable order regardless of the order in which
readdir() returns he entries?

Yes (writing the testcases demonstrated a need for predictable output).

quoted

diff --git a/t/t6400-for-each-repo.sh b/t/t6400-for-each-repo.sh

This command does not look like "6 - the revision tree commands" to
me. "7 - the porcelainish commands concerning the working tree" or
"9 - the git tools" may be a better match?

Ok, how about t9003?

quoted

new file mode 100755
index 0000000..af02c0c

--- /dev/null
+++ b/t/t6400-for-each-repo.sh

@@ -0,0 +1,150 @@
+#!/bin/sh
+#
+# Copyright (c) 2013 Lars Hjemli
+#
+
+test_description='Test the git-for-each-repo command'
+
+. ./test-lib.sh
+
+qname="with\"quote"
+qqname="\"with\\\"quote\""

If Windows does not have problems with paths with dq in it, then
this is fine, but I dunno.  Otherwise, you may want to exclude the
c-quote testing from the main part of the test, and have a single
test that has prerequisite for filesystems that can do this at the
end of the script.

I'll check my patch on msysgit before resending.

quoted

+test_expect_success "setup" '
+     test_create_repo clean &&
+     (cd clean && test_commit foo1) &&
+     git init --separate-git-dir=.cleansub clean/gitfile &&
+     (cd clean/gitfile && test_commit foo2 && echo bar >>foo2.t) &&
+     test_create_repo dirty-idx &&
+     (cd dirty-idx && test_commit foo3 && git rm foo3.t) &&
+     test_create_repo dirty-wt &&
+     (cd dirty-wt && mv .git .linkedgit && ln -s .linkedgit .git &&

Some platforms are symlink-challenged.  Can we do this test without
"ln -s"?  SYMLINKS prereq wouldn't be very useful for the setup
step, as all the remaining tests won't work without setting up the
test scenario.

I added this test to check the DT_UNKNOWN/DT_LINK case in walk() so
I'd rather not drop it, but it can be moved into a standalone,
SYMLINKS-enabled testcase.

Thanks for the review.

-- 
larsh