Only configure git user if needed

[khardix]

There are multiple git behaviors linked to valid user configuration; one among them is the --signoff flag and/or format.signOff configuration. Always setting the user to rpm-build <rpm-build> can then cause git to emit nonsense lines, which can not be easily turned off just for the affected repositories.

Setting the user only if one is not already configured should give best of both worlds – git will not bail out on unknown user and the actual author of commits will be kept in Signed-off-By trailers.

---

I did not manage to run the test suite, so I'm hoping for CI to kick in. 🤞

[ffesti]

Not sure if this is really the right thing to do here. Many people will have a name and email set in their global git config. This change makes it impossible to overwrite that via macros. I can imagine people not wanting their general git settings ending up in there.

[pmatilai]

If you have proper git formatted patches, I think you'll have better luck with -S git_am instead of plain -S git, the latter is really only intended to get random patches under at least a little bit of version control.

[khardix]

Not sure if this is really the right thing to do here. Many people will have a name and email set in their global git config. This change makes it impossible to overwrite that via macros. I can imagine people not wanting their general git settings ending up in there.

Not sure what you mean. You can still set the GIT_COMMIT_AUTHOR envvar or the --author flag in the macros, and it will overwrite anything configured. It's actually the thing I'm counting on, since AFAIK all the other git-related macros specify the --author flag, so for the macros, the behavior should not even change.

If you have proper git formatted patches, I think you'll have better luck with -S git_am instead of plain -S git, the latter is really only intended to get random patches under at least a little bit of version control.

I'm using -S git_am. The thing I'm trying to fix manifests in 2 cases:

1. I'm making new commits on top of the already applied ones from scratch (i.e. distro-specific Makefile patches or similar, not-worth-upstreaming small changes) and then exporting them as new patches. These new patches are now From: rpm-build <rpm-build>, unless I remember to explicitly specify myself as author on every commit (which is definitely not part of my muscle memory).
2. I have, in my global config, format.signOff = true. Any patches I export from the git are now Signed-off-by: rpm-build <rpm-build>, which does not make a whole lot of sense. AFAIK this part cannot be changed with envvar, it always uses user.name and user.email.

---

In case it's relevant, this is my usual workflow when doing an RPM package update:

1. I have %autosetup -S git_am in spec, and run {fed,cent,rh}pkg prep (rpmbuild -bp).
2. In case of any issues, cd unpacked-source, manually adjust what's needed, git am --continue if I was adjusting existing patch or git commit when I needed new changes not importable from elsewhere.
3. Finally, if any changes were necessary, git format-patch -o.. master to re-export all the applied patches to dist-git.

[pmatilai]

Any patches I export from the git are now Signed-off-by: rpm-build , which does not make a whole lot of sense.

This is easy to agree with. And easist dealt with by adding --no-signoff to our git commit commands, it's the right thing to do really.

[khardix]

This is easy to agree with. And easist dealt with by adding --no-signoff to our git commit commands, it's the right thing to do really.

Unfortunately won't help me. ;-) It's not signing-off the patches when it makes them (am/commit), it's signing them off when I export them (format-patch) – and that is something outside of your control. Either I have to remember to add --no-signoff to my commands, or disable it globally.

Alternatively, if we want to stay with the rpm-build user in all cases, adding a line of git config format.signOff false to the init macro would also solve it, overwriting my global setup on the repo level. However, this could lead to a "whack-a-mole" situation when we will try to undo any possible global git option that would interact with the repo config in unexpected way. I was trying to move in the direction of least astonishment, keeping the repo-level configuration as close to any other git repo the user could have on their machine.

[pmatilai]

Oh, I didn't know format-patches would apply signoffs too. Seems kinda strange, but then in an email-based workflow...

In any case, this already is a case of whack-a-mole, the git usage inside rpmbuild is nothing like any normal repo usage is, so we keep running into these wrinkles every now and then. This sure isn't the first, nor the last.

But you said you're using git_am. In which case rpm really shouldn't override the author at all. So that seems like the real bug here. The non-am version was added first and there the user/email simply isn't expected to exist and rpm has to put something there, whereas with git am the user info has to be there already. So I think the right thing to do, really, is to separate the setup of these two different forms. Right now, git_am setup simply calls git setup.

[khardix]

@pmatilai Oh, now I think I'm getting the problem @ffesti was worried about – when using just -S git, anyone with properly configured git would end up being marked as the author of the non-git patches, whether they like it or not – and there is nothing they can easily do to switch back to using rpm-build <rpm-build>. Thanks for the explanation.

Based on the feedback, I'm turning this PR into a draft; let me try to implement the setup split for git and git_am.

[khardix]

So, the attempt is here. I'm splitting the original setup macros into several named steps, then combining them back as needed. I'm not 100% satisfied with the names, but I figured they are good enough to throwing the implementation up for discussion.

[dmnks]

Looks good to me overall, two suggestions though:

1. Four helper macros seems way too many, maybe just merge __scm_configure_git_common to __scm_init_git?
2. Split the commit into two: one that only does the macro split and another one that replaces __scm_configure_git_am with %{nil}

[dmnks]

Thinking about it again, 2. might not be worth the trouble (you'd need to duplicate the macro body in the first commit) perhaps, so take that with a pinch of salt 😄

[dmnks]

3. In either case, the commit message may need a more accurate subject line. That is, it's not just splitting the macros, but more importantly, fixing the git-am authorship issue.

[khardix]

Since I was going back to reword the commit, it was not that hard to split it into two. Let me know if this looks good to you.