On Fri, Dec 11, 2020 at 02:48:17AM +0000, Josh Rickmar wrote:
> On Fri, Dec 11, 2020 at 01:55:11AM +0100, Stefan Sperling wrote:
> > On Fri, Dec 11, 2020 at 01:27:04AM +0100, Stefan Sperling wrote:
> > > If the histedit log message editor exits without saving its buffer,
> > > we currently throw away log messages of all commits which were folded.
> > > Only the last commit message is preserved, which could be something
> > > meaningless like "fixup".
> > > 
> > > This patch makes us preserve the initial editor buffer content as-is.
> > > That's not going to be an ideal log message, but doesn't throw away
> > > information and stands out visually because the newly created log
> > > message will start with a comment like:
> > > [[[
> > > # log message of folded commit aabbcc00...
> > > ]]]
> > > 
> > > Git's approach is to concatentate all log messages and strip comments.
> > > That has other downsides such as resulting in a potentially nonsensical
> > > log message.
> > > I'd like to stick to a simple "what you see is what you get" approach.
> > >  
> > > Problem reported by jrick
> > > 
> > > ok?
> > 
> > Off-list, jrick suggested to keep the behaviour intact where histedit's
> > commit operation is aborted if the saved log message is empty.
> > 
> > Differentiating between "empty message" and "unchanged message" should
> > be a good thing to do in general.
> > 
> > ok?
> > 
> > (This is the same change as the previous patch but with the empty log
> > message behaviour kept as-is.)
> 
> This patch behaves as advertised with histedit -f, but I'm hitting an
> unexpected error (got: object <sha1> not found) when using histedit -m
> and saving an empty commit message.  On the main branch, under this
> test Got will continue through the histedit and does not modify the
> original commit message at all.  I think we'd want to make it work
> like histedit -f will after this patch, where the histedit is left in
> an interrupted state.

After looking at this some more, I fully agree.

I became a bit side-tracked by the problem you've described above, where
saving an empty log message after 'got he -m' results in "got: object $foo
not found" instead of the expected "commit message cannot be empty".

The root of that problem is that got_error_msg and got_error_no_obj both
declare a static struct got_error and return a pointer to it. The histedit
code attempts to abort the entire histedit operation after the first commit
fails. Deep in got_worktree_histedit_abort() we try to read the worktree's
base commit object from pack files. If this fails because the commit isn't
packed, we end up overwriting the original error with "object not found".

I try to find a solution for the above problem in the error subsystem.
It also means that code which attempts to manage two errors at once (such
code that declares *err, *close_err) may produce an unexpected message.

For now, this removes any attempts to clean up after histedit runs into an
error. The user will now see why the histedit operation has failed, and 
may decide whether to fix things up and continue or abort the operation.

Our tests seem happy with this.

ok?

diff 448d2d7f0de229dc970fbb6bb61a32615877ef08 /home/stsp/src/got
blob - c8dbc08063b2590cb230bbb89b7864078f5b1a4d
file + got/got.c
--- got/got.c
+++ got/got.c
@@ -8820,13 +8820,8 @@ cmd_histedit(int argc, char *argv[])
 		if (edit_script_path) {
 			error = histedit_load_list(&histedit_cmds,
 			    edit_script_path, repo);
-			if (error) {
-				got_worktree_histedit_abort(worktree, fileindex,
-				    repo, branch, base_commit_id,
-				    update_progress, &upa);
-				print_update_progress_stats(&upa);
+			if (error)
 				goto done;
-			}
 		} else {
 			const char *branch_name;
 			branch_name = got_ref_get_symref_target(branch);
@@ -8834,25 +8829,15 @@ cmd_histedit(int argc, char *argv[])
 				branch_name += 11;
 			error = histedit_edit_script(&histedit_cmds, &commits,
 			    branch_name, edit_logmsg_only, fold_only, repo);
-			if (error) {
-				got_worktree_histedit_abort(worktree, fileindex,
-				    repo, branch, base_commit_id,
-				    update_progress, &upa);
-				print_update_progress_stats(&upa);
+			if (error)
 				goto done;
-			}
 
 		}
 
 		error = histedit_save_list(&histedit_cmds, worktree,
 		    repo);
-		if (error) {
-			got_worktree_histedit_abort(worktree, fileindex,
-			    repo, branch, base_commit_id,
-			    update_progress, &upa);
-			print_update_progress_stats(&upa);
+		if (error)
 			goto done;
-		}
 
 	}