git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Shourya Shukla <periperidip@gmail.com>
To: Philippe Blain <levraiphilippeblain@gmail.com>
Cc: gitster@pobox.com, christian.couder@gmail.com,
	levraiphilippeblain@gmail.com, git@vger.kernel.org
Subject: Re: [PATCH 1/2] rm: changes in the '.gitmodules' are staged after using '--cached'
Date: Fri, 19 Feb 2021 20:49:13 +0530	[thread overview]
Message-ID: <20210219151913.GA6254@konoha> (raw)
In-Reply-To: <0577f84b-f594-6b8a-76a2-29fb9453ee25@gmail.com>

On 18/02 03:14, Philippe Blain wrote:
> 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:

I have no idea how I missed that one. Apologies, will make the change.

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

Okay. I will use the above message.

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

Correct.

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

Understood. I have to let the working tree '.gitmodules' be left as-is
and only change the copy in the index.


  parent reply	other threads:[~2021-02-19 15:20 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
2021-02-18 20:39     ` Philippe Blain
2021-02-19 15:19     ` Shourya Shukla [this message]
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

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

  git send-email \
    --in-reply-to=20210219151913.GA6254@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).