Page MenuHomePhabricator

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

Authored by epriestley on Oct 31 2019, 3:03 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Mar 10, 6:15 AM
Unknown Object (File)
Feb 3 2024, 11:22 PM
Unknown Object (File)
Dec 26 2023, 2:55 PM
Unknown Object (File)
Dec 24 2023, 7:52 PM
Unknown Object (File)
Dec 23 2023, 3:24 PM
Unknown Object (File)
Dec 18 2023, 5:24 PM
Unknown Object (File)
Nov 30 2023, 3:31 AM
Unknown Object (File)
Nov 25 2023, 10:45 PM
Subscribers

Details

Reviewers
Pawka
artms
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 Passed
Unit
Tests Skipped
Build Status
Buildable 23607
Build 32448: Run Core Tests
Build 32447: arc lint + arc unit

Event Timeline

artms requested review of this revision.Oct 31 2019, 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.

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...

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

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.

image.png (166×453 px, 18 KB)

epriestley abandoned this revision.
epriestley edited reviewers, added: artms; removed: epriestley.

Presumably mooted by T13552. If you believe there is still a bug here, please file it via Discourse or your organization's support channel.