"log_follow_rename": true setting works properly for show_file_at_commit, checkout_file_at_commit

[kylebebak]

• "log_follow_rename": true setting works properly for show_file_at_commit, checkout_file_at_commit
• This uses existing filename_at_commit function, in the same way it's used here in blame_file_atcommit
• I've tested this locally to verify both show_file_at_commit and checkout_file_at_commit work as expected, for files that have been renamed and files that haven't been renamed

[kaste]

At this point all three predicates should be True all the time if I'm reading this correctly.

There is also

GitSavvy/core/commands/log_graph.py

Lines 2702 to 2704 in 8afe2de

	def checkout_file_at_commit(self, commit_hash, file_path):
	self.checkout_ref(commit_hash, fpath=file_path)
	util.view.refresh_gitsavvy_interfaces(self.window)

which would need the same patch I think.

[kylebebak]

Do you mean the three expressions in the if statement, if commit_hash and file_path and self.savvy_settings.get("log_follow_rename"):?

Type checker says file_path is Unknown | None in line 254 above, and self.filename_at_commit takes (str, str), so I figured I'd just make sure both were truthy

As for self.savvy_settings.get("log_follow_rename"), that's a user setting that's not guaranteed to be truthy in general, right?

---

Agreed on making same patch in log_graph.py` module, I'll do that

[kaste]

As for self.savvy_settings.get("log_follow_rename"), that's a user setting that's not guaranteed to be truthy in general, right?

Ah right, looks like log_follow_rename can be False still, but self._commit_hash is always truthy here and self._file_path too otherwise the menu item would not appear (see def update_actions(self)).

[kaste]

I don't think the changes for "show_file_at_commit" do work. Not sure I do get them though. What confuses me is that we get the filename_at_commit here but don't use it on L103 and L105 too. Is that intentional?

Also the whole show_file_at_commit view has [p]revious and [n]ext handlers, starting at L201 below. Do they just work or did you miss them?

[kylebebak]

I realize there's an error in the code I wrote. It should be checking self.savvy_settings.get("log_follow_rename"):, not self.savvy_settings.get("blame_follow_rename"):

The code works for me in both cases, but that's because I have both of these enabled in my GitSavvy settings

	"log_follow_rename": true,
	"blame_follow_rename": true,

I just fixed this

---

What confuses me is that we get the filename_at_commit here but don't use it on L103 and L105 too

Thanks for catching this, I just fixed this as well

Also the whole show_file_at_commit view has [p]revious and [n]ext handlers, starting at L201 below. Do they just work or did you miss them?

I tried making this code work and couldn't

[kaste]

Generally very welcome change. I think the show_file_at_commit view is more complicated to do right though. Maybe you should split this PR in two; esp. as the checkout part looks almost trivial.

[kylebebak]

@kaste

I think the bug you found in the show_file_at_commit code I wrote above is fixed now. The [n] and [p] hotkeys to page through commits are not working

I think having show_file_at_commit working for renamed files, even without the n/p hotkey support, is valuable and could go in with this PR. If you think that's essential to get these hotkeys right along with enabling show_file_at_commit for log_follow_rename, I'll make another PR

[kaste]

I actually wonder why the defaults for log_follow_rename and blame_follow_rename are ... wrong? At least they seem wrong. Like you I have set them to true.

[kaste]

Actually n and p should work because it also informs us if the implementation is correct here or hacky. At least we must know why it doesn't work.) Generally, with show_file_at_commit, your approach is surprising in that you patch the "refresh" command/function. If show_file_at_commit were a function I would say, shouldn't we call it with the resolved (commit, file_name) pair. Isn't the resolving in the "refresh" instead of the constructor function too late and surprising? Basically we call show_file_at_commit(commit_hash, file_path) but then show commit_hash, different_file_path. Does this make everything easier or more complex?