bug#63261: Recent changes to git config cause errors for non-committers

bug#63261: Recent changes to git config cause errors for non-committers

[Brian Cully via Bug reports for GNU Guix]

I've run into two issues with the recent changes to git config 
integration:

1) All commits must now be signed, even if you're not a 
committer. This breaks just tons of things, including 
rebasing. I'm not sure how to fix this without just disabling that 
configuration line altogether.

2) Some ‘make’ rules now require git to be installed so that ‘git 
config’ can add ‘etc/git/gitconfig’ to the local 
configuration. So, for instance, ‘guix shell --pure -D guix -- 
make’ will now fail. Calls to git should be prefixed with a test 
to see if there is a git executable in the path.

-bjc

bug#63261: Recent changes to git config cause errors for non-committers

[Maxim Cournoyer]

Hi!

Thanks for the feedback.

Brian Cully <bjc@spork.org> writes:

> I've run into two issues with the recent changes to git config
> integration:
>
> 1) All commits must now be signed, even if you're not a
> committer. This breaks just tons of things, including rebasing. I'm
> not sure how to fix this without just disabling that configuration
> line altogether.

Could you elaborate on 'tons of things' ? :-)  And why does it break
rebasing?  I rebase my branches often while signing the commits, so I'll
need more details to understand the issue.

The idea was to distribute the basic configuration that makes
collaborating on Guix easier, by auto-configuring things that previously
were left to do manually.  By having non-committers also follow the
committers' flow, it also prepares them to become committers, if they
wish :-).

> 2) Some ‘make’ rules now require git to be installed so that ‘git
> config’ can add ‘etc/git/gitconfig’ to the local configuration. So,
> for instance, ‘guix shell --pure -D guix -- make’ will now fail. Calls
> to git should be prefixed with a test to see if there is a git
> executable in the path.

This one is a clear problem, for example causing issues to build a
release tarball of Guix where git shouldn't be a requirement.  It should
be easy to fix with a test as you suggest.

-- 
Thanks,
Maxim

bug#63261: [PATCH] Makefile.am: Only auto-configure Git when available.

[Maxim Cournoyer]

* Makefile.am (.git/hooks/pre-push): Only run recipe if the '.git' directory
exists.  Make it silent.
(.git/config): Likewise, and also check if the 'git' command is available.

Reported-by: Brian Cully <bjc@spork.org>
---
 Makefile.am | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 4a9124e0c2..8af69f88ac 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1115,10 +1115,14 @@ cuirass-jobs: $(GOBJECTS)
 
 # Git auto-configuration.
 .git/hooks/pre-push: etc/git/pre-push
-	cp etc/git/pre-push .git/hooks/pre-push
+	$(AM_V_at)if test -d .git; then \
+	cp etc/git/pre-push .git/hooks/pre-push; \
+	fi
 
 .git/config: etc/git/gitconfig
-	git config include.path ../etc/git/gitconfig
+	$(AM_V_at)if command -v git >/dev/null && test -d .git; then \
+	git config include.path ../etc/git/gitconfig; \
+	fi
 
 nodist_noinst_DATA = .git/hooks/pre-push .git/config
 

base-commit: ae6643326a983d141fd8320a755181d21e5a1f2f
-- 
2.39.2

bug#63261: Recent changes to git config cause errors for non-committers

[Maxim Cournoyer]

Hi,

Brian Cully via Bug reports for GNU Guix <bug-guix@gnu.org> writes:

> I've run into two issues with the recent changes to git config
> integration:
>
> 1) All commits must now be signed, even if you're not a
> committer. This breaks just tons of things, including rebasing. I'm
> not sure how to fix this without just disabling that configuration
> line altogether.
>
> 2) Some ‘make’ rules now require git to be installed so that ‘git
> config’ can add ‘etc/git/gitconfig’ to the local configuration. So,
> for instance, ‘guix shell --pure -D guix -- make’ will now fail. Calls
> to git should be prefixed with a test to see if there is a git
> executable in the path.

2. has been fixed (also by Ludovic, ah!).  I'm not sure a good solution
can be found for 1. but it seems reasonable to me that contributors
working on Guix learn to sign their commits with GnuPG; that way they
are on the right path to become a Guix committer, already getting up to
speed with the required practices.

Closing, but we can continue discussing it.

-- 
Thanks,
Maxim

bug#63261: Recent changes to git config cause errors for non-committers

[Leo Famulari]

Can someone help me by pointing to the original discussion of this
change?

bug#63261: Recent changes to git config cause errors for non-committers

[Maxim Cournoyer]

Hi Leo,

Leo Famulari <leo@famulari.name> writes:

> Can someone help me by pointing to the original discussion of this
> change?

It was the team.scm issue in https://issues.guix.gnu.org/58813 that led
to this change.

-- 
Thanks,
Maxim

