public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
From: Fangrui Song <i@maskray.me>
To: Pedro Alves <pedro@palves.net>
Cc: Simon Marchi <simark@simark.ca>,
	Overseers mailing list <overseers@sourceware.org>,
	 Mark Wielaard <mark@klomp.org>, Tom Tromey <tom@tromey.com>,
	Jeff Law <jeffreyalaw@gmail.com>,
	 Joseph Myers <josmyers@redhat.com>,
	Jonathan Wakely <jwakely.gcc@gmail.com>,
	libc-alpha@sourceware.org,  Jason Merrill <jason@redhat.com>,
	gcc@gcc.gnu.org, gdb@sourceware.org,  binutils@sourceware.org
Subject: Re: Updated Sourceware infrastructure plans
Date: Thu, 2 May 2024 16:05:21 -0700	[thread overview]
Message-ID: <DS7PR12MB5765990D14AB84FB00C82180CB182@DS7PR12MB5765.namprd12.prod.outlook.com> (raw)
In-Reply-To: <9a7111c2-d570-4d26-8fe9-f34834ae1eab@palves.net>

On Thu, May 2, 2024 at 8:35 AM Pedro Alves <pedro@palves.net> wrote:
>
> On 2024-05-01 22:04, Simon Marchi wrote:
> > The Change-Id trailer works very well for Gerrit: once you have the hook
> > installed you basically never have to think about it again, and Gerrit
> > is able to track patch versions perfectly accurately.  A while ago, I
> > asked patchwork developers if they would be open to support something
> > like that to track patches, and they said they wouldn't be against it
> > (provided it's not mandatory) [1].  But somebody would have to implement
> > it.
> >
> > Simon
> >
> > [1] https://github.com/getpatchwork/patchwork/issues/327
>
> +1000.  It's mind boggling to me that people would accept Gerrit, which
> means that they'd accept Change-Id:, but then they wouldn't accept
> Change-Id: with a different system...  :-)
>

Gerrit uses "Change-Id:" as stable identifiers to track patches.
https://gregoryszorc.com/blog/2020/01/07/problems-with-pull-requests-and-how-to-fix-them/
has some analysis how they are much better than the alternative smart way.

Perhaps URLs as stable identifiers will work better.
If a reader wants to find relevant discussions, they can just click
the link in many browsers, terminals, and editors.

Currently, searching for discussions about a specific commit requires
searching its title on https://inbox.sourceware.org/gcc-patches/ .
For older patches, I might even need to dig through
https://gcc.gnu.org/pipermail/gcc-patches/YYYY-MMMM/ archives.

I agree with Jeff that principal reviewers will drive improvement to
the code review process.
I am sharing two code review services LLVM has used.

---

Between 2012 and Sep 2023, LLVM had relied on its self-hosted
Phabricator instance for code review.
Fetching a patch to your local branch was as simple as `arc patch D12345`.
Similarly, creating or updating a patch involved `arc diff`.

I believe other code review services provide similar command line functionality,

---

In September 2023, LLVM transitioned to GitHub for code review.
I really dislike its code review service (however, this is a large
step forward than email based code review). From
https://maskray.me/blog/2023-09-09-reflections-on-llvm-switch-to-github-pull-requests

> On the other hand, GitHub structures the concept of pull requests around branches and enforces a branch-centric workflow. A pull request centers on the difference (commits) between the base branch and the feature branch. GitHub does not employ a stable identifier for commit tracking. If commits are rebased, reordered, or combined, GitHub can easily become confused.
>
> When you force-push a branch after a rebase, the user interface displays a line such as "force-pushed the BB branch from X to Y". Clicking the "compare" button in GitHub presents something like git diff X..Y, which includes unrelated commits. Ideally, GitHub would show the difference between the two patch files, as Phabricator does, but it only displays the difference between the two head commits. These unrelated in-between commits might be acceptable for projects with lower commit frequency but can be challenging for a project with a code frequency of 100+ commits every day.
>
> The fidelity of preserving inline comments after a force push has always been a weakness. The comments may be presented as "outdated". In the past, there was a notorious "lost inline comment" problem. Nowadays, the situation has improved, but some users still report that inline comments may occasionally become misplaced.

