Skip to content

Commit

Permalink
package-lock.json: remove obsolete fluent-syntax (#1818)
Browse files Browse the repository at this point in the history 
This should've been removed as part of c52a603.
  • Loading branch information
@julen
julen authored Jan 17, 2021
1 parent 4add156 commit 7a7a787
Showing 1 changed file with 0 additions and 5 deletions.
5 changes: 0 additions & 5 deletions package-lock.json
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 comments on commit 7a7a787

@julen
Copy link
Collaborator Author

@julen julen commented on 7a7a787 Jan 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mathjazz I ignore how commits are merged to master (I sense it's squash+merge) but I notice it causes metadata loss unfortunately. Compare the headers of these two commits:

  1. Commit in master: https:/mozilla/pontoon/commit/7a7a787bff3ba31999f206615ac08a589d1ae848.patch
  2. Original commit in PR: https://patch-diff.githubusercontent.com/raw/mozilla/pontoon/pull/1818.patch

Date and author differ. I'm assuming the date in the first commit reflects the new squashed commit, which technically makes sense given that it's a different commit. Nonetheless, the username metadata is totally wrong with respect to the original intent, and can cause incorrect attribution or exposure of sensitive email addresses (the squashed commit info comes straight from GitHub as explained at isaacs/github#1368, which is very unfortunate).

So would it be possible to avoid this in the future and keep the original author metadata, please? 🙏

@mathjazz
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As part of the review process, we ask contributors to send followup changes as additional commits, which makes it easier to review subsequent changes and follow the discussion in the PR in retrospect. It doesn't make much sense for all of these commits to be pushed to master separately. So we instead "Squash and merge" commits into a single commit with a reference to the PR in its commit message, which allows us to follow the "1 bug - 1 PR - 1 commit" principle. And we follow it most of the time.

I see your point, though. That's very unfortunate. I assume "Rebase and merge" would work as expected in the case like #1818?

@julen
Copy link
Collaborator Author

@julen julen commented on 7a7a787 Jan 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if "Rebase and merge" suffers from the same issue or not. For sure no local git client will have this problem, regardless of how commits are incorporated to the master branch.

Please sign in to comment.