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.)

[pkryger]

Thank you for the reporting and the contribution!

I have been completely unaware of the issues with fringe-columns. I think the fix you provided is almost good to go - if you don't mind just following up on the comment I left there and updating related tests (you can run make test for that - see Makefile/and or test.yml for more options) I think it would be good to go.

As for the difftastic-difft-environment - I like the idea and I am willing to include it in some form. However, I think this one will require a bit more work. Besides the documentation comment, I'd like to discuss the following:

1. I'd appreciate if you add tests for the new functionality. These could be a new tests or just a couple extra assertions in existing tests focusing on difftastic--build-git-process-environment.
2. I think some update will be needed to difftastic-rerun to ensure difftastic-difft-environment value is included in subsequent difft runs (e.g., when user types g in a difftastic-mode buffer). This again will need test updates to ensure this won't slip through cracks in future.
3. I'd appreciate if you can change other difftastic commands respect the value of difftastic-difft-environment as well. E.g. difftastic-files doesn't use difftastic--build-git-process-environment. This would apply to the rerun functionality, and have I already mentioned tests 😉.
4. Having said that, I am planning to refactor the git related commands to use -c argument (see manual. This is in a hope of simplifying the rerun functionality. However I am not sure when I can get around to to that (weeks? months?). And with the difftastic-difft-environment it seems that both git related commands as well as the the non-git related commands would need to take care of managing process-environment. I am not asking you to do anything here - just to keep that in mind when doing necessary changes.

[pkryger]

I think this deserves a mention in documentation. Would you mind adding a sentence or two to either section Usage or section Customization? Perhaps mentioning intended use, that is the temporary binding?

Please note the comments in Documentation Authoring or CI will complain. But if you struggle setting it up, I can take care of it.

[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 668991e on emreyolcu:main
into c51d996 on pkryger:main.

[pkryger]

Are you willing to add necessary documentation changes in this PR? Or perhaps you'd rather have it merged and log a new issue stating that difftastic-difft-environment doesn't work with non-git commands (perhaps list them) and that tests and documentation needs to be added? Or perhaps, you'd rather update this PR to only focus on the width issue, and submit a new one for the environment bit?

I approved the CI run and it's all green. I wouldn't be concerned about the Coveralls. Changes decreased number of testable lines, and not all code is tested under every Emacs version, resulting in per-version coverage drop. But overall the coverage stayed at 100%.

I can see that window-max-chars-per-line takes FACE argument. Which lead me to asking should a default face be used. I don't think I know about frame faces enough, but I wonder about a case where a current window face's font width (is this the right terminology?) is some non-standard width (e.g., some fancy text-buffer), then what would the width of a newly created window be? I don't expect to answer, if you don't have it at hand. Just wondering.

Please let me know WDYT about the other comments as well.

[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]

I've pushed my changes. Could you let me know whether you have any other feedback?

Thank you for these updates. I am happy to merge (with the fix in other comment), but would like to hear your opinion on how difftastic-rerun should work with difftastic-difft-environment.

Currently difftastic-rerun scrapes the current value of difftastic-difft-environment. However, if the variable is only let-bound when command executes, e.g.

(let ((difftastic-difft-environment '("TEST_VAR=test-value")))
  (difftastic-files "file-A" "file-B"))

then difft run by difftastic-files' will use it. However, when - in the difftasticbuffer user presses <kbd>g</kbd> (a.k.a.,difftastic-rerun), the TEST_VARwill not be present in thedifft` process environment spawned by the rerun command.

I think sticking a value of difftastic-difft-environment in difftastic--metadata under a new key (say, difft-environment) and then using it in difftastic--rerun may be a good idea. Likely just add relevant line in sentinel lambdas in difftastic--git-with-difftastic and difftastic--files-internal.

What I am not sure is how `difftastic-rerun' semantic should work exactly. At least the following comes to my mind:

1. Should the value from metadata override current value of difftastic-difft-environment?
2. Should the value of difftastic-difft-environment override the value stored in metadata?
3. Should the value of difftastic-difft-environment be prepended to the value stored in metadata?
4. Should the value of difftastic-difft-environment be appended to the value stored in metadata?

My naïve thinking is that it should be one of options 2 or 3. Or perhaps have a configuration that allows user to select what they want to do? This sounds like an overkill. Since you're the primary consumer of this functionality, you may know a better solution.

I also don't mind if you'd like to directly apply any fixes on top of either commit.

I have only tried the proposed fix. Perhaps I will have some time to work on this in the coming week. Please let me know if you prefer one of the semantics above.

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.

Thank you for the explanation. Makes sense.

RC mostly for the fix in test.

[pkryger]

Suggested change

	=> '("sh" "-c" "echo -n $TEST_VAR")))
	=> '("sh" "-c" "printf '%s' $TEST_VAR")))

I have checked, this one works on macOS, see fix and CI execution

[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.