Am 24.02.2011 00:07, schrieb Jonathan Nieder:
> Jens Lehmann wrote:
>
>> To be able to access all commits of populated submodules referenced by the
>> superproject it is sufficient to only then let "git fetch" recurse into a
>> submodule when the new commits fetched in the superproject record new
>> commits for it.
>
> Exciting stuff. This would use the default refspec rather than fetching
> the bare minimum of commits in submodules, right?
Yup, AFAIK I have no means right now to tell "git fetch" to only fetch
certain commits. But we were talking at the GitTogether that this might
be useful and with a bit of tweaking the code would support that too.
>> --- a/Documentation/fetch-options.txt
>> +++ b/Documentation/fetch-options.txt
>> @@ -73,6 +73,14 @@ ifndef::git-pull[]
>> Prepend <path> to paths printed in informative messages
>> such as "Fetching submodule foo". This option is used
>> internally when recursing over submodules.
>> +
>> +--submodule-default=[yes|on-demand]::
>> + This option is used internally to set the submodule recursion default
>> + to either a boolean configuration value representing "true" (for
>> + unconditonal recursion) or to "on-demand" (when only those submodules
>> + should be fetched of which new commits have been fetched in its
>> + superproject).
>> + Using the "--[no-]recurse-submodules" option ignores this setting.
>
> Could this be combined with the --recurse-submodules, with a "last instance
> of the option wins" rule? Something like:
>
> --recurse-submodules[=(yes | no | changed)]::
> --no-recurse-submodules::
Nope, as this only sets the default. "--recurse-submodules" overrides
anything configured, which is not what we want here. This option should
only set the default.
>> +++ b/submodule.c
> [...]
>> @@ -248,11 +263,73 @@ void set_config_fetch_recurse_submodules(int value)
> [...]
>> +static void submodule_collect_changed_cb(struct diff_queue_struct *q,
> [...]
>> + if (S_ISGITLINK(p->one->mode)) {
>> + /* NEEDSWORK: We should honor the name configured in
>> + * the .gitmodules file of the commit we are examining
>> + * here to be able to correctly follow submodules
>> + * being moved around. */
>
> Hmm, a sort of variant on rename detection. Does "git submodule update" /
> "git checkout --recurse-submodules" use .gitmodules to move pre-populated
> subrepositories?
Not yet ;-)
>> + /* Submodule is new or was moved here */
>> + /* NEEDSWORK: When the .git directories of submodules
>> + * live inside the superprojects .git directory some
>> + * day we should fetch new submodules directly into
>> + * that location too when config or options request
>> + * that so they can be checked out from there. */
>> + continue;
>
> Maybe this can be mentioned in a BUGS section on the git-fetch(1)
> manpage to give readers a warning and clue about the intended
> meaning of --recurse-submodules?
>
> I'm afraid a certain kind of person might get used to the "don't fetch
> new submodules" behavior (e.g., if upstream is including unneeded
> convenience copies of libraries right and left in addition to some
> useful submodules).
I'm not sure I understand what you mean by this, right now this can
only work for populated submodules. I hope this will change soon, but
I'm not quite there yet ;-)
>> +void check_for_new_submodule_commits(unsigned char new_sha1[20])
>> +{
>> + struct rev_info rev;
>> + struct commit *commit;
>> + int argc = 5;
>> + const char *argv[] = {NULL, NULL, "--not", "--branches", "--remotes", NULL};
>
> Nit: maybe
>
> const char *argv[] = ...;
> int argc = ARRAY_SIZE(argv) - 1;
>
> ?
Fine by me!
>> +
>> + init_revisions(&rev, NULL);
>> + argv[1] = xstrdup(sha1_to_hex(new_sha1));
>
> Maybe:
>
> char *new_rev;
> ...
> argv[1] = new_rev = xstrdup(...);
> ...
> free(new_rev);
I'm not sure I get what the extra variable gains us ...
>> + setup_revisions(argc, argv, &rev, NULL);
>> + if (prepare_revision_walk(&rev))
>> + die("revision walk setup failed");
>> +
>
> Maybe a comment so the reader doesn't have to delve deeper?
?
> /*
> * Collect checked out submodules that have changed upstream
> * in "changed_submodule_paths".
> */
>
>> + while ((commit = get_revision(&rev))) {
> [...]
>> +++ b/t/t5526-fetch-submodules.sh
>> @@ -192,4 +192,113 @@ test_expect_success "--no-recurse-submodules overrides config setting" '
> [...]
>> + echo "Fetching submodule submodule" > expect.out.sub &&
>> + echo "From $pwd/." > expect.err.sub &&
>> + echo " $head1..$head2 master -> origin/master" >> expect.err.sub
>
> I wonder if we should be testing the output in this much detail.
>
> Wouldn't it be nicer to check the remote-tracking refs to make sure
> the lasting effects were correct, rather than the detailed output
> format?
Yes, that makes sense!
> So: aside from the option name, nothing but nits. Thanks; that was
> fun.
Thanks!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html