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

Backport scripts #335

Merged
merged 32 commits into from Jan 25, 2019

Conversation

Projects
None yet
2 participants
@Mte90
Copy link
Collaborator

Mte90 commented Jan 16, 2019

Description

Search a commit by the ticket number and merge in ClassicPress

How has this been tested?

#332, #333

How use it

Execute wp-merge-to-cp.sh, in case of conflicts fix it. Do a git add ... to add the changes and execute rename-wp-commit.sh that will change also the commit text and push on your fork.

Execute bin/backport-wp-commit.sh NNNN where NNNN is a WordPress changeset (commit) number.

  • In case of conflicts:
    • Fix the conflicts in your editor
    • Run git add .
    • Run git cherry-pick --continue and use the existing commit message
  • Push to your fork and submit a PR.

@Mte90 Mte90 force-pushed the Mte90:backport-script branch from 395e0ff to 1d1d941 Jan 16, 2019

Show resolved Hide resolved bin/wp-merge-to-cp.sh Outdated
Show resolved Hide resolved bin/wp-merge-to-cp.sh Outdated
Show resolved Hide resolved bin/wp-merge-to-cp.sh Outdated
Show resolved Hide resolved bin/rename-wp-commit.sh Outdated
@nylen

This comment has been minimized.

Copy link
Member

nylen commented Jan 16, 2019

Thanks for doing this, it will save some time once it's finished. However we still need to be much more precise about how we do the backports, and work from WP changesets instead of tickets.

@nylen

This comment has been minimized.

Copy link
Member

nylen commented Jan 16, 2019

Also: it should be possible to combine these two scripts into one.

We can use GIT_EDITOR=./backport-add-merge-message.sh git cherry-pick --edit ...

See https://git-scm.com/docs/git-cherry-pick#_options

The backport-add-merge-message.sh is not really an "editor", it's a script that we can write that just adds a line to the message.

@Mte90

This comment has been minimized.

Copy link
Collaborator Author

Mte90 commented Jan 18, 2019

It is time for testing :-)

For the GIT_EDITOR I think that can break the local one configured in the process or in case we another script that is likea watcher for changes that automatically do git add and when there are no conflicts execute the other script.

Mte90 added some commits Jan 18, 2019

@Mte90

This comment has been minimized.

Copy link
Collaborator Author

Mte90 commented Jan 18, 2019

Tested locally (infact the push is commented for the moment).

./bin/wp-merge-to-cp.sh 44297 master
Fix the conflicts and
git add src/wp-admin/includes/post.php
run
./bin/rename-wp-commit.sh 44297 master

Check with git log the last commit in the branch merge/wp-r44297.

I am only wondering if we want that the pr start with WP-[changeset] instead of WP-[ticket] but as we putting the link to everything in the commit message I think that changeset is fine enough.

@nylen

This comment has been minimized.

Copy link
Member

nylen commented Jan 18, 2019

For the GIT_EDITOR I think that can break the local one configured in the process

I guess you didn't understand what I meant :)

Do you mind if i try it out here on this branch?

Show resolved Hide resolved bin/wp-merge-to-cp.sh Outdated
Show resolved Hide resolved bin/wp-merge-to-cp.sh Outdated
Show resolved Hide resolved bin/wp-merge-to-cp.sh Outdated
Show resolved Hide resolved bin/wp-merge-to-cp.sh Outdated
Show resolved Hide resolved bin/wp-merge-to-cp.sh Outdated
Show resolved Hide resolved bin/wp-merge-to-cp.sh Outdated
Show resolved Hide resolved bin/wp-merge-to-cp.sh Outdated
Show resolved Hide resolved bin/wp-merge-to-cp.sh Outdated
Show resolved Hide resolved bin/wp-merge-to-cp.sh Outdated
@nylen

This comment has been minimized.

Copy link
Member

nylen commented Jan 19, 2019

@Mte90 I made some updates here.

This is a more complicated script, but I fixed everything I could think of. It also edits the commit message automatically now:

  • transform [12345] into WP changeset link
  • transform #12345 into WP ticket link
  • mark Props lines as coming from WP so we can identify them later
  • remove git-svn-id: line
  • remove duplicate blank lines
  • add Merges ... to ClassicPress line for tracking backported commits

Please test and let me know if you see any issues. Otherwise I think this will save us a lot o time in the future.

Show resolved Hide resolved bin/backport-wp-commit.sh Outdated
@Mte90

This comment has been minimized.

Copy link
Collaborator Author

Mte90 commented Jan 19, 2019

Wow basically you rewritten everything!
With a bit of perl and a lot of other git command that I didn't know.
I will test soon :-)

@nylen

This comment has been minimized.

Copy link
Member

nylen commented Jan 19, 2019

Ok. I don't know if I explained this part very well, so now there is one script that does everything including editing the commit message.

Also I don't think it's a good idea for this script to push to GitHub automatically, users should be able to inspect the commits first.

@Mte90

This comment has been minimized.

Copy link
Collaborator Author

Mte90 commented Jan 21, 2019

screenshot_20190121_110537

This is what I get and seems that everything works.
I am only wondering if we need all this verbosity , also if it is better that the command to do in case of conflicts are more easy to see in the output (maybe with more space at left).

@nylen

This comment has been minimized.

Copy link
Member

nylen commented Jan 21, 2019

Thanks for testing, you're right, it is a bit hard to read the default output of the script because there is a lot of it.

I am only wondering if we need all this verbosity

We can hide all the stuff that starts with + or >, and add a --verbose / -v flag that shows it again. What do you think?

It's also a good idea to show the output of those commands if it fails, but this shouldn't be too hard either.

also if it is better that the command to do in case of conflicts are more easy to see in the output (maybe with more space at left).

I don't think we should add more spaces to the output of git status, because then we will lose its colored output.

How about if we make this part show in red instead?

=======
WARNING: Conflict detected!
Fix and commit the files marked as 'both modified' before proceeding:
=======

If we hide extra output with --verbose / -v this will also help make it easier to read.

@Mte90

This comment has been minimized.

Copy link
Collaborator Author

Mte90 commented Jan 21, 2019

Parameter for verbosity isn't a bad idea.
For commands like download we can use > /dev/null that redirect only the stdout and not stderr, so in case of errors they will be printed.

If we can change colors is not a bad idea after all will improve a lot the readability of the output.

@nylen

This comment has been minimized.

Copy link
Member

nylen commented Jan 22, 2019

For commands like download we can use > /dev/null that redirect only the stdout and not stderr, so in case of errors they will be printed.

I did something a little different with the same goal. When a command fails, I am showing the output and the stderr together.

Output if there is no conflict:

Output if there is a conflict:

Help output:

$ bin/backport-wp-commit.sh
Usage: bin/backport-wp-commit.sh [-c] [-v] WP_CHANGESET_NUMBER

  WP_CHANGESET_NUMBER   The SVN changeset number to port.  If this change depends
                        on other changes, make sure they have already been ported.
  -c, --current-branch  Apply the commit directly to the current branch instead of
                        creating a new branch.
  -v, --verbose         Show intermediate commands and their output.
@nylen

This comment has been minimized.

Copy link
Member

nylen commented Jan 24, 2019

@Mte90 do you have any further feedback here? if this looks OK as a starting point then let's get it merged and start using it.

@Mte90

This comment has been minimized.

Copy link
Collaborator Author

Mte90 commented Jan 25, 2019

for me is good :-)

@nylen nylen merged commit 8da55f9 into ClassicPress:develop Jan 25, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment