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

Task #1178 problem with Commit Synchronization in Collaborative Environment #1179

Conversation

DimitriDiantos
Copy link

To resolve this, I suggest reviewing the code behind the "commit project to repository" button, ensuring that it performs an update before each commit.

Copy link

codecov bot commented Mar 22, 2024

Codecov Report

Merging #1179 (3658d6f) into development (8027909) will decrease coverage by 0.06%.
Report is 2 commits behind head on development.
The diff coverage is n/a.

❗ Current head 3658d6f differs from pull request most recent head 161fb89. Consider uploading reports for the commit 161fb89 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                @@
##             development    #1179      +/-   ##
=================================================
- Coverage          86.90%   86.85%   -0.06%     
+ Complexity          5147     5144       -3     
=================================================
  Files                629      629              
  Lines              25659    25659              
  Branches            2331     2331              
=================================================
- Hits               22298    22285      -13     
- Misses              2578     2590      +12     
- Partials             783      784       +1     

see 5 files with indirect coverage changes

@franzTobiasDLR
Copy link
Member

If your not yet done with your feature, please add the "Draft:" thing to the title of the PR, so that it is clear that it should not yet be merged... And only add us as reviers once our done ;) Otherwise we don't know when do look at the changes... ;)

@DimitriDiantos DimitriDiantos changed the title Task #1178 problem with Commit Synchronization in Collaborative Environment WIP #1178 problem with Commit Synchronization in Collaborative Environment Mar 28, 2024
@DimitriDiantos DimitriDiantos marked this pull request as draft March 28, 2024 15:09
@DimitriDiantos DimitriDiantos changed the title WIP #1178 problem with Commit Synchronization in Collaborative Environment Task #1178 problem with Commit Synchronization in Collaborative Environment Mar 28, 2024
@DimitriDiantos DimitriDiantos marked this pull request as ready for review April 10, 2024 08:21
@Override
protected void doCommit(IProject project, String message, IProgressMonitor monitor) throws Exception {

// Update before committing
updateBeforeCommit(project, monitor);
Copy link
Member

Choose a reason for hiding this comment

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

I think if you implement it here, it only works for git not SVN... Maybe move the implementation to the AVersionControlCommitHandler?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree to that

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your comments. i moved it in this class.

@@ -43,7 +43,7 @@ protected void doUpdate(IProject project, IProgressMonitor monitor) throws Excep

Repository gitRepository = RepositoryMapping.getMapping(project).getRepository();

// Build the dialog in the UI thread
// Build the dialog in the UI threads
Copy link
Contributor

@dellerDLR dellerDLR Apr 11, 2024

Choose a reason for hiding this comment

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

I think the old comment is right, there is just one UI thread :)

Copy link
Author

Choose a reason for hiding this comment

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

Yes, i think too.

@Override
protected void doCommit(IProject project, String message, IProgressMonitor monitor) throws Exception {

// Update before committing
updateBeforeCommit(project, monitor);
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree to that


// If the commit message contains the phrase "Backend Local Commit Before Pull", extract the actual commit message
if (commitMessage.contains("Backend Local Commit Before Pull")) {
commitMessage = commitMessage.replace("Backend Local Commit Before Pull: ", "");
Copy link
Member

Choose a reason for hiding this comment

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

I think you don't need this if-statemenet... If there is nothing like this in the string, nothing gets replaces ;)
But maybe use a string constant for "Back Local...", maybe you can even use the the constant from the implementation...

Copy link
Author

@DimitriDiantos DimitriDiantos Apr 17, 2024

Choose a reason for hiding this comment

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

I think we need it because I removed it and tried to run the test which failed. So I chose to make the string consistent.
1

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of changing (replacing "Backend Local Commit Before Pull: ") the string inside the test, I think the cleaner version would be to change the string in the assert function in the test

Copy link
Author

Choose a reason for hiding this comment

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

Okay @dellerDLR. I did now. You can check now.

@@ -27,34 +27,35 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you can reset this file as you didn't do any changes to it?

Copy link
Author

Choose a reason for hiding this comment

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

I did it :) .

@franzTobiasDLR franzTobiasDLR merged commit ba21d83 into development Oct 8, 2024
12 checks passed
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.

Issue with Commit Synchronization in Collaborative Environment.
4 participants