Core commit credit and log messages are inconsistent between patches and merges

Maybe but that doesn't mean you can abuse the critical bug tag.
This won't change in the near future unless we have a github style way of working.

Personally (yes this is just my opinion, so don't attack me on it) I think we get credited in some way, on each patch a list of contributors is attached. The only difference is that one is listed "by webchick" and the other "by crell" for example.
So I have hard time to understand why this is so unfair. Btw I think sun made a patch to dreditor so you can change the author. But than again there can only be one author (if I'm not mistaken) so in the core patch process you have to choose someone that is the most important asset to a patch. Thats not fair. In a merge approach every small addition gets credits.

I'm up for a discussion but don't move this to critical, it's hard enough to maintain the critical list alrdy.

This is as important as any of the process based critical tasks I've seen in this queue before. Worth further discussion.

The value of git merges is not to be taken for granted, but just like sandbox commits are hidden on profile pages, so could Drupal core.

Cross-linking http://groups.drupal.org/node/161659

fwiw I think it is wonky doing half patches (and --author is useless for core hence the git notes discussion) and half branches. So I'd much prefer that 'only core committers get commit credit' and we stick to commit log parsing for stats etc. Since there are various automated tools out there (whether ohloh or certifiedtorock) that rely on this information, it should be consistent.

No it's a different issue, but if we could standardize on git notes in core and encourage it as a best practice in contrib, then there'd be less people using --author.

Less inflammatory title, it's the merge/commit from the sandbox that messes up the commit credit, not the actual sandbox itself.

It's absolutely valid that a hybrid merge/patch workflow is wonky. Commit credits is only one place where it gets wonky.

The solution isn't to drop merge-based workflow, or to hack it to credit differently. The solution is to more fully embrace a merge-based workflow, which is inherently superior to a patch-based workflow. That "merely" requires pushing forward the development of our git infrastructure, pushing toward per-issue-repositories so that everything is a merge (like most projects are doing now anyways). That's pending on the Drupal 7 port of Drupal.org, I believe, which the DA is working on now.

Yeah I'd rather have an interim solution for now since we might well not have anything like that workflow available to us until Drupal 9 starts development or some time after.

True enough. My recommendation for credits is to include both author mentions and "by" messages in tag clouds for Drupal 8, and acknowledge that they're not completely comparable so all numbers are a bit fuzzy.

(And to get core committers more comfortable with git-based workflows so that people who want to do issues that way now can do so. Maybe we can make it easier to flag explicitly where a given patch's branch is, so committers don't need to ask for copy-paste instructions?)

chx: What you're suggesting is "remove all git credit so that everyone is equal, using our old broken system". I cannot say I am fond of that approach. Whatever we do, there will be a transitional period where some things are patches and some are merges, and saying "screw you, you don't count" to either one is bad.

Also, squashing all merges defeats the whole purpose of having a merge. We *want* the history of the branch. If we didn't want the history, then a patch would have the exact same effect. git merge --squash renders all of the work done to use git properly entirely irrelevant. That's a no-go for me.

In the CVS/patch workflow, the original DBTNG patch that took 4 months and a half dozen people to write counted the same as someone who fixed a typo in a help message. No, it wasn't fair by any stretch of the imagination. It was unfair differently. :-)

We had this debate already in the Drupal initiatives announcement group, so referencing one particular comment that summarizes both sides: http://groups.drupal.org/node/148184#comment-527894

Something Sam pointed out back then that we could do is put a hack in drupalorg_vcs module (or whatever) to simply remove Drupal core from the committers list (and user profile pages, etc.) for people who are not "the" core committers. This would only affect the website, not git log. But it would have at least somewhat of an equalizing effect.

However, since then it's become clear to me that merge-based workflows are the way the entire rest of the internet is going, and so I actually think a solution like that moves us backwards, not forwards. OTOH, per-issue repos are nowhere near anyone's todo list, from what I know. When we did the big community voting extravaganza this feature rated far below where I expected (it's on the second page, behind lots of other DX improvements like #493074: Back-link to the commit as a comment on the related issue.), whereas the solution at #1451668: Provide better integration of sandboxes and parent projects was in the top 15 or so (that of course only solves the visibility issue, however, not the credit issue). And even if we were to have per-issue repos or some other sort of merge-based workflow more fundamentally integrated into the site, as Sam points out in that linked discussion, there will nearly always be situations where we have hybrid approaches to contributing to support one-off fixes and new users grappling with the concepts of version control.

The credit issue is really hard, I hate it too, and we don't have a good solution. :\

In case you somehow missed it, I agree with you. I never use the author flag on patches I commit, because I hate the rotten "two-tier" commit credit and think it's a complete community disaster for a collaborative team.

I'm just acknowledging that merge commits are not going away anytime soon, and in fact they are likely to become more prevalent over the coming years.

Copy/pasting my comments from that linked discussion for posterity:

My concerns are:
a) Creating a special "first class" commit credit for some special people but not others creates inequality, and inequality creates bad blood in a collaborative team.
b) Because the only way to get that special first-class credit (commits to "Drupal" showing up on your user profile) is through keeping your code out of patches and in another git repo somewhere (github, sandbox, etc.) it will encourage people to do that, rather than use the core issue queue which is what reviewers/committers actually read.
c) We've had demonstrably terrible problems already with initiative visibility in non-core issues, external sandboxes (Bartik, Field API, etc.) and I'm concerned this will only make fragmentation/visibility worse. :\

