git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Shourya Shukla <shouryashukla.oo@gmail.com>
Cc: chriscool@tuxfamily.org, git@vger.kernel.org, gitster@pobox.com,
	peff@peff.net
Subject: Re: [PATCH 1/1][RFC][GSoC] submodule: using 'is_writing_gitmodules_ok()' for a stricter check
Date: Fri, 14 Feb 2020 14:28:38 +0100 (CET)
Message-ID: <nycvar.QRO.7.76.6.2002141428020.46@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20200213163819.6495-1-shouryashukla.oo@gmail.com>

Hi Shourya,

On Thu, 13 Feb 2020, Shourya Shukla wrote:

> I understand your point of view here. What I am trying to say is that we
> must update our .gitmodules if atleast the function
> 'is_writing_gitmodules_ok()' passes.

Well, you know, I totally overlooked something: your patch replaces

	if (!file_exists(GITMODULES_FILE)) /* Do nothing without .gitmodules */

by

	if (is_writing_gitmodules_ok())

which is incorrect: it should _at least_ replace it with

	if (!is_writing_gitmodules_ok())

Note the `!`. The reason is that this statement guards an early exit from
the function, indicating an error. In particular, the code before said: if
this file does not exist, error out.

The new code (with the `!`) would say: if the file does not exist, _or if
`is_writing_gitmodules_ok()` fails, error out.

But that function does not do what we want: if we rewrite the code in the
way you suggested, then we would _no longer_ error out if the file is
missing if it at least is in the index or in `HEAD`.

But if the file is missing, we cannot edit it, which is what both the
"update" and the "remove" code path want to do.

> Before, we used to pass the if condition if our .gitmomdules existed and
> it did not matter if there were any traces of it in the index.

Exactly. If there is no `.gitmodules` file on disk, we cannot edit it.
Period.

It does not matter whether there is a copy in the index or in `HEAD`: the
`git mv` and `git rm` commands want to work _on the worktree_ by default.

Side note: From a cursory read of the callers in `builtin/rm.c`, I suspect
that `git rm --cached` actually does not handle the `.gitmodules` file
correctly: it would not edit it in that case, but we would want it to be
edited _in the index_.

> > But we're in the function called `update_path_in_gitmodules()` which
> > suggests that we're working on an existing, valid `.gitmodules`.
>
> But we still originally(before my patch) checked for the existence of
> .gitmodules right?

We checked for the _non_-existence.

> The functions exits with error in case of absence of the file(which
> should happen).

Yes.

And your patch changes this so that the file _can_ be absent, _as long_ as
it exists either in the index or in the tip commit of the current branch.

But the code then goes on to call `config_set_in_gitmodules_file_gently()`
or `git_config_rename_section_in_file()`, respectively. Both of these
functions _expect_ the file to exist.

Therefore, the condition that your patch now allows would lead to
incorrect behavior. A test case would have caught this, which is actually
a good reminder that patches that change behavior should always be
accompanied by changes/additions to the test suite to document the
expected behavior.

> > So I do not think that we can proceed if `.gitmodules` is absent from
> > disk, even if in case that it is _also_ absent from the index and from
> > the current branch.
>
> Yes that is one case, but the other case is that _if_ the file exists,
> it **should** not exist in the index or our current branch(which must be
> necessary to ensure before making any updates to the file right?).

Assuming that you are talking about the conditions that have to be met
_not_ to error out early from those functions, I disagree: both of these
functions operate on the `.gitmodules` _file_. They require that file. It
must exist. Otherwise we must error out early. Which the existing code
already does.

> This is the case which was not covered before but I have tried to cover
> it in my patch.

If you truly want to cover the case where we want to edit the
`.gitmodules` file even if it does not exist on disk, but only in the
index and/or the current branch, then those functions need _quite_ a bit
more work to actually pull the contents from the index, and/or from the
tip commit, _and_ to put the modified contents into the index.

However, I am not at all sure that that is a wise thing to do (except in
the case that we're talking about `git rm`'s `--cached` option,
in which case you would _definitely_ need quite a bit more modifications
e.g. to extend the signature of at least `remove_path_from_gitmodules()`
to indicate the desire _not_ to work on the worktree file but on the index
instead, and that mode should not even allow `.gitmodules` to be absent
from worktree and index but only exist in the tip commit of the current
branch).

So I am afraid that the patch is incorrect as-is. It would require a
clearer idea of what its goal is, which would have to be reflected in the
commit message, and it would have to be accompanied by a regression test
case.

As things stand, I don't think that this patch is going in the right
direction.

If, on the other hand, the direction is changed to try to support the
`--cached` option, I would agree that that would be going toward the right
direction.

Ciao,
Johannes

> Is this explanation correct?
>
> Regards,
> Shourya Shukla
>

      reply	other threads:[~2020-02-14 13:28 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-11 17:03 [PATCH 0/1] [RFC][GSoC] submodule: enforcing stricter checks Shourya Shukla
2020-02-11 17:03 ` [PATCH 1/1][RFC][GSoC] submodule: using 'is_writing_gitmodules_ok()' for a stricter check Shourya Shukla
2020-02-13 13:42   ` Johannes Schindelin
2020-02-13 16:38     ` Shourya Shukla
2020-02-14 13:28       ` Johannes Schindelin [this message]

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=nycvar.QRO.7.76.6.2002141428020.46@tvgsbejvaqbjf.bet \
    --to=johannes.schindelin@gmx.de \
    --cc=chriscool@tuxfamily.org \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    --cc=shouryashukla.oo@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

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

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

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git