list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Johannes Schindelin <>
To: Shourya Shukla <>
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: <> (raw)
In-Reply-To: <>

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 */


	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.

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


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

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

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

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


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

  List information:

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

  git send-email \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror
	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/ \
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
 note: .onion URLs require Tor:

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

AGPL code for this site: git clone