Webchick: Calling It™ on community collaboration disasters since June 2011. :P~

When I'm doing contrib work, I actually go out of my way to credit people in git messages when I can precisely because that's the way the tool is setup to work. I know a number of other contrib maintainers who do so as well, although it's definitely not universal.

Doing everything via squashes means that we are using this fancy uber-powerful VCS, and then throwing out all of its useful features so that we can use it as CVS with different syntax just to prop up the old-style credit system. Aside from meaning the entire Git migration was a waste, that also is going to alienate, well, every contributor we can think of who has come from any other project. Git-with-merge has won. Using it as a CVS front-end will only serve to prevent people from getting involved.

We *want* the development history. We *want* to see how code evolved. That's insanely useful information if you want to go back in time with, say, git blame. And you can even track down the individual person on the WSCCI team who wrote that particular part of the kernel, rather than just "Oh, well, kernel, hm, hey Crell, what's this do?"

As another data point, Symfony does everything via merges but encourages people to rebase a lot to create a clean history. That means most of the things I've done upstream in Symfony are credited as one, perhaps 2 commits, even if they took a few rounds to get to. But it's still a merge, and still has history, and, yes, still credits using the tool itself rather than some side-commentary.

(To be clear, my preference for merges has nothing to do with Symfony; I was pro-merge long before Symfony ever entered the picture.)

We spoke about this briefly in IRC today, since we're over thresholds again on critical tasks.

This issue is correctly filed as a critical task. The credit imbalance has tremendous community implications. But the fact is, there's really nothing we can do about this until we have some means of pushing everyone through the same contribution method, which (looking to the future) means using the full extent of Git's functionality. Whether this takes the form of pull requests, or per-issue repos, or better integration of sandboxes with their parent projects, that's clearly the way forward IMO.

What that means is there's literally nothing we can do about this issue until Drupal.org is on Drupal 7. When this happens, we will be in a position to create whatever additional functionality we need in order to ensure all contributors contribute and are credited equally. Until then, we are unfortunately in this murky "middle zone" while we figure out those interaction patterns and what works and what doesn't.

I checked with chx, and he was ok on marking this issue "postponed" until Drupal.org is on D7. Unfortunately, I don't think this will happen in time for D8's feature freeze (or maybe just barely), which means the functionality will definitely not be ready until perhaps Drupal 9's release cycle. But that would then give us a few months to envision the ultimate approach that we want which is equitable to all, and propose it as a spec to the DA.

I disagree that there's "nothing to be done" until our entire core process is adjusted.

Short-term solution

1. Give sandbox owners and core maintainers guidance for merging properly and how to get rid of superfluous commits. (Edit: It's the sandbox maintainer's responsibility to prepare a clean branch, but maintainers should understand what they're merging in. Show maintainers git log --graph.)
2. Be a little more picky about what we merge in. Maybe reviewing the git history for the repo becomes part of the review process.
3. Parse both git log mentions and commit authorship for D8 commit metrics.
   • The quickest way to do this is to simply omit core maintainers from the "author" part of the count, with the assumption that none of them are working in sandboxes, too. Merge commits themselves should also be omitted from the count. Simple regexes.
   • Alternately, it should possible to identify which branch a commit originally came from, so we only count authorship for commits from the sandboxes. (Still omit the merge commits themselves in this case.)

That gives us a more fair D8 tag cloud, and we cold probably even tweak d.o in the short term to use the same information (now that the d.o testing profile works again, yay). (CTR and ohloh don't come anywhere close to reflecting our actual contribution metrics anyway, and CTR hasn't been updated in over a year.)

Mid-term solution

Use git notes.

1. Define a standard for git notes and patch Dreditor to generate it. (For now, just put the same information in the notes that we currently put in the commit message.)
2. Retroactively add notes to previous commits on 8.x:
   • For commits on the main branch, retroactively parse commit log messages and inject them into the notes for those commits. (We'd have to hack in a workaround for Dries' not-a-real-merge merges.)
   • For commits downstream, and/or that do not have commit messages in the standard "Issue #123456 by foo, bar" format, add the author to the notes instead.

Note this isn't an entirely trivial undertaking because of inconsistencies that will need to be special-cased, but it's still something we can do right now. As with the short-term solution, it can also be easily parsed for contribution metrics and d.o profiles.

Long-term solution

As above. Our issue queues and processes use git more like the rest of the world, etc. Ponies.

I'm reluctant to unpostpone this given the vitriol earlier in the issue (and that I'm on the fence about it being a critical task generally), but I don't think this should be postponed.

