Refactor show hunk to avoid circular dependency

[wyuenho]

Please merge #257 first.

The diff is https://github.com/wyuenho/diff-hl/compare/fix-256...wyuenho:diff-hl:refactor-show-hunk

This is in preparation of an upcoming PR I want to make that will affect both diff-hl-show-hunk-posframe and diff-hl-show-hunk-inline.

This is just a file rename, renamed the prefix from diff-hl-inline-popup to diff-hl-show-hunk-inline, removed an import from diff-hl-show-hunk, moved 2 defcustoms to diff-hl-inline-popup and added the appropriate aliases.

[dgutov]

Hi!

Could you describe the circularity the description is referring to? ATM, I think, the direction is one-way between the show-hunk and the inline-popup subpackages.

Have you considered options that would avoid renaming the moved options like diff-hl-show-hunk-inline-popup-hide-hunk?

I think you probably have a point here, but I'd rather avoid spending time guessing the reasoning.

(The linked PR gives a hint, but still.)

[wyuenho]

Ignore the renames, it's not important other than consistency.

Basically, I need to refer to diff-hl-show-hunk-ignorable-command-p in diff-hl-inline-popup, so naturally I thought of requiring diff-hl-show-hunk. Turns out diff-hl-show-hunk requires diff-hl-inline-popup in reverse for nothing other than an autoloaded function diff-hl-inline-popup that refers to names in both files. I mean, if it's an autoloaded function, surely it can live in diff-hl-inline-popup right?

[dgutov]

Basically, I need to refer to diff-hl-show-hunk-ignorable-command-p in diff-hl-inline-popup, so naturally I thought of requiring diff-hl-show-hunk

Why not just declare-function? I think we can assume that diff-hl-show-hunk is always loaded by then.

