Page MenuHomePhabricator

Force `PhabricatorRepositoryGitCommitMessageParserWorker` to be executed when changing commit from non permanent to permanent which ensures that corresponding Differential Revision is closed
Needs ReviewPublic

Authored by artms on Thu, Oct 31, 3:03 PM.

Details

Reviewers
epriestley
Pawka
Group Reviewers
Blessed Reviewers
Summary

This looks like a regression, without only option - PhabricatorRepositoryGitCommitMessageParserWorker task is skipped because worker thinks that commit is already properly processed

Test Plan
  1. Create diff and push that change into non permanent ref, make sure diff doesn't contain references to diff (Differential_Revision: xx)
  2. Push that commit to permanent ref
  3. Phabricator parses commit and actually closes corresponding Differential Revision and not only marks commit as published

Diff Detail

Repository
rP Phabricator
Branch
only
Lint
Lint OK
Unit
Unit Tests Skipped
Build Status
Buildable 23607
Build 32448: Run Core Tests
Build 32447: arc lint + arc unit

Event Timeline

artms created this revision.Thu, Oct 31, 3:03 PM
artms requested review of this revision.Thu, Oct 31, 3:04 PM

Phabricator parses commit and actually closes corresponding Differential Revision and not only marks commit as published

This is expected and (I believe) desired.

The workflow it corresponds to is:

  • Alice sends her changes for review.
  • Alice pushes her changes to tmp-alice-bugfix-123 (which is not a permanent ref) to "back them up" or "share them". This should not close the associated revision.
  • Later, Alice lands her changes to master (which is a permanent ref). This should close the associated revision.

Oh, sorry, I think I misunderstood what you're describing in your test plan -- you're saying that you're currently observing the revision not closing, and believe this fixes it. I'll see if I can reproduce this.

artms added a comment.EditedThu, Oct 31, 4:32 PM

Oh, sorry, I think I misunderstood what you're describing in your test plan -- you're saying that you're currently observing the revision not closing, and believe this fixes it. I'll see if I can reproduce this.

Yeap, it marks commit as published but doesn't close corresponding diffs and tasks, it looks like it is not affecting if there is metadata in commit message itself. If we again forcibly (via repository reparse) ask to parse commit - diffs are closed. I observed that same message parsing is executed twice(during commit of change to master which previously was on pushed branch), one doesnt close tasks because it thinks commit is on non permanent ref and second one is not executed because commit is alrrady partly processed by first parsing...

artms added a comment.Thu, Oct 31, 4:38 PM

I will try to provide a script to reproduce flow one of our tools does while creating this regresion some time later today/tonight.

artms added a comment.Thu, Oct 31, 8:58 PM

Reproduction script for some working checked out repository on master and repository permanentRefs is master:

head -c 20 /dev/random | base64 > giberish
git add -A
git commit -m 'Add giberish'
HEAD=$(git rev-list --reverse HEAD~..HEAD)
title=$(git log -n1 --pretty=format:%s HEAD)
title_q=$(printf '%s' "$title" | jq --raw-input --slurp .)
rawdiff=$(git diff-tree --root --cc --no-commit-id HEAD|jq --raw-input --slurp .)
diffid=$(echo "{\"diff\":${rawdiff}}" | arc call-conduit differential.createrawdiff | jq -e -r .response.id )
revid=$(echo "{\"diffid\": $diffid, \"fields\":{\"title\": $title_q}}" | arc call-conduit differential.createrevision | jq -r .response.revisionid)
git push origin HEAD:refs/heads/non-permanent-refs/D$revid
arc diff -m 'Message' --update $revid --head $HEAD "${HEAD}^"
sleep 60
# make sure pushed branch is parsed and then push changes, need to make sure metadata Differential Revision is not added so we cannot use `arc land` here
git push origin master

Result - master branch updated, commit is marked as published, differential revision is not closed and on repository view - commit has no attachment to differential revision.