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

Change git-cp to use cleaner branch approach #1169

Merged
merged 5 commits into from
Oct 8, 2024
Merged

Conversation

MattiasEh
Copy link
Contributor

Change git-cp to use cleaner branch approach, per https://stackoverflow.com/a/46484848/32841 and https://devblogs.microsoft.com/oldnewthing/20190919-00/?p=102904

This approach avoids the need for messy temporary files and therefore eliminates the class of problems related to those--which means that this resolves #1090. It also makes for a cleaner history (at least IMHO) and should affect the original branch atomically.

My changes seem to work well for me, but I don't have a great way to test regressions for this fairly significant approach-change.

@MattiasEh
Copy link
Contributor Author

@spacewander I'd appreciate any suggestion you have for moving this forward. I tried to follow the contribution instructions but I may have gotten something wrong. Also, I am confused by the lint check error:

Run masesgroup/retrieve-changed-files@v3
Base commit: 5c3fb1b
Head commit: 83cb9c3
Error: The head commit for this pull_request event is not ahead of the base commit. Please submit an issue on this action's GitHub repo.

Thanks in advance!

@spacewander
Copy link
Collaborator

@MattiasEh
Would you merge the main branch and check the CI is passed?

Maybe we can change our CI code according to the jitterbit/get-changed-files#19 (comment). (It will go as another pull request)

bin/git-cp Outdated
git merge $MERGE_OPT "${DESTINATION_SAVED}" "${INTERMEDIATE_SAVED}" -m "Duplicate ${CURRENT_FILENAME} history."
# Switch to the original branch and merge this back in.
git checkout -
git merge --no-ff "$BRANCH_NAME" -m "Copying $CURRENT_FILENAME into $DESTINATION_FILENAME"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This produces a commit with the repeated message (there is already a message Copying .... before it).
Could we optimize it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd be happy to change the commit messages to be more descriptive. The reason I made them like this is because this merge message is the key one directly on the branch, so I figured it should be a summary message for the whole operation.

Here's an example of what results when I used this version of git-cp, run thrice, to turn one file into four copies--setting up for a commit that prunes down all four into just the parts each should keep:
image

With this context, what do you think are the best messages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you prefer these messages?
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

These messages look good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! I have pushed the better commit messages and taken this PR out of Draft. Thanks, @spacewander .

@MattiasEh MattiasEh marked this pull request as ready for review October 6, 2024 18:29
@spacewander spacewander merged commit 233686d into tj:main Oct 8, 2024
5 checks passed
@spacewander
Copy link
Collaborator

@MattiasEh
Merged. Thanks!

@MattiasEh
Copy link
Contributor Author

@MattiasEh Merged. Thanks!

Happy to help! And thank you, @spacewander !

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.

2 participants