git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Shourya Shukla <periperidip@gmail.com>
To: git@vger.kernel.org
Cc: gitster@pobox.com, christian.couder@gmail.com,
	levraiphilippeblain@gmail.com
Subject: [RFC] [BUDFIX] 'git rm --cached <submodule>' does not stage the changed .gitmodules
Date: Sun, 7 Feb 2021 20:11:44 +0530	[thread overview]
Message-ID: <20210207144144.GA42182@konoha> (raw)

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


             reply	other threads:[~2021-02-07 14:44 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-07 14:41 Shourya Shukla [this message]
2021-02-07 19:30 ` [RFC] [BUDFIX] 'git rm --cached <submodule>' does not stage the changed .gitmodules Junio C Hamano
2021-02-07 19:34   ` Junio C Hamano
2021-02-08  7:23   ` Shourya Shukla
2021-02-08 18:37     ` Junio C Hamano
2021-02-09  3:55   ` Philippe Blain

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210207144144.GA42182@konoha \
    --to=periperidip@gmail.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=levraiphilippeblain@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).