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

Expand Repository.MergeMutable to cover more fields #684

Merged
merged 1 commit into from
Mar 14, 2024

Conversation

isker
Copy link
Contributor

@isker isker commented Nov 8, 2023

There was no test coverage, so I added some.

Closes #683.

@isker
Copy link
Contributor Author

isker commented Nov 8, 2023

I've only handled the few fields that look trivially okay, but MergeMutable might want to handle more:

  • Source (should be mutable just like URL?)
  • priority (should be mutable? can ignore RawConfig entries named "priority"?)
  • Rank (sounds like it should be mutable)
  • LatestCommitDate (? but maybe this is redundant with Branches, one should not change without the other)

And I'm not sure about the two Tombstone fields.

Even if some of those should be added, I am also fine to stop with these if you prefer 🌞.

@keegancsmith keegancsmith requested a review from a team November 8, 2023 05:00
Copy link
Member

@keegancsmith keegancsmith left a comment

Choose a reason for hiding this comment

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

I confirmed that templates and URL are set before we call anything that goes down this path in everything. However, in the case of sourcegraph's indexserver we don't have those fields set so this will misfire. So can't land this just yet, but that is on us. I will take a look soon.

  • Source (should be mutable just like URL?)
  • LatestCommitDate (? but maybe this is redundant with Branches, one should not change without the other)

These should only change if we reindex content. So should ignore it in this function.

  • priority (should be mutable? can ignore RawConfig entries named "priority"?)
  • Rank (sounds like it should be mutable)

Yeah these indeed are mutable. But I'd need to read some more code to work out if its safe to change, lets keep this PR to just the fields you changed.

And I'm not sure about the two Tombstone fields.

This you should ignore as well.

@isker
Copy link
Contributor Author

isker commented Nov 27, 2023

@keegancsmith any update?

@keegancsmith
Copy link
Member

@keegancsmith any update?

Apologies mostly away from keyboard this week (team meetup). On my list to tackle for next week.

@isker
Copy link
Contributor Author

isker commented Jan 1, 2024

@keegancsmith happy new year 😄.

There was no test coverage, so I added some.

Closes sourcegraph#683.
@isker isker force-pushed the expand-mergemutable branch from 6101c5e to 6f2e25b Compare March 8, 2024 01:30
@isker
Copy link
Contributor Author

isker commented Mar 8, 2024

@keegancsmith I've fixed the merge conflicts that developed on this. Please take a look.

@keegancsmith
Copy link
Member

So sorry for dropping this. We welcome open source contributions and I've dropped the ball here. Will take a look today.

@keegancsmith keegancsmith self-assigned this Mar 14, 2024
Copy link
Member

@keegancsmith keegancsmith left a comment

Choose a reason for hiding this comment

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

LGTM. I tested this a bunch and how it interacts with Sourcegraph. It all just works because in sg we never set URL or template fields. Sorry again about the delays.

@keegancsmith keegancsmith merged commit 4448a45 into sourcegraph:main Mar 14, 2024
8 checks passed
@isker
Copy link
Contributor Author

isker commented Mar 14, 2024

No worries, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Repository.MergeMutable doesn't handle changes to *Template fields
2 participants