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

Stream Mapping mode #86

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

notdanhan
Copy link

@notdanhan notdanhan commented Oct 12, 2024

this allows you to use Stream view mappings as per the request of
#83

Built and runs on WSL and OpenSuse Tumbleweed

Copy link

Thanks for the contribution! Before we can merge this, we need @notdanhan to sign the Salesforce Inc. Contributor License Agreement.

@notdanhan
Copy link
Author

I signed the CLA and the bot crashed?

@@ -1,177 +1,179 @@
/*
Copy link
Author

Choose a reason for hiding this comment

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

the clang_format file triggered this noise

std::string GetFlushRate() const { return GetParameter("--flushRate"); };
std::string GetNoColor() const { return GetParameter("--noColor"); };
std::string GetNoMerge() const { return GetParameter("--noMerge"); };
std::string GetStreamMappings() const { return GetParameter("--streamMappings"); };
Copy link
Author

Choose a reason for hiding this comment

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

the change in this file

PRINT("Excluded paths: " << exclusions.size());
if (maxChanges > -1)
{
ERR("MaxChanges and StreamMappings cannot be used together! exiting.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a specific reason for this? If this is enabled, you wouldn't be able to clone the depot in batches and instead it will always need to be done in a single shot. This could be a problem, but I guess we don't have anyone asking for batches right now except for Sourcegraph

Copy link
Author

Choose a reason for hiding this comment

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

I suppose it'll probably work as-is with the max changes stuff since they're sorted in together, I'll have to add extra logic for pruning the vector of ChangeLists at the end, shouldn't take too long to do.

PRINT("Path: " << mapped.stream2 << " has " << temp.size() << " changes!");
changes.insert(changes.end(), std::make_move_iterator(temp.begin()), std::make_move_iterator(temp.end()));
}
std::sort(changes.begin(), changes.end());
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a genius way of sorting CLs coming from different streams. Amazing!

Copy link
Contributor

@twarit-waikar twarit-waikar left a comment

Choose a reason for hiding this comment

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

Other than this, some tests for the new STDHelper you added would be great!

Thanks a lot for working on this feature!

@@ -287,7 +373,7 @@ int Main(int argc, char** argv)
cl.WaitForDownload();

std::string fullName = cl.user;
std::string email = "deleted@user";
std::string email = "deleted@user.com";
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should make any changes to this string. This is very much legacy at this point which we can't break for people already using this string to detect deleted users in the generated Git history.

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

Successfully merging this pull request may close these issues.

2 participants