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

From: Junio C Hamano <[email-masked]>
Date: Sun, 07 Mar 2021 12:29:30 -0800

Shourya Shukla <[email-masked]> writes:

> On 22/02 11:29, Junio C Hamano wrote:
>> Shourya Shukla <[email-masked]> 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.

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