Should commit signing always be required for local work? [was Re: bug#63261: Recent changes to git config cause errors for non-committers]

[Leo Famulari]

On Mon, May 15, 2023 at 09:38:33AM -0400, Maxim Cournoyer wrote:
> Hi Leo,
> 
> Leo Famulari <leo@famulari.name> writes:
> 
> > Can someone help me by pointing to the original discussion of this
> > change?
> 
> It was the team.scm issue in https://issues.guix.gnu.org/58813 that led
> to this change.

Thanks for pointing it out.

In my opinion, this change should be discussed more publicly so that a
consensus can be reached about it.

This change affects the workflow of everyone that hacks on Guix,
including people who are trying it for the very first time.

It's a big change.

It's not okay to only discuss such a big change in a ticket with the
title "can't substitute etc/teams.scm command as doc suggests".

I am asking for this to be reverted until a consensus is reached.

Re: Should commit signing always be required for local work? [was Re: bug#63261: Recent changes to git config cause errors for non-committers]

[Maxim Cournoyer]

Hi Leo,

Leo Famulari <leo@famulari.name> writes:

> On Mon, May 15, 2023 at 09:38:33AM -0400, Maxim Cournoyer wrote:
>> Hi Leo,
>> 
>> Leo Famulari <leo@famulari.name> writes:
>> 
>> > Can someone help me by pointing to the original discussion of this
>> > change?
>> 
>> It was the team.scm issue in https://issues.guix.gnu.org/58813 that led
>> to this change.
>
> Thanks for pointing it out.
>
> In my opinion, this change should be discussed more publicly so that a
> consensus can be reached about it.
>
> This change affects the workflow of everyone that hacks on Guix,
> including people who are trying it for the very first time.
>
> It's a big change.
>
> It's not okay to only discuss such a big change in a ticket with the
> title "can't substitute etc/teams.scm command as doc suggests".
>
> I am asking for this to be reverted until a consensus is reached.

Thanks for voicing your thoughts on this.  I originally thought the
included fragments (via the 'include.path' git config option) could we
overridden by a user but it seems they can't, making the change more
intrusive than it should have been.

I've reverted just the offending bits:

--8<---------------cut here---------------start------------->8---
diff --git a/etc/git/gitconfig b/etc/git/gitconfig
index 831fbce6e7..907ad01804 100644
--- a/etc/git/gitconfig
+++ b/etc/git/gitconfig
@@ -1,6 +1,3 @@
-[commit]
-        gpgsign = true
-
 [diff "scheme"]
        xfuncname = "^(\\(define.*)$"
 
--8<---------------cut here---------------end--------------->8---

Let me know if something is amiss.

-- 
Thanks,
Maxim

Re: Should commit signing always be required for local work? [was Re: bug#63261: Recent changes to git config cause errors for non-committers]

[Josselin Poiret]

[-- Attachment #1: Type: text/plain, Size: 872 bytes --]

Hi Maxim and Leo,

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

> Thanks for voicing your thoughts on this.  I originally thought the
> included fragments (via the 'include.path' git config option) could we
> overridden by a user but it seems they can't, making the change more
> intrusive than it should have been.

Just FYI, it seems that it was possible to override that behavior by
making sure the include was before the overriding gpgsign = false in the
repo's .git/config.  Still, it's probably best to let users judge what
their workflow should be.

I'm curious Leo, in general (not Guix because we have a pre-push hook),
how do you make sure you always publish signed commits?  I don't want to
put unsigned commits anywhere except locally, but it feels like I might
just forget to sign them before pushing.

Best,
-- 
Josselin Poiret

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 682 bytes --]

Re: Should commit signing always be required for local work? [was Re: bug#63261: Recent changes to git config cause errors for non-committers]

[Simon Tournier]

Hi Josselin,

On ven., 19 mai 2023 at 11:34, Josselin Poiret <dev@jpoiret.xyz> wrote:

> I'm curious Leo, in general (not Guix because we have a pre-push hook),
> how do you make sure you always publish signed commits?  I don't want to
> put unsigned commits anywhere except locally, but it feels like I might
> just forget to sign them before pushing.

Well, I am not Leo. :-) Maybe I misunderstand your question but usually
my file ~/.gitconfig contains my default; say always sign.  Then
locally, for some project [1], I set other options with the local file
.git/config of the repository.

And for the ones I do not want to sign locally but I will push signed, I
have pre-push hooks.  Note, in practise, I do not have such
configuration. :-)

1: https://gitlab.inria.fr/guix-hpc/website


Cheers,
simon

Re: Should commit signing always be required for local work? [was Re: bug#63261: Recent changes to git config cause errors for non-committers]

[Vagrant Cascadian]

[-- Attachment #1: Type: text/plain, Size: 1474 bytes --]

On 2023-05-19, Simon Tournier wrote:
> On ven., 19 mai 2023 at 11:34, Josselin Poiret <dev@jpoiret.xyz> wrote:
>> I'm curious Leo, in general (not Guix because we have a pre-push hook),
>> how do you make sure you always publish signed commits?  I don't want to
>> put unsigned commits anywhere except locally, but it feels like I might
>> just forget to sign them before pushing.
>
> Well, I am not Leo. :-) Maybe I misunderstand your question but usually
> my file ~/.gitconfig contains my default; say always sign.  Then
> locally, for some project [1], I set other options with the local file
> .git/config of the repository.
>
> And for the ones I do not want to sign locally but I will push signed, I
> have pre-push hooks.  Note, in practise, I do not have such
> configuration. :-)

This is basically a show-stopper for me working on guix right now. I
intentionally do not have access to my openpgp key on Guix System
machines. This completely breaks my workflow.

Neither changing ~/.gitconfig not .git/config in the working repository
seems to work around this.

I think the case can be made that not requiring signatures will actually
prevent unintentional changes from getting pushed to the archive, as the
server-side hooks will prevent unsigned changes from landing in the
repository... this is why I prefer to leave my local work-in-progress
changes unsigned. I only sign things I am confident I might want to
push.

Please revert ASAP.

live well,
  vagrant

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: Should commit signing always be required for local work? [was Re: bug#63261: Recent changes to git config cause errors for non-committers]

[Felix Lechner via Development of GNU Guix and the GNU System distribution.]

Hi Vagrant,

On Wed, May 24, 2023 at 2:01 PM Vagrant Cascadian <vagrant@debian.org> wrote:
>
> Please revert ASAP.

I am not affected, but if I understood Maxim's message from 5/18
correctly, the signature requirement was already reverted:

   https://git.savannah.gnu.org/cgit/guix.git/commit/?id=03b453cfe54756bcec6b7c7dfaf71566d84c7a75

Peace,
Felix

Re: Should commit signing always be required for local work? [was Re: bug#63261: Recent changes to git config cause errors for non-committers]

[Vagrant Cascadian]

[-- Attachment #1: Type: text/plain, Size: 1832 bytes --]

On 2023-05-24, Felix Lechner via "Development of GNU Guix and the GNU System distribution." wrote:
> On Wed, May 24, 2023 at 2:01 PM Vagrant Cascadian <vagrant@debian.org> wrote:
>>
>> Please revert ASAP.
>
> I am not affected, but if I understood Maxim's message from 5/18
> correctly, the signature requirement was already reverted:
>
>    https://git.savannah.gnu.org/cgit/guix.git/commit/?id=03b453cfe54756bcec6b7c7dfaf71566d84c7a75

True! Figured this out, but wow it was circuitous!

So, the very non-obvious behavior was triggered by using git worktrees
with a mix of updated and not very updated checkouts...

So I have ~/src/guix and a ~/src/guix-workspace which is a worktree from
~/src/guix...

When I ran "./configure && ./bootstrap ... && make" from
~/src/guix-workspace it installed a rule in ~/src/guix/.git/config:

  [include]
  path = ../etc/git/gitconfig

So there was no obvious entry for [commit] in the configuration
file... so on a glance I missed that. Manually adding:

  [commit]
  gpgsign=false

Did work around the problem.

My initial check for ~/src/guix-workspace/.git/config was wrong, as it
was a worktree and .git is a file... so it obviously did not show any
configuration at a non-resolvable path.

Also updating the git checkout at ~/src/guix fixed the issue, even
though I was *working* from ~/src/guix-workspace and my working
directory etc/git/gitconfig had the fix...

Having an [include] entry pointing to files that were not in my working
directory from some arbitrary checkout (I will reiterate my working
directory actually *did* have the fix!) kind of broke my mind a bit.

I get it now. I think. :)

This is not the first time I have been burned by using worktrees, but it
was certainly the most convoluted!


live well,
  vagrant

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: Should commit signing always be required for local work? [was Re: bug#63261: Recent changes to git config cause errors for non-committers]

[Leo Famulari]

[-- Attachment #1: Type: text/plain, Size: 1255 bytes --]

On Fri, May 19, 2023 at 11:34:35AM +0200, Josselin Poiret wrote:
> I'm curious Leo, in general (not Guix because we have a pre-push hook),
> how do you make sure you always publish signed commits?  I don't want to
> put unsigned commits anywhere except locally, but it feels like I might
> just forget to sign them before pushing.

In general, I don't rigorously sign Git commits for projects that aren't
Guix.

You could set "gpgsign = true" in '~/.gitconfig'.

I do sign commits sometimes for non-Guix projects, but without a
code-authentication system like Guix's, I don't perceive a strong reason
to always sign commits.

There is *some* reason to always sign commits, which is to provide an
unambiguous statement of authorship / provenance. But, it doesn't seem
like most projects have a mechanism with which to derive value from the
signatures. Also, it doesn't seem like there is much demand for this, in
general.

Git itself offers nothing, so each project has to design their own
solution. I doubt many projects would consider that effort to be
worthwhile. Instead they rely on the access controls of their
centralized repo, typically Github, and Github's security seems fine in
practice.

I think that Guix is pushing the state of the art here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]