[v2,3/7] merge-ort: fix type of local 'clean' var in handle_content_merge()

Commit Message

Elijah Newren June 19, 2024, 3 a.m. UTC

From: Elijah Newren <[e-mail]>

handle_content_merge() returns an int.  Every caller of
handle_content_merge() expects an int.  However, we declare a local
variable 'clean' that we use for the return value to be unsigned.  To
make matters worse, we also assign 'clean' the return value of
merge_submodule() in one codepath, which is defined to return an int.
It seems that the only reason to have 'clean' be unsigned was to allow a
cutesy bit manipulation operation to be well-defined.  Fix the type of
the 'clean' local in handle_content_merge().

Signed-off-by: Elijah Newren <[e-mail]>
---
 merge-ort.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Derrick Stolee June 28, 2024, 2:44 a.m. UTC | #1

On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <[e-mail]>
> 
> handle_content_merge() returns an int.  Every caller of
> handle_content_merge() expects an int.  However, we declare a local
> variable 'clean' that we use for the return value to be unsigned.  To
> make matters worse, we also assign 'clean' the return value of
> merge_submodule() in one codepath, which is defined to return an int.
> It seems that the only reason to have 'clean' be unsigned was to allow a
> cutesy bit manipulation operation to be well-defined.  Fix the type of
> the 'clean' local in handle_content_merge().

> @@ -2184,7 +2184,8 @@ static int handle_content_merge(struct merge_options *opt,
>   		free(result_buf.ptr);
>   		if (ret)
>   			return -1;
> -		clean &= (merge_status == 0);
> +		if (merge_status > 0)
> +			clean = 0;
>   		path_msg(opt, INFO_AUTO_MERGING, 1, path, NULL, NULL, NULL,
>   			 _("Auto-merging %s"), path);
>   	} else if (S_ISGITLINK(a->mode)) {

Even after this removal of this cute bitflip, there is one more
subtle use still in the code:

diff --git a/merge-ort.c b/merge-ort.c
index 8dfe80f1009..569014eef31 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -2629,7 +2629,8 @@ static char *check_for_directory_rename(struct 
merge_options *opt,
  	new_path = handle_path_level_conflicts(opt, path, side_index,
  					       rename_info,
  					       &collisions[side_index]);
-	*clean_merge &= (new_path != NULL);
+	if (*clean_merge && !new_path)
+		*clean_merge = 0;

  	return new_path;
  }

I had to think very carefully about this cleverness to be sure
that this conversion is right (and I'm only mostly sure). When
(new_path != NULL) is false, then we definitely set *clean_merge
to zero. Otherwise, we set it to 1 (but only when it was already
1 or -1). Technically, this does change the behavior by not
squashing -1 into 1, but that is less likely to be an existing
value of *clean_merge.

There are other uses of "clean &= collect_renames(...)" but that
appears to be fine because collect_renames() never results in an
abort state (returning -1).

Thanks,
-Stolee

Patch

diff --git a/merge-ort.c b/merge-ort.c
index dc62aaf6d11..be0f5bc3838 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -2109,7 +2109,7 @@  static int handle_content_merge(struct merge_options *opt,
 	 * merges, which happens for example with rename/rename(2to1) and
 	 * rename/add conflicts.
 	 */
-	unsigned clean = 1;
+	int clean = 1;
 
 	/*
 	 * handle_content_merge() needs both files to be of the same type, i.e.
@@ -2184,7 +2184,8 @@  static int handle_content_merge(struct merge_options *opt,
 		free(result_buf.ptr);
 		if (ret)
 			return -1;
-		clean &= (merge_status == 0);
+		if (merge_status > 0)
+			clean = 0;
 		path_msg(opt, INFO_AUTO_MERGING, 1, path, NULL, NULL, NULL,
 			 _("Auto-merging %s"), path);
 	} else if (S_ISGITLINK(a->mode)) {