Patch attribution

Hi all,

With D72468, the developer policy document was changed to indicate
that we should use the git commit author to indicate the patch author,
instead of the old "Patch by" method. I have some questions about
this:

1. The "Patch by" approach is still being widely used. Is this
something we should be concerned about? Should we try to raise
awareness about this issue? Although the git commit author approach is
cleaner, arguably the old method still works as well as it did before,
both for humans and tooling (if any?).

2. Often Phabricator users don't have an email in their profile (or
even a name). With the "Patch by" approach this was mostly fine, as
you could just add "Patch by Joe Random (joernd)", or just "Patch by
joernd", when only the Phabricator username was available. How do you
approach this issue when using the git commit author instead? Do you
use a commit author without an email address? Do you ask for those
details in the review and wait until that information is available,
before you commit?

3. IIRC, arc will sometimes automatically use the patch author as the
git author, but certainly not always. Do you know what the
preconditions are for that? (Maybe a filled-in profile in
Phabricator?)

Thanks!

Best,
Luís

Hi all,

With D72468, the developer policy document was changed to indicate
that we should use the git commit author to indicate the patch author,
instead of the old “Patch by” method. I have some questions about
this:

  1. The “Patch by” approach is still being widely used. Is this
    something we should be concerned about? Should we try to raise
    awareness about this issue? Although the git commit author approach is
    cleaner, arguably the old method still works as well as it did before,
    both for humans and tooling (if any?).

  2. Often Phabricator users don’t have an email in their profile (or
    even a name). With the “Patch by” approach this was mostly fine, as
    you could just add “Patch by Joe Random (joernd)”, or just “Patch by
    joernd”, when only the Phabricator username was available. How do you
    approach this issue when using the git commit author instead? Do you
    use a commit author without an email address? Do you ask for those
    details in the review and wait until that information is available,
    before you commit?

The e-mail headers have contained the e-mail address of the author every time that I checked.

  1. IIRC, arc will sometimes automatically use the patch author as the
    git author, but certainly not always. Do you know what the
    preconditions are for that? (Maybe a filled-in profile in
    Phabricator?)

This last part has to do with how the patch is uploaded afaik. If the patch was uploaded via arc in a Git repo, then the info is recorded and restored via arc on the other end.

Hi all,

With D72468, the developer policy document was changed to indicate
that we should use the git commit author to indicate the patch author,
instead of the old "Patch by" method. I have some questions about
this:

1. The "Patch by" approach is still being widely used. Is this
something we should be concerned about? Should we try to raise
awareness about this issue? Although the git commit author approach is
cleaner, arguably the old method still works as well as it did before,
both for humans and tooling (if any?).

I just commented on some of such instances.

2. Often Phabricator users don't have an email in their profile (or
even a name). With the "Patch by" approach this was mostly fine, as
you could just add "Patch by Joe Random (joernd)", or just "Patch by
joernd", when only the Phabricator username was available. How do you
approach this issue when using the git commit author instead? Do you
use a commit author without an email address? Do you ask for those
details in the review and wait until that information is available,
before you commit?

The e-mail headers have contained the e-mail address of the author every time that I checked.

3. IIRC, arc will sometimes automatically use the patch author as the
git author, but certainly not always. Do you know what the
preconditions are for that? (Maybe a filled-in profile in
Phabricator?)

This last part has to do with how the patch is uploaded afaik. If the patch was uploaded via arc in a Git repo, then the info is recorded and restored via arc on the other end.

`arc diff` uploaded patches have the information. `git format-patch
-1` uploaded patches patches may have it, too. `git diff` surely does
not retain the authorship.

The problem with using Git author is that buildbot will send emails on failure to the author rather than the committer which is undesirable because authors usually don’t have committer access (otherwise they’d commit the change themselves) and cannot revert the change.

I raised this in http://lists.llvm.org/pipermail/llvm-dev/2020-January/138838.html and it was pointed out that addressing this is infeasible in buildbot 0.8.5 but might be possible after we upgrade to 2.4.0 or newer. Now that buildbot has been upgraded, we should look into it again. Until this issue is addressed, I’d still recommend using the “Patch by” approach.

Regarding 2, I just ask the user for their name and email address.