Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 7 Apr 2015 02:32:04 +0000
From:      Alexey Dokuchaev <danfe@FreeBSD.org>
To:        Adam Weinberger <adamw@adamw.org>
Cc:        Sunpoet Po-Chuan Hsieh <sunpoet@FreeBSD.org>, svn-ports-head@freebsd.org, svn-ports-all@freebsd.org, ports-committers@freebsd.org
Subject:   Re: svn commit: r383472 - head/audio/muse
Message-ID:  <20150407023204.GA44784@FreeBSD.org>
In-Reply-To: <91AB85D3-A8DE-491C-A2D7-4E8D7E1CDC12@adamw.org>
References:  <201504061859.t36IxK0v000969@svn.freebsd.org> <20150407012902.GA22994@FreeBSD.org> <91AB85D3-A8DE-491C-A2D7-4E8D7E1CDC12@adamw.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Apr 06, 2015 at 08:07:33PM -0600, Adam Weinberger wrote:
> > On Mon, Apr 06, 2015 at 06:59:20PM +0000, Sunpoet Po-Chuan Hsieh wrote:
> >> New Revision: 383472
> >> URL: https://svnweb.freebsd.org/changeset/ports/383472
> >> 
> >> [...]
> >> @@ -52,6 +49,6 @@ post-patch:
> >> 
> >> post-install:
> >> 	@${MKDIR} ${STAGEDIR}${DOCSDIR}
> >> -	${INSTALL_DATA} ${PORTDOCS:S,^,${WRKSRC}/,} ${STAGEDIR}${DOCSDIR}
> >> +	(cd ${WRKSRC} && ${INSTALL_DATA} ${PORTDOCS} ${STAGEDIR}${DOCSDIR})
> > 
> > Please do not change working and readable code into equivalent less-readable
> > one.
> 
> ${PORTDOCS:S,^,${WRKSRC}/,}

This construct is known well enough (and used throughout the tree often
enough) to be immediately recognizable and understood.  It is nicely concise
yet still readable: using variable substitution does not make it cryptic,
in this particular case.

> is not readable. Especially not compared to the pseudo-English statement
> into which sunpoet expanded it:
> 
> cd ${WRKSRC} && ${INSTALL_DATA} ${PORTDOCS}

I'm pretty sure it didn't come from sunpoet@.  Kato is notoriously known for
including non-functional changes in his PRs alongside with real changes to a
port.  Particularly, Kato often touches working code and replaces it with
his own [inferior, bogus, uncalled for] version(s).

As I've previously had explained, "cd ${WRKSRC} && ${INSTALL_DATA}" is two-
command construct (vs. single, $cwd-agnostic command), it typically gets
longer and thus can cause line wrapping, but most importantly that it should
not have been committed in the first place as being gratuitous change.

> You're not getting much attention when you are choosing a different thing
> to care about each day.  Pick just one or two topics that are most
> important to you, and work to get those behaviors fixed.

What if everything is important?  Frankly, I'd like to have a better way to
"get those behaviors fixed" and stop micro-managing, but don't see a viable
alternative to reviewing commits (except, perhaps, improving the PHB, but
that's orthogonal to peer reviews and given how often it is being violated,
it's highly unlikely that it would replace them in a foreseeable future).

./danfe



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150407023204.GA44784>