Bug 1550251 - diff-highlight needs building as part of the git package build
Summary: diff-highlight needs building as part of the git package build
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: git
Version: 27
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Todd Zullinger
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2018-02-28 20:40 UTC by Richard Fearn
Modified: 2018-06-01 12:19 UTC (History)
7 users (show)

Fixed In Version: git-2.14.4-1.fc27
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-06-01 12:19:50 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)

Description Richard Fearn 2018-02-28 20:40:08 UTC
Description of problem:

Since git 2.13.3, diff-highlight needs to be built (see https://github.com/git/git/commit/0c977dbc8180892af42d7ab9235fd3e51d6c4078):

  make -C contrib/diff-highlight/

The spec file currently cleans up unnecessary files in other contrib subdirectories, so it could do that for diff-highlight, too:

  # Clean up contrib/diff-highlight to avoid cruft in the git-core-doc docdir
  rm -rf contrib/diff-highlight/{DiffHighlight.pm,Makefile,diff-highlight.perl,shebang.perl,t}

The only remaining problem is that the spec removes the executable bit from every file in contrib. This means the packaged diff-highlight script won't be executable by default, which would be annoying. This might help:

  chmod a+x contrib/diff-highlight/diff-highlight

That would mean git-core-doc would contain an executable, though.

Another possibility might be to put it /usr/libexec/git-core - perhaps in a new subdirectory, /usr/libexec/git-core/contrib, or even its own subdirectory of that, /usr/libexec/git-core/contrib/diff-highlight. Then it could be included in the git-core package.

Comment 1 Todd Zullinger 2018-03-16 04:29:57 UTC
Hi Richard,

I added diff-highlight in my local builds, which should eventually reach fedora.  You can test this COPR: https://copr.fedorainfracloud.org/coprs/tmz/git/ (this is currently 2.17.0.rc0).

The commit adding this is on the devel branch of my git fork:

    https://src.fedoraproject.org/fork/tmz/rpms/git/c/9bcb66d7

> The only remaining problem is that the spec removes the executable bit from
> every file in contrib. This means the packaged diff-highlight script won't
> be executable by default, which would be annoying. This might help:
> 
>   chmod a+x contrib/diff-highlight/diff-highlight
> 
> That would mean git-core-doc would contain an executable, though.

Yeah, we explicitly remove the exec bit from files in the docs, to avoid the rpm dependency generators from pulling in unwanted requirements found in the example scripts.

> Another possibility might be to put it /usr/libexec/git-core - perhaps in a
> new subdirectory, /usr/libexec/git-core/contrib, or even its own
> subdirectory of that, /usr/libexec/git-core/contrib/diff-highlight. Then it
> could be included in the git-core package.

We also don't want any perl dependencies in git-core.  Removing the perl dependency was the initial motivation for splitting git-core from git. :)

Instead, I've installed diff-highlight in /usr/share/git-core/contrib/diff-highlight.  I used /usr/share/git-core/contrib for two reasons: 1) diff-highlight is just a perl script, so /usr/share seems more appropriate, and 2) we already have other contrib bits under /usr/share/git-core.  I think that's a reasonable location (unless diff-highlight is rewritten in C and still left in contrib, which seems unlikely).

It could certainly fit in /usr/libexec/git-core/contrib as well, but that would mean adding another location for a contrib dir and I thought it seemed more likely to be a cause of confusion than it was worth.  (I could be wrong about all this.  But that was my thought process, in any case. ;) )

The documentation is still at /usr/share/doc/git/contrib/diff-highlight/README.  Adjusting the paths from the README, you can try out the diff-highlight program with:

    git log -p --color | /usr/share/git-core/contrib/diff-highlight

Or make it the default by setting pager.{diff,log,show} in .gitconfig, e.g.:

    [pager]
	    log = /usr/share/git-core/contrib/diff-highlight | less
	    show = /usr/share/git-core/contrib/diff-highlight | less
	    diff = /usr/share/git-core/contrib/diff-highlight | less

If you can test this and let me know if you run into any issues, it would be most appreciated.

Comment 2 Richard Fearn 2018-03-17 08:33:27 UTC
Thanks very much Todd for looking at this! I will try it out and report back.

Comment 3 Richard Fearn 2018-03-21 23:23:18 UTC
Yep, this works well. I think the choices you've made are fine. diff-highlight itself is in the git package, and its README is in git-core-doc.

I did notice that git-core-doc also includes DiffHighlight.pm (i.e. it's not removed by the "clean up to avoid cruft" part of the spec), but that might have been intentional. Doesn't affect diff-highlight itself, though.

Would it be possible to apply this change to the F27/F28 packages, as well as rawhide? I've cherry-picked your commit to the f27 branch (currently 2.14.3) and built git using mock; that too works fine.

Thanks a lot!

Comment 4 Todd Zullinger 2018-03-22 02:17:18 UTC
Thanks for testing Richard!

Indeed it was intentional that DiffHighlight.pm is left in the doc dir.  That makes it easier for someone to discover, as that was split out of the script in order to allow others to re-use it.  I don't know if eventually it will end up installed in the perl-Git module tree or not.

I've got 2.17.0-rc1 building for f28 now, as I intend to push that before f28 is released.  (It's just waiting on a really slow -- or hung -- ppc64le builder now.)

