git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Philippe Blain <levraiphilippeblain@gmail.com>
To: Shourya Shukla <periperidip@gmail.com>, git@vger.kernel.org
Cc: gitster@pobox.com, christian.couder@gmail.com,
	Javier Mora <javier.moradesambricio@rtx.com>
Subject: Re: [PATCH 1/2] rm: changes in the '.gitmodules' are staged after using '--cached'
Date: Thu, 18 Feb 2021 15:14:43 -0500	[thread overview]
Message-ID: <0577f84b-f594-6b8a-76a2-29fb9453ee25@gmail.com> (raw)
In-Reply-To: <20210218184931.83613-2-periperidip@gmail.com>

Hello Shourya,

Le 2021-02-18 à 13:49, Shourya Shukla a écrit :
> Earlier, on doing a 'git rm --cached <submodule>' did not modify the
> '.gitmodules' entry of the submodule in question hence the file was not
> staged. Change this behaviour to remove the entry of the submodule from
> the '.gitmodules', something which might be more expected of the
> command.

We prefer using the imperative mood for the commit message title,
the present tense for describing the actual state of the code,
and finally the imperative mood again to give order to the code base
to change its behaviour [1]. So something like the following would fit more
into the project's conventions:


     rm: stage submodule removal from '.gitmodules' when using '--cached'

     Currently, using 'git rm --cached <submodule>' removes submodule <submodule> from the index
     and leaves the submodule working tree intact in the superproject working tree,
     but does not stage any changes to the '.gitmodules' file, in contrast to
     'git rm <submodule>', which removes both the submodule and its configuration
     in '.gitmodules' from the worktree and index.
     
     Fix this inconsistency by also staging the removal of the configuration of the
     submodule from the '.gitmodules' file, leaving the worktree copy intact, a behaviour
     which is more in line with what might be expected when using '--cached'.


However, this is *not* what you patch does; it also removes the relevant
section from the '.gitmodules' file *in the worktree*, which is not acceptable
because it is exactly contrary to what '--cached' means.

This was verified by running Javier's demonstration script that I included in the
Gitgitgadget issue [2], which I copy here:


~~~
rm -rf some_submodule top_repo

mkdir some_submodule
cd some_submodule
git init
echo hello > hello.txt
git add hello.txt
git commit -m 'First commit of submodule'
cd ..
mkdir top_repo
cd top_repo
git init
echo world > world.txt
git add world.txt
git commit -m 'First commit of top repo'
git submodule add ../some_submodule
git status  # both some_submodule and .gitmodules staged
git commit -m 'Added submodule'
git rm --cached some_submodule
git status  # only some_submodule staged
~~~

With your changes, at the end '.gitmodules' is modified in both the
worktree and the index, whereas we would want it to be modified
*only* in the index.

And we would want it to be staged for deletion (and only deleting the config
entry and keeping an empty ".gitmodules' file in the index)
if the user is removing the only submodule in the superproject.


> ---
>   builtin/rm.c | 48 +++++++++++++++++++++++++++---------------------
>   1 file changed, 27 insertions(+), 21 deletions(-)
> 

Once implemeted correctly (leaving the worktree version of '.gitmodules'
intact), that patch should also change the documentation to stay up-to-date,
since the "Submodules" section of Documentation/git-rm.txt states [3]:

     If it exists the submodule.<name> section in the gitmodules[5] file will
     also be removed and that file will be staged (unless --cached or -n are used).


Cheers,
Philippe.

[1] https://git-scm.com/docs/SubmittingPatches#describe-changes
[2] https://github.com/gitgitgadget/git/issues/750
[3] https://git-scm.com/docs/git-rm#_submodules

  reply	other threads:[~2021-02-18 20:17 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-18 18:49 [RFC][PATCH 0/2] rm: changes in the '.gitmodules' are staged after using '--cached' Shourya Shukla
2021-02-18 18:49 ` [PATCH 1/2] " Shourya Shukla
2021-02-18 20:14   ` Philippe Blain [this message]
2021-02-18 20:39     ` Philippe Blain
2021-02-19 15:19     ` Shourya Shukla
2021-02-18 22:03   ` Junio C Hamano
2021-02-19 15:24     ` Shourya Shukla
2021-02-20  3:31       ` Junio C Hamano
2021-02-18 18:49 ` [PATCH 2/2] t3600: amend test 46 to check for '.gitmodules' modification Shourya Shukla
2021-02-18 20:21   ` Philippe Blain
2021-02-22 17:26 ` [PATCH v2 0/1] rm: stage submodule removal from '.gitmodules' Shourya Shukla
2021-02-22 17:26   ` [PATCH v2 1/1] rm: stage submodule removal from '.gitmodules' when using '--cached' Shourya Shukla
2021-02-22 18:58     ` Junio C Hamano
2021-03-05 17:58       ` Shourya Shukla
2021-03-05 21:39         ` Junio C Hamano
2021-02-22 19:29     ` Junio C Hamano
2021-03-07 16:46       ` Shourya Shukla
2021-03-07 20:29         ` Junio C Hamano
2021-03-09  7:13           ` Shourya Shukla
2021-03-09 20:47             ` Junio C Hamano

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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=0577f84b-f594-6b8a-76a2-29fb9453ee25@gmail.com \
    --to=levraiphilippeblain@gmail.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=javier.moradesambricio@rtx.com \
    --cc=periperidip@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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).