Re: [WIP/PATCH 1/9] submodule: prepare for recursive checkout of submodules
Am 03.02.2014 23:23, schrieb Junio C Hamano: > Jens Lehmann writes: > >> This commit adds the functions and files needed for configuration, > > Please just say "Add the functions and files needed for ...". Roger that. >> +++ b/Documentation/recurse-submodules-update.txt >> @@ -0,0 +1,8 @@ >> +--[no-]recurse-submodules:: >> +Using --recurse-submodules will update the work tree of all >> +initialized submodules according to the commit recorded in the >> +superproject if their update configuration is set to checkout'. If > > That single quote does not seem to be closing any matching quote. > > The phrase "according to" feels a bit too fuzzy. Merging the commit > to what is checked out is one possible implementation of "according to". > Applying the diff between the commit and what is checked out to work > tree is another. Resetting the work tree files to exactly match the > commit would be yet another. > > I think "update the work trees to the commit" (i.e. lose the > "according") would be the closest to what you are trying to say > here. > >> +local modifications in a submodule would be overwritten the checkout >> +will fail unless forced. Without this option or with >> +--no-recurse-submodules is, the work trees of submodules will not be >> +updated, only the hash recorded in the superproject will be updated. > > It is unclear what happens if their update configuration is set to > something other than 'checkout'. Jonathan already proposed a better description, will use that in the next round. >> diff --git a/submodule.c b/submodule.c >> index 613857e..b3eb28d 100644 >> --- a/submodule.c >> +++ b/submodule.c >> @@ -382,6 +384,48 @@ int parse_fetch_recurse_submodules_arg(const char *opt, >> const char *arg) >> ... >> +int option_parse_update_submodules(const struct option *opt, >> + const char *arg, int unset) >> +{ >> +if (unset) { >> +*(int *)opt->value = RECURSE_SUBMODULES_OFF; >> +} else { >> +if (arg) >> +*(int *)opt->value = >> parse_update_recurse_submodules_arg(opt->long_name, arg); >> +else >> +*(int *)opt->value = RECURSE_SUBMODULES_ON; >> +} > > You can easily unnest to lose {} > > if (unset) > value = off; > else if (arg) > value = parse...; > else > value = on; Yeah, that's better. > Also I suspect that git_config_maybe_bool() natively knows how to > handle arg==NULL, so > > if (unset) > value = off; > else > value = parse...; > > is sufficient? Will try. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [WIP/PATCH 1/9] submodule: prepare for recursive checkout of submodules
Am 04.02.2014 01:01, schrieb Jonathan Nieder: > Jens Lehmann wrote: >> --- /dev/null >> +++ b/Documentation/recurse-submodules-update.txt >> @@ -0,0 +1,8 @@ >> +--[no-]recurse-submodules:: >> +Using --recurse-submodules will update the work tree of all >> +initialized submodules according to the commit recorded in the >> +superproject if their update configuration is set to checkout'. If >> +local modifications in a submodule would be overwritten the checkout >> +will fail unless forced. Without this option or with >> +--no-recurse-submodules is, the work trees of submodules will not be >> +updated, only the hash recorded in the superproject will be updated. > > Tweaks: > > * Spelling out "--no-recurse-submodules, --recurse-submodules" (imitating >e.g. --decorate in git-log(1)) > > * Shortening, using imperative mood > > * Skipping description of safety check, since it matches how checkout >works in general > > That would make > > --no-recurse-submodules:: > --recurse-submodules:: > Perform the checkout in submodules, too. This only affects > submodules with update strategy `checkout` (which is the > default update strategy; see `submodule..update` in > link:gitmodules[5]). > + > The default behavior is to update submodule entries in the superproject > index and to leave the inside of submodules alone. That behavior can > also > be requested explicitly with --no-recurse-submodules. Much better, thanks! > Ideas for further work: > > * The safety check probably deserves a new section where it could be >described in detail alongside a description of the corresponding check >for plain checkout. Then the description of the -f option could >point to that section. Good idea. > * What happens when update = merge, rebase, or !command? I think >skipping them for now like suggested above is fine, but: > >- It would be even better to error out when there are changes to carry > over with update = merge or rebase In the first round I'd rather do nothing (just like we do now) for merge or rebase. These two should be tackled in a follow up series (especially as I currently do not think everybody agrees on the desired behavior when the branch config is set yet) >- Better still to perform the rebase when update = rebase > >- I have no idea what update = merge should do for non-fast-forward > moves The same it does for checkout when we would overwrite local changes: error out before doing anything and let the user sort things out? >> --- a/submodule.c >> +++ b/submodule.c >> @@ -16,6 +16,8 @@ static struct string_list config_name_for_path; >> static struct string_list config_fetch_recurse_submodules_for_name; >> static struct string_list config_ignore_for_name; >> static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND; >> +static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF; >> +static int option_update_recurse_submodules = RECURSE_SUBMODULES_DEFAULT; > > Confusingly, config_update_recurse_submodules is set using the > --recurse-submodules-default option, not configuration. There's > precedent for that in fetch.recurseSubmodules handling, but perhaps > a comment would help --- something like > > /* >* When no --recurse-submodules option was passed, should git fetch >* from submodules where submodule..fetchRecurseSubmodules >* doesn't indicate what to do? >* >* Controlled by fetch.recurseSubmodules. The default is determined by >* the --recurse-submodules-default option, which propagates >* --recurse-submodules from the parent git process when recursing. >*/ > static int config_fetch_recurse_submodules = > RECURSE_SUBMODULES_ON_DEMAND; > > /* >* When no --recurse-submodules option was passed, should git update >* the index and worktree within submodules (and in turn their >* submodules, etc)? >* >* Controlled by the --recurse-submodules-default option, which >* propagates --recurse-submodules from the parent git process >* when recursing. >*/ > static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF; Makes lots of sense. > [...] >> @@ -382,6 +384,48 @@ int parse_fetch_recurse_submodules_arg(const char *opt, >> const char *arg) >> } >> } >> >> +int parse_update_recurse_submodules_arg(const char *opt, const char *arg) >> +{ >> +switch (git_config_maybe_bool(opt, arg)) { >> +case 1: >> +return RECURSE_SUBMODULES_ON; >> +case 0: >> +return RECURSE_SUBMODULES_OFF; >> +default: >> +if (!strcmp(arg, "checkout")) >> +return RECURSE_SUBMODULES_ON; > > Hm, is this arg == checkout case futureproofing for when > --recurse-submodules learns to handle submodules
Re: [WIP/PATCH 1/9] submodule: prepare for recursive checkout of submodules
Jens Lehmann wrote: > This commit adds the functions and files needed for configuration, > documentation, setting the default behavior and determining if a > submodule path should be updated automatically. Yay! [...] > Documentation/recurse-submodules-update.txt | 8 + > submodule.c | 50 > + > submodule.h | 6 > 3 files changed, 64 insertions(+) > create mode 100644 Documentation/recurse-submodules-update.txt I like the shared documentation snippet. Ok, naive questions and overly pedantic nitpicking follow. Patch with a couple of suggested changes at the end. [...] > --- /dev/null > +++ b/Documentation/recurse-submodules-update.txt > @@ -0,0 +1,8 @@ > +--[no-]recurse-submodules:: > + Using --recurse-submodules will update the work tree of all > + initialized submodules according to the commit recorded in the > + superproject if their update configuration is set to checkout'. If > + local modifications in a submodule would be overwritten the checkout > + will fail unless forced. Without this option or with > + --no-recurse-submodules is, the work trees of submodules will not be > + updated, only the hash recorded in the superproject will be updated. Tweaks: * Spelling out "--no-recurse-submodules, --recurse-submodules" (imitating e.g. --decorate in git-log(1)) * Shortening, using imperative mood * Skipping description of safety check, since it matches how checkout works in general That would make --no-recurse-submodules:: --recurse-submodules:: Perform the checkout in submodules, too. This only affects submodules with update strategy `checkout` (which is the default update strategy; see `submodule..update` in link:gitmodules[5]). + The default behavior is to update submodule entries in the superproject index and to leave the inside of submodules alone. That behavior can also be requested explicitly with --no-recurse-submodules. Ideas for further work: * The safety check probably deserves a new section where it could be described in detail alongside a description of the corresponding check for plain checkout. Then the description of the -f option could point to that section. * What happens when update = merge, rebase, or !command? I think skipping them for now like suggested above is fine, but: - It would be even better to error out when there are changes to carry over with update = merge or rebase - Better still to perform the rebase when update = rebase - I have no idea what update = merge should do for non-fast-forward moves > --- a/submodule.c > +++ b/submodule.c > @@ -16,6 +16,8 @@ static struct string_list config_name_for_path; > static struct string_list config_fetch_recurse_submodules_for_name; > static struct string_list config_ignore_for_name; > static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND; > +static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF; > +static int option_update_recurse_submodules = RECURSE_SUBMODULES_DEFAULT; Confusingly, config_update_recurse_submodules is set using the --recurse-submodules-default option, not configuration. There's precedent for that in fetch.recurseSubmodules handling, but perhaps a comment would help --- something like /* * When no --recurse-submodules option was passed, should git fetch * from submodules where submodule..fetchRecurseSubmodules * doesn't indicate what to do? * * Controlled by fetch.recurseSubmodules. The default is determined by * the --recurse-submodules-default option, which propagates * --recurse-submodules from the parent git process when recursing. */ static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND; /* * When no --recurse-submodules option was passed, should git update * the index and worktree within submodules (and in turn their * submodules, etc)? * * Controlled by the --recurse-submodules-default option, which * propagates --recurse-submodules from the parent git process * when recursing. */ static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF; [...] > @@ -382,6 +384,48 @@ int parse_fetch_recurse_submodules_arg(const char *opt, > const char *arg) > } > } > > +int parse_update_recurse_submodules_arg(const char *opt, const char *arg) > +{ > + switch (git_config_maybe_bool(opt, arg)) { > + case 1: > + return RECURSE_SUBMODULES_ON; > + case 0: > + return RECURSE_SUBMODULES_OFF; > + default: > + if (!strcmp(arg, "checkout")) > + return RECURSE_SUBMODULES_ON; Hm, is this arg == checkout ca
Re: [WIP/PATCH 1/9] submodule: prepare for recursive checkout of submodules
Jens Lehmann writes: > This commit adds the functions and files needed for configuration, Please just say "Add the functions and files needed for ...". > +++ b/Documentation/recurse-submodules-update.txt > @@ -0,0 +1,8 @@ > +--[no-]recurse-submodules:: > + Using --recurse-submodules will update the work tree of all > + initialized submodules according to the commit recorded in the > + superproject if their update configuration is set to checkout'. If That single quote does not seem to be closing any matching quote. The phrase "according to" feels a bit too fuzzy. Merging the commit to what is checked out is one possible implementation of "according to". Applying the diff between the commit and what is checked out to work tree is another. Resetting the work tree files to exactly match the commit would be yet another. I think "update the work trees to the commit" (i.e. lose the "according") would be the closest to what you are trying to say here. > + local modifications in a submodule would be overwritten the checkout > + will fail unless forced. Without this option or with > + --no-recurse-submodules is, the work trees of submodules will not be > + updated, only the hash recorded in the superproject will be updated. It is unclear what happens if their update configuration is set to something other than 'checkout'. > diff --git a/submodule.c b/submodule.c > index 613857e..b3eb28d 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -382,6 +384,48 @@ int parse_fetch_recurse_submodules_arg(const char *opt, > const char *arg) > ... > +int option_parse_update_submodules(const struct option *opt, > +const char *arg, int unset) > +{ > + if (unset) { > + *(int *)opt->value = RECURSE_SUBMODULES_OFF; > + } else { > + if (arg) > + *(int *)opt->value = > parse_update_recurse_submodules_arg(opt->long_name, arg); > + else > + *(int *)opt->value = RECURSE_SUBMODULES_ON; > + } You can easily unnest to lose {} if (unset) value = off; else if (arg) value = parse...; else value = on; Also I suspect that git_config_maybe_bool() natively knows how to handle arg==NULL, so if (unset) value = off; else value = parse...; is sufficient? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html