threads / rfc / 55115

[RFC] [BUDFIX] 'git rm --cached <submodule>' does not stage the changed .gitmodules

Subject: [RFC] [BUDFIX] 'git rm --cached <submodule>' does not stage the changed .gitmodules

## tl;dr

6 messages between Feb 7, 2021 and Feb 9, 2021.

replies: 5people: 3as markdown or json

Shourya Shukla· Feb 7, 2021, 14:41 UTC · lore
Hello all,

I was lurking around 'gitgitgadget/git' when I saw this potential BUG added by Phillipe Blaine (reported by Javier Mora): https://github.com/gitgitgadget/git/issues/750

Link to the original mail by Javier: https://lore.kernel.org/git/ea91c2ea29064079914f6a522db5115a@UUSALE0Z.utcmail.com/

In brief, 'git rm' does not stage the changed '.gitmodules' file when we use the '--cached' option. Technically speaking, Git used to behave this way only and hence this is not an unknown case. The test 45 of 't3600-rm.sh' already is prepared for this scenario and checks for exactly the scenario as Javier describes.

So, my question is, do we need to fix this to make sure that the changed '.gitmodules' is staged? I feel that we should because: since the SM becomes irrelevant after executing 'git rm --cached', it's entry in '.gitmodules' is a plain burden and is of no practical use.

The fault is in this section of 'builtin/rm.c': https://github.com/git/git/blob/v2.30.0/builtin/rm.c#L378-L402

This part:
	const char *path = list.entry[i].name;
	if (list.entry[i].is_submodule) {
		strbuf_reset(&buf);
		strbuf_addstr(&buf, path);
		if (remove_dir_recursively(&buf, 0))
			die(_("could not remove '%s'"), path);
		removed = 1;
		if (!remove_path_from_gitmodules(path))
			gitmodules_modified = 1;
		continue;
	}

Needs to be executed irrespective of whether '--cached' is passed to the command or not. In particular, the following if-statement is of utmost importance:

	if (!remove_path_from_gitmodules(path))
		gitmodules_modified = 1;

Since the variable 'gitmodules_modified' is 0 when we pass 'cached', it is not staged later here:

	if (gitmodules_modified)
		stage_updated_gitmodules(&the_index);

And its entry is not removed from the file. What should be done about this? I would appreciate your opinions.

Regards, Shourya Shukla

Junio C Hamano· Feb 7, 2021, 19:30 UTC · re: Shourya Shukla · lore

Re: [RFC] [BUDFIX] 'git rm --cached <submodule>' does not stage the changed .gitmodules

Shourya Shukla <periperidip@gmail.com> writes:
> So, my question is, do we need to fix this to make sure that the changed
> '.gitmodules' is staged?

When "--cached" is given, the user is asking the module to be removed ONLY from the index, without removing it from the working tree, no?

So I think ".gitmodules" in the working tree should not be touched at all.

Removing the entry for the module from the ".gitmodules" registered in the index, when a submodule registered in the index, might be desirable, and what you say here

> And its entry is not removed from the file. What should be done about
> this? I would appreciate your opinions.
may be related to it.

But I doubt it is a good idea to let "git rm" be the one touching ".gitmodules" either in the index or in the working tree for that to happen.

The reason I am hesitant to teach anything about ".gitmodules" to the basic level tools like "add", "rm" is because I consider, while the "gitlink" design that allows the tip-commit from a submodule in the superproject is a good thing to be done at the structural level in the core part of Git, administrative information stored in the ".gitmodules" is not part of pure "Git" and alternative designs on top of the core part of Git that uses different strategy other than what we have are possible and they could even turn out to be better than what we currently have. In other words, I have this suspicion that the ".gitmodules" based submodule handling we currently have, done using "git submodule" command, should not be the only and final form of submodule support Git would offer.

That leads me to think that anything that touch ".gitmodules" should be done with "git submodule" suite of commands, not by the low level "add", "rm", etc. Such a separation of concern would allow a new "git submodule2" design that may be radically different from the current ".gitmodules" one to be introduced, possibly even replacing, or living next to each other, the current "git submodule" together with ".gitmodules" file, without affecting the low-level "add", "rm" tools at all.

