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 >
