git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Shourya Shukla <periperidip@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, christian.couder@gmail.com,
	levraiphilippeblain@gmail.com
Subject: Re: [PATCH 1/2] rm: changes in the '.gitmodules' are staged after using '--cached'
Date: Fri, 19 Feb 2021 20:54:36 +0530	[thread overview]
Message-ID: <20210219152436.GB6254@konoha> (raw)
In-Reply-To: <xmqqblchdoej.fsf@gitster.g>

On 18/02 02:03, Junio C Hamano wrote:
> Shourya Shukla <periperidip@gmail.com> writes:
> 
> > +		if (list.entry[i].is_submodule) {
> > +			/*
> > +			 * Then, unless we used "--cached", remove the filenames from
> > +			 * the workspace. If we fail to remove the first one, we
> > +			 * abort the "git rm" (but once we've successfully removed
> > +			 * any file at all, we'll go ahead and commit to it all:
> > +			 * by then we've already committed ourselves and can't fail
> > +			 * in the middle)
> > +			 */
> > +			if (!index_only) {
> > +				struct strbuf buf = STRBUF_INIT;
> >  				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;
> > +				strbuf_release(&buf);
> 
> Since we won't come to this block when doing index_only, we are
> allowed to touch the working tree contents and files.  We indeed do
> "rm -rf" of the submodule working tree and touch .gitmodules file
> that is in the working tree.
> 
> >  			}
> > +			if (!remove_path_from_gitmodules(path))
> > +				gitmodules_modified = 1;
> > +			continue;
> 
> But this looks wrong.  It might be OK to remove from the .gitmodules
> stored in the index, but I fail to see why it is justified to touch
> the working tree file when "--cached" is given.

No no, you are correct. Phillipe pointed out the same thing. I don't
know how I made this mistake.

> > +		}
> > +		if (!index_only) {
> >  			if (!remove_path(path)) {
> >  				removed = 1;
> >  				continue;
> > @@ -396,11 +398,15 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
> >  			if (!removed)
> >  				die_errno("git rm: '%s'", path);
> >  		}
> > -		strbuf_release(&buf);
> > -		if (gitmodules_modified)
> > -			stage_updated_gitmodules(&the_index);
> 
> Assuming that it is somehow justifiable that removing the entry from
> the .gitmodules in the index (again, I do not think it is
> justifiable to remove from the working tree file), we no longer can
> use stage_updated_gitmodules() helper as-is.
> 
> I think you'd need to
> 
>  - Add a variant of remove_path_from_gitmodules() that can remove
>    the given submodule from the .gitmodules in the index entry
>    without touching the working tree.  The change could be to update
>    the function to take an extra "index_only" parameter and a
>    pointer to an index_state instance, and
> 
>    (1) if !index_only, then edit the .gitmodules file in the working
>        tree to remove the entry for path;
> 
>    (2) in both !index_only and index_only cases, read .gitmodules
>        file from the index, edit to remove the entry for path, and
>        add the result to the index.
> 
>    and return 0 for success (e.g. if path is not a submoudle or no
>    entry for it is found in .gitmodules, it may return -1).
> 
>  - Since the previous point will maintain the correct contents in
>    the index in all cases, get rid of gitmodules_modified and calls
>    to stage_updated_gitmodules().  The call to write_locked_index()
>    at the end will take care of the actual writing out of the index.
> 
> if we want to teach "rm --cached" to update only the index, and "rm"
> to update both the index and the working tree, of ".gitmodules".

Yeah, this approach seems perfect. I will do it this way.

> Having said that, I still do not think it is a good direction to go
> to teach low level "rm", "mv" etc. to know about ".gitmodules" (yes,
> yes, I know that some changes that I consider to be mistakes have
> already happened---that does not mean we cannot correct our course
> and it does not mean it is OK to make things even worse).  Such a
> "how does a submodule get managed" policy decision belongs to the
> "git submodule" subcommand, I would think.


Let's do it this way. I will deliver a v2 of this patch, if we get
comments from anyone stating that this should not go forward, then we
will drop this patch or do what is suggested. Else, queue this patch for
now (given that this does not break anything, obviously) and maybe put
up a RFC for the method you suggested. I am saying this because we have
not received any conclusive evidence of whether this patch should carry
on or not (not trying to disregard your take).

What do you say?


  reply	other threads:[~2021-02-19 15:25 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
2021-02-18 22:03   ` Junio C Hamano
2021-02-19 15:24     ` Shourya Shukla [this message]
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=20210219152436.GB6254@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.
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).