Bug 217732 – [git-webkit] Use contributors.json

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

git-webkit should rely on contributors.json, this will allow us to be more explicit about the author of a given change.

[Radar WebKit Bug Importer]

<rdar://problem/70309518>

[Jonathan Bedard]

Created attachment 411376 [details]
Patch

[Dean Johnson]

Comment on attachment 411376 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411376&action=review

> Tools/Scripts/git-webkit:32
> +for contributor in CommitterList().contributors():

Nit: Can we reformat this file with the standard `if __name__ == '__main__'` and a main() function to go with it?

> Tools/Scripts/git-webkit:38
> +            Contributor.bank[alias] = c

My previous concern about this approach was that we were modifying values on a static object then relying on those values existing in other scripts/modules. 

Can we find a way to organize the code so we are explicit about creating some kind of ContributorList, then pass that around? Or call a function which returns that list, then provide that result as an argument to the parser?

[Jonathan Bedard]

Comment on attachment 411376 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411376&action=review

>> Tools/Scripts/git-webkit:38
>> +            Contributor.bank[alias] = c
> 
> My previous concern about this approach was that we were modifying values on a static object then relying on those values existing in other scripts/modules. 
> 
> Can we find a way to organize the code so we are explicit about creating some kind of ContributorList, then pass that around? Or call a function which returns that list, then provide that result as an argument to the parser?

I think the question here is what the most clear architecture is, because we have some limitations. The first forbids the simplest "function which returns a list" approach, because webkitscmpy is not supposed to know the specifics about contributors.json. We could pass a generator function in, but that seems strictly worse than passing a list in.

We could pass a list in, but then we would either need to a) set the list to some kind of global store of contributors for webkitscmpy to consume (which is basically what we're doing now, just explicitly) or b) pass the list to the repository classes which would then use those lists to normalize contributor names.

I figured that the global approach would usually be more correct because in the rare case where a single script knows about multiple repositories, it seems desirable for a person contributing to both repositories to resolve to the same contributor object.

[Jonathan Bedard]

Created attachment 415322 [details]
Patch

[Jonathan Bedard]

The uploaded patch has the repository own the record of contributors. The one downside to this architecture is that commits which are orphaned from a repository need to specify a Contributor and can't rely on an in-process global record. The compromize is to allow json encoding of the full contributor object.

[Jonathan Bedard]

Created attachment 415323 [details]
Patch

[Jonathan Bedard]

Created attachment 415416 [details]
Patch

r=me. Using mapping is clearer than 'bank'.

[EWS]

Committed r270447: <https://trac.webkit.org/changeset/270447>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415416 [details].