I have been considering updating f27 to the current git release as well.  For a while we'd been keeping the same git release for an entire release, but I'm personally in favor of rebasing that at least every so often during a release cycle.  The git community does a very good job of not making breaking changes, so I think we'd be better off updating git in the current fedora releases to pick up the smaller bug fixes and improvements which don't get applied to a maintenance release.

I have also been keeping a COPR for builds from the master/rawhide branch for EPEL and Fedora at https://copr.fedorainfracloud.org/coprs/g/git-maint/git/.  I'll probably wait until 2.17.0 is final before updating that, which should be in the next few weeks.

Comment 5 Fedora Update System 2018-03-22 03:29:54 UTC
git-2.17.0-0.1.rc1.fc28 has been submitted as an update to Fedora 28. https://bodhi.fedoraproject.org/updates/FEDORA-2018-4171b66b98

Comment 6 Fedora Update System 2018-03-22 14:14:07 UTC
git-2.17.0-0.1.rc1.fc28 has been pushed to the Fedora 28 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-4171b66b98

Comment 7 Fedora Update System 2018-03-22 21:55:47 UTC
git-2.17.0-0.1.rc1.fc28.1 has been submitted as an update to Fedora 28. https://bodhi.fedoraproject.org/updates/FEDORA-2018-4171b66b98

Comment 8 Fedora Update System 2018-03-23 14:43:40 UTC
git-2.17.0-0.1.rc1.fc28.1 has been pushed to the Fedora 28 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-4171b66b98

Comment 9 Richard Fearn 2018-03-23 18:41:35 UTC
> Indeed it was intentional that DiffHighlight.pm is left in the doc dir. 
> That makes it easier for someone to discover, as that was split out of the
> script in order to allow others to re-use it.

I thought that would be the case.

> I have been considering updating f27 to the current git release as well. 
> For a while we'd been keeping the same git release for an entire release,
> but I'm personally in favor of rebasing that at least every so often during
> a release cycle.  The git community does a very good job of not making
> breaking changes, so I think we'd be better off updating git in the current
> fedora releases to pick up the smaller bug fixes and improvements which
> don't get applied to a maintenance release.

Whether F27 is updated to 2.17, or stays with 2.14, I'd be very happy to see the diff-highlight change applied to the F27 package, too.

In the meantime, I'm testing the F28 2.17 packages you submitted, but on F27.

Thanks a lot!

Comment 10 Fedora Update System 2018-03-27 18:18:50 UTC
git-2.17.0-0.1.rc1.fc28.2 has been submitted as an update to Fedora 28. https://bodhi.fedoraproject.org/updates/FEDORA-2018-4171b66b98

Comment 11 Fedora Update System 2018-03-27 23:22:47 UTC
git-2.17.0-0.1.rc1.fc28.2 has been pushed to the Fedora 28 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-4171b66b98

Comment 12 Fedora Update System 2018-03-28 22:35:17 UTC
git-2.17.0-0.2.rc2.fc28 has been submitted as an update to Fedora 28. https://bodhi.fedoraproject.org/updates/FEDORA-2018-be5660a0d9

Comment 13 Fedora Update System 2018-03-29 13:57:11 UTC
git-2.17.0-0.2.rc2.fc28 has been pushed to the Fedora 28 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-be5660a0d9

Comment 14 Fedora Update System 2018-04-02 22:38:07 UTC
git-2.17.0-1.fc28 has been submitted as an update to Fedora 28. https://bodhi.fedoraproject.org/updates/FEDORA-2018-be5660a0d9

Comment 15 Fedora Update System 2018-04-03 15:56:31 UTC
git-2.17.0-1.fc28 has been pushed to the Fedora 28 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-be5660a0d9

Comment 16 Fedora Update System 2018-04-11 23:01:56 UTC
git-2.17.0-1.fc28 has been pushed to the Fedora 28 stable repository. If problems still persist, please make note of it in this bug report.

Comment 17 Fedora Update System 2018-05-28 01:13:30 UTC
git-2.14.3-4.fc27 has been submitted as an update to Fedora 27. https://bodhi.fedoraproject.org/updates/FEDORA-2018-080a3d7866

Comment 18 Fedora Update System 2018-05-28 14:24:11 UTC
git-2.14.3-4.fc27 has been pushed to the Fedora 27 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-080a3d7866

Comment 19 Fedora Update System 2018-05-29 19:23:27 UTC
git-2.14.4-1.fc27 has been submitted as an update to Fedora 27. https://bodhi.fedoraproject.org/updates/FEDORA-2018-080a3d7866

Comment 20 Fedora Update System 2018-05-30 13:41:06 UTC
git-2.14.4-1.fc27 has been pushed to the Fedora 27 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-080a3d7866

Comment 21 Fedora Update System 2018-06-01 12:19:50 UTC
git-2.14.4-1.fc27 has been pushed to the Fedora 27 stable repository. If problems still persist, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.