Bug 241978 - Review Request: tig - Text-mode interface for git
Review Request: tig - Text-mode interface for git
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
:
: 241990 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-05-31 21:59 EDT by James Bowes
Modified: 2013-01-10 04:53 EST (History)
3 users (show)

See Also:
Fixed In Version: tig-0.17-1.el6
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-06-02 12:29:55 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description James Bowes 2007-05-31 21:59:57 EDT
Spec URL: http://jbowes.dangerouslyinc.com/tmp/tig.spec
SRPM URL: http://jbowes.dangerouslyinc.com/tmp/tig-0.7-1.fc7.src.rpm
Description: Tig is a git repository browser that additionally can act as a pager for output from various git commands.

When browsing repositories, it uses the underlying git commands to present the
user with various views, such as summarized revision log and showing the commit
with the log message, diffstat, and the diff.

Using it as a pager, it will display input from stdin and colorize it.
Comment 1 James Bowes 2007-05-31 22:04:33 EDT
rpmlint output:

$ rpmlint ~/rpmbuild/SRPMS/tig-0.7-1.fc7.src.rpm 
$ rpmlint ~/rpmbuild/RPMS/i386/tig-*
W: tig spurious-executable-perm /usr/share/man/man1/tig.1.gz
W: tig spurious-executable-perm /usr/share/man/man5/tigrc.5.gz

The man pages are installed 755, this has already been fixed upstream, so I can
patch before the next release, if needed.
Comment 2 Jason Tibbitts 2007-06-01 01:00:44 EDT
Looks good.  I do think a quick chmod of the manpages is in order just for the
sake of cleanliness, although I can't imagine how the weird permissions could
actually hurt anything.

The compiler is not called with the proper set of flags, which leads to a busted
debuginfo package among other things.  I believe it suffices to pass CFLAGS on
the make line:
   make CFLAGS='%{optflags}' %{?_smp_mflags} all doc-man

The only other suggestion I have is that while don't think there's anything
wrong with the summary and description you have, perhaps not everyone who sees
the summary or description will know what git is.  Maybe something like:

Summary: Text-mode interface for the git revision control system
%description
Tig is a repository browser for the git revision control system that...

Review:
* source files match upstream:
   ad611a9449ff9786892183874ee79255ab92e82b3d1251556cd1a0cd1224ee12  
   tig-0.7.tar.gz
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
O summary is OK (see comments above, not a blocker)
O description is OK ("")
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
* BuildRequires are proper.
X compiler flags are appropriate.
* %clean is present.
* package builds in mock (development, x86_64).
* package installs properly
X debuginfo package is mostly empty.
* rpmlint is silent.
* final provides and requires are sane:
   tig = 0.7-1.fc7
  =
   git-core
   libncurses.so.5()(64bit)
   libtinfo.so.5()(64bit)
