{"thread":{"id":"55587","subject":"git am / patch utility not applying a .patch to the correct function","startedAt":"2021-04-29T14:26:01Z","lastAt":"2021-04-29T19:46:35Z","messageCount":2,"participants":["Alejandro Sanchez","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"423269","messageId":"CAF-Z3RqEz9mnnosew+TfNkV2ysaTkGj_ZXpD-4NWO7th9v_-CA@mail.gmail.com","threadId":"55587","inReplyTo":null,"subject":"git am / patch utility not applying a .patch to the correct function","fromName":"Alejandro Sanchez","fromEmail":"alex@schedmd.com","sentAt":"2021-04-29T14:25:42Z","receivedAt":"2021-04-29T14:26:01Z","isPatch":false,"sender":{"key":"alex@schedmd.com","avatar":null},"body":"Hi,\n\nIn the SchedMD/Slurm project there's a source file with two functions\nthat have a very similar implementation with some slight variations,\nnamely _eval_nodes_dfly() and _eval_nodes_topo():\n\nhttps://github.com/SchedMD/slurm/blob/slurm-20.11/src/plugins/select/cons_tres/job_test.c#L1458\n\nhttps://github.com/SchedMD/slurm/blob/slurm-20.11/src/plugins/select/cons_tres/job_test.c#L2099\n\nBefore this Slurm commit:\n\nhttps://github.com/SchedMD/slurm/commit/63e94c2ccbb62aea84cfb0b808761de2bb64e74c\n\nWhen I attempted to git am a patch clearly targeted for\n_eval_nodes_dfly(), the result shown by git show resulted in the\nmodified code in _eval_nodes_topo() instead. A colleague also reported\nthis is also happening with the 'patch' utility without git, we're not\nsure if they both share any common logic or library behind the scenes.\n\nI've attached a sequence of commands showing the contents of the patch\nand the result after applying.\n\nalex@polaris:~/t$ lsb_release -a\nNo LSB modules are available.\nDistributor ID: Debian\nDescription: Debian GNU/Linux 10 (buster)\nRelease: 10\nCodename: buster\nalex@polaris:~/t$ git --version\ngit version 2.29.2\nalex@polaris:~/t$\n\n\nalex@polaris:~/slurm/source$ cat ~/Downloads/bug11401_2011_coverity_v5.patch\nFrom cde2c60482e704cbb9f5677ea283902c26c3765f Mon Sep 17 00:00:00 2001\nFrom: Carlos Tripiana Montes <tripiana@schedmd.com>\nDate: Thu, 29 Apr 2021 14:33:18 +0200\nSubject: [PATCH] select/cons_tres with dragonfly: Fix coverity NULL dereference\nContinuation of 8475ae9d77e30585ffd2733e97dfc2b5a07c7cab\nCoverity CID 221511\nBug 11401\n---\n src/plugins/select/cons_tres/job_test.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\ndiff --git a/src/plugins/select/cons_tres/job_test.c b/src/plugins/select/cons_tres/job_test.c\nindex 2d25345945..ff125fafa3 100644\n--- a/src/plugins/select/cons_tres/job_test.c\n+++ b/src/plugins/select/cons_tres/job_test.c\n@@ -2045,7 +2045,8 @@ static int _eval_nodes_dfly(job_record_t *job_ptr,\n \trc = SLURM_ERROR;\n fini:\n-\tif (job_ptr->req_switch > 0 && rc == SLURM_SUCCESS) {\n+\tif (job_ptr->req_switch > 0 && switch_node_bitmap &&\n+\t    rc == SLURM_SUCCESS) {\n \t\tint leaf_switch_count = 0;\n \t\t/* Count up leaf switches. */\n-- \n2.25.1\nalex@polaris:~/slurm/source$ git am -3 -i ~/Downloads/bug11401_2011_coverity_v5.patch\nCommit Body is:\n--------------------------\nselect/cons_tres with dragonfly: Fix coverity NULL dereference\nContinuation of 8475ae9d77e30585ffd2733e97dfc2b5a07c7cab\nCoverity CID 221511\nBug 11401\n--------------------------\nApply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: y\nApplying: select/cons_tres with dragonfly: Fix coverity NULL dereference\nalex@polaris:~/slurm/source$ git show HEAD\ncommit f52a3b08cd895820c376577519c5413de7259bb4 (HEAD -> slurm-20.11)\nAuthor:     Carlos Tripiana Montes <tripiana@schedmd.com>\nAuthorDate: Thu Apr 29 14:33:18 2021 +0200\nCommit:     Alejandro Sanchez <alex@schedmd.com>\nCommitDate: Thu Apr 29 15:35:36 2021 +0200\n    select/cons_tres with dragonfly: Fix coverity NULL dereference\n    Continuation of 8475ae9d77e30585ffd2733e97dfc2b5a07c7cab\n    Coverity CID 221511\n    Bug 11401\ndiff --git a/src/plugins/select/cons_tres/job_test.c b/src/plugins/select/cons_tres/job_test.c\nindex acb2ae30df..bc3a81f965 100644\n--- a/src/plugins/select/cons_tres/job_test.c\n+++ b/src/plugins/select/cons_tres/job_test.c\n@@ -2626,7 +2626,8 @@ static int _eval_nodes_topo(job_record_t *job_ptr,\n        rc = SLURM_ERROR;\n fini:\n-       if (job_ptr->req_switch > 0 && rc == SLURM_SUCCESS) {\n+       if (job_ptr->req_switch > 0 && switch_node_bitmap &&\n+           rc == SLURM_SUCCESS) {\n                int leaf_switch_count = 0;\n                /* Count up leaf switches. */\nalex@polaris:~/slurm/source$\n"},{"id":"423294","messageId":"YIsM9DDAoqywKmnK@coredump.intra.peff.net","threadId":"55587","inReplyTo":"CAF-Z3RqEz9mnnosew+TfNkV2ysaTkGj_ZXpD-4NWO7th9v_-CA@mail.gmail.com","subject":"Re: git am / patch utility not applying a .patch to the correct function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-29T19:45:56Z","receivedAt":"2021-04-29T19:46:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 29, 2021 at 04:25:42PM +0200, Alejandro Sanchez wrote:\n\n> When I attempted to git am a patch clearly targeted for\n> _eval_nodes_dfly(), the result shown by git show resulted in the\n> modified code in _eval_nodes_topo() instead. A colleague also reported\n> this is also happening with the 'patch' utility without git, we're not\n> sure if they both share any common logic or library behind the scenes.\n\nThey don't share code, but this is an inherent issue with patches. If\nthe file you're applying the patch to is not at the exact same state as\nthe original (say, some other content has been introduced which moves\nthe patched lines around), then the tools will use the context lines in\nthe patch to try to find the right spot.\n\nUsually this does what you want, but it can occasionally find the wrong\nspot (e.g., if the lines in the context are repeated, and especially if\nthe \"right\" spot was changed so that the context doesn't match).\n\n> I've attached a sequence of commands showing the contents of the patch\n> and the result after applying.\n\nYour patch here is corrupted:\n\n> diff --git a/src/plugins/select/cons_tres/job_test.c b/src/plugins/select/cons_tres/job_test.c\n> index 2d25345945..ff125fafa3 100644\n> --- a/src/plugins/select/cons_tres/job_test.c\n> +++ b/src/plugins/select/cons_tres/job_test.c\n> @@ -2045,7 +2045,8 @@ static int _eval_nodes_dfly(job_record_t *job_ptr,\n>  \trc = SLURM_ERROR;\n>  fini:\n> -\tif (job_ptr->req_switch > 0 && rc == SLURM_SUCCESS) {\n> +\tif (job_ptr->req_switch > 0 && switch_node_bitmap &&\n> +\t    rc == SLURM_SUCCESS) {\n>  \t\tint leaf_switch_count = 0;\n>  \t\t/* Count up leaf switches. */\n\nIt claims 7 lines in the pre-image, but there are only 5 (context lines\n+ the \"-\" lien). And likewise, 2 lines are missing from the post-image.\n\nI'm guessing this has to do with cutting-and-pasting into the mail,\nbecause your \"git show\" output is likewise corrupted. The correct patch\nis:\n\ndiff --git a/src/plugins/select/cons_tres/job_test.c b/src/plugins/select/cons_tres/job_test.c\nindex 2d25345945..ff125fafa3 100644\n--- a/src/plugins/select/cons_tres/job_test.c\n+++ b/src/plugins/select/cons_tres/job_test.c\n@@ -2045,7 +2045,8 @@ static int _eval_nodes_dfly(job_record_t *job_ptr,\n \trc = SLURM_ERROR;\n \n fini:\n-\tif (job_ptr->req_switch > 0 && rc == SLURM_SUCCESS) {\n+\tif (job_ptr->req_switch > 0 && switch_node_bitmap &&\n+\t    rc == SLURM_SUCCESS) {\n \t\tint leaf_switch_count = 0;\n \n \t\t/* Count up leaf switches. */\n\nStarting at the state of 63e94c2ccbb62aea84cfb0b808761de2bb64e74c^,\nwhich you mentioned, the context doesn't match line 2045 (which is in\n_eval_nodes_dfly):\n\n  $ sed -n '2045,+7p' src/plugins/select/cons_tres/job_test.c\n  \trc = SLURM_ERROR;\n  \n  fini:\n  \tif (job_ptr->req_switch > 0 && rc == SLURM_SUCCESS) {\n  \t\t/* req_switch == 1 here; enforced at the top of the function. */\n  \t\tleaf_switch_count = 0;\n  \n  \t\t/* count up leaf switches */\n\nThere's an extra comment at the top of the block, and the case differs\nin the lower comment. But the context _does_ match the later block of\ncode in _eval_nodes_topo():\n\n  $ sed -ne '2626,+7p' src/plugins/select/cons_tres/job_test.c\n  \trc = SLURM_ERROR;\n  \n  fini:\n  \tif (job_ptr->req_switch > 0 && switch_node_bitmap &&\n  \t    rc == SLURM_SUCCESS) {\n  \t\tint leaf_switch_count = 0;\n  \n  \t\t/* Count up leaf switches. */\n\nAnd hence that's where both git and patch will apply it.\n\nOne other confusion may be that the hunk header says\n\"_eval_nodes_dfly()\" in it. This is purely informational for a human\nreader, and not taken into account by the patch application code (which\ndoes not even understand things like functions in the first place).\n\n-Peff\n"}]}