So from that point of view, if we were to fix the system, it may be preferrable to make "git rm [--options] <submodule>" only about the submodule in the working tree and/or the index, without touching ".gitmodules" at all, and let "git submodule rm [--cached] <submodule>" be the interface on top. The implementation of "git submodule rm [--cached]" may use "git rm [--cached]" internally as a building block to deal with the index and/or the working tree, but the info kept in ".gitmodules" for administrative reasons should be dealt within "git submodule" without exposing any such policy to the lower level tools like "git rm" and "git add".

Having said all that, please do not take anything I say about submodule design as the final decision. It is just an opinion by one development community member (i.e. me) and there are a lot more people who are heavily invested in the current design and interested in improving it than I am.

Thanks.
Junio C Hamano· Feb 7, 2021, 19:34 UTC · re: Junio C Hamano · lore

Re: [RFC] [BUDFIX] 'git rm --cached <submodule>' does not stage the changed .gitmodules

Junio C Hamano <gitster@pobox.com> writes:
Show 15 quoted lines
> Shourya Shukla <periperidip@gmail.com> writes:
>
>> So, my question is, do we need to fix this to make sure that the changed
>> '.gitmodules' is staged?
>
> When "--cached" is given, the user is asking the module to be
> removed ONLY from the index, without removing it from the working
> tree, no?
>
> So I think ".gitmodules" in the working tree should not be touched
> at all.
>
> Removing the entry for the module from the ".gitmodules" registered
> in the index, when a submodule registered in the index, might be
> desirable, and what you say here
typofix: "registered in the index IS REMOVED, might be" is what I meant.
Show 9 quoted lines
>
>> And its entry is not removed from the file. What should be done about
>> this? I would appreciate your opinions.
>
> may be related to it.
>
> But I doubt it is a good idea to let "git rm" be the one touching
> ".gitmodules" either in the index or in the working tree for that to
> happen.
Shourya Shukla· Feb 8, 2021, 07:23 UTC · re: Junio C Hamano · lore

Re: [RFC] [BUDFIX] 'git rm --cached <submodule>' does not stage the changed .gitmodules

On 07/02 11:30, Junio C Hamano wrote:
Show 24 quoted lines
> Shourya Shukla <periperidip@gmail.com> writes:
> 
> > So, my question is, do we need to fix this to make sure that the changed
> > '.gitmodules' is staged?
> 
> When "--cached" is given, the user is asking the module to be
> removed ONLY from the index, without removing it from the working
> tree, no?
> 
> So I think ".gitmodules" in the working tree should not be touched
> at all.
> 
> Removing the entry for the module from the ".gitmodules" registered
> in the index, when a submodule registered in the index, might be
> desirable, and what you say here
> 
> > And its entry is not removed from the file. What should be done about
> > this? I would appreciate your opinions.
> 
> may be related to it.
> 
> But I doubt it is a good idea to let "git rm" be the one touching
> ".gitmodules" either in the index or in the working tree for that to
> happen.

We can remove the entry of the SM from the '.gitmodules' at least no? Since the SM won't be relevant to us. At the end an empty '.gitmodules' file would stand.

Show 32 quoted lines
> The reason I am hesitant to teach anything about ".gitmodules" to
> the basic level tools like "add", "rm" is because I consider, while
> the "gitlink" design that allows the tip-commit from a submodule in
> the superproject is a good thing to be done at the structural level
> in the core part of Git, administrative information stored in the
> ".gitmodules" is not part of pure "Git" and alternative designs on
> top of the core part of Git that uses different strategy other than
> what we have are possible and they could even turn out to be better
> than what we currently have.  In other words, I have this suspicion
> that the ".gitmodules" based submodule handling we currently have,
> done using "git submodule" command, should not be the only and final
> form of submodule support Git would offer.
> 
> That leads me to think that anything that touch ".gitmodules" should
> be done with "git submodule" suite of commands, not by the low level
> "add", "rm", etc.  Such a separation of concern would allow a new
> "git submodule2" design that may be radically different from the
> current ".gitmodules" one to be introduced, possibly even replacing,
> or living next to each other, the current "git submodule" together
> with ".gitmodules" file, without affecting the low-level "add", "rm"
> tools at all.
> 
> So from that point of view, if we were to fix the system, it may be
> preferrable to make "git rm [--options] <submodule>" only about the
> submodule in the working tree and/or the index, without touching
> ".gitmodules" at all, and let "git submodule rm [--cached]
> <submodule>" be the interface on top.  The implementation of "git
> submodule rm [--cached]" may use "git rm [--cached]" internally as a
> building block to deal with the index and/or the working tree, but
> the info kept in ".gitmodules" for administrative reasons should be
> dealt within "git submodule" without exposing any such policy to the
> lower level tools like "git rm" and "git add".

