Re: [PATCH 1/2] entry.c: submodule recursing: respect force flag correctly

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 29, 2017, 21:30 UTC
Message-ID: <xmqq37dvahzw.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170329184519.GW31294@aiede.mtv.corp.google.com>

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 8 quoted lines

> Stefan Beller wrote:
>
>> In case of a non-forced worktree update, the submodule movement is tested
>> in a dry run first, such that it doesn't matter if the actual update is
>> done via the force flag. However for correctness, we want to give the
>> flag is specified by the user.
>
> "for correctness" means "to avoid races"?

Sorry, but neither explanation makes much sense to me.

The codepath the patch touches says "if the submodule is not populated, then checkout the submodule by switching from NULL (nothing checked out) to the commit bound to the index of the superproject; otherwise, checkout the submodule by switching from HEAD (what is currently checked out) to the commit in the index".

Where does that "tested in a dry run first" come into play? Whatever code calls checkout_entry(), does it call it twice, first with a "--dry-run" option and then without one? How does this codepath respond differently to these two invocations, and how does this change affect the way these two invocations behave?

Show 27 quoted lines

>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  entry.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/entry.c b/entry.c
>> index d2b512da90..645121f828 100644
>> --- a/entry.c
>> +++ b/entry.c
>> @@ -287,7 +287,7 @@ int checkout_entry(struct cache_entry *ce,
>>  			} else
>>  				return submodule_move_head(ce->name,
>>  					"HEAD", oid_to_hex(&ce->oid),
>> -					SUBMODULE_MOVE_HEAD_FORCE);
>> +					state->force ? SUBMODULE_MOVE_HEAD_FORCE : 0);
>
> Looks like a good change.
>
> This moves past the 80-column margin.  I wish there were a tool like
> gofmt or clang-format that would take care of formatting for us.
>
> This isn't the only place SUBMODULE_MOVE_HEAD_FORCE is used in the
> file.  Do they need the same treatment?
>
> Thanks,
> Jonathan