Use for .. of in more places in JS library. NFC

[sbc100]

No description provided.

[kripken]

@sbc100 @brendandahl apologies, but I am yet again having trouble reading incremental updates to a force-pushed PR. You two pointed out ways to do that in the past but none seem to work here AFAICT?

There are two force-pushes since my review, and buttons "compare" for them:

• https://github.com/emscripten-core/emscripten/compare/677a02b03f1286d5acfe56bb14f42b5f171b0785..b7f1760694305d120be06ef39a52128934f41960
• https://github.com/emscripten-core/emscripten/compare/b7f1760694305d120be06ef39a52128934f41960..e5b12b8c8ab002ac37a636b011baa65c6e6c0525

The first contains changes to EM_JS code - likely from a merge to main? But that makes it hard to see what actually changed for this PR.

The second looks like a small incremental update that I can read.

[sbc100]

Apologies, I promised and failed to try to switch to a non-squashing workflow.

For this PR will will try to split up again (and force push the result) so you will see the delta as a separate PR

[sbc100]

Done. You should see two commits now

[kripken]

Thanks, but even of those last two commits, the first is quite large, and contains lots of stuff I've reviewed already. How can I avoid re-reading code I've already reviewed, in this flow?

It does seem like github could show "diff compared to the previous state", and I thought that was what "show changes" was... but neither that nor the new separate commits appear to be diffs vs previous code?

[sbc100]

Thanks, but even of those last two commits, the first is quite large, and contains lots of stuff I've reviewed already. How can I avoid re-reading code I've already reviewed, in this flow?

The first commit is all the code you reviewed already. The second commit is the new code.

It does seem like github could show "diff compared to the previous state", and I thought that was what "show changes" was... but neither that nor the new separate commits appear to be diffs vs previous code?

That could be an issue with the fact that in this PR you had already looking at the "the previous state".. do you know how github tracks that? Can you look at "diffs vs previous code" more than once.. or is the state permanently changes by the act of looking?

My recollection of the agreement we came to was that I would switch to workflow where I did not squash commits. I did not agree to avoid rebases. So hopefully its not rebasing alone that is confusing github, since switching to a merge workflow locally would be a huge pain for me :(

[kripken]

The first commit is all the code you reviewed already.

How can I tell it is the commit I reviewed before?

I am not being facetious. It is the first commit, yes, but it appears after my review in the github history here. It could be different if you made a mistake somehow?

That could be an issue with the fact that in this PR you had already looking at the "the previous state".. do you know how github tracks that? Can you look at "diffs vs previous code" more than once.. or is the state permanently changes by the act of looking?

I'm not sure how github works. All I did was look at the files in the file tab, and later when I saw you updated, I clicked on the "see changes" links because those were force-pushes.

Surely there is a way to see the actual diff compared to before, but I can't find it...

The closest is the per-file view. If I click "viewed" on a file, it is hidden. It then becomes unhidden if a later push changes it. This is almost what I want... except that a single typo fix makes the entire file appear changed. Still, this option usually saves me a lot of time, and it makes things usable. But PRs like this with many changes to the same file stretch the limits...

Ideally for me, no PR would both

• Have force-pushed squashes
• Have many changes to a single file

PRs that do have both, and that have multiple force-pushes, end up with duplicate work for me.

[sbc100]

The first commit is all the code you reviewed already.

How can I tell it is the commit I reviewed before?

I have no idea. How does github track that? Is their tracking good enough to recognize identical-but-rebased commits? Again, no idea, but I would hope so.

Its not a github feature I use myself.

I am not being facetious. It is the first commit, yes, but it appears after my review in the github history here. It could be different if you made a mistake somehow?

I'm not sure that you mean by "it appears after my review in the github history here". The first commit here is from 3 days ago according my log. Your review looks like it was from 2 hours ago?

I guess github use CommitDate which is different to AuthorDate. AuthorDate is what git itself normally shows in the log. When I do git log on this branch is shows the first commit as from 3 days ago:

commit b2a0fcc1c0160e3947250fcb9dc78676872f145e
Author: Sam Clegg <sbc@chromium.org>
Date:   Fri Mar 7 16:07:14 2025

    Use `for .. of` in more places in JS library. NFC

[sbc100]

Ideally for me, no PR would both

• Have force-pushed squashes

Yes I think this is what we agreed. I'll try to remember not to do this... although its proving hard.

• Have many changes to a single file

I'm not sure we can solve that. I would hope that as long as we don't have force-pushed squashes then you can just look at the individual commits so see the most recent updates to a branch?

I don't know how to automatic that "which commits have I already looked at" thing unless github has some way to track that.

I would hope that in most cases you would just want to look at the most recent change which will likely have "feedback" as its comment :)

