Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore(.gitattributes): add merge settings for .gas-report and .gas-sn… #32

Closed
wants to merge 1 commit into from
Closed

chore(.gitattributes): add merge settings for .gas-report and .gas-sn… #32

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitattributes
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
lib/** linguist-vendored

* text=auto eol=lf

.gas-report merge=ours
.gas-snapshot merge=ours
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think these changes make sense.
Here's my reasoning:

  1. First of all, merge=ours keeps what is in the currently checked out branch. Meaning, if you're on master and your say git merge feature, it'll keep the generated reports from master (this is likely the other way around when rebasing, in which case this would make more sense however
  2. Even in the case of rebasing, you'll get false positives. Here's how:

For example, say master has evolved with changes that had an impact on gas, so the reports are updated. In feature, which is not yet rebased on top of master you also made changes that impact gas, so in that branch, reports are also changed but in a different way than in master.

Now you decide to rebase your feature on top of master. These changes will keep the reports from feature regardless of what's in master.

The problem now is that your gas reports only include numbers that didn't take into account the changes from master and your reports are flawed. So you never have a guarantee that your report is indeed up-to-date unless you run the gas report command again.

So in short:

  1. Depending on workflow merge=ours gives you the wrong report
  2. Even if workflow and report is aligned, it'll potentially be out of date
  3. To be really sure you have the latest numbers, you'll have to run gas report after rebase

I don't think we should add this here.

@gravityblast you opinion please.

Copy link
Author

Choose a reason for hiding this comment

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

How can we solve that? I want to stop merge errors on those files.

As I see, when we are in a branch, the changes there contain the most recent reports

Copy link
Collaborator

Choose a reason for hiding this comment

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

As I see, when we are in a branch, the changes there contain the most recent reports

I've tried to explain above. This is not always the case.
Any scenario in which master has evolved with changes that have impact on gas, will result in an out of date gas report after rebase+merge because your feature branch didn't have those changes.

You need to run gas report generation again to be sure you have the latest numbers.

How can we solve that? I want to stop merge errors on those files.

The way I do it is, when there's a merge conflict due to gas reports, I re-run gas report generation and continue the rebase. Repeat until done.

Then you always have the latest state of gas report ensured.

Loading