Change format for git commit message

Adding a bit more details to the format

I believe this should be D.O. Customization not infra.

Updated title to reflect the decision was only for Core.

Is it? I thought all GitLab related things were under Infrastructure :-) I don't think this has anything to do with D.O beside the Credit & Committing box which will be phased out with GitLab.

I thought all GitLab related things were under Infrastructure

The Merge Request GitLab message templates for Core can be set by any user with the GitLab Maintainer permission for the Core project (all full core committers), no need to involve anyone outside the Core project.

I did forget that there might be an on-commit web hook. Does that enforce a commit message format for core?

Adding the needs IS update tag for clarification on what is being targeted for change.

I believe core committers still don’t use GitLab for merging MRs. They have their own local scripts and end up applying everything as a patch locally, running some pre-commit checks, and pushing the resulting commit.

Ideally, “the tooling” would make it easy to make these readable commit messages for everyone, not just core committers. The default commit message in the d.o issue UI is dutifully implementing what has been our convention for years, but it’s making out git histories really awful, hard Ti read and harder to use / understand. The goal of the policy issue was to get core to decide a new convention with the explicit hope that most / all of contrib would follow their lead.

Step 0 could be to modify the d.o issue hit message UI to stop injecting usernames at all.
Step 1 could be to fix it to use the [#xxxxx] $type: $title convention
Step 2 could be figuring out how to implement this in GitLab with as much automation as possible
Step 3 could be to understand why core committers still have a patch-based workflow and see if we can unify that with GitLab workflow
…

I’m not sure what queue is most appropriate, but I’m not sure it really matters. Maybe we’re not going to spend another minute on the d.o issue UI’s commit message tooling. Maybe this is entirely a GitLab configuration thing.

But I hope it can be fixed in a way that either makes life better for all of contrib, or at least is extremely easy to reuse across projects so contrib can trivially opt in.

Thanks!
-Derek

I asked for this issue to be opened under the infrastructure project. Micromanaging where it is isn’t necessary until implementation, either project is fine.

As far as I know, the reasons core committers are not using GitLab’s merge UI are the commit message format, which this hopefully solves, and https://gitlab.com/gitlab-org/gitlab/-/issues/26902

Ideally we set up GitLab’s merge UI to do the right thing by default, and discard the commit message drafting from Drupal.org. GitLab’s commit message templates are documented at https://docs.gitlab.com/ee/user/project/merge_requests/commit_templates....

The next step is to set up a contrib project with a template containing %{co_authored_by} and %{reviewed_by}.

If that is workable, we can make it the default for new projects, and bulk update projects that haven’t set their own template.

If that is workable, we can make it the default for new projects, and bulk update projects that haven’t set their own template

We should probably obtain community maintainer consensus before such a push to existing projects?

The core issue was focused on core. The core team hopes that contrib would follow, no discussion was had with the community in general on it being pushed out as a default for all projects.

You are not forced to do anything with it :-) you can just keep committing like you've always done. I also think already a lot of contrib maintainers aired their opinion on the core issue.

Made https://www.drupal.org/project/test_commit_message → with you all as maintainers.

I've looked at how this could work and one thing that will become nasty is that eventuelally the default MR title cannot be changed in gitlab itself it seems. See https://gitlab.com/gitlab-org/gitlab/-/issues/16080

This is an issue, since when we move onto gitlab issues, this means MR's will end up with:

Resolve "Fix tests and translations" Which is the default. This is maybe even broken in the docs of gitlab right now. Since that states: "Fix tests and translations".

So things are getting harder every minute while looking at this to do this all in there.

We could still do for the squash message:

%{title}

Committed-by: %{merged_by}
%{co_authored_by}
%{reviewed_by}

