On Wed, Jul 26, 2023 at 05:23:04PM +0200, Christian Weisgerber wrote:
> Stefan Sperling:
> 
> > > Oh sorry this diff won't work as-is since it assumes that the
> > > blob ID passed in already matches the file on disk. I will have
> > > to revisit this, adding an object ID lookup.
> > 
> > Try this one please. Also fixes small leaks in unlikely error paths.
> 
> That doesn't quite work:
> 
> 81474c680f54 ->  fold commit: revert: Implement PAC support.
> ...
> 4b1696fbab55 -> 3790f01fbf94: llvm: revert to 13.0.0
> ...
> 0d86ca54faea -> 31c8452f490b: llvm: update to 15.0.7
> ...
> got: duplicate entry in tree object
> 
> 
> Also, aborting the operation:
> 
> $ got he -a
> ...
> got: gnu/llvm/lldb/source/Plugins/Process/OpenBSD/CMakeLists.txt: duplicate file index entry
> 
> 
> Hmpf.  FWIW, "got up" recovers from this.

Sorry about that. I missed that removing modified files and locally
added files require different operations on the file index.

Here is a fixed version, with a test to confirm that it is now
working as intended.

-----------------------------------------------
 allow modified files to be deleted during merges if content is found the repo
 
 Problem reported by naddy@
 
