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) [thread overview]
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
>
prev parent 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
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).