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

[Shourya Shukla <periperidip@gmail.com>]

On 07/03 12:29, Junio C Hamano wrote:
> Shourya Shukla <periperidip@gmail.com> writes:
> 
> > 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.
> 
> > 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.

Ahhh. Understood and will work on it. BTW then when does
GITMODULES_INDEX even fulfill its purpose? Its name can confuse anyone
into thinking what it made me think: it is the index copy of the
gitmodules.

Is it something which is to be changed in the near future?