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

Semantics of DONTBUILD pushes - first line only or entire commit message? #43

Open
jfx2006 opened this issue Aug 10, 2023 · 1 comment

Comments

@jfx2006
Copy link

jfx2006 commented Aug 10, 2023

This push to comm-central was a single commit that included "DONTBUILD" towards the end of its (lengthy) commit message.

This was unexpected, so bug 1847987 was filed.

The expected behavior (from the developer who pushed as well as the patch author) is that only the first line of the commit message is checked for "DONTBUILD". This aligns with how tools such as Lando add "DONTBUILD" to commit messages.

The first line of a commit message (or subject in Git terms apparently) is a common pattern. We see it when viewing the history of a Github repository, or the summary page of a repo on hg.mozilla.org. The reviewer and approver flags on Gecko checkins also appear on the first line, not later in the message.

I checked various places, looking for documented expected behavior of DONTBUILD, and didn't come up with anything.

The current implementations in various taskgraph projects all pretty much look like (example from https://searchfox.org/mozilla-central/rev/503938c13ef2dd174705dc0f6d0683ae43074ccc/taskcluster/gecko_taskgraph/decision.py#404-408):

# ..but can be overridden by the commit message: if it contains the special
# string "DONTBUILD" and this is an on-push decision task, then use the
# special 'nothing' target task method.
if "DONTBUILD" in commit_message and options["tasks_for"] == "hg-push":
    parameters["target_tasks_method"] = "nothing"

Upstream taskgraph is similar, allowing for "github-push" in addition to "hg-push".

My proposal is to change how DONTBUILD pushes are determined by only checking the first line of the commit message.

This would likely involve adding a new method to the Repository abstract class and its children get_commit_message_firstline. Mercurial includes a template filter: hg log -T '{desc|firstline}\n' handles it nicely. For Git: "git log -n1 --format=%s". Alternatively, splitting the entire message on line breaks and taking [0] would likely work, but seems potentially error prone.

Another data point: The "treeclosure" Mercurial server hook (that works with TreeStatus) appears to check the entire message for the magic words "CLOSED TREE" and approver flags. Reference

There is plenty of precedent for checking the entire commit message for these magic strings. I find it unexpected, as did another Thunderbird developer. If this proposal is not implemented, this behavior should at least be documented better.

@jfx2006
Copy link
Author

jfx2006 commented Aug 10, 2023

Additionally, Treescript writes its magic words to the first line of a commit message. Convention and expectations don't line up with implementation.

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

No branches or pull requests

1 participant