(see https://git.drupalcode.org/help/user/project/merge_requests/commit_templ...)

Another possible problem is when we don't have all people in the MR, there mightve been a new version, or they only contibuted in the issue itself. Then we need to manually add them also.

So there are quite a few holes need need to work out. Or don't try to rush to the end of the line and start with d.o. @dww has a point with his steps. But until we have proper credits/issues in gitlab, we cant completely cover it there (or know what we are missing).

Thanks for testing! Alas that it's getting harder and harder to resolve this via GitLab.

We're going to have a credit UI on new.drupal.org going forward. I wonder if it's worth adding something to that to spit out an appropriate commit message, like we have on d.o issues now? I wonder how difficult / easy that would be. Maybe easier than trying to implement it in GitLab?

We're going to have a credit UI on new.drupal.org going forward. I wonder if it's worth adding something to that to spit out an appropriate commit message, like we have on d.o issues now? I wonder how difficult / easy that would be. Maybe easier than trying to implement it in GitLab?

I guess as an in-between solution that's fine. That's exactly what I'm doing now when merging and squashing merge requests :-)

WIP so take with a grain of salt, but yes, I'm implementing conventional commits in the new UI ever since I saw this issue.

Twig: https://git.drupalcode.org/project/contribution_records/-/blob/1.0.x/tem...
JS: https://git.drupalcode.org/project/contribution_records/-/blob/1.0.x/js/...

There are some TODOs in the code, but the idea is there.

Just so we dont forget, we can help decidiing what tag to use with some sort of info like this cheatsheet: https://github.com/pvdlg/conventional-changelog-metahub#commit-types

Ok, i need to store my work unttil now, this is testable on here, will work more later. But don't wanna loose this hehe. Didn't end up using your code @fjgarlin sorry

Is uses new data- tags on the checkbox, have the php change for that ready. When you submitted 'patches' you are an author, otherwise a reviewer.

Todo:
Make sure the current committer/merger is in there
When category === feature requerst -> feat:
Wehn category === task -> task:
Others: chore: ? not sure yet.

$ = jQuery;
// Hard overwrite for testing
Drupal.settings.drupalOrg.issueCreditMessageTemplate = "
            
              
              
              ✨
              Allow specifying `meta` data on JSON:API objects
                Needs work
              
             !type: !message\n\n!byAuthors\n!byReviewers";
Drupal.settings.drupalOrg.issueCreditTemplate = "git commit -m '" + Drupal.settings.drupalOrg.issueCreditMessageTemplate + "'";


Drupal.drupalorgUpdateIssueCredit = function () {


  // Hardcoded changee to checkbox until php is testable
  $('#drupalorg-issue-credit-form input[type=checkbox][id^=by-]').each(function(){
    let $ele = $(this);
    let numbers = $ele.parents('tr').find('td');

    let patches = $(numbers[2]).clone()    //clone the element
      .children() //select all the children
      .remove()   //remove all the children
      .end()  //again go back to selected element
      .text();

    let files = $(numbers[4]).clone()    //clone the element
      .children() //select all the children
      .remove()   //remove all the children
      .end()  //again go back to selected element
      .text();

    let comments = $(numbers[6]).clone()    //clone the element
      .children() //select all the children
      .remove()   //remove all the children
      .end()  //again go back to selected element
      .text();

    $ele.attr('data-patches', patches);
    $ele.attr('data-files', files);
    $ele.attr('data-comments', comments);
  });


  var addCredit = $('#drupalorg-issue-credit-form input[name=add_credit]').val(),
    message = $('#drupalorg-issue-credit-form input[name=message]').val(),
    byHtml = [],
    byAuthor = [],
    byAuthorMsg = [],
    byReviewer = [],
    byReviewerMsg = []
  ;

  // Collect authors and reviewers.
  $('#drupalorg-issue-credit-form input[type=checkbox][id^=by-]' + (Drupal.settings.drupalOrg.isMaintainer ? ':checked' : '.saved-by-maintainer')).each(function () {
    let $this = $(this);

    // Already mentioned as author.
    if (byAuthor.indexOf($this.data('by')) !== -1 || byReviewer.indexOf($this.data('by')) !== -1) {
      return;
    }


    let userName = $this.data('by');
    if ($this.data('patches') && $this.data('patches') > 0) {
      byAuthor.push(userName);
      byAuthorMsg.push('Authored-by: ' + userName + ' (' + userName + '@no-reply.git.drupal.org)');
    } else {
      byReviewer.push(userName);
      byReviewerMsg.push('Reviewed-by: ' + userName + ' (' + userName + '@git.drupal.org)');
    }

    byHtml.push($.trim($this.next('label').html()));
  });

  console.log(byAuthor);
  console.log(byAuthorMsg);
  console.log(byReviewer);
  console.log(byReviewerMsg);

  // By default, credit others as reviewers.
  if (typeof addCredit !== 'undefined' && addCredit !== '') {
    let creditOthers = addCredit.split(',').map($.trim);
    creditOthers.forEach(function (creditOther) {
      byReviewer.push(creditOther);
      byReviewerMsg.push('Reviewed-by: ' + creditOther + ' (' + creditOther + '@git.drupal.org)');
    });
  }


  console.log(byAuthor);
  console.log(byAuthorMsg);
  console.log(byReviewer);
  console.log(byReviewerMsg);

  // Fill out credit summary.
  if (Drupal.settings.drupalOrg.isMaintainer) {
    if (byHtml.length) {
      $('#drupalorg-issue-credit-form .credit-summary').html(Drupal.t('<strong>Giving credit to</strong> !credits', {'!credits': byHtml.join(', ')}));
    }
    else {
      $('#drupalorg-issue-credit-form .credit-summary').html(Drupal.t('Expand and select contributors to give credit.'));
    }
  }
  else {
    if (byHtml.length) {
      if (Drupal.settings.drupalOrg.isFixed) {
        $('#drupalorg-issue-credit-form .credit-summary').html(Drupal.t('<strong>Credit given to</strong> !credits', {'!credits': byHtml.join(', ')}));
      }
      else {
        $('#drupalorg-issue-credit-form .credit-summary').html(Drupal.t('<strong>Draft credit given to</strong> !credits<br><small><em>Credit is assigned when maintainers comment, and granted when the issue is fixed.</em></small>', {'!credits': byHtml.join(', ')}));
      }
    }
    else {
      $('#drupalorg-issue-credit-form .credit-summary').html(Drupal.t('A maintainer has not commented and given credit yet.'));
    }
  }

  // For non-maintainers, repopulate from checkboxes for commit message templating.
  if (!Drupal.settings.drupalOrg.isMaintainer) {
    byAuthor = [];
    byAuthorMsg = [];
    byReviewer = [];
    byReviewerMsg = [];

    $('#drupalorg-issue-credit-form input[type=checkbox][id^=by-]:checked').each(function () {
      var $this = $(this);

      // Already mentioned as author.
      if (byAuthor.indexOf($this.data('by')) !== -1 || byReviewer.indexOf($this.data('by')) !== -1) {
        return;
      }

      let userName = $this.data('by');
      if ($this.data('patches') && $this.data('patches') > 0) {
        byAuthor.push(userName);
        byAuthorMsg.push('Authored-by: ' + userName + ' (' + userName + '@no-reply.git.drupal.org)');
      } else {
        byReviewer.push(userName);
        byReviewerMsg.push('Reviewed-by: ' + userName + ' (' + userName + '@git.drupal.org)');
      }
    });

    // By default, credit others as reviewers.
    if (typeof addCredit !== 'undefined' && addCredit !== '') {
      let creditOthers = addCredit.split(',').map($.trim);
      creditOthers.forEach(function (creditOther) {
        byReviewer.push(creditOther);
        byReviewerMsg.push('Reviewed-by: ' + creditOther + ' (' + creditOther + '@git.drupal.org)');
        byHtml.push($.trim($this.next('label').html()));
      });
    }
  }

  // Fill out template. It has already been translated server-side.
  $('#drupalorg-issue-credit-form textarea[name=command]').val(Drupal.formatString(Drupal.settings.drupalOrg.issueCreditTemplate, {
    '!message': message.replace(/'/g, "'\\''"),
    '!byAuthors': (byAuthorMsg.length > 0 ? byAuthorMsg.join("\n") : '').replace(/'/g, "'\\''"),
    '!byReviewers': (byReviewerMsg.length > 0 ? byReviewerMsg.join("\n") : '').replace(/'/g, "'\\''"),
  }));
  $('#drupalorg-issue-credit-form textarea[name=command-message]').val(Drupal.formatString(Drupal.settings.drupalOrg.issueCreditMessageTemplate, {
    '!message': message,
    '!byAuthors': (byAuthorMsg.length > 0 ? byAuthorMsg.join("\n") : '').replace(/'/g, "'\\''"),
    '!byReviewers': (byReviewerMsg.length > 0 ? byReviewerMsg.join("\n") : '').replace(/'/g, "'\\''"),
  }));
};
Drupal.drupalorgUpdateIssueCredit();

Committed a first iteration of the implementation. If you want to test locally.

$ = jQuery;
// Hard overwrite for testing
Drupal.settings.drupalOrg.issueCreditMessageTemplate = "
            
              
              
              ✨
              Allow specifying `meta` data on JSON:API objects
                Needs work
              
             !type: !message\n\n!byAuthors\n!byReviewers";
Drupal.settings.drupalOrg.issueCreditTemplate = "git commit -m '" + Drupal.settings.drupalOrg.issueCreditMessageTemplate + "'";

Drupal.drupalorgUpdateIssueCredit = function () {
....COPY FROM MR
};
Drupal.drupalorgUpdateIssueCredit();

This should work on any issue.

We might need a Committed-By: {currentUser}.

Also we need to decide what email adres to use. We could use a generic no-reply so mail adresses do not get added for all time even when people change their email adress visibility on d.o. But i do see real email adresses of committers in the commit log. So not sure what is wise.

We’re trying to not use jquery on js code if possible unless there is a solid reason to use it. I can try the MR for a POC but we’d want jquery removed in the final version.

Ah, sure. Though since it was adjustments it would be ok.

But a small rewrite is fine.

I've just seen that the current file handling that uses jQuery: https://git.drupalcode.org/project/drupalorg/-/blob/7.x-3.x/drupalorg/js...

So leave the code as is for now then

I applied the MR to this d.o copy (mine had changes about the new system): https://kbentham-drupal.dev.devdrupal.org/project/drupal/issues/3464771

I can see that \n\n\n is getting into the message.

It's a great beginning!! I made some comments about the emails and about the above issue.

THanks, did some quick checks in the car. Think we can use $GLOBALS['user']['extern_uid'] which should be the git user id. [ID]-[username]@git....

Still though we could have a look at getting the public user email from gitlab. That seems doable. Only question then becomes speed since we would need to save that somewhere. But perhaps we should be more sane in the hook that actually fills that field.

Dunno, perhaps tonight ill find some time to continue;

I wonder if it's worth adding something to that to spit out an appropriate commit message, like we have on d.o issues now?

I’ll note this might also be a reason to invest in client side tooling.

Yes can add the UI and it can be useful, however worth looking at your daily operations and analyzing it this is the sort of task that can be put into a local script to parse the issues, prompt the committer for credit/title/description, push the credit, Squash, merge, push to GDO, close the MR and the issue all in a single command saving committers time. Just throwing it out there as “long term” how to possibly be more efficient as more and more moves into Gitalab where a proper full API exists.

Also we need to decide what email adres to use.

The email challenge is probably the one negative of moving this out of GDO and trying to use a UI that can’t (shouldn’t) have access to the full email database.

If GDO does make the public address avaliable that’s a good solution. We might need ✨ Allow users to set GitLab Commit email Active (looks like it’s on the same page but a different field as the commit email). (This issue reminded me I needed to re-open that one.)

If we can’t use the public email field I would suggest:

For committers:
We should probably use whatever is in their commit.

Users can set their anonymous address before pushing to GDO.

Some of us want our real addresses for traceability and manageability (you can’t verify you “own” an anonymous D.O. address). GDO Infrastructure has seen similar requests with those importing commits from IIRC GitLab in the past where it’s not associated with their D.O. account.

For reviewers:
Probably a bit harder might just have to assume an anonymous address.

I'm not a fan of "local tooling should solve this" as an approach. There are 1000s of maintainers on *.d.o. Telling each of them to solve this common need themselves is infeasible. Our collaboration platform handles (nearly) everything else, it should solve this problem in a general way, too.

There's no way to fully automate the credit assessment. That boils down to humans having to make decisions. So it can't all happen "automatically", anyway. If a human has to go through the issue and/or MR to decide who meaningfully contributed and twiddle a UI to record all that, that same UI can both save those results into the DB, and spit out this copy/paste block of formatted text to use for the commit message.

*.d.o definitely can and should have access to email addresses. I'd be in favor of a d.o-stored field attached to user accounts that indicates the preferred email address for commits. That looks like what ✨ Allow users to set GitLab Commit email Active is about. If not explicitly set otherwise, would default to a no-reply. But if users (like me) want to have a valid email address that we control in all our commits (and commit references), we could opt-in to something else. Then this credit UI can honor the user's preference for which email to use for Reviewed-by, Co-authored-by, etc.

Meanwhile, amazing work, @bbrala! Thanks so much for picking this up and running with it. 🙏 Our long nightmare of "Issue #xyz by incomprehensible, list, of, user, names, before, you, know, what, changed" is looking to soon be over! 🎉

Ty dww.

We do need something to decide what email is to be used. In the related issue it tells us that we cannot know user vs commit in gitpab, but wouldnt commits show up as gitlanId-userUid@git.drupal.org or something? That seems good enough to couple? Or is the problem the fact you can set any mail or username in your local git?

Not sure i see the problem.

Except we need to know what the correct email address is.

Reapplied the MR in https://kbentham-drupal.dev.devdrupal.org/project/drupal/issues/3464771.

We are getting:

Notice: Array to string conversion in drupal_attributes() (line 2516 of /var/www/dev/kbentham-drupal.dev.devdrupal.org/htdocs/includes/common.inc).
Notice: Array to string conversion in drupal_attributes() (line 2516 of /var/www/dev/kbentham-drupal.dev.devdrupal.org/htdocs/includes/common.inc).
Notice: Array to string conversion in drupal_attributes() (line 2516 of /var/www/dev/kbentham-drupal.dev.devdrupal.org/htdocs/includes/common.inc).
Notice: Array to string conversion in drupal_attributes() (line 2516 of /var/www/dev/kbentham-drupal.dev.devdrupal.org/htdocs/includes/common.inc).

And markup:
td #attributes="Array Array"

I'm adding a few suggestions and a question to the MR.

Oopsie, not sure why we didnt see this before, but the atrtribute is set AFTER the $row[$key] was overwritten with an render array. I've fixed that.

I also applied/committed the suggestions. Thanks for having a look.

Still an open questino in #27 regarding the 'others'. This would be a non issue if one first saves before we commit. But that does seem not very user friendly.

I did a small change as the array key wasn't still correct and reapplied the changes. I also left some feedback in the MR about a small thing and some other part that I don't think it's needed at all.

Bit things are looking really good already.

Test URL: https://kbentham-drupal.dev.devdrupal.org/project/drupal/issues/3464771

This is not a great time to be implementing this on Drupal.org. We are a few weeks away from migrating issue credit, which will require implementing this again. There are some technical hurdles to implementing this in D7, nothing insurmountable, but our time is better spent on the migration.

Unless there’s a compelling reason to have this right now, we should postpone. The D7 site should be considered feature-frozen until after migration except for things that move migration forward.