Skip to content

Commit

Permalink
git: set deterministic change id and add lore link to commits
Browse files Browse the repository at this point in the history
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" + <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.
  • Loading branch information
dlatypov committed Nov 18, 2020
1 parent e67a514 commit 8c22b72
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 13 deletions.
39 changes: 30 additions & 9 deletions src/git.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
import re
import shutil
import subprocess
import tempfile
from message import lore_link
import message_dao
from patch_parser import Patch, Patchset
from absl import logging
Expand All @@ -36,6 +38,9 @@ class _Git(object):
def __init__(self, git_dir: str) -> None:
self._git_dir = git_dir

def __call__(self, verb: str, *args) -> str:
return _git(verb, *args, cwd=self._git_dir)

def clone(self, remote, *args) -> str:
return _git('clone', *args, '--', remote, self._git_dir)

Expand Down Expand Up @@ -72,8 +77,6 @@ def _parse_gerrit_patch_push(gerrit_result: str) -> str:
logging.info('change_id = %s', change_id)
return change_id

CURL_CHANGE_ID_CMD = 'curl -Lo `git rev-parse --git-dir`/hooks/commit-msg https://gerrit-review.googlesource.com/tools/hooks/commit-msg ; chmod +x `git rev-parse --git-dir`/hooks/commit-msg'

class GerritGit(object):
def __init__(self, git_dir: str, cookie_jar_path: str, url: str, project: str, branch: str) -> None:
self._git_dir = git_dir
Expand All @@ -85,10 +88,7 @@ def __init__(self, git_dir: str, cookie_jar_path: str, url: str, project: str, b
def shallow_clone(self, depth=1) -> str:
return self._git.clone(self._remote, '--depth', str(depth), '--single-branch', '--branch', self._branch)

def amend_commit(self) -> str:
return self._git.commit('--amend', '--no-edit')

def apply_patch(self, patch: Patch) -> str:
def _apply_patch(self, patch: Patch) -> str:
try:
return self._git.am(patch.text_with_headers)
except subprocess.CalledProcessError as e:
Expand All @@ -98,6 +98,28 @@ def apply_patch(self, patch: Patch) -> str:
_git('am', '--abort', cwd=self._git_dir)
raise

def _set_trailers(self, patch: Patch) -> str:
"""Assuming `patch` is HEAD, edits the commit message before we upload to Gerrit.
Note: normally, we'd rely on a applypatch-msg hook for this.
But we have more context here, namely the message id so we can include a
link to the original message in Lore.
Args:
patch: the patch we've just applied.
"""
commit_hash = self._git('rev-parse', 'HEAD').rstrip('\n')
original_message = self._git('log', '-1', '--format=%B').rstrip('\n')

change_id_trailer = f'Change-Id: I{commit_hash}'
lore_trailer = 'Lore-Link: ' + lore_link(patch.message_id)

with tempfile.NamedTemporaryFile(mode='wt') as f:
f.write(original_message)
self._git('interpret-trailers', '--in-place', '--if-exists=addIfDifferent',
'--trailer', change_id_trailer, '--trailer', lore_trailer, f.name)
return self._git.commit('--amend', '-F', f.name)

def push_changes(self) -> str:
try:
return self._git.push(f'HEAD:refs/for/{self._branch}%notify=NONE')
Expand All @@ -106,8 +128,8 @@ def push_changes(self) -> str:
raise

def push_patch(self, patch: Patch) -> Patch:
self.apply_patch(patch)
self.amend_commit()
self._apply_patch(patch)
self._set_trailers(patch)
gerrit_output = self.push_changes()
change_id = _parse_gerrit_patch_push(gerrit_output)
patch.change_id = change_id
Expand All @@ -120,7 +142,6 @@ def setup_git_dir(self, clone_depth=1) -> None:
self._git.config('user.name', '"lkml-gerrit-bridge"')
# TODO: Change config to use a service account instead of @willliu
self._git.config('user.email', '"willliu@google.com"')
subprocess.run(CURL_CHANGE_ID_CMD, cwd=self._git_dir, shell=True)

def cleanup_git_dir(self) -> None:
shutil.rmtree(self._git_dir)
Expand Down
8 changes: 4 additions & 4 deletions src/message.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ class Patchset(object):
def __init__(self) -> None:
self.patches = [] # type: List[Patch]

def lore_link(message_id: str) -> str:
return 'https://lore.kernel.org/linux-kselftest/' + message_id[1:-1]

class Message(object):
def __init__(self, id, subject, from_, in_reply_to, content, archive_hash) -> None:
self.id = id
Expand Down Expand Up @@ -61,12 +64,9 @@ def __str__(self) -> str:
def __repr__(self) -> str:
return str(self)

def lore_link(self) -> str:
return 'https://lore.kernel.org/linux-kselftest/' + self.id[1:-1]

def debug_info(self) -> str:
return (f'Message ID: {self.id}\n'
f'Lore Link: {self.lore_link()}\n'
f'Lore Link: {lore_link(self.id)}\n'
f'Commit Hash: {self.archive_hash}')

def parse_message_from_str(raw_email: str, archive_hash: str) -> Message:
Expand Down

0 comments on commit 8c22b72

Please sign in to comment.