Re: [PATCH] recursive submodules: detach HEAD from new state

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Stefan Beller wrote:

> When a submodule is on a branch and in its superproject you run a
> recursive checkout, the branch of the submodule is updated to what the
> superproject checks out. This is very unexpected in the current model of
> Git as e.g. 'submodule update' always detaches the submodule HEAD.
>
> Despite having plans to have submodule HEADS not detached in the future,
> the current behavior is really bad as it doesn't match user expectations
> and it is not checking for loss of commits (only to be recovered via the
> reflog).

I think the corrected behavior doesn't match user expectations,
either.

Could this patch include some documentation to help users know what to
expect?

> Detach the HEAD unconditionally in the submodule when updating it.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> This is a resend of [1], which did not receive any attention.

Yikes.  Yes, this bug looks problematic.  Thanks for working on it.

> I improved the commit message laying out the current state of affairs,
> arguing that any future plan should not weigh in as much as the current
> possible data loss.

Can you elaborate on what you mean about data loss?  At first glance
it would seem to me that detaching HEAD could lead to data loss since
there isn't a branch to keep track of the user's work.  Are you saying
the current behavior of updating whatever branch HEAD is on (which,
don't get me wrong, is a wrong behavior that needs fixing) bypassed
the reflog?

Thanks,
Jonathan

> [1] https://public-inbox.org/git/20170630003851.17288-1-sbeller@google.com/
[...]
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1653,7 +1653,8 @@ int submodule_move_head(const char *path,
>  			cp.dir = path;
>  
>  			prepare_submodule_repo_env(&cp.env_array);
> -			argv_array_pushl(&cp.args, "update-ref", "HEAD", new, NULL);
> +			argv_array_pushl(&cp.args, "update-ref", "HEAD",
> +					 "--no-deref", new, NULL);
>  
>  			if (run_command(&cp)) {
>  				ret = -1;