Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

let set diff-highlight=true work when the diff-highlight executable is not on $PATH #662

Merged
merged 7 commits into from
May 20, 2018
Merged

let set diff-highlight=true work when the diff-highlight executable is not on $PATH #662

merged 7 commits into from
May 20, 2018

Conversation

rolandwalker
Copy link
Contributor

diff-highlight lives under contrib and is probably not on the user's $PATH in most installs

@rolandwalker rolandwalker force-pushed the find-diff-highlight branch 6 times, most recently from fbe85da to abb6af7 Compare July 19, 2017 01:32
@jonas
Copy link
Owner

jonas commented Jul 26, 2017

I played around with this one and I really like the idea how I'd prefer that the lookup does an lstat to verify the location of the diff-highlight script. WDYT?

@rolandwalker
Copy link
Contributor Author

Manually finding the location would also have the advantage of being able to invoke diff-highlight when the package manager failed to create the executable (at some revision it became required to run make in the …/contrib/diff-highlight dir).

@rolandwalker rolandwalker force-pushed the find-diff-highlight branch 4 times, most recently from 22ee909 to 6a0e167 Compare August 2, 2017 12:08
@rolandwalker rolandwalker changed the title let set diff-highlight=true work when the diff-highlight executable is not on $PATH WIP let set diff-highlight=true work when the diff-highlight executable is not on $PATH Aug 3, 2017
@rolandwalker rolandwalker force-pushed the find-diff-highlight branch 3 times, most recently from 82b46dd to 26a8f6e Compare August 5, 2017 22:06
@rolandwalker
Copy link
Contributor Author

Revised, with probably too much refactor. Willing to squash as always.

  • executing git --exec-path was relatively expensive so the result is preserved. A few other things could realize similar savings.
  • due to caching, setting diff-highlight multiple times as was done in the tests no longer works. This is construed as a feature.
  • logic about how to invoke the diff-highlight utility seems unrelated to viewing diffs, so it was spun out into apps.c. If you don't like a new file, options.c would make more sense than diff.c.
  • The "rescue" scenario started in recent versions of git, and happens to me currently with git from Macports. The user lacks a shebanged diff-highlight executable unless an extra make step was done. But this can be trivially worked around by invoking perl directly. Unfortunately test coverage for this code path doesn't seem possible at the moment.

Handling ofdiff-highlight execution failure wants a few improvements, but was left unchanged.

@rolandwalker rolandwalker changed the title WIP let set diff-highlight=true work when the diff-highlight executable is not on $PATH let set diff-highlight=true work when the diff-highlight executable is not on $PATH Aug 6, 2017
@rolandwalker rolandwalker force-pushed the find-diff-highlight branch 2 times, most recently from d2b1405 to e9df2cc Compare August 7, 2017 15:30
@rolandwalker
Copy link
Contributor Author

valgrind doesn't like the final test case

@rolandwalker rolandwalker force-pushed the find-diff-highlight branch 2 times, most recently from 7bf0bce to ab712c3 Compare August 15, 2017 14:40
@rolandwalker
let set diff-highlight=true work when not on $PATH
f570546 
 * diff-highlight is probably not on the user's $PATH in most installs
 * find it relative to git --exec-path
 * also rescue the case where diff-highlight isn't a prepared executable
@rolandwalker
Copy link
Contributor Author

valgrind issue addressed

@jonas jonas merged commit f49fc7c into jonas:master May 20, 2018
@rolandwalker rolandwalker deleted the find-diff-highlight branch May 21, 2018 13:55
rolandwalker added a commit to rolandwalker/tig that referenced this pull request May 21, 2018
@rolandwalker
Let set diff-highlight=true work when the diff-highlight executable…
970c1b7 
… is not on $PATH (jonas#662)

* recast expand_path as path_expand
* recast SIZEOF_CMD as SIZEOF_MED_STR
* define _PATH_DEFPATH, conditionally incl paths.h
* path_search() generalized "which"
* stub apps.c for external applications
* let set diff-highlight=true work when not on $PATH
  * diff-highlight is probably not on the user's $PATH in most installs
  * find it relative to git --exec-path
  * also rescue the case where diff-highlight isn't a prepared executable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rolandwalker @jonas