* %check is not present (no upstream test suite) but package runs fine.
* no shared libraries are added to the regular linker search paths.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
X file permissions are appropriate (executable manpages)
* no scriptlets present.
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* no headers.
* no pkgconfig files.
* no static libraries.
* no libtool .la files.
* not a GUI app.
Comment 3 Jason Tibbitts 2007-06-01 02:02:06 EDT
*** Bug 241990 has been marked as a duplicate of this bug. ***
Comment 4 James Bowes 2007-06-01 07:15:25 EDT
(In reply to comment #2)
> Looks good.  I do think a quick chmod of the manpages is in order just for the
> sake of cleanliness, although I can't imagine how the weird permissions could
> actually hurt anything.
> 
> The compiler is not called with the proper set of flags, which leads to a busted
> debuginfo package among other things.  I believe it suffices to pass CFLAGS on
> the make line:
>    make CFLAGS='%{optflags}' %{?_smp_mflags} all doc-man
> 
> The only other suggestion I have is that while don't think there's anything
> wrong with the summary and description you have, perhaps not everyone who sees
> the summary or description will know what git is.  Maybe something like:
> 
> Summary: Text-mode interface for the git revision control system
> %description
> Tig is a repository browser for the git revision control system that...

This all sounds good. I've updated the spec with your suggestions. Thanks, Jason.

New locations:
Spec URL: http://jbowes.dangerouslyinc.com/tmp/tig.spec
SRPM URL: http://jbowes.dangerouslyinc.com/tmp/tig-0.7-2.fc7.src.rpm
Comment 5 Jeffrey C. Ollie 2007-06-01 11:35:54 EDT
A few of things that I did in my spec (none of them blockers):

1. Build the HTML documentation and list them in %doc.  After they are built
you'll need to convert CRLF line endings to LF line endings.

2. Include the manual.txt in %doc.

3. Added -DVERSION\\\"%{version}-%{release}\\\" to the CFLAGS.  That way tig
will print the rpm release when you type "tig --version".  Could help with
debugging down the road...

4. Use %{_mandir} and %{_prefix} instead of hard-coded paths in the "make
install" line.

Otherwise my spec and your spec are nearly identical.  Feel free to list me as a
co-maintainer once the package is approved.
Comment 6 James Bowes 2007-06-01 12:20:06 EDT
(In reply to comment #5)
> A few of things that I did in my spec (none of them blockers):
> 
> 1. Build the HTML documentation and list them in %doc.  After they are built
> you'll need to convert CRLF line endings to LF line endings.
> 
> 2. Include the manual.txt in %doc.
> 
> 3. Added -DVERSION\\\"%{version}-%{release}\\\" to the CFLAGS.  That way tig
> will print the rpm release when you type "tig --version".  Could help with
> debugging down the road...
> 
> 4. Use %{_mandir} and %{_prefix} instead of hard-coded paths in the "make
> install" line.
> 

I went ahead and updated the spec with these, with the exception of #3. It feels
kind of weird to me, but if you feel strongly about it, we can put it in later.

New locations:
Spec URL: http://jbowes.dangerouslyinc.com/tmp/tig.spec
SRPM URL: http://jbowes.dangerouslyinc.com/tmp/tig-0.7-3.fc7.src.rpm

> Otherwise my spec and your spec are nearly identical.  Feel free to list me as a
> co-maintainer once the package is approved.

Will do!
Comment 7 Jeffrey C. Ollie 2007-06-01 12:28:15 EDT
(In reply to comment #6)
> (In reply to comment #5)
> > 3. Added -DVERSION\\\"%{version}-%{release}\\\" to the CFLAGS.  That way tig
> > will print the rpm release when you type "tig --version".  Could help with
> > debugging down the road...
> 
> with the exception of #3. It feels
> kind of weird to me, but if you feel strongly about it, we can put it in later.

It's not a big deal to me, just something I picked up from the
upstream spec file.

Comment 8 Jason Tibbitts 2007-06-02 01:12:14 EDT
Looking good to me:

Symmary and %description look great now.
Compiler flags are OK and debuginfo package is complete.
Permissions of the manpages are OK.

The other additions are OK.  The included documentation is getting larger than
the program itself, but the whole thing's so tiny that it doesn't much matter.

APPROVED
Comment 9 James Bowes 2007-06-02 07:00:06 EDT
New Package CVS Request
=======================
Package Name: tig
Short Description: Text-mode interface for the git revision control system
Owners: jbowes@redhat.com, jeff@ocjtech.us
Branches: F-7
InitialCC: 
Comment 10 James Bowes 2007-06-02 07:00:41 EDT
(In reply to comment #8)
> Looking good to me:

Thanks for the review, Jason!
Comment 11 Jason Tibbitts 2007-06-02 11:58:43 EDT
CVS done.
Comment 12 Steve Traylen 2011-08-14 05:42:08 EDT
Package Change Request
======================
Package Name: tig
New Branches: el5 el6
Owners: stevetraylen

I asked for an EPEL branch in bug #722951 some time ago.

I will maintain these branches now.

Steve.
Comment 13 Jon Ciesla 2011-08-14 07:25:06 EDT
Git done (by process-git-requests).
Comment 14 Fedora Update System 2011-08-15 04:20:32 EDT
tig-0.17-1.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/tig-0.17-1.el6
Comment 15 Fedora Update System 2011-08-15 04:20:45 EDT
tig-0.17-1.el5.1 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/tig-0.17-1.el5.1
Comment 16 Fedora Update System 2011-08-31 18:55:44 EDT
tig-0.17-1.el5.1 has been pushed to the Fedora EPEL 5 stable repository.
Comment 17 Fedora Update System 2011-08-31 18:58:32 EDT
tig-0.17-1.el6 has been pushed to the Fedora EPEL 6 stable repository.

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