On Mon, Jul 24, 2023 at 06:13:45PM +1000, Mark Jamsek wrote:
> Stefan Sperling <stsp@stsp.name> wrote:
> > On Fri, Jul 21, 2023 at 03:14:06PM +0200, Stefan Sperling wrote:
> > > A problem that applies to both this patch and got branch -l:
> > > A mixed-commit work tree will be displayed as up-to-date since the current
> > > logic only considers the work tree's global base commit.
> > > Could we iterate over the file index and check whether any file is based
> > > on a different commit, and if so show the out-of-date symbol ~?
> > 
> > The above suggestion I made results in a performance regression,
> > as Omar pointed out on IRC:
> > 
> >   20:47 <@op2> one consequence of the new work tree base commit marker
> >                in tog is that it is slower to open in src.git :/
> >   20:48 <@op2> xterm stays blank for ~2 seconds before it start listing
> >                commits, while in 0.90 it was istantaneous
> > 
> > The patch below attempts to fix this regression by letting the
> > log thread walk the file index once it has fetched the initial
> > batch of commits required to fill the display. In my testing this
> > lets tog start up as fast at it did before and already accept input
> > before the marker appears in a large work tree.
> > The delay is still noticable in some cases, e.g. when pressing page-down
> > right after starting tog, the display will only scroll further once the
> > commit marker has been drawn. But this is a small one-time lag and I could
> > live with it.
> 
> I actually tested with src.git and didn't notice such a delay, but
> that's likely due to machine differences. I'm on a T14s Gen 3 i7-1260P
> and the load delay is negligible for me; not instantaneous but sub 0.5s.
> This approach is much better though!

That may be too fast to notice the issue.
I can notice it on an x250 with sata SSD. It would probably be much
worse on a system with a slow spinning disk.

> The diff works as expected for me but we need to keep one check that's
> needed else we draw garbage when tog_base_commit.marker is uninitialised
> (annotated below).


That suggests we are initializing this state much too late.
How about initializing the non-zero values in main()?
This fixes the garbage values exposed by the test suite for me.
  
diff /home/stsp/src/got
commit - 2d9a8424e0be050d91aa43c21b4cbafba2b21d51
path + /home/stsp/src/got
blob - 7460e9344c4e5b28dd17ca3a1d586cb14e0a5a2d
file + tog/tog.c
--- tog/tog.c
+++ tog/tog.c
@@ -4434,8 +4434,6 @@ set_tog_base_commit(struct got_repository *repo, struc
 	if (tog_base_commit.id == NULL)
 		return got_error_from_errno( "got_object_id_dup");
 
-	tog_base_commit.idx = -1;
-	tog_base_commit.marker = GOT_WORKTREE_STATE_UNKNOWN;
 	return NULL;
 }
 
@@ -10124,6 +10122,9 @@ main(int argc, char *argv[])
 			tog_diff_algo = GOT_DIFF_ALGORITHM_MYERS;
 	}
 
+	tog_base_commit.idx = -1;
+	tog_base_commit.marker = GOT_WORKTREE_STATE_UNKNOWN;
+
 	if (cmd == NULL) {
 		if (argc != 1)
 			usage(0, 1);

> However, we also need to add the wait instruction to
> tog log because it breaks regress as it is. I can investigate this in an
> hour or two though.

That would help, thank you. I tried getting this to work but it's not
super trivial (WIP patch below).

> > Regarding 'got branch -l' I am podering moving mixed-commit detection
> > to 'got status' since this command is already walking the file index.
> > Or will we accept branch -l being slower now?
> 
> Likewise, the performance impact on 'got branch -l' is negligible for me
> so I'm equally happy to keep it as is or move it to 'got status' :)
> 
> I suppose, now that we show it in tog, it won't be as missed from
> 'branch -l'? Or perhaps we could use another flag for annotating the
> state of the work tree's current branch like we do now (got branch -s)?
> Although I'm not really a fan of that last idea tbh.

Maybe we can keep got branch as-is and let 'got status' print a message when
it sees mixed-commits? We are paying the performance price during status anyway.

WIP patch for the tests:


diff /home/stsp/src/got
commit - 2d9a8424e0be050d91aa43c21b4cbafba2b21d51
path + /home/stsp/src/got
blob - d10c8e4e269c29522a88fa8b7f94b5658cacd469
file + regress/tog/log.sh
--- regress/tog/log.sh
+++ regress/tog/log.sh
@@ -397,7 +397,8 @@ test_log_commit_keywords()
 	done
 
 	cat <<-EOF >$TOG_TEST_SCRIPT
-	SCREENDUMP
+WAIT_FOR_UI	wait for log thread to finish
+SCREENDUMP
 	EOF
 
 	cat <<-EOF >$testroot/view.expected
blob - 7460e9344c4e5b28dd17ca3a1d586cb14e0a5a2d
file + tog/tog.c
--- tog/tog.c
+++ tog/tog.c
@@ -370,6 +370,7 @@ struct tog_log_thread_args {
 	struct got_repository *repo;
 	int *pack_fds;
 	int log_complete;
+	pthread_cond_t log_loaded;
 	sig_atomic_t *quit;
 	struct commit_queue_entry **first_displayed_entry;
 	struct commit_queue_entry **selected_entry;
@@ -3397,6 +3398,15 @@ log_thread(void *arg)
 			a->commits_needed = 0;
 		else {
 			if (a->commits_needed == 0 && !a->load_all) {
+				if (tog_io.wait_for_ui) {
+					errcode = pthread_cond_signal(
+					    &a->log_loaded);
+					if (errcode && err == NULL)
+						err = got_error_set_errno(
+						    errcode,
+						    "pthread_cond_signal");
+				}
+
 				errcode = pthread_cond_wait(&a->need_commits,
 				    &tog_mutex);
 				if (errcode) {
@@ -3411,6 +3421,13 @@ log_thread(void *arg)
 		}
 	}
 	a->log_complete = 1;
+	if (tog_io.wait_for_ui) {
+		errcode = pthread_cond_signal(&a->log_loaded);
+		if (errcode && err == NULL)
+			err = got_error_set_errno(errcode,
+			    "pthread_cond_signal");
+	}
+
 	errcode = pthread_mutex_unlock(&tog_mutex);
 	if (errcode)
 		err = got_error_set_errno(errcode, "pthread_mutex_unlock");
@@ -3841,6 +3858,14 @@ open_log_view(struct tog_view *view, struct got_object
 		goto done;
 	}
 
+	if (using_mock_io) {
+		int rc;
+
+		rc = pthread_cond_init(&s->thread_args.log_loaded, NULL);
+		if (rc)
+			return got_error_set_errno(rc, "pthread_cond_init");
+	}
+
 	s->thread_args.commits_needed = view->nlines;
 	s->thread_args.graph = thread_graph;
 	s->thread_args.real_commits = &s->real_commits;
@@ -3885,6 +3910,17 @@ show_log_view(struct tog_view *view)
 			err = trigger_log_thread(view, 1);
 			if (err)
 				return err;
+			if (tog_io.wait_for_ui) {
+				int rc;
+
+				rc = pthread_cond_wait(&s->thread_args.log_loaded,
+				    &tog_mutex);
+				if (rc)
+					return got_error_set_errno(rc,
+					    "pthread_cond_wait");
+				tog_io.wait_for_ui = 0;
+			}
+
 		}
 	}