Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use deterministic Gerrit ChangeIds #29

Merged
merged 1 commit into from
Nov 18, 2020

Conversation

dlatypov
Copy link
Collaborator

Problem:
Non-deterministic change ids mean we get duplicate changes uploaded to
Gerrit for the same patch if the Bridge gets restarted.

When issue #14 is implemented, this will not matter as much, but there's
still a chance for updates not to be persisted in the DB.

Solution:
Use a deterministic change id, i.e. "I" + .

If we try and create a Gerrit change for the same patch, I'm assuming
the collision with the pre-existing change id will prevent us from doing
so.

Alternative:
As noted in the file, we could consider using a applypatch-msg hook.

But we can do more if we edit the commit directly from Python, since we
have access to more information, e.g. the lore link.
Add in a "Lore-Link" footer while we're here to demonstrate this.

This iteration of the code is essentially a direct translation of what I
had originally prototyped in bash for the hook.

@dlatypov dlatypov requested a review from bjh83 November 18, 2020 17:48
@dlatypov
Copy link
Collaborator Author

We don't seem to have a good way to test this.

Hacked up something locally.

Added a fake commit 984d0a560cfa

Ran a test that effectively does

    # Assume fake commit created out-of-band is HEAD
    fake_patch = Patch('message_id', 'text', 'headers', set_index=1, comments=[], change_id=None)
    gerrit_git._set_trailers(fake_patch)

    head = gerrit_git._git('show', 'HEAD')
    print(head)
    self.assertIn('Lore-Link', head)
    self.assertIn('Change-Id', head)

(Note: I had initially tried git commit --allow-empty in the python test, but then it would require git commit --amend --allow-empty as well)

commit 8586d45c65b2b4603e7fe5d1e902756f19b8ef68
Author: lkml-gerrit-bridge <willliu@google.com>
Date:   Wed Nov 18 10:09:25 2020 -0800

    Change-Id: I984d0a560cfa18c6ecb3bb79100f92468da70a8c
    Lore-Link: https://lore.kernel.org/linux-kselftest/essage_i

(Note: the truncation on the message id is expected with the current code. I don't remember why exactly it does this, but that's what lore_link() does)

So this seems to work afaict.

@dlatypov
Copy link
Collaborator Author

Ah Brendan brought up the fact that the parent commit still isn't stable.
So the applied patch's SHA won't be stable either.

If we had issue #20, then we'd respect base-commit and we'd have a stable SHA.
But not only is that still open (I forgot), it's not present on all messages (sadly), so we can't rely on it being stable.

We can instead do something like hash the message.
Will send out an updated version when I get the chance.

Problem:
Non-deterministic change ids mean we get duplicate changes uploaded to
Gerrit for the same patch if the Bridge gets restarted.

When issue google#14 is implemented, this will not matter as much, but there's
still a chance for updates not to be persisted in the DB.

Solution:
Use a deterministic change id, i.e. "I" + <commit sha of v1>.

If we try and create a Gerrit change for the same patch, I'm assuming
the collision with the pre-existing change id will prevent us from doing
so.

Alternative:
As noted in the file, we could consider using a `applypatch-msg` hook.

But we can do more if we edit the commit directly from Python, since we
have access to more information, e.g. the lore link.
Add in a "Lore-Link" footer while we're here to demonstrate this.

This iteration of the code is essentially a direct translation of what I
had originally prototyped in bash for the hook.
@dlatypov
Copy link
Collaborator Author

We can instead do something like hash the message.
Will send out an updated version when I get the chance.

Updated.

Now running the local hacky test, I get

commit 83e6f95773dc9ceb2d7f5081784fabf9d7031007
Author: lkml-gerrit-bridge <willliu@google.com>
Date:   Wed Nov 18 10:09:25 2020 -0800

    Change-Id: Iba01f0a3e0123651915008bc5786c42a4673d6cd
    Lore-Link: https://lore.kernel.org/linux-kselftest/essage_i

This caught a few errors, like the need to encode and to not forget to get the .hexdigest().
So I feel fairly confident that this should work as intended now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants