Re: [workflow] Automatically close bug report when a patch is committed

[Simon Tournier]

Hi Giovanni,

Before commenting, let me repeat. ;-)

        That’s said, I am not against the proposal.  I just have mixed
        feelings and before deploying I strongly suggest to review if
        the proposal covers the intent.


On Fri, 15 Sep 2023 at 09:16, Giovanni Biscuolo <g@xelera.eu> wrote:

> Before commenting, just let me repeat that we are _copying_ the
> 'Change-Id' idea (and related possible implementation issues) from
> Gerrit:
>
> https://gerrit-review.googlesource.com/Documentation/user-changeid.html
>
> This means that Somewhere™ in our documentation we should start
> explaining that:
>
> --8<---------------cut here---------------start------------->8---
>
> Our code review system needs to identify commits that belong to the same
> review.  For instance, when a proposed patch needs to be modified to
> address the comments of code reviewers, a second version of that patch
> can be sent to guix-patches@gnu.org.  Our code review system allows
> attaching those 2 commits to the same change, and relies upon a
> Change-Id line at the bottom of a commit message to do so.  With this
> Change-Id, our code review system can automatically associate a new
> version of a patch back to its original review, even across cherry-picks
> and rebases.
>
> --8<---------------cut here---------------end--------------->8---
>
> In other words, 'Change-Id' is /just/ metadata automatically added to
> help in code review **tracking**, specificcally helping "across
> cherry-picks and rebases" [1]
>
> Sorry if I'm repeating things probably already understood!

All this does not address my concern. :-)


>> 1. The hook must be installed.

[...]

> If this is stil not properly documented it will be fixed.

Maybe… and it will be another item in the already very long list of
steps to complete before contributing.  This is one of my concern: add
yet another thing to an already complicated process for contributing.


>> 2. The hook must not be in conflict with user configuration.
>
> I guess you mean not in conflict with other locally installed git hooks:
> since AFAIU we **already** have a locally installed git hook, this is
> already a requirement and this is something the user (contributor)
> should be aware of.

AFAIK, we have a pre-push hook.  This hook is only useful for the small
set of people who push the code.  And we can assume that this set of
people is skilled and are able to invest if something is unexpected.
Being Guix committer implies such commitment, IMHO.

Here, we are speaking for a Git hook applied to all.  That’s a different
situation, IMHO.

As an user and simple contributor, if I already have a global pre-commit
hook and then when I want to contribute to Guix, I potentially hit some
unexpected behaviour or even conflict, then as an user, either a. I give
up, drop and move my interest to something else than Guix, either b. I
spend some time for fixing this annoyance and that is pure friction (no
value for me and no value for the project).

> If this is stil not properly documented it will be fixed.

Yes, yet another thing to an already complicated process for
contributing.


>> 3. The generated Change-Id string can be mangled by some user unexpected
>>    action.
>
> Contributors and committers should not delete or change and already
> existing 'Change-Id', this will be documented.

Yes, yet another thing to an already complicated process for
contributing.


>> Many things can rail out on user side.  For an example, base-commit is
>> almost appearing systematically in submitted patches almost 3 years
>> later.
>
> I don't understand how this could impact the addition of the
> patch-tracking metadata named 'Change-Id'
>
>> The patches of some submissions are badly formatted.  Etc.
>
> I don't understand what is the problem of having a 'Change-Id' (in
> commit messages) in badly formatted patch submissions.

In these both example, I just pointed that we already have some
potential frictions.  And here, another one is added.

What I know from my experience at work and from the small experience
backed by my reading of help-guix or some bug reports is that:
robustness is hard in an environment you do not control.  More action we
add in that uncontrolled environment and weaker the robustness becomes;
and weak robustness means friction.


Again, I am not saying the proposal is not worth – I am following the
discussion with interest. :-) I am just trying to ask if this proposal
will really fix an concrete annoyance compared to the relative
complexity it adds, and if it is the right balance between the part and
the counter-part.

Cheers,
simon