Customize or turn off PR chain in autogenerated PR message

[tir38]

From docs for create-pr

If the newly-created PR is stacked atop another PR, the actual PR description posted to GitHub will be prepended with a section listing the entire related chain of PRs.

How can I turn this off?

[PawelLipski]

Hey, there's no switch for turning this off yet... I'll add a git config key. Given high amount of commercial work, I'll only have time for such small accumulated requests around late August/early September... I invite you to provide contribution if you'd like to see the switch rolled out earlier!

[sgregson]

I'd love if this could be done as a user template rather than a flag, what do you think? If this is directionally interesting (I used a Jinja2 template flavor here), I'd be happy to contribute.

Folks who want to suppress the PR message could provide an empty string, while others (myself) could implement a less obtrusive template. The default template would look something like this:

# Based on {{base.pr_number}}

## Full chain of {{pr.short_name}}s as of {{current_date}}

{% for ancestor in pr.chain %}
* {{ancestor.ref}}
  `{{ancestor.head}}` ➔ `{{ancestor.base}}`
{% endfor %}

replacing this code, potentially:

git-machete/git_machete/client.py

Lines 2544 to 2550 in 029cd67

	prepend += f'# Based on {prs_for_base_branch[0].display_text(fmt=False)}\n\n'
	current_date = utils.get_current_date()
	prepend += f'## Full chain of {pr_short_name}s as of {current_date}\n\n'
	for ancestor_pr in pr_path:
	prepend += f'* {ancestor_pr.display_text(fmt=False)}:\n'
	prepend += f' `{ancestor_pr.head}` ➔ `{ancestor_pr.base}`\n'
	prepend += '\n'

[PawelLipski]

Wow this looks nice! just one consideration before we can start, would you be able to easily add this functionality without adding 3rd party deps to git-machete? so far I kept the requirements for production package empty... ofc this rule isn't set in stone, but I'd rather stick to it to avoid unexpected problems in multiple package systems where git-machete is available 😅

[sgregson]

good callout (and props on an empty requirements!) it's very do-able without adding a 3rd party dep, I'd mainly need to solve for (or drop) the ability to customize items of the ancestor list. Jinja templates make that nice/easy with special cases for handling block and inline templates, but it'd be easy enough to make a light version of that with user-provided native python Template strings.

A version like this would treat a substring of the template as the ancestor item template (absolutely open to feedback). For the default template:

# Based on $base_pr_number

## Full chain of ${pr_shortname}s as of $current_date

<ANCESTOR_ITEM_START>
* $ancestor_ref
  `$ancestor_head` ➔ `$ancestor_base`
<ANCESTOR_ITEM_END>

using this git-machete data:

top level variables	variables for the ancestor item region
$base_pr_number	$ancestor_ref
$pr_shortname	$ancestor_head
$current_date	$ancestor_base

implementation brainstorm

Jinja handles fussy distinctions between an inline template (eg. <ANCESTOR_ITEM_START>| `$ancestor_head` ➔ `$ancestor_base` <ANCESTOR_ITEM_END>) and a block template as in the default example. I think a basic implementation of their logic is something like "if the start and end template items are on different lines, trim the whitespace and join each ancestor item with a newline".

I could let the user pass a glue string via the template somehow (eg <ANCESTOR_ITEM_START glue=" | ">) if that doesn't seem too fussy. I'm hoping to only have the one user config setting.

# inline template, glued with empty string
<ANCESTOR_ITEM_START>`$ancestor_head` ➔ `$ancestor_base` / <ANCESTOR_ITEM_END>

# block templates, would be glued with a newline (all three are equivalent)
<ANCESTOR_ITEM_START>* $ancestor_ref
  `$ancestor_head` ➔ `$ancestor_base`<ANCESTOR_ITEM_END>

<ANCESTOR_ITEM_START>
* $ancestor_ref
  `$ancestor_head` ➔ `$ancestor_base`<ANCESTOR_ITEM_END>

<ANCESTOR_ITEM_START>
* $ancestor_ref
  `$ancestor_head` ➔ `$ancestor_base`
<ANCESTOR_ITEM_END>

[PawelLipski]

Jinja templates make that nice/easy with special cases for handling block and inline templates, but it'd be easy enough to make a light version of that with user-provided native python Template strings.

Actually... AFAICS native Python templates only offer simple variable substitution, without loops/conditionals, right? so the loop body would need to be extracted by git-machete code, and only then the extracted body passed to string.Template (?)

Jinja handles fussy distinctions between an inline template

Hard to say if I have any preference here... but I think this should be kept as simple as possible, even at the expense of flexibility (given that it's just rendering of a PR message and not a general-purpose templating engine)

A version like this would treat a substring of the template as the ancestor item template (absolutely open to feedback)

TBH I like Jinja-esque syntax of {% for ancestor in ... %} more... but it's really just cosmetics, to be smoothed out later 🧼

Looking forward to your PR! remember to open on the top of develop (despite master being the default branch in this repo) ⚒️

[chriscz]

This is a great suggestion.

Related to your comment on production dependencies, Pawel, at some point in the future it may be worth generating a binary for machete using something like Nuitka, that way including Jinja should be less of a pain.

[chriscz]

Oh nevermind this. I just saw the README mentioned pex 🤕

[PawelLipski]

WIP for the original request (turn off PR chain) at PR #1306

[PawelLipski]

@tir38 v3.29.0 is about to be released, use git config machete.github.prDescriptionIntroStyle none

[frank-west-iii]

I was going to open a new issue, but this discussion is relevant to my current problem I figured I would tack it on here and move it to a separate issue if deemed appropriate.

As a personal preference the Intro style commenting detracts from the objective of the PR description so it creates a bit more conflict in reading through the what the PR is about from a reviewers standpoint, but I like having it available.

Would you be amenable to adding a config flag to set the description insert mode?

def __get_pr_description_insert_mode_from_config(self, spec: CodeHostingSpec) -> PRDescriptionInsertMode:
    config_key = spec.git_config_keys.pr_description_insert_mode
    return PRDescriptionInsertMode.from_string(
        value=self.__git.get_config_attr(key=config_key, default_value="prepend"),
        from_where=f"`{config_key}` git config key"
    )

The name conflict with the wording Intro causes some mental conflict, but I'm unsure if anything can be done about that. i.e. would be "better" as prDescriptionStyle since this wouldn't always be an intro with an insert mode approach.

[frank-west-iii]

Realizing I left out that the ultimate goal would be to allow appending the description instead of prepending.

[PawelLipski]

Good idea indeed... and in fact I've realized that it's even doable right now with minimal changes to the implementation (in particular, no need to add new config keys). The "intro" (or maybe, generated section) is always placed by git machete git{hub,lab} between <!-- start git-machete generated --> and <!-- end git-machete generated --> HTML comments, so that it can be easily replaced by retarget-pr invocations without complicated parsing.

Still, I've now realized this does not work with create-pr when the markers are already placed in the original description template (e.g. from .github/pull_request_template.md). Lemme fix this overlook + also add relevant docs to make it less a hack and more like an official solution. Issue: #1328

For now, the workaround is to create the PR with git machete github create-pr, let it put the generated section at the beginning, and then move the section around, together with its delimiting comments. All subsequent retarget-prs will keep the section wherever it is.

[PawelLipski]

Fixed in PR #1330, soon to be released as v3.29.3