Page MenuHomeFreeBSD

net-mgmt/argus3-clients: Convert to OPTIONSNG
ClosedPublic

Authored by bofh on Jan 10 2015, 10:23 AM.

Details

Reviewers
bapt
marino
Summary
  • Pass Maintainership
  • Wrap conditional DOCS installation

PR: 196550
Differential Revision: https://reviews.freebsd.org/DXXXX
Submitted by: pauls@utdallas.edu
Approved by: xxxx(mentor)

Test Plan

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

bofh updated this revision to Diff 3086.Jan 10 2015, 10:23 AM
bofh retitled this revision from to net-mgmt/argus3-clients: Pass Maintainership.
bofh updated this object.
bofh edited the test plan for this revision. (Show Details)
bofh added reviewers: bapt, marino.
bofh set the repository for this revision to rP FreeBSD ports repository.
marino edited edge metadata.Jan 10 2015, 10:34 AM

The logic is different. Before it installed the manpages only if mysql option was selected, now it's doing it all the time. Was it wrong before and now it's being corrected?

Also, the "pass maintainership" is the least important change here, so it shouldn't be the title. I'd probably choose the conversion of options as the main change for the title.

bofh added a comment.Jan 10 2015, 10:42 AM
In D1479#4, @marino wrote:

The logic is different. Before it installed the manpages only if mysql option was selected, now it's doing it all the time. Was it wrong before and now it's being corrected?

There were two .if ${PORT_OPTIONS:MMYSQL} blocks in do-install, I have merged it into one. Unfortunately the patch file is not showing the first appearance of .if ${PORT_OPTIONS:MMYSQL}.

Also, the "pass maintainership" is the least important change here, so it shouldn't be the title. I'd probably choose the conversion of options as the main change for the title.

But the bugzilla ticket is only for passing maintainership. So which one gets higher preference?

There were two .if ${PORT_OPTIONS:MMYSQL} blocks in do-install, I have merged it into one. Unfortunately the patch file is not showing the first appearance of .if ${PORT_OPTIONS:MMYSQL}.

ok. I thought that might be a possibility based on the commit message text.

Also, the "pass maintainership" is the least important change here, so it shouldn't be the title. I'd probably choose the conversion of options as the main change for the title.

But the bugzilla ticket is only for passing maintainership. So which one gets higher preference?

The most important change.
Remember, you can have a many-to-1 relationship with PRs to commits. I could combine 3 PRs into a single commit, and I'm going to write a commit message that describes the commit as a unit. The listed PRs are simply reference material. In your case, you are improving the port and "throwing in" the change of maintainer as a minor addition.

bofh retitled this revision from net-mgmt/argus3-clients: Pass Maintainership to net-mgmt/argus3-clients: Convert to OPTIONSNG.Jan 10 2015, 11:17 AM
bofh updated this object.
bofh edited edge metadata.
In D1479#6, @marino wrote:

Also, the "pass maintainership" is the least important change here, so it shouldn't be the title. I'd probably choose the conversion of options as the main change for the title.

But the bugzilla ticket is only for passing maintainership. So which one gets higher preference?

The most important change.
Remember, you can have a many-to-1 relationship with PRs to commits. I could combine 3 PRs into a single commit, and I'm going to write a commit message that describes the commit as a unit. The listed PRs are simply reference material. In your case, you are improving the port and "throwing in" the change of maintainer as a minor addition.

Thanks for the details.

marino accepted this revision.Jan 10 2015, 11:21 AM
marino edited edge metadata.

When the PR doesn't cover all the changes, it's helpful to indicate what changes belong to the PR with [1], [2], [3], etc, markers

e.g.

net-mgmt/argus3-clients: Convert to OPTIONSNG

- Pass Maintainership [1]
- Wrap conditional DOCS installation

PR: 196550 [1]
Differential Revision: https://reviews.freebsd.org/DXXXX
Submitted by: pauls@utdallas.edu
Approved by: marino(mentor)

So I think you should make that change to the commit message.

This revision is now accepted and ready to land.Jan 10 2015, 11:21 AM
bofh closed this revision.Jan 10 2015, 11:26 AM

That was "approval contingent" on the change I mentioned in the comment. I guess you didn't see the comment, only the approval? It was committed without the [1] marker.

bofh added a comment.Jan 10 2015, 11:34 AM

Sorry, my mistake. I didn't see the comment.