Opened 7 weeks ago

Closed 2 weeks ago

#31314 closed enhancement (implemented)

Modify git-merge-forward.sh so it can create test merge branches

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: git-scripts, network-team-roadmap-october
Cc: gaba Actual Points: 0.8
Parent ID: #31178 Points: 1
Reviewer: asn Sponsor: Sponsor31-must

Description

We should modify git-merge-forward.sh so it takes an optional branch base name.

If the branch base name is set to bug1234, the script should:

  1. Create bug1234_029 from the local maint-0.2.9
  2. Create bug1234_035 from the local maint-0.3.5, and merge bug1234_029 into it
  3. Repeat step 2 with each supported maint branch, and master

We might also want to modify git-push-all.sh to take a branch base name, and push the branches created by git-merge-forward.sh.

Child Tickets

Change History (16)

comment:1 Changed 7 weeks ago by teor

Sponsor: Sponsor31-can

Gaba, I think this ticket was on our roadmap for sponsor 31?

comment:2 Changed 7 weeks ago by teor

Cc: gaba added

comment:3 Changed 7 weeks ago by gaba

Teor: I'm not seeing this ticket in the roadmap that we drafted in July.

comment:4 in reply to:  3 Changed 7 weeks ago by teor

Parent ID: #31178

Replying to gaba:

Teor: I'm not seeing this ticket in the roadmap that we drafted in July.

It's part of #31178.

I'm not sure if we will do the other part of #31178, which is the automatic pull requests.

comment:5 Changed 6 weeks ago by teor

Actual Points: 0.6
Milestone: Tor: 0.4.2.x-final
Status: assignedneeds_review

Please see my pull request:

It is based on #29879, because they both modify git-push-all.sh.

Here is an example:

$ cd maint-0.2.9
# goes to the backport directory
$ git checkout ticket99999_backport
# checks out the backport branch
$ git-merge-forward.sh -t ticket99999
# creates ticket99999_029 from ticket99999_backport
# merges forward into ticket99999_035 ... ticket99999_master
$ git-push-all.sh -t ticket99999 -r gh-rw
# pushes ticket99999_029 ... ticket99999_master to remote gh-rw

comment:6 Changed 6 weeks ago by teor

Sponsor: Sponsor31-canSponsor31-must

Match parent sponsor must.

comment:7 Changed 6 weeks ago by teor

Keywords: network-team-roadmap-october added

comment:8 Changed 5 weeks ago by teor

I added some extra features, see the changes file for details.

comment:9 Changed 5 weeks ago by teor

Actual Points: 0.60.8

comment:10 Changed 5 weeks ago by asn

Reviewer: asn

comment:11 Changed 4 weeks ago by asn

Status: needs_reviewneeds_revision

Hmm, I'm missing some context on what this feature is for and who is supposed to use it and for which use case. That'd help me with review and testing here.

Also should there be some documentation of how this test branch feature should work? I see some comments about "test branch mode" in git-merge-forward.sh but I don't quite understand how it works. The "test branch example" section seems useful, but it would be nice to have it in words too.

Finally, any ideas on how to test this? I feel more confident testing a shell script than just reading shell code.

Marking as needs_revision so that the above questions get answered to some capacity.

PS: I'm leaving for camp tomorrow morning so I will probably not get to review this whole thing this week, and it might drag into next week. I have ACKed #29879 in the meanwhile.

comment:12 in reply to:  11 Changed 4 weeks ago by teor

Status: needs_revisionneeds_review

Replying to asn:

Hmm, I'm missing some context on what this feature is for and who is supposed to use it and for which use case. That'd help me with review and testing here.

Users:

  • anyone who writes a code that is backported
  • anyone who merges backports (including alpha backports)

Use case:

  • you have a backport commit, but you want to test if it can be merged forward to master
  • you want to run CI on the merged forward branches, without pushing upstream

Also should there be some documentation of how this test branch feature should work? I see some comments about "test branch mode" in git-merge-forward.sh but I don't quite understand how it works. The "test branch example" section seems useful, but it would be nice to have it in words too.

The script generates creates new branches using the commits that are currently checked out in each worktree, and then merged the worktree forward into those branches.

Basically, you get the same merges as the standard merge forward script. But the merge forward creates new branches, instead of modifying maint-* and master.

Finally, any ideas on how to test this? I feel more confident testing a shell script than just reading shell code.

Make a change to your maint-0.2.9 worktree, commit it, then run the script with "-t test_branch". Make source you have a bunch of new branches test_branch_029,035,…,master that are merged forward correctly.

Marking as needs_revision so that the above questions get answered to some capacity.

Once you're happy with the answers I can add some more docs to the script.

Last edited 4 weeks ago by teor (previous) (diff)

comment:13 Changed 3 weeks ago by asn

Status: needs_reviewneeds_revision

I'm happy with the answers.

I did some testing with -t and -u and it seems to work fine. The code also seems fine. I was mostly worried about opening up to command injection issues based on the more complicated cmd evaluations, but I did not find anything suspicious, given that attacker-controlled arguments are branch names which can't contain special characters.

I noticed that shellcheck is giving out a failure that we might want to fix:

In ./scripts/git/git-merge-forward.sh line 360:
  printf "%s Handling branch \\n" "$MARKER" "${BYEL}$target${CNRM}"
                                                    ^-- SC2154: target is referenced but not assigned.

Also, a bit more docs to document the use of the test branch mode would be nice.

Other than that, looks good to me!

Marking as needs_revision for the above.

comment:14 in reply to:  13 Changed 3 weeks ago by teor

Status: needs_revisionneeds_review

See my new pull request:

I had to rebase it, because of the shellcheck fixes in #31519.

664e6a3 is the first new commit, the rest are rebases of the old PR.

Replying to asn:

I was mostly worried about opening up to command injection issues based on the more complicated cmd evaluations, but I did not find anything suspicious, given that attacker-controlled arguments are branch names which can't contain special characters.

The branch names are controlled by the "-t" argument, which is supplied by the person running the script.

But I quoted as many arguments as possible, see d0e31b4.

I noticed that shellcheck is giving out a failure that we might want to fix:

In ./scripts/git/git-merge-forward.sh line 360:
  printf "%s Handling branch \\n" "$MARKER" "${BYEL}$target${CNRM}"
                                                    ^-- SC2154: target is referenced but not assigned.

Fixed in 340ff7f.

Also, a bit more docs to document the use of the test branch mode would be nice.

See 664e6a3.

I also did some more shellcheck fixes, see e155598.

comment:15 Changed 2 weeks ago by asn

Status: needs_reviewmerge_ready

That's great! Thanks a lot!

comment:16 Changed 2 weeks ago by nickm

Resolution: implemented
Status: merge_readyclosed

Merged to master!

Note: See TracTickets for help on using tickets.