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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 24, 2017, 21:33 UTC
Message-ID: <xmqq8tjdcyf1.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170724180312.GE13924@aiede.mtv.corp.google.com>

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 12 quoted lines

> 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.

Show 17 quoted lines

>
> 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;