Page MenuHomePhabricator

[land-bot] Preserve committer name and email
AbandonedPublic

Authored by jasonbcox on Jun 23 2020, 21:41.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Summary

Despite the user-supplied conduit token being passed during landing,
the local git config is still used when applying arc patch, which overwrites
the committer (but not author) information. This patch uses the conduit token
to fetch the user's name and email to use as committer, ensuring the land bot
is not taking credit as the committer (which happened for 04394f92dce and some others).

Test Plan

CONDUIT_TOKEN=<token> ./land-patch.sh --dry-run --revision Dxxxx
Try changing out CONDUIT_TOKEN with some other user's token and observe with git show
that the committer information is set appropriately.

Event Timeline

Can you explain how it works ? Are the exported variables interpreted by arc land (I don't see any reference in the doc) ?

contrib/source-control-tools/land-patch.sh
120

Is it guaranteed that the phabricator name/email pair is the same as the user git's name/email ?

deadalnix added inline comments.
contrib/source-control-tools/land-patch.sh
120

It is not.

Can you explain how it works ? Are the exported variables interpreted by arc land (I don't see any reference in the doc) ?

GIT_COMMITER_(NAME|EMAIL) are git environment variables that can be used to override committer info. This is relevant
during the arc patch call in autopatch.sh. Normally the committer info is the current user's config, but abc-bot is landing this change
so the committer needs to be set to match the info of the supplied conduit token.

contrib/source-control-tools/land-patch.sh
120

The phab name/email is set via your profile. If this configuration turns out to be not acceptable, an alternative approach is to pass the user's configured name/email at the time of calling arc land-via-bot. I'm happy to hear use cases where you'd prefer name/email to be different between git and phab.

deadalnix requested changes to this revision.Jun 24 2020, 16:28

Why don't pull the change from the statging repo rather than doing this?

This revision now requires changes to proceed.Jun 24 2020, 16:28

Why don't pull the change from the statging repo rather than doing this?

The latest staging repo state may match the patch, but the title, summary, test plan, and reviewers are not guaranteed to be up-to-date.

deadalnix requested changes to this revision.Jun 28 2020, 15:11
deadalnix added inline comments.
contrib/source-control-tools/land-patch.sh
120

Just have arc send the relevant infos, instead of trying to guess, possibly wrong.

This revision now requires changes to proceed.Jun 28 2020, 15:11