Hmmmm.. You are correct here. But, won't we be replicating the functionality of 'git rm [--options] <submodule>' when we create another new command say 'git submodule rm [--options] <submodule>'. I might be being a bit naive here so take this with a grain of salt. For now, we could make sure that the submodule does not have a trace in the '.gitmodules' for the very least, no?

Show 5 quoted lines
> Having said all that, please do not take anything I say about
> submodule design as the final decision.  It is just an opinion by
> one development community member (i.e. me) and there are a lot more
> people who are heavily invested in the current design and interested
> in improving it than I am.

Yeah, I will tag along a couple of others who I think might help. Thank you for your opinion BTW :)

Thanks, Shourya Shukla

Junio C Hamano· Feb 8, 2021, 18:37 UTC · re: Shourya Shukla · lore

Re: [RFC] [BUDFIX] 'git rm --cached <submodule>' does not stage the changed .gitmodules

Shourya Shukla <periperidip@gmail.com> writes:
Show 29 quoted lines
> On 07/02 11:30, Junio C Hamano wrote:
>> Shourya Shukla <periperidip@gmail.com> writes:
>> 
>> > So, my question is, do we need to fix this to make sure that the changed
>> > '.gitmodules' is staged?
>> 
>> When "--cached" is given, the user is asking the module to be
>> removed ONLY from the index, without removing it from the working
>> tree, no?
>> 
>> So I think ".gitmodules" in the working tree should not be touched
>> at all.
>> 
>> Removing the entry for the module from the ".gitmodules" registered
>> in the index, when a submodule registered in the index, might be
>> desirable, and what you say here
>> 
>> > And its entry is not removed from the file. What should be done about
>> > this? I would appreciate your opinions.
>> 
>> may be related to it.
>> 
>> But I doubt it is a good idea to let "git rm" be the one touching
>> ".gitmodules" either in the index or in the working tree for that to
>> happen.
>
> We can remove the entry of the SM from the '.gitmodules' at least no?
> Since the SM won't be relevant to us. At the end an empty '.gitmodules'
> file would stand.

I agree that .gitmodules needs to be modified in the index (but not in the working tree) to make things consistent in the worldview of "git submodule" subsystem. I am just saying that I doubt "git rm" is a good place to perform an operation that is required only by the particular kind of submodule design (namely, "git submodule" that works with ".gitmodules"), as I said below.

Show 6 quoted lines
>> The reason I am hesitant to teach anything about ".gitmodules" to
>> the basic level tools like "add", "rm" is because ...
> ...
> Hmmmm.. You are correct here. But, won't we be replicating the
> functionality of 'git rm [--options] <submodule>' when we create another
> new command say 'git submodule rm [--options] <submodule>'.

Well, that is what I meant by "'git submodule rm [--cached]' may use "git rm [--cached]" internally as a building block".

When a better design of submodule subsystem appears, it might or might not use ".gitmodules", but when it wants to remove the submodule only from the index, it would do so by internally calling "git rm --cached" to implement that part of the feature, in addition to its own bookkeeping.

It won't be a replication of the functionality---dealing with the index and working tree would be done by "git rm" called by "git submodule rm".

Philippe Blain· Feb 9, 2021, 03:55 UTC · re: Junio C Hamano · lore

Re: [RFC] [BUDFIX] 'git rm --cached <submodule>' does not stage the changed .gitmodules

