Bug 241978

Summary: Review Request: tig - Text-mode interface for git
Product: [Fedora] Fedora Reporter: James Bowes <jbowes>
Component: Package ReviewAssignee: Jason Tibbitts <j>
Status: CLOSED ERRATA QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: bkearney, jeff, steve.traylen
Target Milestone: ---Flags: j: fedora-review+
gwync: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: tig-0.17-1.el6 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2007-06-02 16:29:55 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description James Bowes 2007-06-01 01:59:57 UTC
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-06-01 02:04:33 UTC
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 05:00:44 UTC
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 06:02:06 UTC
*** Bug 241990 has been marked as a duplicate of this bug. ***

Comment 4 James Bowes 2007-06-01 11:15:25 UTC
(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 15:35:54 UTC
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 16:20:06 UTC
(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 16:28:15 UTC
(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 05:12:14 UTC
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 11:00:06 UTC
New Package CVS Request
=======================
Package Name: tig
Short Description: Text-mode interface for the git revision control system
Owners: jbowes, jeff
Branches: F-7
InitialCC: 

Comment 10 James Bowes 2007-06-02 11:00:41 UTC
(In reply to comment #8)
> Looking good to me:

Thanks for the review, Jason!

Comment 11 Jason Tibbitts 2007-06-02 15:58:43 UTC
CVS done.

Comment 12 Steve Traylen 2011-08-14 09:42:08 UTC
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 Gwyn Ciesla 2011-08-14 11:25:06 UTC
Git done (by process-git-requests).

Comment 14 Fedora Update System 2011-08-15 08:20:32 UTC
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 08:20:45 UTC
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 22:55:44 UTC
tig-0.17-1.el5.1 has been pushed to the Fedora EPEL 5 stable repository.

Comment 17 Fedora Update System 2011-08-31 22:58:32 UTC
tig-0.17-1.el6 has been pushed to the Fedora EPEL 6 stable repository.