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

> Lars Hjemli <hjemli@gmail.com> writes:
>
>> 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'
>

>> 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.
>

> 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 "--".

>> +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?

>> +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.

>> +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?

>> 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?

>> 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.

>> +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.