Hi Junio,
[CC'ing the original submitter of the issue]
Le 2021-02-07 à 14:30, Junio C Hamano a écrit :
Show 20 quoted lines
> Shourya Shukla <periperidip@gmail.com> writes:
> 
>> So, my question is, do we need to fix this to make sure that the changed
>> '.gitmodules' is staged?
> 
> When "--cached" is given, the user is asking the module to be
> removed ONLY from the index, without removing it from the working
> tree, no?
> 
> So I think ".gitmodules" in the working tree should not be touched
> at all.
> 
> Removing the entry for the module from the ".gitmodules" registered
> in the index, when a submodule registered in the index [IS REMOVED], might be
> desirable, and what you say here
> 
>> And its entry is not removed from the file. What should be done about
>> this? I would appreciate your opinions.
> 
> may be related to it.

This seems to be what the original email [1] was about, i.e. Javier seemed to expect that the changes to ".gitmodules" should be staged but the worktree version of the file untouched. This would be more in line (I think) with the current behaviour of 'git rm <submodule>', which removes the submodule worktree and relevant sections of '.gitmodules' in some cases since 95c16418f0 (rm: delete .gitmodules entry of submodules removed from the work tree, 2013-08-06).

Show 5 quoted lines
> 
> But I doubt it is a good idea to let "git rm" be the one touching
> ".gitmodules" either in the index or in the working tree for that to
> happen.
>

That's already the behaviour, at least for 'git rm <submodule>', see the commit cited above, and the whole topic that introduced it, b02f5aeda6 (Merge branch 'jl/submodule-mv', 2013-09-09) that added some knowledge of '.gitmodules' to 'git mv' and 'git rm'.

Show 32 quoted lines
> The reason I am hesitant to teach anything about ".gitmodules" to
> the basic level tools like "add", "rm" is because I consider, while
> the "gitlink" design that allows the tip-commit from a submodule in
> the superproject is a good thing to be done at the structural level
> in the core part of Git, administrative information stored in the
> ".gitmodules" is not part of pure "Git" and alternative designs on
> top of the core part of Git that uses different strategy other than
> what we have are possible and they could even turn out to be better
> than what we currently have.  In other words, I have this suspicion
> that the ".gitmodules" based submodule handling we currently have,
> done using "git submodule" command, should not be the only and final
> form of submodule support Git would offer.
> 
> That leads me to think that anything that touch ".gitmodules" should
> be done with "git submodule" suite of commands, not by the low level
> "add", "rm", etc.  Such a separation of concern would allow a new
> "git submodule2" design that may be radically different from the
> current ".gitmodules" one to be introduced, possibly even replacing,
> or living next to each other, the current "git submodule" together
> with ".gitmodules" file, without affecting the low-level "add", "rm"
> tools at all.
> 
> So from that point of view, if we were to fix the system, it may be
> preferrable to make "git rm [--options] <submodule>" only about the
> submodule in the working tree and/or the index, without touching
> ".gitmodules" at all, and let "git submodule rm [--cached]
> <submodule>" be the interface on top.  The implementation of "git
> submodule rm [--cached]" may use "git rm [--cached]" internally as a
> building block to deal with the index and/or the working tree, but
> the info kept in ".gitmodules" for administrative reasons should be
> dealt within "git submodule" without exposing any such policy to the
> lower level tools like "git rm" and "git add".

I personnally think this is not the direction I wish Git would go in. Submodules are hard, and part of the reason they are disliked so much is because they were not initially well integrated in the rest of the system, and this "belief" has stuck despite major efforts e.g. adding '--recurse-submodules' and flags and 'submodule.recurse' configs for several commands.

So I would really prefer for "core" Git commands -- as you call them, for me all of the porcelain commands are on the same level -- to be more intelligent submodule-wise, like in this case, adding functionality to 'git rm' instead of adding another 'git submdule' subcommand.

Cheers,
Philippe.
[1] https://lore.kernel.org/git/ea91c2ea29064079914f6a522db5115a@UUSALE0Z.utcmail.com/T/#u

← back to recent threads