Thankfully, getcord/spr comes to a rescue.
User branches allow me to create/update a patch using `spr diff` like
`arc diff` for Phabricator.

  reply	other threads:[~2024-05-02 23:11 UTC|newest]

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-04-17 23:27 Mark Wielaard
2024-04-18  6:04 ` Thomas Koenig
2024-04-18  8:14   ` FX Coudert
2024-04-18  9:01     ` Christophe Lyon
2024-04-18 11:38     ` Janne Blomqvist
2024-04-18 12:01       ` Generated files in libgfortran for Fortran intrinsic procedures (was: Updated Sourceware infrastructure plans) Tobias Burnus
2024-04-18 12:32         ` Martin Uecker
2024-04-19  9:35   ` Updated Sourceware infrastructure plans Jonathan Wakely
2024-04-18 15:56 ` Joseph Myers
2024-04-18 17:37   ` Frank Ch. Eigler
2024-04-18 17:54     ` Joseph Myers
2024-04-18 18:29     ` Matt Rice
2024-04-22 15:39     ` Tom Tromey
2024-04-23  2:55       ` Jason Merrill
2024-04-23  3:12         ` Simon Marchi
2024-04-23  3:24         ` Tom Tromey
2024-04-23  3:51           ` Jason Merrill
2024-04-23  8:56             ` Mark Wielaard
2024-04-23  9:39               ` Richard Earnshaw (lists)
2024-04-23 15:08             ` Tom Tromey
2024-04-23 15:25               ` Simon Marchi
2024-04-24  8:49                 ` Aktemur, Tankut Baris
2024-04-23  4:06           ` Ian Lance Taylor
2024-04-23  9:30           ` Richard Earnshaw (lists)
2024-04-23 13:51             ` Ian Lance Taylor
2024-05-01 19:15           ` Jeff Law
2024-05-01 19:38             ` Jonathan Wakely
2024-05-01 20:20               ` Mark Wielaard
2024-05-01 20:53                 ` Tom Tromey
2024-05-01 21:04                   ` Simon Marchi
2024-05-02 15:35                     ` Pedro Alves
2024-05-02 23:05                       ` Fangrui Song [this message]
     [not found]                       ` <DS7PR12MB57651DA3A5C22B2847C13580CB182@DS7PR12MB5765.namprd12.prod.outlook.com>
2024-05-07 16:17                         ` Joseph Myers
2024-05-10 10:43                           ` Ben Boeckel
2024-05-01 20:04             ` Jason Merrill
2024-05-01 21:26               ` Mark Wielaard
2024-05-01 22:01                 ` Sergio Durigan Junior
2024-05-02 12:54                 ` Claudio Bantaloukas
2024-05-02 15:33                 ` Pedro Alves
2024-05-03  2:59                   ` Ian Lance Taylor
2024-05-04 19:56                 ` Ben Boeckel
2024-05-05  5:22                   ` Benson Muite
2024-05-06 13:58                     ` Ben Boeckel
2024-05-07 16:26                   ` Joseph Myers
2024-05-01 21:38               ` Jeff Law
2024-05-02  6:47                 ` Richard Biener
2024-05-02 11:29                   ` Ian Lance Taylor
2024-05-02 14:26                   ` Simon Marchi
2024-05-02 11:45                 ` Mark Wielaard
2024-05-01 22:56               ` Tom Tromey
2024-04-23 10:34         ` Florian Weimer
2024-04-22 10:01   ` Mark Wielaard
2024-04-22 13:23     ` Joseph Myers
2024-04-19  9:33 ` Jonathan Wakely
2024-04-22 10:24   ` Mark Wielaard
2024-04-22 11:40     ` Jonathan Wakely
2024-04-23  0:48   ` Frank Ch. Eigler

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DS7PR12MB5765990D14AB84FB00C82180CB182@DS7PR12MB5765.namprd12.prod.outlook.com \
    --to=i@maskray.me \
    --cc=binutils@sourceware.org \
    --cc=gcc@gcc.gnu.org \
    --cc=gdb@sourceware.org \
    --cc=jason@redhat.com \
    --cc=jeffreyalaw@gmail.com \
    --cc=josmyers@redhat.com \
    --cc=jwakely.gcc@gmail.com \
    --cc=libc-alpha@sourceware.org \
    --cc=mark@klomp.org \
    --cc=overseers@sourceware.org \
    --cc=pedro@palves.net \
    --cc=simark@simark.ca \
    --cc=tom@tromey.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).