diff 7c67cf56d3e4bab38eebe16a51f0ffb790694738 5f5af15cd633829e1362bbf7843315248d3c41c0
commit - 7c67cf56d3e4bab38eebe16a51f0ffb790694738
commit + 5f5af15cd633829e1362bbf7843315248d3c41c0
blob - c2e52f2b6af12e7f6d173ad130885b820fa1bd5e
blob + 2cdafab5a89da7e31a55bc887af42b14226637d8
--- lib/worktree.c
+++ lib/worktree.c
@@ -2999,6 +2999,7 @@ merge_file_cb(void *arg, struct got_blob_object *blob1
 	int local_changes_subsumed;
 	FILE *f_orig = NULL, *f_deriv = NULL, *f_deriv2 = NULL;
 	char *id_str = NULL, *label_deriv2 = NULL;
+	struct got_object_id *id = NULL;
 
 	if (blob1 && blob2) {
 		ie = got_fileindex_entry_get(a->fileindex, path2,
@@ -3124,18 +3125,63 @@ merge_file_cb(void *arg, struct got_blob_object *blob1
 			if (ie)
 				got_fileindex_entry_mark_deleted_from_disk(ie);
 			break;
+		case GOT_STATUS_MODIFY: {
+			FILE *blob1_f;
+			off_t blob1_size;
+			int obj_type;
+			/*
+			 * Delete the file only if its content already
+			 * exists in the repository.
+			 */
+			err = got_object_blob_file_create(&id, &blob1_f,
+			    &blob1_size, path1);
+			if (err)
+				goto done;
+			if (fclose(blob1_f) == EOF) {
+				err = got_error_from_errno("fclose");
+				goto done;
+			}
+
+			/* Implied existence check. */
+			err = got_object_get_type(&obj_type, repo, id);
+			if (err) {
+				if (err->code != GOT_ERR_NO_OBJ)
+					goto done;
+				err = (*a->progress_cb)(a->progress_arg,
+				    GOT_STATUS_CANNOT_DELETE, path1);
+				goto done;
+			} else if (obj_type != GOT_OBJ_TYPE_BLOB) {
+				err = (*a->progress_cb)(a->progress_arg,
+				    GOT_STATUS_CANNOT_DELETE, path1);
+				goto done;
+			}
+			err = (*a->progress_cb)(a->progress_arg,
+			    GOT_STATUS_DELETE, path1);
+			if (err)
+				goto done;
+			err = remove_ondisk_file(a->worktree->root_path,
+			    path1);
+			if (err)
+				goto done;
+			if (ie)
+				got_fileindex_entry_mark_deleted_from_disk(ie);
+			break;
+		}
 		case GOT_STATUS_ADD: {
-			struct got_object_id *id;
 			FILE *blob1_f;
 			off_t blob1_size;
 			/*
-			 * Delete the added file only if its content already
+			 * Delete the file only if its content already
 			 * exists in the repository.
 			 */
 			err = got_object_blob_file_create(&id, &blob1_f,
 			    &blob1_size, path1);
 			if (err)
 				goto done;
+			if (fclose(blob1_f) == EOF) {
+				err = got_error_from_errno("fclose");
+				goto done;
+			}
 			if (got_object_id_cmp(id, id1) == 0) {
 				err = (*a->progress_cb)(a->progress_arg,
 				    GOT_STATUS_DELETE, path1);
@@ -3152,14 +3198,10 @@ merge_file_cb(void *arg, struct got_blob_object *blob1
 				err = (*a->progress_cb)(a->progress_arg,
 				    GOT_STATUS_CANNOT_DELETE, path1);
 			}
-			if (fclose(blob1_f) == EOF && err == NULL)
-				err = got_error_from_errno("fclose");
-			free(id);
 			if (err)
 				goto done;
 			break;
 		}
-		case GOT_STATUS_MODIFY:
 		case GOT_STATUS_CONFLICT:
 			err = (*a->progress_cb)(a->progress_arg,
 			    GOT_STATUS_CANNOT_DELETE, path1);
@@ -3243,6 +3285,7 @@ done:
 	if (f_deriv2 && fclose(f_deriv2) == EOF && err == NULL)
 		err = got_error_from_errno("fclose");
 	free(id_str);
+	free(id);
 	free(label_deriv2);
 	free(ondisk_path);
 	return err;
blob - 2850c77ea9f07c4d604211acb189455b07692252
blob + 5ef42917a9efb422c5a96479122b03533f9019bc
--- regress/cmdline/histedit.sh
+++ regress/cmdline/histedit.sh
@@ -1542,6 +1542,88 @@ test_histedit_fold_delete_add() {
 	test_done "$testroot" "$ret"
 }
 
+# if a previous commit edits a file, and it is folded into a commit
+# that deletes the same file, the file will be deleted by histedit
+test_histedit_fold_edit_delete() {
+	local testroot=`test_init histedit_fold_edit_delete`
+
+	local orig_commit=`git_show_head $testroot/repo`
+
+	echo "modify alpha" > $testroot/repo/alpha
+	(cd $testroot/repo && git add alpha)
+	git_commit $testroot/repo -m "modified alpha"
+	local old_commit1=`git_show_head $testroot/repo`
+
+	git_rm $testroot/repo alpha
+	git_commit $testroot/repo -m "deleted alpha"
+	local old_commit2=`git_show_head $testroot/repo`
+
+	got checkout -c $orig_commit $testroot/repo $testroot/wt > /dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	echo "fold $old_commit1" > $testroot/histedit-script
+	echo "pick $old_commit2" >> $testroot/histedit-script
+	echo "mesg folded changes" >> $testroot/histedit-script
+
+	(cd $testroot/wt && got histedit -F $testroot/histedit-script \
+		> $testroot/stdout)
+
+	local new_commit1=`git_show_head $testroot/repo`
+
+	local short_old_commit1=`trim_obj_id 28 $old_commit1`
+	local short_old_commit2=`trim_obj_id 28 $old_commit2`
+	local short_new_commit1=`trim_obj_id 28 $new_commit1`
+
+	echo "G  alpha" >> $testroot/stdout.expected
+	echo "$short_old_commit1 ->  fold commit: modified alpha" \
+		>> $testroot/stdout.expected
+	echo "D  alpha" >> $testroot/stdout.expected
+	echo "$short_old_commit2 -> $short_new_commit1: folded changes" \
+		>> $testroot/stdout.expected
+	echo "Switching work tree to refs/heads/master" \
+		>> $testroot/stdout.expected
+
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	if [ -e $testroot/wt/alpha ]; then
+		echo "removed file alpha still exists on disk" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	(cd $testroot/wt && got status > $testroot/stdout)
+
+	echo -n > $testroot/stdout.expected
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	(cd $testroot/wt && got log -l2 | grep ^commit > $testroot/stdout)
+	echo "commit $new_commit1 (master)" > $testroot/stdout.expected
+	echo "commit $orig_commit" >> $testroot/stdout.expected
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+	fi
+
+	test_done "$testroot" "$ret"
+}
+
 test_histedit_fold_delete_add() {
 	local testroot=`test_init histedit_fold_delete_add`
 
@@ -2531,6 +2613,7 @@ run_test test_histedit_fold_delete_add
 run_test test_histedit_split_commit
 run_test test_histedit_duplicate_commit_in_script
 run_test test_histedit_fold_add_delete
+run_test test_histedit_fold_edit_delete
 run_test test_histedit_fold_delete_add
 run_test test_histedit_fold_only
 run_test test_histedit_fold_only_empty_logmsg