On 22-07-19 04:40pm, Stefan Sperling wrote:
> On Wed, Jul 20, 2022 at 12:07:16AM +1000, Mark Jamsek wrote:
> > As per the TODO item: open a log view of the selected annotated line.
> > 
> > diff refs/heads/main refs/heads/dev/blamelog
> > commit - 314c3f148b7679296136b460ea9a8f0d4c74d437
> > commit + 1de4f1ca80d74e859beae5c3633adcf960c79938
> > blob - 765c5ecd53d258c8af195523d4fb1fea0c8f41d3
> > blob + 80d025e6069d90b874a54b9778401bf07c5b84b1
> > --- tog/tog.1
> > +++ tog/tog.1
> > @@ -406,6 +406,10 @@ currently selected line's commit.
> >  Reload the
> >  .Cm blame
> >  view with the previously blamed commit.
> > +.It Cm L
> > +Open a
> > +.Cm log
> > +view for the currently selected annotated line.
> 
> Hmm. I'm wondering if we should change the tree view's l key to L in
> a follow-up change? Because l is now a horizontal movement key in
> most views, using L for opening a log view seems better.

That sounds better to me too. 'l' is often "right" so it would be good
to keep that standard; especially as we already provide j/k vis-a-vis
down/up. It would be inconsistent to provide conventional hjkl keys in
some views, but have 'l' do something else in another.

Relatedly, should this become convention; that is, uppercase alpha
keymaps used to open other views? So, besides l := L in the tree view,
t := T in the log and ref views; and r := R in the log and tree views.

> > +log_annotated_line(struct tog_view **new_view, int begin_y, int begin_x,
> > +    struct got_repository *repo, struct got_object_id *id)
> > +{
> > +	struct tog_view		*log_view;
> > +	struct got_reference	*ref = NULL;
> > +	char			*id_str = NULL;
> > +	const char		*ref_str = NULL;
> > +	const struct got_error	*err = NULL;
> > +
> > +	*new_view = NULL;
> > +
> > +	err = got_object_id_str(&id_str, id);
> > +	if (err)
> > +		return err;
> > +
> > +	err = got_ref_open(&ref, repo, id_str, 0);
> > +	if (err == NULL)
> > +		ref_str = got_ref_get_name(ref);
> > +	else if (err->code != GOT_ERR_NOT_REF)
> > +		goto done;
> 
> This code assumes that a reference might exist which is named
> after the commit ID of an annotated line...? That does not really
> make a lot of sense.
> 
> This code was apparently copied from cmd_log(), where "start_commit"
> is user-provided input and may well be string such as "origin/main",
> which would then be resolved to an object ID.
> But in log_annotated_line() we definitely have an object ID already.

Yes, that was pretty senseless. I should've thought about it more after
some sleep; I was running on empty last night. I know what I was
thinking at the time: we have fsl_sym_to_* APIs in libf that resolve
hashes to Fossil's Special Tags and references/branches, and vice versa.
But re-reading and thinking about it now, no, it doesn't make sense
either way.

> I think you can just set ref_str = GOT_REF_HEAD for now.
> This may be slightly wrong iin case tog was started in a work tree.
> But more work will be needed to obtain the correct value in that case.
> Most commands use got_worktree_get_head_ref_name() to show the
> current branch of the work tree by default. The log and tree views
> are storing it in s->head_ref_name to keep the branch name available
> for Ctrl+L and allow new commits on this branch to be loaded.
> I suppose tog could store this name globally somewhere, such that you
> could reuse it here, too.
> That can be done as a separate change, though. And it is not very
> critical since the log view will open up just fine at the desired
> commit either way.

Thanks for the explanation!

Revised and rebased diff with suggested changes below. This makes
log_annotated_line() much simpler.  Tests with your fix suggest it's
working as intended.

diff refs/heads/main refs/heads/dev/blamelog
commit - 62b21d332b3a92a7f99022d68bc8fd98a4682d33
commit + c4f9c52af60d93fd5aaf5c89ae05b05c23e61547
blob - 765c5ecd53d258c8af195523d4fb1fea0c8f41d3
blob + 80d025e6069d90b874a54b9778401bf07c5b84b1
--- tog/tog.1
+++ tog/tog.1
@@ -406,6 +406,10 @@ currently selected line's commit.
 Reload the
 .Cm blame
 view with the previously blamed commit.
+.It Cm L
+Open a
+.Cm log
+view for the currently selected annotated line.
 .It Cm /
 Prompt for a search pattern and start searching for matching lines.
 The search pattern is an extended regular expression.
blob - 82d0b3a26a196343c75bd02c869e612c75190979
blob + faad34ec3608cf918768529fef5b7302397093ea
--- tog/tog.c
+++ tog/tog.c
@@ -5725,10 +5725,32 @@ show_blame_view(struct tog_view *view)
 }

 static const struct got_error *
+log_annotated_line(struct tog_view **new_view, int begin_y, int begin_x,
+    struct got_repository *repo, struct got_object_id *id)
+{
+	struct tog_view		*log_view;
+	const struct got_error	*err = NULL;
+
+	*new_view = NULL;
+
+	log_view = view_open(0, 0, begin_y, begin_x, TOG_VIEW_LOG);
+	if (log_view == NULL)
+		return got_error_from_errno("view_open");
+
+	err = open_log_view(log_view, id, repo, GOT_REF_HEAD, "", 0);
+	if (err)
+		view_close(log_view);
+	else
+		*new_view = log_view;
+
+	return err;
+}
+
+static const struct got_error *
 input_blame_view(struct tog_view **new_view, struct tog_view *view, int ch)
 {
 	const struct got_error *err = NULL, *thread_err = NULL;
-	struct tog_view *diff_view;
+	struct tog_view *diff_view, *log_view;
 	struct tog_blame_view_state *s = &view->state.blame;
 	int eos, nscroll, begin_y = 0, begin_x = 0;

@@ -5909,6 +5931,45 @@ input_blame_view(struct tog_view **new_view, struct to
 			break;
 		break;
 	}
+	case 'L': {
+		struct got_object_id *id = NULL;
+
+		view->count = 0;
+		id = get_selected_commit_id(s->blame.lines, s->blame.nlines,
+		    s->first_displayed_line, s->selected_line);
+		if (id == NULL)
+			break;
+
+		if (view_is_parent_view(view))
+			view_get_split(view, &begin_y, &begin_x);
+		err = log_annotated_line(&log_view, begin_y, begin_x,
+		    s->repo, id);
+		if (err)
+			break;
+		if (view_is_parent_view(view) &&
+		    view->mode == TOG_VIEW_SPLIT_HRZN) {
+			err = view_init_hsplit(view, begin_y);
+			if (err)
+				break;
+		}
+
+		view->focussed = 0;
+		log_view->focussed = 1;
+		log_view->mode = view->mode;
+		log_view->nlines = view->lines - begin_y;
+		if (view_is_parent_view(view)) {
+			view_transfer_size(log_view, view);
+			err = view_close_child(view);
+			if (err)
+				return err;
+			err = view_set_child(view, log_view);
+			if (err)
+				return err;
+			view->focus_child = 1;
+		} else
+			*new_view = log_view;
+		break;
+	}
 	case KEY_ENTER:
 	case '\r': {
 		struct got_object_id *id = NULL;

-- 
Mark Jamsek <fnc.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68