"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Stefan Sperling <stsp@stsp.name>
Subject:
Re: histedit: How to fold M, D?
To:
Christian Weisgerber <naddy@mips.inka.de>
Cc:
gameoftrees@openbsd.org
Date:
Tue, 25 Jul 2023 15:24:16 +0200

Download raw body.

Thread
On Tue, Jul 25, 2023 at 02:35:45PM +0200, Christian Weisgerber wrote:
> Stefan Sperling:
> 
> > The "different content" check for deletions is supposed to be a
> > safe-guard in cases where comnands such as rebase, merge, cherrypick,
> > and backout delete files. If file content is not 100% the same then
> > we cannot assume that the file being deleted is the "same".
> 
> Absolutely.  If I have a branch that deletes files and I rebase it,
> I want a conflict if the contents of those files are different.
> 
> In fact that is my situation.  The files have changed, and I add a
> commit that reverts those changes, and then rebase the branch on
> top.  Which works fine.  But afterwards, I would like to fold the
> revert commit into the rebased branch.
> 
> > However, your histedit situation is a different use case.
> 
> Yes.  How doas the "different content" check even apply to a folding
> operation??

It is because histedit uses the same code as rebase to do merging.
Folding is implemented as a series of cherrypick merges without
creating commits in-between.

Below is a relatively simple patch you could try.
Consider the deletion-rule we have in place for locally added files:
If the file's content can be found in the repository then we can delete it.

Applying this logic to files in modified status should make your histedit
folding case "just work" since the intermediate merged content should by
definition appear in the repository, unless you have make other edits.
Because this approach affects all merges, not just histedit, it might
lead to other surprises in other edge cases. But any files deleted
here should always remain recoverable, provided the corresponding
blob isn't garbaged-collected during gotadmin cleanup. So I would
consider this approach safe for now.

Surprisingly, the test suite keeps passing with this (ignoring the
unrelated tog/log.sh test failure which regres builders have been
telling this mailing list about already).

An alternative to this patch would be to special-case the folding case
by passing down a flag which tweaks the behaviour of this code path just
for folding. But I don't have enough spare time to write such a diff now.

diff /home/stsp/src/got
commit - 48bfa43b34c38fcf5e33c1e63dbe5dbd8c5865ce
path + /home/stsp/src/got
blob - c2e52f2b6af12e7f6d173ad130885b820fa1bd5e
file + lib/worktree.c
--- lib/worktree.c
+++ lib/worktree.c
@@ -3124,12 +3124,13 @@ 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:
 		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,
@@ -3159,7 +3160,6 @@ merge_file_cb(void *arg, struct got_blob_object *blob1
 				goto done;
 			break;
 		}
-		case GOT_STATUS_MODIFY:
 		case GOT_STATUS_CONFLICT:
 			err = (*a->progress_cb)(a->progress_arg,
 			    GOT_STATUS_CANNOT_DELETE, path1);