{"thread":{"id":"52781","subject":"[PATCH 0/1] [RFC][GSoC] submodule: enforcing stricter checks","startedAt":"2020-02-11T17:04:12Z","lastAt":"2020-02-14T13:28:52Z","messageCount":5,"participants":["Shourya Shukla","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"391541","messageId":"20200211170359.31835-1-shouryashukla.oo@gmail.com","threadId":"52781","inReplyTo":null,"subject":"[PATCH 0/1] [RFC][GSoC] submodule: enforcing stricter checks","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-02-11T17:03:58Z","receivedAt":"2020-02-11T17:04:12Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Greetings everyone!\n\nI have tried to improve the checks in some functions of 'submodule.c'\nand attempted to make them stricter. Initially, not all conditions were\nsatisfied by the functions 'update_path_in_gitmodules()' and\n'remove_path_from_gitmodules()' while changing(updating/removing paths)\nthe '.gitmodules' file.\n\nNow, on implementing the 'is_writing_gitmodules_ok()' function in one of the\nif cases of the functions, all the conditions are checked before returning a\nvalue unlike before.\n\nThanks,\nShourya Shukla\n\nShourya Shukla (1):\n  submodule: using 'is_writing_gitmodules_ok()' for a stricter check\n\n submodule.c | 16 ++++++++++++++--\n 1 file changed, 14 insertions(+), 2 deletions(-)\n\n-- \n2.20.1\n"},{"id":"391542","messageId":"20200211170359.31835-2-shouryashukla.oo@gmail.com","threadId":"52781","inReplyTo":"20200211170359.31835-1-shouryashukla.oo@gmail.com","subject":"[PATCH 1/1][RFC][GSoC] submodule: using 'is_writing_gitmodules_ok()' for a stricter check","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-02-11T17:03:59Z","receivedAt":"2020-02-11T17:04:17Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"The if conditions of the functions 'update_path_in_gitmodules()'\nand 'remove_path_from_gitmodules()' are not catering to every\ncondition encountered by the function. On detailed observation,\none can notice that .gitmodules cannot be changed (i.e. removal\nof a path or updation of a path) until these conditions are satisfied:\n\n    1. The file exists\n    2. The file, if it does not exist, should be absent from\n       the index and other branches as well.\n    3. There should not be any unmerged changes in the file.\n    4. The submodules do not exist or if the submodule name\n       does not match.\n\nOnly the conditions 1, 3 and 4 were being satisfied earlier. Now\non changing the if statement in one of the places, the condition\n2 is satisfied as well.\n\nSigned-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n---\n submodule.c | 16 ++++++++++++++--\n 1 file changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 3a184b66ab..f7836a6851 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -107,7 +107,13 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)\n \tconst struct submodule *submodule;\n \tint ret;\n \n-\tif (!file_exists(GITMODULES_FILE)) /* Do nothing without .gitmodules */\n+\t/* If .gitmodules file is not safe to write(update a path) i.e.\n+\t * if it does not exist or if it is not present in the working tree\n+\t * but lies in the index or in the current branch.\n+\t * The function 'is_writing_gitmodules_ok()' checks for the same.\n+\t * and exits with failure if above conditions are not satisfied\n+\t*/\n+\tif (is_writing_gitmodules_ok())\n \t\treturn -1;\n \n \tif (is_gitmodules_unmerged(the_repository->index))\n@@ -136,7 +142,13 @@ int remove_path_from_gitmodules(const char *path)\n \tstruct strbuf sect = STRBUF_INIT;\n \tconst struct submodule *submodule;\n \n-\tif (!file_exists(GITMODULES_FILE)) /* Do nothing without .gitmodules */\n+\t/* If .gitmodules file is not safe to write(remove a path) i.e.\n+\t * if it does not exist or if it is not present in the working tree\n+\t * but lies in the index or in the current branch.\n+\t * The function 'is_writing_gitmodules_ok()' checks for the same.\n+\t * and exits with failure if above conditions are not satisfied\n+\t*/\n+\tif (is_writing_gitmodules_ok())\n \t\treturn -1;\n \n \tif (is_gitmodules_unmerged(the_repository->index))\n-- \n2.20.1\n\n"},{"id":"391661","messageId":"nycvar.QRO.7.76.6.2002131435301.46@tvgsbejvaqbjf.bet","threadId":"52781","inReplyTo":"20200211170359.31835-2-shouryashukla.oo@gmail.com","subject":"Re: [PATCH 1/1][RFC][GSoC] submodule: using 'is_writing_gitmodules_ok()' for a stricter check","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-02-13T13:42:40Z","receivedAt":"2020-02-13T13:42:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Shourya,\n\nOn Tue, 11 Feb 2020, Shourya Shukla wrote:\n\n> The if conditions of the functions 'update_path_in_gitmodules()'\n> and 'remove_path_from_gitmodules()' are not catering to every\n> condition encountered by the function. On detailed observation,\n> one can notice that .gitmodules cannot be changed (i.e. removal\n> of a path or updation of a path) until these conditions are satisfied:\n>\n>     1. The file exists\n>     2. The file, if it does not exist, should be absent from\n>        the index and other branches as well.\n\nI don't think that other branches matter in this context.\n\n>     3. There should not be any unmerged changes in the file.\n>     4. The submodules do not exist or if the submodule name\n>        does not match.\n>\n> Only the conditions 1, 3 and 4 were being satisfied earlier. Now\n> on changing the if statement in one of the places, the condition\n> 2 is satisfied as well.\n\nLet's see how this is done...\n\n> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n> ---\n>  submodule.c | 16 ++++++++++++++--\n>  1 file changed, 14 insertions(+), 2 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index 3a184b66ab..f7836a6851 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -107,7 +107,13 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)\n>  \tconst struct submodule *submodule;\n>  \tint ret;\n>\n> -\tif (!file_exists(GITMODULES_FILE)) /* Do nothing without .gitmodules */\n> +\t/* If .gitmodules file is not safe to write(update a path) i.e.\n> +\t * if it does not exist or if it is not present in the working tree\n> +\t * but lies in the index or in the current branch.\n> +\t * The function 'is_writing_gitmodules_ok()' checks for the same.\n> +\t * and exits with failure if above conditions are not satisfied\n> +\t*/\n\nStyle: we always begin and end multi-line comments with `/*` and `*/` on\ntheir own line.\n\n> +\tif (is_writing_gitmodules_ok())\n\nHmm. This function is defined thusly:\n\nint is_writing_gitmodules_ok(void)\n{\n\tstruct object_id oid;\n\treturn file_exists(GITMODULES_FILE) ||\n\t\t(get_oid(GITMODULES_INDEX, &oid) < 0 && get_oid(GITMODULES_HEAD, &oid) < 0);\n}\n\nAha! So this tries to ensure that the `.gitmodules` file exists on disk,\nor if it does not, then it should not exist in the index nor in the\n_current_ branch.\n\nBut we're in the function called `update_path_in_gitmodules()` which\nsuggests that we're working on an existing, valid `.gitmodules`.\n\nSo I do not think that we can proceed if `.gitmodules` is absent from\ndisk, even if in case that it is _also_ absent from the index and from the\ncurrent branch.\n\n>  \t\treturn -1;\n>\n>  \tif (is_gitmodules_unmerged(the_repository->index))\n> @@ -136,7 +142,13 @@ int remove_path_from_gitmodules(const char *path)\n>  \tstruct strbuf sect = STRBUF_INIT;\n>  \tconst struct submodule *submodule;\n>\n> -\tif (!file_exists(GITMODULES_FILE)) /* Do nothing without .gitmodules */\n> +\t/* If .gitmodules file is not safe to write(remove a path) i.e.\n> +\t * if it does not exist or if it is not present in the working tree\n> +\t * but lies in the index or in the current branch.\n> +\t * The function 'is_writing_gitmodules_ok()' checks for the same.\n> +\t * and exits with failure if above conditions are not satisfied\n> +\t*/\n> +\tif (is_writing_gitmodules_ok())\n\nHere, we want to remove a path from `.gitmodules`, so I think that the\nsame analysis applies as above.\n\nIn other words, I think that the existing code is correct and does not\nneed to be patched.\n\nCiao,\nJohannes\n\n>  \t\treturn -1;\n>\n>  \tif (is_gitmodules_unmerged(the_repository->index))\n> --\n> 2.20.1\n>\n>\n"},{"id":"391670","messageId":"20200213163819.6495-1-shouryashukla.oo@gmail.com","threadId":"52781","inReplyTo":"nycvar.QRO.7.76.6.2002131435301.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/1][RFC][GSoC] submodule: using 'is_writing_gitmodules_ok()' for a stricter check","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-02-13T16:38:19Z","receivedAt":"2020-02-13T16:38:27Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Hello Johannes,\n\nI understand your point of view here. What I am trying to say is that we must update\nour .gitmodules if atleast the function 'is_writing_gitmodules_ok()' passes.\n\nBefore, we used to pass the if condition if our .gitmomdules existed and it did not matter\nif there were any traces of it in the index.\n\n> But we're in the function called `update_path_in_gitmodules()` which\n> suggests that we're working on an existing, valid `.gitmodules`.\n\nBut we still originally(before my patch) checked for the existence of .gitmodules right?\nThe functions exits with error in case of absence of the file(which should happen).\n\n> So I do not think that we can proceed if `.gitmodules` is absent from\n> disk, even if in case that it is _also_ absent from the index and from the\n> current branch.\n\nYes that is one case, but the other case is that _if_ the file exists, it **should** not\nexist in the index or our current branch(which must be necessary to ensure before making\nany updates to the file right?). This is the case which was not covered before but I have\ntried to cover it in my patch.\n\nIs this explanation correct?\n\nRegards,\nShourya Shukla\n"},{"id":"391749","messageId":"nycvar.QRO.7.76.6.2002141428020.46@tvgsbejvaqbjf.bet","threadId":"52781","inReplyTo":"20200213163819.6495-1-shouryashukla.oo@gmail.com","subject":"Re: [PATCH 1/1][RFC][GSoC] submodule: using 'is_writing_gitmodules_ok()' for a stricter check","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-02-14T13:28:38Z","receivedAt":"2020-02-14T13:28:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Shourya,\n\nOn Thu, 13 Feb 2020, Shourya Shukla wrote:\n\n> I understand your point of view here. What I am trying to say is that we\n> must update our .gitmodules if atleast the function\n> 'is_writing_gitmodules_ok()' passes.\n\nWell, you know, I totally overlooked something: your patch replaces\n\n\tif (!file_exists(GITMODULES_FILE)) /* Do nothing without .gitmodules */\n\nby\n\n\tif (is_writing_gitmodules_ok())\n\nwhich is incorrect: it should _at least_ replace it with\n\n\tif (!is_writing_gitmodules_ok())\n\nNote the `!`. The reason is that this statement guards an early exit from\nthe function, indicating an error. In particular, the code before said: if\nthis file does not exist, error out.\n\nThe new code (with the `!`) would say: if the file does not exist, _or if\n`is_writing_gitmodules_ok()` fails, error out.\n\nBut that function does not do what we want: if we rewrite the code in the\nway you suggested, then we would _no longer_ error out if the file is\nmissing if it at least is in the index or in `HEAD`.\n\nBut if the file is missing, we cannot edit it, which is what both the\n\"update\" and the \"remove\" code path want to do.\n\n> Before, we used to pass the if condition if our .gitmomdules existed and\n> it did not matter if there were any traces of it in the index.\n\nExactly. If there is no `.gitmodules` file on disk, we cannot edit it.\nPeriod.\n\nIt does not matter whether there is a copy in the index or in `HEAD`: the\n`git mv` and `git rm` commands want to work _on the worktree_ by default.\n\nSide note: From a cursory read of the callers in `builtin/rm.c`, I suspect\nthat `git rm --cached` actually does not handle the `.gitmodules` file\ncorrectly: it would not edit it in that case, but we would want it to be\nedited _in the index_.\n\n> > But we're in the function called `update_path_in_gitmodules()` which\n> > suggests that we're working on an existing, valid `.gitmodules`.\n>\n> But we still originally(before my patch) checked for the existence of\n> .gitmodules right?\n\nWe checked for the _non_-existence.\n\n> The functions exits with error in case of absence of the file(which\n> should happen).\n\nYes.\n\nAnd your patch changes this so that the file _can_ be absent, _as long_ as\nit exists either in the index or in the tip commit of the current branch.\n\nBut the code then goes on to call `config_set_in_gitmodules_file_gently()`\nor `git_config_rename_section_in_file()`, respectively. Both of these\nfunctions _expect_ the file to exist.\n\nTherefore, the condition that your patch now allows would lead to\nincorrect behavior. A test case would have caught this, which is actually\na good reminder that patches that change behavior should always be\naccompanied by changes/additions to the test suite to document the\nexpected behavior.\n\n> > So I do not think that we can proceed if `.gitmodules` is absent from\n> > disk, even if in case that it is _also_ absent from the index and from\n> > the current branch.\n>\n> Yes that is one case, but the other case is that _if_ the file exists,\n> it **should** not exist in the index or our current branch(which must be\n> necessary to ensure before making any updates to the file right?).\n\nAssuming that you are talking about the conditions that have to be met\n_not_ to error out early from those functions, I disagree: both of these\nfunctions operate on the `.gitmodules` _file_. They require that file. It\nmust exist. Otherwise we must error out early. Which the existing code\nalready does.\n\n> This is the case which was not covered before but I have tried to cover\n> it in my patch.\n\nIf you truly want to cover the case where we want to edit the\n`.gitmodules` file even if it does not exist on disk, but only in the\nindex and/or the current branch, then those functions need _quite_ a bit\nmore work to actually pull the contents from the index, and/or from the\ntip commit, _and_ to put the modified contents into the index.\n\nHowever, I am not at all sure that that is a wise thing to do (except in\nthe case that we're talking about `git rm`'s `--cached` option,\nin which case you would _definitely_ need quite a bit more modifications\ne.g. to extend the signature of at least `remove_path_from_gitmodules()`\nto indicate the desire _not_ to work on the worktree file but on the index\ninstead, and that mode should not even allow `.gitmodules` to be absent\nfrom worktree and index but only exist in the tip commit of the current\nbranch).\n\nSo I am afraid that the patch is incorrect as-is. It would require a\nclearer idea of what its goal is, which would have to be reflected in the\ncommit message, and it would have to be accompanied by a regression test\ncase.\n\nAs things stand, I don't think that this patch is going in the right\ndirection.\n\nIf, on the other hand, the direction is changed to try to support the\n`--cached` option, I would agree that that would be going toward the right\ndirection.\n\nCiao,\nJohannes\n\n> Is this explanation correct?\n>\n> Regards,\n> Shourya Shukla\n>\n"}]}