Jonathan Nieder <jrnie...@gmail.com> writes:
> 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?
Also, while I do agree with you that the problem exists, it is
unclear why this patch is a solution and not a hack that sweeps a
problem under the rug.
It is unclear why this "silently detach HEAD without telling the
user" is a better solution than erroring out, for example [*1*].
[Footnote]
*1* For example, I would imagine that the problem can also be
"fixed" by detecting that the HEAD is on a branch, and noticing
that it will force rewinding of that branch if we did update-ref
in this codepath, and signal the failure to the caller of
submodule_move_head() without making the damage larger. And
tell the user what is going on, and perhaps suggest to detach
HEAD to avoid clobbering their branch.
>
> Thanks, Jonathan
>
>> [1] https://public-inbox.org/git/20170630003851.17288-1-sbel...@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;
