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.name.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.name.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 without
'update = checkout', too?
Right.
Is it safe to leave it out for now?
Yes it is.
[...]
+int submodule_needs_update(const
Re: [WIP/PATCH 1/9] submodule: prepare for recursive checkout of submodules
Am 03.02.2014 23:23, schrieb Junio C Hamano:
Jens Lehmann jens.lehm...@web.de 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
Jens Lehmann jens.lehm...@web.de 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
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.name.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.name.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 case futureproofing for when