Talked to webchick about my post above briefly and edited it a bit for clarity. The short-term solution is something we should start right now; the other (more technical) solutions can wait until after code freeze if we improve our processes for sandbox merges now.

You can only sometimes do an end-of-process "tidying" rebase. For the kernel branch, we were merging from 8.x frequently rather than rebasing because a lot of people had their hands in it, and rebasing would have caused a total mess. All of those merges from 8.x, though, meant that rebasing kernel against 8.x first would have been virtually impossible; lots of commits only make sense (logical or technical) in historical context. Some of the branches coming in from other people, though, I did rebase before merging in to tidy up and to reduce the number of merge commits.

The routing branch, so far, is almost entirely me so I've been rebasing as I go, including tidying up history. That will work as long as I'm the only one with any serious fingers in that pot. When/if we get more than me and sometimes Kat working on it, we won't be able to do that anymore.

I deliberately did the routing branch differently as an experiment, and that's what I've found so far. "Rebase as you go instead of merge" is fine if there's only one person working on it, but not if there's multiple people with their hands in the code.

That said, I've also been self-rebasing before push to get rid of most "oops, forgot this file" type commits, which is generally good practice anyway. (commit --amend is great for that.) Even then, though, when you have more atomic commits you will by nature generate more of them than in a patch workflow, where the atom is simply much larger. (eg, if a new system includes 5 new moving parts that are self-contained, that *should* be 5 commits or more in Git whereas it would be one patch for the whole system in CVS.) So, the definition of "superfluous" is a bit different.

"Good Git practices" is a worthwhile thing to include, though, or simply reference to ProGit.org.

This user http://drupal.org/user/1888132 is on the list of core contributors:

http://drupal.org/node/3060/committers

only for that:
- 'title' => t('Standard derivation'),
+ 'title' => t('Standard deviation'),

http://drupalcode.org/project/drupal.git/commit/7a5b902

If I correct a typo in core, what should I do to get on that list as well?

That happened because it was during the Views port, which was merged in from a sandbox.

This is annoying but it's by definition not blocking release, moving to normal/active. I'd be tempted to suggest we make a point of sorting it out before 9.x gets opened up for development - if we'd moved to git notes before 8.x was opened up we'd not have had this issue at all.

Perhaps we should make it a critical against 9.x and try to solve it before opening up 9.x for development?

@webchick: Coming from #21:

I never use the author flag on patches I commit, because I hate the rotten "two-tier" commit credit and think it's a complete community disaster for a collaborative team.

I don't understand how this creates a "two-tier" system. In fact, I think the opposite is true. If merge commits always get credit, then wouldn't using the "--author" tag ensure that we have more credit for patches, bringing it up to the level of merge commits? This actually makes things more fair, not less.

And for the record, in my one example over at #2055019: Syslog configure link missing on Extend/Modules page, you wouldn't have needed to use "--author". I already provided the Git metadata with "git format-patch" so you simply could have used "git am".

Basically, I'm shocked that the way I've been working in contrib for years (giving and receiving credit) doesn't actually happen with patches in core.

@colan It is a two-tier system because only one person can get credit. In the core queue, an issue can NEVER have only one participant, because of the peer-review system. Often times a reviewer can spend more time and energy than the patch author (for example, verifying that all changes of a search-and-replace are valid).

Because git does not allow for multiple authors, we have to devise an alternate system.

In contrib, patches are often committed with no peer-review, and there is more often a clear "champion" of an issue doing the work, which is why credit is more often given there.

Right. Until and unless we move to something like Github pull requests, which would give equal credit to *all* participants in getting a core issue to RTBC, it's not really fair to use the --author flag, basically. At least for core.

There has been no discussion here for 9 years. Is this particular issue about git usage still relevant?

I am setting the status to Postponed (maintainer needs more info). If there isn't confirmation that this is still relevant, it may be closed after three months.

Thanks!

We 'fixed' this by never merging anything into core, and only committing downloaded patches.

There is probably a new issue about merging gitlab MRs into core with squash commits and a templated commit message somewhere.

@catch, thanks.

Based on that comment I am closing this issue. I searched for another issue about merging GitLab MRs but didn't find one so can't add that here.

Also, There is a recent related issue about the format of the git commit message #3439331: [policy] Decide on format of commit message