Re: [PATCH v2 1/1] rm: stage submodule removal from '.gitmodules' when using '--cached'

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 7, 2021, 20:29 UTC
Message-ID: <xmqqblbu907p.fsf@gitster.c.googlers.com>
In-Reply-To: <20210307164644.GA8702@konoha>

Shourya Shukla <periperidip@gmail.com> writes:

Show 18 quoted lines

> On 22/02 11:29, Junio C Hamano wrote:
>> Shourya Shukla <periperidip@gmail.com> writes:
>> 
>> > +	if (git_config_rename_section_in_file(index_only ? GITMODULES_INDEX :
>> > +					      GITMODULES_FILE, sect.buf, NULL) < 0) {
>> 
>> Also, is it really sufficient to pass GITMODULES_INDEX as the first
>> argument to this function to tweak what is in the index?
>> 
>> git_config_copy_or_rename_section_in_file() which is the
>> implementation of that helper seems to always want to work with a
>> file that is on disk, by making unconditional calls to
>> hold_lock_file_for_update(), fopen(), fstat(), chmod(), etc.
>> 
>> So I suspect that there are much more work needed.  
>
> I am not able to comprehend _why_ we need so much more work. To me it
> seems to work fine.

Show 6 quoted lines

> The flow now is something like:
>
> 1. If !index_only i.e., '--cached' is not passed then remove the entry
> of the SM from the working tree copy of '.gitmodules' i.e.,
> GITMODULES_FILE. If there are any unstaged mods in '.gitmodules', we do
> not proceed with 'git rm'.

That side is fine, especially if we are extending the "when doing 'git rm PATH' (without '--cached'), PATH must match between the index and the working tree" to "when doing 'git rm SUBMODULE', not just SUBMODULE but also '.gitmodules' must match between the index and the working tree", then adjusting the entry for SUBMODULE in '.gitmodules' in the working tree and adding the result to the index would give the same result as editing '.gitmodules' both in the index and in the working tree independently.

But the problem is that there is no way "--cached" case would work with your code.

> What exactly do we need to change then?

Have you traced what happens when you make this call

>> > +	if (git_config_rename_section_in_file(index_only ? GITMODULES_INDEX :
>> > +					      GITMODULES_FILE, sect.buf, NULL) < 0) {

with index_only set? i.e. GIT_MODULES_INDEX passed as the config_filename argument?

The first parameter to the git_config_rename_section_in_file() names a filename in the working tree to be edited. Writing ':.gitmodules' does not make the function magically work in-core without touching the working tree. It will make it update a file (likely not tracked) whose name is ":.gitmodules" in the working tree, no?

Presumably you want to edit in-index .gitmodules without touching the working tree file, but the call is not doing that---and it would take much more work to teach it do so.

And a cheaper way out would be how I outlined in the message you are responding to, i.e. write out the in-index .gitmodules to a temporary file, let git_config_rename_section_in_file() tweak that temporary file, and add it back into the index.