Skip to content

Change git-cp to use cleaner branch approach#1169

Merged
spacewander merged 5 commits intotj:mainfrom
MattiasEh:main
Oct 8, 2024
Merged

Change git-cp to use cleaner branch approach#1169
spacewander merged 5 commits intotj:mainfrom
MattiasEh:main

Conversation

@MattiasEh
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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)

Comment thread 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@spacewander
Copy link
Copy Markdown
Collaborator

@MattiasEh
Merged. Thanks!

@MattiasEh
Copy link
Copy Markdown
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