Re: [PATCH 1/1][RFC][GSoC] submodule: using 'is_writing_gitmodules_ok()' for a stricter check

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Feb 14, 2020, 13:28 UTC
Message-ID: <nycvar.QRO.7.76.6.2002141428020.46@tvgsbejvaqbjf.bet>
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_.

Show 5 quoted lines

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

Show 7 quoted lines

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

Show 5 quoted lines

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