Bug 164417 - [maintainer-update] [patch] devel/dulwich: Update to 0.8.3
Summary: [maintainer-update] [patch] devel/dulwich: Update to 0.8.3
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: Normal Affects Only Me
Assignee: Jason Helfman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-23 18:30 UTC by Marco Bröder
Modified: 2012-01-26 18:30 UTC (History)
0 users

See Also:


Attachments
file.diff (1.85 KB, patch)
2012-01-23 18:30 UTC, Marco Bröder
no flags Details | Diff
bugfix.patch (410 bytes, patch)
2012-01-25 09:53 UTC, Marco Bröder
no flags Details | Diff
patch.txt (474 bytes, text/plain; charset=us-ascii)
2012-01-25 17:12 UTC, Jason Helfman
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Marco Bröder 2012-01-23 18:30:06 UTC
Update to 0.8.3 release.

Tested: portlint, tinderbox build, pkg_add / pkg_delete, runtime

Thanks!

Fix: Patch attached with submission follows:
Comment 1 Jason Helfman freebsd_committer 2012-01-23 18:40:00 UTC
Responsible Changed
From-To: freebsd-ports-bugs->jgh

I'll take it.
Comment 2 dfilter service freebsd_committer 2012-01-24 21:51:45 UTC
jgh         2012-01-24 21:51:31 UTC

  FreeBSD ports repository

  Modified files:
    devel/dulwich        Makefile distinfo pkg-plist 
  Log:
  - Update to 0.8.3
  
  PR:     ports/164417
  Approved by:    rene (mentor)
  
  Revision  Changes    Path
  1.13      +4 -2      ports/devel/dulwich/Makefile
  1.12      +2 -2      ports/devel/dulwich/distinfo
  1.8       +6 -0      ports/devel/dulwich/pkg-plist
_______________________________________________
cvs-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/cvs-all
To unsubscribe, send any mail to "cvs-all-unsubscribe@freebsd.org"
Comment 3 Jason Helfman freebsd_committer 2012-01-24 21:51:54 UTC
State Changed
From-To: open->closed

Committed, with minor changes. Thanks!
Comment 4 Marco Bröder 2012-01-25 09:53:38 UTC
On Tuesday 24 January 2012 22:51:54 you wrote:
> Synopsis: [maintainer-update] [patch] devel/dulwich: Update to 0.8.3
> 
> State-Changed-From-To: open->closed
> State-Changed-By: jgh
> State-Changed-When: Tue Jan 24 21:51:54 UTC 2012
> State-Changed-Why:
> Committed, with minor changes. Thanks!
> 
> http://www.freebsd.org/cgi/query-pr.cgi?pr=164417


Hello,

thank you for taking the PR and committing!

May I ask you to revert these 'minor changes', please?
Reasons below. Patch attached.

First, the change was completely unnecessary.

Second, the loop is neither necessary nor efficient and just a waste of
resources in itself compared to the previous version.

Third, the 'cd ${WRKSRC}' is also in the loop and is executed for every
doc in PORTDOCS, so instead of just one directory change there are now
five of them. That is both wrong (bug) and a waste of resources.

Forth, the loop turns a simple and efficient oneliner into three lines,
which is unnecessary, explicitly unwanted and inefficient.

For the reasons above and the general goal to keep the Makefiles simple,
efficient and small, the change should be reverted to the previous
version, which is much better in every aspect and was explicitly
introduced for good reasons.

Thank you very much for your understanding!

Regards
Comment 5 Jason Helfman freebsd_committer 2012-01-25 17:12:06 UTC
PORTSDOC is only a list of documents, however it doesn't mean that proper
usage should not be taken into account for installation. Since this is a
list of files, it should not be used in cooperation with the "install"
binary in the manner it was used previously.

Please consider the attached patch, as a compromise.

-jgh

-- 
Jason Helfman         | FreeBSD Committer
jgh@FreeBSD.org       | http://people.freebsd.org/~jgh
Comment 6 Marco Bröder 2012-01-25 18:19:26 UTC
On Wednesday 25 January 2012 18:12:06 Jason Helfman wrote:
> PORTSDOC is only a list of documents, however it doesn't mean that proper
> usage should not be taken into account for installation. Since this is a
> list of files, it should not be used in cooperation with the "install"
> binary in the manner it was used previously.

Could you please explain what was wrong / why it should not be used that way? 
I would like to understand it.

The install(1) manual page states as synopsis:

install [options] file1 ... fileN directory

And since ${PORTDOCS} will be substituted with its content (files) before 
execution, it is just the same as above synopsis line. I do not know what is 
wrong with it. Did I missed something?

BTW, sunpoet@ introduced the oneliner in revision 1.7 more than a year ago. 
Since then none of the other committers complained about that line.

It is funny that one committer introduced it and another one reverts it. There 
should be some sort of agreement. Otherwise it is an uncomfortable situation 
for the maintainer.

> Please consider the attached patch, as a compromise.

If you actually insist on the for loop, then yes, please take that instead.

Thank you for your passion and time.

Regards
Comment 7 Chris Rees 2012-01-25 18:49:54 UTC
I'll jump in at this point, and point out that it was me who told
Jason that he should be using for loops in this way.

Sorry for the confusion here-- you're correct that it's a valid usage
in most cases.  The danger comes when people use something like:

PORTDOCS= bluurgh bluurgh2

${INSTALL_DATA} ${WRKSRC}/${PORTDOCS} ${DOCSDIR}

which expands to

${INSTALL_DATA} ${WRKSRC}/bluurgh bluurgh2 ${DOCSDIR}

I'm sure you can see the problem here.  The cd in this case is a nice
alternative method around that, and I'm sorry that that was missed--
please blame me and not Jason in this case.

Normally changes like this aren't worth the CVS noise, but I
understand that the principle of your requests as maintainer override
this; we could revert it now or on the next update.  I'll leave the
choice up to you.

Chris
Comment 8 Marco Bröder 2012-01-26 18:28:07 UTC
> Sorry for the confusion here-- you're correct that it's a valid usage
> in most cases. The danger comes when people use something like:
> 
> PORTDOCS= bluurgh bluurgh2
> 
> ${INSTALL_DATA} ${WRKSRC}/${PORTDOCS} ${DOCSDIR}
> 
> which expands to
> 
> ${INSTALL_DATA} ${WRKSRC}/bluurgh bluurgh2 ${DOCSDIR}
> 
> I'm sure you can see the problem here.

Ah, that is it. Thanks for your explanation!

> The cd in this case is a nice alternative method around that, and I'm sorry 
> that that was missed-- please blame me and not Jason in this case.

It is really not my intention to blame anyone. I know that you (all 
committers) do a pretty good job and I appreciate that very much. I just 
wanted to understand it, which I do now.

> Normally changes like this aren't worth the CVS noise,

Agreed, but ...

> but I understand that the principle of your requests as maintainer override
> this; we could revert it now or on the next update. I'll leave the
> choice up to you.

It would be nice if it could be done now. So could you please do it? I believe 
upstream will not release the next update anytime soon.

Many thanks!

Regards