[sbc100]

Looks like there is an open issue for "changes since last review" + rebasing: https://github.com/orgs/community/discussions/141845

[kripken]

I'm not sure that you mean by "it appears after my review in the github history here". The first commit here is from 3 days ago according my log. Your review looks like it was from 2 hours ago?

I mean that it appears after it, in the github history here on the main page of the PR, see below image. I believe that correctly represents that you pushed it after my review. Before that, the commits appeared before my review.

START_IMAGE

END_IMAGE

[kripken]

The problem with looking at individual commits as you suggest is that their hashes change with the force-pushes (as shown in that image). So I must either trust they are the same blindly, or I need to manually verify it. This seems less than ideal.

Merges rather than force-pushes avoid this.

I am fine with doing a little more work in general, but when a PR has many changes to a single file that becomes somewhat tedious, and so far I don't see a solution from my side.

As a github reviewer, has this never been an issue for you @sbc100 ?

[kripken]

(regardless of the ongoing discussion, i have re-read relevant parts of this, lgtm)

[kripken]

I suppose another option here is for me to wait on review until the PR is fully ready to land and all tests have passed (so many force-pushes are unlikely). Perhaps in larger PRs we should do that?

[sbc100]

I mean that it appears after it, in the github history here on the main page of the PR, see below image. I believe that correctly represents that you pushed it after my review.

True, but that is a quirk of the github UI. If you use git log to show the same commits you see a different ordering and you see that that commit is from Friday.

I'm not saying your or github is wrong.. just that github could maybe be improved in this area.

[sbc100]

As a github reviewer, has this never been an issue for you @sbc100 ?

Honestly I never trust the github UI to show me changes "since you last looked at it" and be accurate.

Also, I pretty much always want to see the whole change, and never some subset of it. I find that without looking that the whole change I don't get the full context. If and when I end up in the "since you last looked at it" view of a PR it actually frustrates me and I wish it would never do that :)

[sbc100]

But having said that, maybe there are times when I would want compare two different version of a PR. But I would normally want to do that explicitly and with more power than simple the "since you last looked at it" think. Something like gerrit where this is more explicit would be really nice.

[kripken]

Also, I pretty much always want to see the whole change,

What do you do in a large PR that gets lots of updates over time? Re-read the whole thing after each update?

[sbc100]

Also, I pretty much always want to see the whole change,

What do you do in a large PR that gets lots of updates over time? Re-read the whole thing after each update?

Maybe I don't have a lot of experience with such PRs.. but so far I suppose I've mostly chosen to re-read the whole thing each time.

I don't doubt that in such cases being able to see just the fixes would nice though.

Maybe this is one of the reasons I like to make ppl break up their PRs :)

But for PRs such as this one that are inherently wide and large I do so that value in being able to see just what changed. Hopefully just looking at commits individually will be enough going forward? Even though its annoying if github doesn't keep track of which was the last one you reviewed, its certainly better than having them squashed, right ?

[kripken]

But for PRs such as this one that are inherently wide and large I do so that value in being able to see just what changed. Hopefully just looking at commits individually will be enough going forward? Even though its annoying if github doesn't keep track of which was the last one you reviewed, its certainly better than having them squashed, right ?

I don't think the commits individually help me much. If I need to go back to a PR the next day, I wouldn't recall the commit structure from back then. And as mentioned above I'd have no automatic way to know which current commits map to which previous commits, much less be able to tell if the mapping changed somehow (intentionally or not).

So I would end up re-reading it all, at least on a per-file basis. I don't see how to avoid that, unless I write down somewhere "I have read commit 1 and 2 but nothing later" + I also trust that in future force-pushes the the author does not change the structure (does not split 1 into 1a/1b) and does not make any mistakes (adds something to commit 1 that should be in commit 3).

Concretely, though, I think the per-file updates that github has are close to good enough. How about if in large/wide PRs, we do what I suggested before and you ping me for review when they are ready to land, rather than early? If there are no or at least few force-pushes after my review, that seems ok to me. And that way you don't need to make any changes to your workflow.