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.

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

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

Closed
wants to merge 1 commit into from

Conversation

3esmit
Copy link

@3esmit 3esmit commented Sep 12, 2024

…apshot: merge always everything from merging branch - as this is the real value of current gas costs.

Description

This change is important because we keep having problems with merging because of those files. This change will make the merge automatic if the conflict is only on this file. The settings define that the merging from branch will overwrite the merged to files .gas-snapshot and .gas-report

Checklist

Ensure you completed all of the steps below before submitting your pull request:

  • Added natspec comments?
  • Ran pnpm adorno?
  • Ran pnpm verify?

…apshot: merge always everything from merging branch - as this is the real value of current gas costs.
@@ -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.

@0x-r4bbit
Copy link
Collaborator

Closing this as these changes are introducing other problems.
The solution is to simply regenerate the reports when you run into conflicts.

@0x-r4bbit 0x-r4bbit closed this Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants