git am / patch utility not applying a .patch to the correct function

git am / patch utility not applying a .patch to the correct function

[Alejandro Sanchez]

[-- Attachment #1: Type: text/plain, Size: 1162 bytes --]

Hi,

In the SchedMD/Slurm project there's a source file with two functions
that have a very similar implementation with some slight variations,
namely _eval_nodes_dfly() and _eval_nodes_topo():

https://github.com/SchedMD/slurm/blob/slurm-20.11/src/plugins/select/cons_tres/job_test.c#L1458

https://github.com/SchedMD/slurm/blob/slurm-20.11/src/plugins/select/cons_tres/job_test.c#L2099

Before this Slurm commit:

https://github.com/SchedMD/slurm/commit/63e94c2ccbb62aea84cfb0b808761de2bb64e74c

When I attempted to git am a patch clearly targeted for
_eval_nodes_dfly(), the result shown by git show resulted in the
modified code in _eval_nodes_topo() instead. A colleague also reported
this is also happening with the 'patch' utility without git, we're not
sure if they both share any common logic or library behind the scenes.

I've attached a sequence of commands showing the contents of the patch
and the result after applying.

alex@polaris:~/t$ lsb_release -a
No LSB modules are available.
Distributor ID: Debian
Description: Debian GNU/Linux 10 (buster)
Release: 10
Codename: buster
alex@polaris:~/t$ git --version
git version 2.29.2
alex@polaris:~/t$

[-- Attachment #2: git_problem_sequence.txt --]
[-- Type: text/plain, Size: 2553 bytes --]

alex@polaris:~/slurm/source$ cat ~/Downloads/bug11401_2011_coverity_v5.patch
From cde2c60482e704cbb9f5677ea283902c26c3765f Mon Sep 17 00:00:00 2001
From: Carlos Tripiana Montes <tripiana@schedmd.com>
Date: Thu, 29 Apr 2021 14:33:18 +0200
Subject: [PATCH] select/cons_tres with dragonfly: Fix coverity NULL dereference
Continuation of 8475ae9d77e30585ffd2733e97dfc2b5a07c7cab
Coverity CID 221511
Bug 11401
---
 src/plugins/select/cons_tres/job_test.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/plugins/select/cons_tres/job_test.c b/src/plugins/select/cons_tres/job_test.c
index 2d25345945..ff125fafa3 100644
--- a/src/plugins/select/cons_tres/job_test.c
+++ b/src/plugins/select/cons_tres/job_test.c
@@ -2045,7 +2045,8 @@ static int _eval_nodes_dfly(job_record_t *job_ptr,
 	rc = SLURM_ERROR;
 fini:
-	if (job_ptr->req_switch > 0 && rc == SLURM_SUCCESS) {
+	if (job_ptr->req_switch > 0 && switch_node_bitmap &&
+	    rc == SLURM_SUCCESS) {
 		int leaf_switch_count = 0;
 		/* Count up leaf switches. */
-- 
2.25.1
alex@polaris:~/slurm/source$ git am -3 -i ~/Downloads/bug11401_2011_coverity_v5.patch
Commit Body is:
--------------------------
select/cons_tres with dragonfly: Fix coverity NULL dereference
Continuation of 8475ae9d77e30585ffd2733e97dfc2b5a07c7cab
Coverity CID 221511
Bug 11401
--------------------------
Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: y
Applying: select/cons_tres with dragonfly: Fix coverity NULL dereference
alex@polaris:~/slurm/source$ git show HEAD
commit f52a3b08cd895820c376577519c5413de7259bb4 (HEAD -> slurm-20.11)
Author:     Carlos Tripiana Montes <tripiana@schedmd.com>
AuthorDate: Thu Apr 29 14:33:18 2021 +0200
Commit:     Alejandro Sanchez <alex@schedmd.com>
CommitDate: Thu Apr 29 15:35:36 2021 +0200
    select/cons_tres with dragonfly: Fix coverity NULL dereference
    Continuation of 8475ae9d77e30585ffd2733e97dfc2b5a07c7cab
    Coverity CID 221511
    Bug 11401
diff --git a/src/plugins/select/cons_tres/job_test.c b/src/plugins/select/cons_tres/job_test.c
index acb2ae30df..bc3a81f965 100644
--- a/src/plugins/select/cons_tres/job_test.c
+++ b/src/plugins/select/cons_tres/job_test.c
@@ -2626,7 +2626,8 @@ static int _eval_nodes_topo(job_record_t *job_ptr,
        rc = SLURM_ERROR;
 fini:
-       if (job_ptr->req_switch > 0 && rc == SLURM_SUCCESS) {
+       if (job_ptr->req_switch > 0 && switch_node_bitmap &&
+           rc == SLURM_SUCCESS) {
                int leaf_switch_count = 0;
                /* Count up leaf switches. */
alex@polaris:~/slurm/source$

Re: git am / patch utility not applying a .patch to the correct function

[Jeff King]

On Thu, Apr 29, 2021 at 04:25:42PM +0200, Alejandro Sanchez wrote:

> When I attempted to git am a patch clearly targeted for
> _eval_nodes_dfly(), the result shown by git show resulted in the
> modified code in _eval_nodes_topo() instead. A colleague also reported
> this is also happening with the 'patch' utility without git, we're not
> sure if they both share any common logic or library behind the scenes.

They don't share code, but this is an inherent issue with patches. If
the file you're applying the patch to is not at the exact same state as
the original (say, some other content has been introduced which moves
the patched lines around), then the tools will use the context lines in
the patch to try to find the right spot.

Usually this does what you want, but it can occasionally find the wrong
spot (e.g., if the lines in the context are repeated, and especially if
the "right" spot was changed so that the context doesn't match).

> I've attached a sequence of commands showing the contents of the patch
> and the result after applying.

Your patch here is corrupted:

> diff --git a/src/plugins/select/cons_tres/job_test.c b/src/plugins/select/cons_tres/job_test.c
> index 2d25345945..ff125fafa3 100644
> --- a/src/plugins/select/cons_tres/job_test.c
> +++ b/src/plugins/select/cons_tres/job_test.c
> @@ -2045,7 +2045,8 @@ static int _eval_nodes_dfly(job_record_t *job_ptr,
>  	rc = SLURM_ERROR;
>  fini:
> -	if (job_ptr->req_switch > 0 && rc == SLURM_SUCCESS) {
> +	if (job_ptr->req_switch > 0 && switch_node_bitmap &&
> +	    rc == SLURM_SUCCESS) {
>  		int leaf_switch_count = 0;
>  		/* Count up leaf switches. */

It claims 7 lines in the pre-image, but there are only 5 (context lines
+ the "-" lien). And likewise, 2 lines are missing from the post-image.

I'm guessing this has to do with cutting-and-pasting into the mail,
because your "git show" output is likewise corrupted. The correct patch
is:

diff --git a/src/plugins/select/cons_tres/job_test.c b/src/plugins/select/cons_tres/job_test.c
index 2d25345945..ff125fafa3 100644
--- a/src/plugins/select/cons_tres/job_test.c
+++ b/src/plugins/select/cons_tres/job_test.c
@@ -2045,7 +2045,8 @@ static int _eval_nodes_dfly(job_record_t *job_ptr,
 	rc = SLURM_ERROR;
 
 fini:
-	if (job_ptr->req_switch > 0 && rc == SLURM_SUCCESS) {
+	if (job_ptr->req_switch > 0 && switch_node_bitmap &&
+	    rc == SLURM_SUCCESS) {
 		int leaf_switch_count = 0;
 
 		/* Count up leaf switches. */

Starting at the state of 63e94c2ccbb62aea84cfb0b808761de2bb64e74c^,
which you mentioned, the context doesn't match line 2045 (which is in
_eval_nodes_dfly):

  $ sed -n '2045,+7p' src/plugins/select/cons_tres/job_test.c
  	rc = SLURM_ERROR;
  
  fini:
  	if (job_ptr->req_switch > 0 && rc == SLURM_SUCCESS) {
  		/* req_switch == 1 here; enforced at the top of the function. */
  		leaf_switch_count = 0;
  
  		/* count up leaf switches */

There's an extra comment at the top of the block, and the case differs
in the lower comment. But the context _does_ match the later block of
code in _eval_nodes_topo():

  $ sed -ne '2626,+7p' src/plugins/select/cons_tres/job_test.c
  	rc = SLURM_ERROR;
  
  fini:
  	if (job_ptr->req_switch > 0 && switch_node_bitmap &&
  	    rc == SLURM_SUCCESS) {
  		int leaf_switch_count = 0;
  
  		/* Count up leaf switches. */

And hence that's where both git and patch will apply it.

One other confusion may be that the hunk header says
"_eval_nodes_dfly()" in it. This is purely informational for a human
reader, and not taken into account by the patch application code (which
does not even understand things like functions in the first place).

-Peff