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

Shourya Shukla [off-list ref] writes:

On 22/02 11:29, Junio C Hamano wrote:

quoted

Shourya Shukla [off-list ref] writes:

quoted

+	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

quoted

quoted

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

On 07/03 12:29, Junio C Hamano wrote:

Shourya Shukla [off-list ref] writes:

quoted

On 22/02 11:29, Junio C Hamano wrote:

quoted

Shourya Shukla [off-list ref] writes:

quoted

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

quoted

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.

quoted

What exactly do we need to change then?

Have you traced what happens when you make this call

quoted

quoted

quoted

+	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?