Re: [libvirt] [PATCH v3 2/6] tests: add targets for building libvirt inside docker containers
On Wed, 2019-04-03 at 11:44 +0100, Daniel P. Berrangé wrote:
On Mon, Apr 01, 2019 at 01:03:21PM +0200, Andrea Bolognani wrote:
> On Mon, 2019-04-01 at 11:39 +0100, Daniel P. Berrangé wrote:
> > On Fri, Mar 29, 2019 at 05:39:59PM +0100, Andrea Bolognani wrote:
> > > On Wed, 2019-03-27 at 17:10 +0000, Daniel P. Berrangé wrote:
> > > > It is neccessary to disable the gnulib submodule commit check
because
> > > > this fails due to the way we have manually cloned submodule repos as
> > > > primary git repos with their own .git directory, instead of letting
> > > > git treat them as submodules in the top level .git directory.
> > > >
> > > > make[1]: Entering directory '/src/build'
> > > > fatal: Not a valid object name origin
> > > > fatal: run_command returned non-zero status for .gnulib
> > > > .
> > > > maint.mk: found non-public submodule commit
> > > > make: *** [/src/maint.mk:1448: public-submodule-commit] Error 1
> > >
> > > This last part is interesting for people looking at the code but not
> > > for users, so I'd leave it out of the commit message.
> >
> > It is important as it shows the maint.mk rule that is causing
> > the problem we are fixing. Without that there's not enough context
> > to undestand the problem.
>
> My point is that this is really only interesting to people hacking
> on Makefile.ci, and it's just noise to all other developers going
> through the git log. So we should not drop the information, just
> move it to a comment inside Makefile.ci, possibly with the shell
> output snipped for brevity.
I really disagree. I want the commit messages to be more verbose so
that details of what's being fixed are clearly visible. If you don't
care about this detail fine, just ignore it, but that's not a reason
to purge cut useful info out of the commit messages.
You're not really fixing anything with this commit, you're
introducing a new feature and presenting its use and motivation
behind it - up until the point where you spend a dozen lines to
explain in detail why a certain implementation details is needed.
This rationale should be in the code rather than in the commit
message, so that the next person wondering what the gl_foo thingy is
there for will be able to learn that without having to dig through
the git history.
At the same time, someone scrolling through the git history might be
interested in the new cool feature you're adding, but will most
likely not care about the implementation detail.
It's also worth noting that you didn't include this detail in the
commit messages for the first two revisions, so clearly you didn't
deem it as critical until I asked about it during review ;)
Anyway, if after reading the above you still want to keep it in
that's fine: I think both my time and yours are better used on
something else than arguing further about this :)
--
Andrea Bolognani / Red Hat / Virtualization