Minor improvements

[emreyolcu]

I've made two minor improvements:

• Added a new variable difftastic-difft-environment, mirroring Magit's magit-git-environment. This variable allows users to affect the environment in which difft is run by, say, let-binding it where needed. Similar to Magit, I've made this a defvar instead of a defcustom, since presumably only advanced users will want to change this, and often not as a customization. My use case for this variable is the following function. I use it to temporarily make Git aware of my home directory, which I keep under version control as a bare repository.

(defun my/link-home ()
  (interactive)
  (require 'difftastic)
  (require 'magit)
  (let* ((environment
          (list (format "GIT_DIR=%s" (expand-file-name "~/.home.git/"))
                (format "GIT_WORK_TREE=%s" (expand-file-name "~/"))))
         (difftastic-difft-environment
          (append environment difftastic-difft-environment))
         (magit-git-environment
          (append environment magit-git-environment)))
    (recursive-edit)))

• When the right fringe is disabled and a line is full, Emacs' display engine inserts a \ at the end and wraps the line. Due to this behavior (which cannot be disabled), running difftastic with the right fringe disabled causes the last character of a full line to overflow to the next line, causing unnecessary wrapping. I've changed the width computation to account for this case and subtract 1 before returning if right fringe is disabled. I've also removed the redundant calls to fringe-columns, because window-width already ignores fringes when computing width.

(I was too lazy to break this into two separate PRs, sorry.)

[emreyolcu]

Thanks for the extensive feedback. I'll address those as soon as I can find some free time.

[coveralls]

coverage: 100.0%. remained the same when pulling 668991e707eda3bf42cf5d29f00fd5677edf2c13 on emreyolcu:main into c51d99666fde2a025211d23f17f5a59e0780e2cc on pkryger:main.

[pkryger]

@emreyolcu I can merge the fix for incorrect number of columns, but simply merging this PR will also drag in difftastic-difft-environment, which I am reluctant to do without knowing what is your preferred approach to addressing the other comments. I can adjust it to what I think will be best, but can't promise when that would be. WDYT?

[emreyolcu]

@emreyolcu I can merge the fix for incorrect number of columns, but simply merging this PR will also drag in difftastic-difft-environment, which I am reluctant to do without knowing what is your preferred approach to addressing the other comments. I can adjust it to what I think will be best, but can't promise when that would be. WDYT?

I've addressed most of the comments in my local copy. I've only got two tests to finish implementing where I wasn't sure about the best way to write them. I've been familiarizing myself with the conventions in the tests you've already written. I'm currently travelling and will probably be able to push an update for you to review sometime next week. Sorry about the delay on this; I've managed to find very little time to work on this but I'm hoping to finish soon.

Note: I've also updated the commit that handled column number computation. You were right to ask about the FACE argument to window-max-chars-per-line function. Unlike window-width, that function takes into account face remappings, so, for instance, if you've increased font width using text-scale-adjust, then window-max-chars-per-line takes the new width into account. This leads to incorrect behavior in cases where we might have remapped the default face in the window from which we invoke difftastic, because width computation is performed using the new width but the difftastic buffer that is later created uses the default font width. I've thus opted to temporarily bind face-remapping-alist to nil before calling window-max-chars-per-line, which gives the correct behavior.

[pkryger]

I've addressed most of the comments in my local copy. I've only got two tests to finish implementing where I wasn't sure about the best way to write them. I've been familiarizing myself with the conventions in the tests you've already written.

Good to hear. If in doubt please ask. I am happy to help. FWIW I have been using various techniques throughout the file so if you can find something that is immediately applicable please do. And if not, please propose something new. I am happy to learn a new tricks 😎.

I'm currently travelling and will probably be able to push an update for you to review sometime next week. Sorry about the delay on this; I've managed to find very little time to work on this but I'm hoping to finish soon.

No worries. I am not in hurry at all. Please prioritise as you see fit.

Note: I've also updated the commit that handled column number computation. You were right to ask about the FACE argument to window-max-chars-per-line function. Unlike window-width, that function takes into account face remappings, so, for instance, if you've increased font width using text-scale-adjust, then window-max-chars-per-line takes the new width into account. This leads to incorrect behavior in cases where we might have remapped the default face in the window from which we invoke difftastic, because width computation is performed using the new width but the difftastic buffer that is later created uses the default font width. I've thus opted to temporarily bind face-remapping-alist to nil before calling window-max-chars-per-line, which gives the correct behavior.

This is a great find! Thank you for taking care of that. Just to let me know as I am not completely familiar with how faces are "assigned" to frames/buffers. What happens if current buffer default face is something different than default-face (e.g., a variable pitch face)? Can that even happen?

[emreyolcu]

I've pushed my changes. Could you let me know whether you have any other feedback? I also don't mind if you'd like to directly apply any fixes on top of either commit.

Just to let me know as I am not completely familiar with how faces are "assigned" to frames/buffers. What happens if current buffer default face is something different than default-face (e.g., a variable pitch face)? Can that even happen?

It can happen via the variable face-remapping-alist. That's how, for instance, variable-pitch-mode works. It remaps the default face to variable-pitch. The function window-max-chars-per-line is aware of that mapping, hence the temporary bind we are using.

[pkryger]

So I decided to go for option 2 (vide). I implemented it on top of your changes and merged it. If you think other option is better please let me know.

Thank you for contributing and patience.

[emreyolcu]

Thanks a lot for the feedback, and sorry for the delay. I hadn't thought about that case since my only use case for this feature involves staying in the scope of the let until we are done with the environment. I was working on Option 3 independently from your commit. It made sense to me to have the same behavior during the first and later times the variable is bound; process-environment gives precedence to the first occurrence of a variable assignment so the resulting behavior in Option 3 is to override the stored value anyway.

I've pushed my changes to https://github.com/emreyolcu/difftastic.el in case you want to take a look, but if you're happy with Option 2 then your version also looks good to me.

[pkryger]

Thanks a lot for the feedback, and sorry for the delay. I hadn't thought about that case since my only use case for this feature involves staying in the scope of the let until we are done with the environment. I was working on Option 3 independently from your commit. It made sense to me to have the same behavior during the first and later times the variable is bound; process-environment gives precedence to the first occurrence of a variable assignment so the resulting behavior in Option 3 is to override the stored value anyway.

I think I started with the same place but reached a conclusion that the option 2 satisfies the requirement. I think it's more flexible to the user, especially when it comes to unsetting some variables for subsequent reruns. If user is interested in a new environment it's up to them to specify it. Albeit it may be a bit awkward to use difftastic--metadata directly to pull these values. Perhaps I can add a hook for when a difft process is done / a difftastic buffer is ready, such that they can store whatever is necessary to create a new difftastic-difft-environment value for a subsequent rerun. Not to mention, that such a hook would be a generic purpose one.

I've pushed my changes to https://github.com/emreyolcu/difftastic.el in case you want to take a look, but if you're happy with Option 2 then your version also looks good to me.

I reviewed them, and I think I would have accepted them shouldn't I have not thought about it more. Although you caught one omission in my implementation, which I have added. Appreciate that!

Please let me know if you feel that Option 3 is better. I may be convinced otherwise 😎

[emreyolcu]

Please let me know if you feel that Option 3 is better. I may be convinced otherwise 😎

I have no strong opinion either way, but I feel like this will stay a niche feature, so to me it makes most sense to keep things simple until someone raises an issue.

Thanks again for developing this package, I use it every day!

[pkryger]

Please let me know if you feel that Option 3 is better. I may be convinced otherwise 😎

I have no strong opinion either way, but I feel like this will stay a niche feature, so to me it makes most sense to keep things simple until someone raises an issue.

That's my guess too. Leaving as is.

Thanks again for developing this package, I use it every day!

I am glad you find it useful! Let me know if you have any questions or feature requests.