diff mbox series

[1/2] Doc: explain submodule.alternateErrorStrategy

Message ID a8125b946227d918865fde0dcbec516474b42386.1574731649.git.jonathantanmy@google.com (mailing list archive)
State New, archived
Headers show
Series Advice upon clone --recurse-submodules --reference | expand

Commit Message

Jonathan Tan Nov. 26, 2019, 1:30 a.m. UTC
Commit 31224cbdc7 ("clone: recursive and reference option triggers
submodule alternates", 2016-08-17) taught Git to support the
configuration options "submodule.alternateLocation" and
"submodule.alternateErrorStrategy" on a superproject.

If "submodule.alternateLocation" is configured to "superproject" on a
superproject, whenever a submodule of that superproject is cloned, it
instead computes the analogous alternate path for that submodule from
$GIT_DIR/objects/info/alternates of the superproject, and references it.

The "submodule.alternateErrorStrategy" option determines what happens
if that alternate cannot be referenced. However, it is not clear that
the clone proceeds as if no alternate was specified when that option is
not set to "die" (as can be seen in the tests in 31224cbdc7). Therefore,
document it accordingly.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
When I said "not clear" above, I mean that it is not clear *to me*, and
I assume that others will feel the same way. Feel free to drop this from
the patch set if the existing documentation is clear to most people.
---
 Documentation/config/submodule.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Jeff King Nov. 27, 2019, 11:32 a.m. UTC | #1
On Mon, Nov 25, 2019 at 05:30:59PM -0800, Jonathan Tan wrote:

> Commit 31224cbdc7 ("clone: recursive and reference option triggers
> submodule alternates", 2016-08-17) taught Git to support the
> configuration options "submodule.alternateLocation" and
> "submodule.alternateErrorStrategy" on a superproject.
> 
> If "submodule.alternateLocation" is configured to "superproject" on a
> superproject, whenever a submodule of that superproject is cloned, it
> instead computes the analogous alternate path for that submodule from
> $GIT_DIR/objects/info/alternates of the superproject, and references it.
> 
> The "submodule.alternateErrorStrategy" option determines what happens
> if that alternate cannot be referenced. However, it is not clear that
> the clone proceeds as if no alternate was specified when that option is
> not set to "die" (as can be seen in the tests in 31224cbdc7). Therefore,
> document it accordingly.
> 
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> When I said "not clear" above, I mean that it is not clear *to me*, and
> I assume that others will feel the same way. Feel free to drop this from
> the patch set if the existing documentation is clear to most people.

FWIW, I learned about these options for the first time from your email,
and I think the suggested change makes the behavior much more clear.

-Peff
Junio C Hamano Nov. 27, 2019, 12:30 p.m. UTC | #2
Jonathan Tan <jonathantanmy@google.com> writes:

> Commit 31224cbdc7 ("clone: recursive and reference option triggers
> submodule alternates", 2016-08-17) taught Git to support the
> configuration options "submodule.alternateLocation" and
> "submodule.alternateErrorStrategy" on a superproject.
> ...
> The "submodule.alternateErrorStrategy" option determines what happens
> if that alternate cannot be referenced. However, it is not clear that
> the clone proceeds as if no alternate was specified when that option is
> not set to "die" (as can be seen in the tests in 31224cbdc7). Therefore,
> document it accordingly.

Given that for everyday use (cf. sha1-file.c::link_alt_odb_entry())
of an alternate is best-effort basis, I have a feeling that it was a
design mistake to have the "error strategy" configuration option in
the first place, and "clone --reference-if-able" was the result of
the same design mistake.  We would have been better off if we made
these the best-effort features as well.

But the ship has sailed long ago---so I think these two are the best
we can do at this point.  Perhaps flipping the default to warn may
be a longer term improvement, too.
diff mbox series

Patch

diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt
index 0a1293b051..b33177151c 100644
--- a/Documentation/config/submodule.txt
+++ b/Documentation/config/submodule.txt
@@ -79,4 +79,6 @@  submodule.alternateLocation::
 submodule.alternateErrorStrategy::
 	Specifies how to treat errors with the alternates for a submodule
 	as computed via `submodule.alternateLocation`. Possible values are
-	`ignore`, `info`, `die`. Default is `die`.
+	`ignore`, `info`, `die`. Default is `die`. Note that if set to `ignore`
+	or `info`, and if there is an error with the computed alternate, the
+	clone proceeds as if no alternate was specified.