It seemed cleaner to me to have diff-hl-inline-popup as-is, though, because that's how it was originally written. We're "incubating" it here (see comments in #147), at least until somebody really wants it separate with proper name.

Keeping that property while adding diff-hl-show-hunk-ignorable-command-p seems more involved, though: it would need some variable that the called of the package could override (to be used in diff-hl-inline-popup--ignorable-command-p). Calling the function directly and silencing the byte-compiler warning gets around that hassle in the short term.

For that matter, diff-hl-show-hunk-posframe could make sense as a separate package as well (providing a variant of the "inline popup" working only with graphical displays - for now), so from the same perspective moving diff-hl-show-hunk-posframe out would also make sense - but that leaves fairly little code in the package, so I wasn't sure it's worth it.

[wyuenho]

It seemed cleaner to me to have diff-hl-inline-popup as-is, though, because that's how it was originally written. We're "incubating" it here (see comments in #147), at least until somebody really wants it separate with proper name.

I don't know man, the problem with "incubation" is, there's never a well-defined end goal and the code can just deteriorate for no reason other than a catch-all "we are incubating". Like, how long do you want to incubate this?

While the declare-function crutch works, I have to ask why you would want to arrange the code in such a way where instead of being able to just look at the top of some files and visualize the dependency DAG in your head, you ask the reader to resolve every dependency at the granularity of names like reading C? These files all live in the same repo, there's no need to treat them as if they are external packages controlled by someone else.

Keeping that property while adding diff-hl-show-hunk-ignorable-command-p seems more involved, though: it would need some variable that the called of the package could override (to be used in diff-hl-inline-popup--ignorable-command-p). Calling the function directly and silencing the byte-compiler warning gets around that hassle in the short term.

I don't understand what you said. Why is it more "involved" ? diff-hl-show-hunk-ignorable-command-p is imported into 2 files, diff-hl-show-hunk-ignorable-command-p refers to a global dynamically scoped variable called diff-hl-show-hunk-ignorable-commands, of which the user can override anywhere anytime. diff-hl-show-hunk-ignorable-commands is not modified by diff-hl-inline-popup, and it doesn't need anything equivalent just for diff-hl-inline-popup like a diff-hl-inline-popup-ignorable-commands. I just need one global variable, and one function that I can import into 2 file, this is as simple as it gets.

For that matter, diff-hl-show-hunk-posframe could make sense as a separate package as well (providing a variant of the "inline popup" working only with graphical displays - for now), so from the same perspective moving diff-hl-show-hunk-posframe out would also make sense - but that leaves fairly little code in the package, so I wasn't sure it's worth it.

What's wrong with keeping every file in this repo? Just keep them organized.

[dgutov]

I don't know man, the problem with "incubation" is, there's never a well-defined end goal and the code can just deteriorate for no reason other than a catch-all "we are incubating". Like, how long do you want to incubate this?

Eh, as long as it "feels" like a separate package wanting to get out. But the other reason (I thought I mentioned, but apparently not), is Git tooling does not deal with moving code between files too well, especially when renaming is added to the picture. vc-print-log got better in the latest release, but vc-annotate might still not show you the original of a moved function, or the context before it was moved.

And we'll have to do something with the renamed defcustoms, etc. It's a pain.

Neither is a strong argument on its own, but argument for the move doesn't look too strong either.

I don't understand what you said. Why is it more "involved" ?

I was referring to the scheme where the inline-popup subpackage stays independent. It'd need a hook or etc to support this kind of customizable callback.

diff-hl-show-hunk-ignorable-commands is not modified by diff-hl-inline-popup, and it doesn't need anything equivalent just for diff-hl-inline-popup like a diff-hl-inline-popup-ignorable-commands. I just need one global variable, and one function that I can import into 2 file, this is as simple as it gets.

Yes, like I said it's okay as a temporary solution (of the "nothing is more permanent" category).

What's wrong with keeping every file in this repo? Just keep them organized.

Nothing's wrong, it's a solid option, just that if somebody else will want to take that package on and give it more attention, they might want to develop it in their own repository. Or not.

[wyuenho]

Eh, as long as it "feels" like a separate package wanting to get out. But the other reason (I thought I mentioned, but apparently not), is Git tooling does not deal with moving code between files too well, especially when renaming is added to the picture.

It didn't "feel" like a separate package wanting to get out. It felt like abandoned code rotting in limbo, this PR actually turns inline into what "feels" like a separate package.

vc-print-log got better in the latest release, but vc-annotate might still not show you the original of a moved function, or the context before it was moved.

That's not a problem with Git tooling, it's a problem with the old and crusty VC mode that fundamentally still operates at the file level as if we are still dealing with RCS and CVS, both of which do not preserve history unless you hack around it. It's high time you abandon that model and rearchitect VC.

Anyway, I've just broken up the commit into a series preserving the file rename information. If there's any more hesitation, I'd appreciate if you lay them all out into a series of steps acceptable to you.

[wyuenho]

@dgutov any idea on how to move this forward?

[wyuenho]

I've broken down this PR into smaller commits, hopefully this is easier to review.

[dgutov]

@wyuenho Thanks for the patience, having the changes split into separate commits indeed made it more transparent.

[tarsius]

This results in a lot of new warnings:

Compiling diff-hl-show-hunk-inline.el...

In toplevel form:
lib/diff-hl/diff-hl-show-hunk-inline.el:49:2: Warning: in defcustom for ‘diff-hl-show-hunk-inline-hide-hunk’: fails to specify containing group
lib/diff-hl/diff-hl-show-hunk-inline.el:53:2: Warning: in defcustom for ‘diff-hl-show-hunk-inline-smart-lines’: fails to specify containing group

In diff-hl-show-hunk-inline:
lib/diff-hl/diff-hl-show-hunk-inline.el:306:9: Warning: assignment to free variable ‘diff-hl-show-hunk--hide-function’
lib/diff-hl/diff-hl-show-hunk-inline.el:312:21: Warning: reference to free variable ‘diff-hl-show-hunk--no-lines-removed-message’
lib/diff-hl/diff-hl-show-hunk-inline.el:314:19: Warning: reference to free variable ‘diff-hl-show-hunk--original-overlay’
lib/diff-hl/diff-hl-show-hunk-inline.el:355:10: Warning: reference to free variable ‘diff-hl-show-hunk-map’
lib/diff-hl/diff-hl-show-hunk-inline.el:361:71: Warning: Alias for ‘diff-hl-show-hunk-inline--current-popup’ should be declared before its referent
lib/diff-hl/diff-hl-show-hunk-inline.el:362:71: Warning: Alias for ‘diff-hl-show-hunk-inline--current-lines’ should be declared before its referent
lib/diff-hl/diff-hl-show-hunk-inline.el:363:71: Warning: Alias for ‘diff-hl-show-hunk-inline--current-index’ should be declared before its referent
lib/diff-hl/diff-hl-show-hunk-inline.el:364:74: Warning: Alias for ‘diff-hl-show-hunk-inline--invoking-command’ should be declared before its referent
lib/diff-hl/diff-hl-show-hunk-inline.el:365:72: Warning: Alias for ‘diff-hl-show-hunk-inline--current-footer’ should be declared before its referent
lib/diff-hl/diff-hl-show-hunk-inline.el:366:72: Warning: Alias for ‘diff-hl-show-hunk-inline--current-header’ should be declared before its referent
lib/diff-hl/diff-hl-show-hunk-inline.el:367:64: Warning: Alias for ‘diff-hl-show-hunk-inline--height’ should be declared before its referent
lib/diff-hl/diff-hl-show-hunk-inline.el:368:79: Warning: Alias for ‘diff-hl-show-hunk-inline--current-custom-keymap’ should be declared before its referent
lib/diff-hl/diff-hl-show-hunk-inline.el:369:68: Warning: Alias for ‘diff-hl-show-hunk-inline--close-hook’ should be declared before its referent
lib/diff-hl/diff-hl-show-hunk-inline.el:370:76: Warning: Alias for ‘diff-hl-show-hunk-inline-hide-hunk’ should be declared before its referent
lib/diff-hl/diff-hl-show-hunk-inline.el:371:78: Warning: Alias for ‘diff-hl-show-hunk-inline-smart-lines’ should be declared before its referent
lib/diff-hl/diff-hl-show-hunk-inline.el:372:75: Warning: Alias for ‘diff-hl-show-hunk-inline-transient-mode-map’ should be declared before its referent

In end of data:
lib/diff-hl/diff-hl-show-hunk-inline.el:356:12: Warning: the function ‘diff-hl-show-hunk-hide’ is not known to be defined.
lib/diff-hl/diff-hl-show-hunk-inline.el:341:8: Warning: the function ‘diff-hl-show-hunk--goto-hunk-overlay’ is not known to be defined.

[dgutov]

@tarsius Thanks! Fixed them all, hopefully.

Guess a byte-compilation check is due in the CI script.