Expose git attribution string in user profiles and menu callback

+++ b/drupalorg_git_gateway/drupalorg_git_gateway.module
@@ -152,6 +152,22 @@ function drupalorg_git_gateway_user($op, &$edit, &$account, $category = NULL) {
+    if (user_access('access content')) {

We need to check not only that the requesting user has access to see this content, but that the viewed user actually has this data available: Git username may not be present.

Potentially more importantly, though, I'm not even sure that we want the Git username there at all - it's not actually used for anything wrt credit.

+++ b/drupalorg_git_gateway/drupalorg_git_gateway.module
@@ -152,6 +152,22 @@ function drupalorg_git_gateway_user($op, &$edit, &$account, $category = NULL) {
+        '#title' => t('Automatic Git e-mail'),

Need something better for this. Some explanatory text that explains what this information is *for* would also be very handy.

Powered by Dreditor.

It should probably be visible only to logged in users.

That's true, but I started this because of attribution, so the point is to let a maintainer to figure out which email address he can use to attribute the author(ie. run a git commit --author="" for #1120698: Typo in MAINTAINERS.txt, aka suggest what to write before the @).

Right sorry, I lost track of that while writing the review in the car. There's no reason to have the separate field to show the git username at all - just the email is sufficient. Since what's left of the @ is completely ignored by the commit mapping algorithm, we can use the git username (if set) or the regular username (if git username isn't set).

+++ b/drupalorg_git_gateway/drupalorg_git_gateway.module
@@ -152,6 +152,17 @@ function drupalorg_git_gateway_user($op, &$edit, &$account, $category = NULL) {
+        '#title' => t('Automatic Git e-mail'),

Given the above, let's change this to "Git attribution".

+++ b/drupalorg_git_gateway/drupalorg_git_gateway.module
@@ -152,6 +152,17 @@ function drupalorg_git_gateway_user($op, &$edit, &$account, $category = NULL) {
+        '#description' => t('If you are maintainer of a project who want to give attribution to a patch you received and you can not use git-am, you can use this URL to give attribution for !username', array('!username' => theme('username', $account))),

Using #description doesn't actually show up.

Regardless, that text needs to be shorter and link to some docs. Which, afaik, do not yet exist. But this would work better:

Learn more about proper Git attribution.

+++ b/drupalorg_git_gateway/drupalorg_git_gateway.module
@@ -152,6 +152,17 @@ function drupalorg_git_gateway_user($op, &$edit, &$account, $category = NULL) {
+        '#value' => sprintf('%s@%d.no-reply.drupal.org', $email_user, $account->uid),

It also makes me uncomfortable to provide only the partial identification string necessary for passing to --author. For the short term, we should use $user->name. So for me, the whole contents would look like

sdboyer <sdboyer@146719.no-reply.drupal.org>

which would mean changing the code to look like this:

'#value' => sprintf('%s <%s@%d.no-reply.drupal.org>', $account->name, $email_user, $account->uid),

A last thought - maybe we should only show this information if the viewing user has Git privileges? No reason they'd need this information if they don't.

Powered by Dreditor.

Yes, I assumed we'd show it just to people with git access. In fact, I'd suggest even formatting as the command like:

--author "X <Y@Z>"

Sorry, but I surely won't visit each user's profile in order to gather some funky author strings for every single commit. Furthermore, I don't see an option to adjust --author in my GUI (TortoiseGit).

When following best practices, a commit message already contains the required information to properly credit contributors.

Implement a pre-receive (or whatever it's called) hook on git.d.o and make it automatically query author e-mails for the stated d.o usernames.

When following best practices, a commit message already contains the required information to properly credit contributors.

This thread is an intermediary solution, at best, for allowing credit to be given in a case where additional Git metadata does not exist.

The "best practice" of names in commit messages is could only possibly qualify as "best" when we were in the stone age of CVS. It sucks. Moreover, *nobody* except you over the entire course of the Git migration (or since launch) has argued that it is sufficient - everyone's been looking for something better. Something real, something not hacked-on.

Furthermore, I don't see an option to adjust --author in my GUI (TortoiseGit).

From everything I've heard about it, TortoiseGit sucks; it encourages bad workflows (for us at least) and presents the wrong options. I've been saying that for a while now. Fortunately, as I said, this is not the "recommended" route, nor one that has much of anything to do with our long-term solution.

Implement a pre-receive (or whatever it's called) hook on git.d.o and make it automatically query author e-mails for the stated d.o usernames.

If you gather a substantial number of informed, invested people who agree that this is the *best* route for our future, then maybe. But until then, I'll remain convinced that it's a horrible idea.

@sdboyer -it would be nice if we had another way to credit multiple people for a commit. I guess the newest versions of git support commit comments?

@sun - I'm not suggesting you do this for every commit, but in cases where a contributor was fully or nearly fully responsible, it's a nice way to give them credit for the commit.

Anyhow - I have no idea what GUI clients do. GitX for OS X also has sign-off as a big front-and-center option but no obvious way to change author. This feature request is oriented to power/CLI users.

BTW- multiple authors seems to be an outstanding concern/bug: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=451880

@sdboyer -it would be nice if we had another way to credit multiple people for a commit. I guess the newest versions of git support commit comments?

I have difficulty conceiving of a situation where there are multiple contributors for a given change where each of them haven't made discrete commits. LOTS of difficulty. Which is what real topic branches + multiple commits + merging is for. *Exactly* what it is for. That's a simple, elegant model. The commit annotations you're talking about are probably git notes - but they're not nearly as elegant as just letting there be real commits in the first place. If someone could come up with a real, consistent way for modelling the data & then actually using them in keeping with that model, though, I'd be open to it. They are the only real hope here.

Anyhow - I have no idea what GUI clients do. GitX for OS X also has sign-off as a big front-and-center option but no obvious way to change author. This feature request is oriented to power/CLI users.

Agreed, this is for CLI. GUIs have no reliable standard at all for the set of features they decide to present, and munging commit ownership is generally not a priority.

--signoff does nothing but append some stuff into the commit message. Certainly a better, more structured way of getting reviewer info into a commit, but a) it changes the commit hash b) it's still in the commit message, which is fugly, and c) look at the text it creates: --signoff is not about authorship, it's about reviewing. It is prominently used in the Linux kernel and core git workflows, and we could use it too - but for reviewing, like it's intended. We should not MIS-use it and attempt to make it refer to authorship.

BTW- multiple authors seems to be an outstanding concern/bug: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=451880

The "outstanding concern/bug" you linked to is a) three and a half years old and dead, b) marked as "wishlist" by the original author, c) not on the main git mailing list where all actual git development occurs, d) completely correct in the observation that multi-author commits would result in massive backwards-incompatibility and therefore finally e) something that everyone should read to better understand why that whole approach is a bad idea. If Git was to introduce something that made its repositories backwards-incompatible, I strongly suspect it would be for the purposes of enhanced data integrity and improving things like, for example, managing upstream rebasing (though the core git folks are sufficiently unconcerned with that case as to probably not feel it needs addressing) - not for something as relatively trivial as retaining authorship. IMO, the only way multiple author headers have a prayer of making it in is if something bigger that causes backwards-incompatibility does go in, then multi-author rides those coattails.

As I said, the only remotely feasible option here is git-notes. They are separate objects that live in their own namespace (with potential for infinite note namespaces) and can be attached to commits after-the-fact without changing the original hashes at all.

@sdboyer - contributor posts a patch that's 75% there for one of my contrib modules. I apply the patch and get it to 100% and commit.

Anyhow - I agree that git is not going to support this natively any time soon. Basically what you are suggesting is that I should take and locally commit a patch with --author (or git apply if git formatted), and then fix it and commit again once it's 100%?

I guess that's ok, but would make more sense if we have issue repos. Doing that now is a pain if I'm bouncing between multiple issues.

Anyhow - I agree that git is not going to support this natively any time soon. Basically what you are suggesting is that I should take and locally commit a patch with --author (or git apply if git formatted), and then fix it and commit again once it's 100%?

Yup. I just pop off a topic branch that I name after the issue nid and do the work in there, then git merge --no-ff it back onto mainline. Keeps it isolated from other work, and will scale well into per-issue repos.

Of course, that ends up being three commits. My long-term hope/vision for statistics, though, is to differentiate between mainline merge commits that bring in topic branches (such as this), incremental commits directly on mainline, and incremental commits in a topic branch.

I think it makes more sense (will reduce errors and make for a faster workflow) to fully format the git option in the output:

--author="AUTHORSHIP_INFO"

So this one line in the patch:

-+      $attribution = sprintf('<code>%s <%s@%d.no-reply.drupal.org></code>', $email_user, $email_user, $account->uid);
++      $attribution = sprintf('<code>--author="%s <%s@%d.no-reply.drupal.org>"</code>', $email_user, $email_user, $account->uid);

For bonus points, a "click to copy to clipboard" bit of JS

I disagree. I think it's necessary. Why do you want to make me type all the extra chars every time when I could just be copying them?

I disagree. I think it's necessary. Why do you want to make me type all the extra chars every time when I could just be copying them?

Sounds to me like you care enough to write the js for it and add it to this patch. I'd happily include it. But I agree with marvil07 - I don't see the need to wait for it, either.

@sdboyer - no, not the JS, just the --author bit.

Here. Added it to the patch with expanded help text plus a fix for the way the link was handled inside t().

- side note, looks like github uses a shockwave, not JS for copying e.g. the git URL. Apparently JS alone can't do this cross-browser, or at all in FF.

If you'd

1. extract these lines into a separate function
2. add a menu router item user/%user/git-author as MENU_CALLBACK
3. and make that invoke the function from 1), simply printing the string (without the surrounding --author="") to screen or alternatively as JSON

...then Dreditor could load and use this info relatively easily.

+++ b/drupalorg_git_gateway/drupalorg_git_gateway.module
@@ -152,6 +160,57 @@ function drupalorg_git_gateway_user($op, &$edit, &$account, $category = NULL) {
+  return sprintf('"%s <%s@%d.no-reply.drupal.org>"', $email_user, $email_user, $account->uid);
...
+  print drupalorg_git_gateway_format_git_attribution($account);
+  exit;

Removing the surrounding double-quotes in the raw format and returning JSON would be preferred

Quite agreed. JSON please.

@marvil07: It's a matter of encoding and decoding.

Revamped this a bit after thinking ahead.

don't exit, return NULL from the page callback

I'm concerned that the menu callback lives at user/N/git -- seems likely to collide at some point, probably sooner rather than later. How about user/N/git-attribution or something more specific and self-documenting? I know it's not user-facing, but still.

If y'all are down with that, I can easily change the path while committing -- no need to reroll. But, I wanted feedback before I just did that unilaterally.

Thanks,
-Derek

I'd be fine with that change.

Committed and pushed. Thanks, folks.

Aaaaand this is broken. God dammit.

https://skitch.com/sdboyer/fjq4t/d.o-infra-git-sdboyer-drupal.org

Horribly broken. :/ It also disappears if you go to your user page with UID (e.g. /user/[user:uid] and not /user).

No idea about the #weight though.

OK, I pushed fixes to make this actually work. Weighting was screwed, and the contents of the commit string needed to be escaped.

Deployed.

Spammers such as http://drupal.org/user/1419260 are showing this git attribution on their profiles. That's pretty confusing. I think we should limit the visibility based on whether or not the user has the git access role.

@marvil07 - that's what was what in your original patch, but that's not the logic I ultimately applied. I could have sworn I said somewhere in this issue that it was the wrong way to do it, but apparently I didn't. In any case, though, restricting visibility to vetted users doesn't make sense, at all. The actual logic that went in is this:

/**
 * Returns whether currently logged in user is allowed to see git author attribution.
 */
function drupalorg_git_gateway_user_git_access() {
  global $user;
  return (user_is_logged_in() && !empty($user->git_consent) && empty($user->git_disabled));
}

As I said to webchick in IRC yesterday, I'm fine with restricting based on the account being viewed, not the person viewing. That especially makes more sense for the JSON callback, so that we can make use of it for external services without having to do some awkward auth.

Tagging.

I think it has been mentionned in #18 already, but the simplest solution would be to work on proper branches for issues (or forks), then merge when accepted through a pull request, like GitHub does.

These were removed from user profile pages.