wip: add footer support #6088

glehmann · 2025-03-19T16:02:55Z

Disclaimer: I open this PR as a way to try a possible implementation for the signed-off-by/change-id/… footer, but this is not ready yet by any means! Moreover, this is my first contribution to jj — you've been warned :-)

I have assumed that all the lines in the footer are of the form a-label-with-dashes: a value up to the end of the line.
That way we can separate a description paragraph that may be just some normal text, and avoid adding the footer to
this last paragraph. For example, in this commit description:

refactor: move X feature to its own crate

Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor
incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis
nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.

I think we don't want the footer to be next to the last paragraph:

refactor: move X feature to its own crate

Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor
incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis
nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.
Signed-off-by: Gaëtan Lehmann <[email protected]>

but rather have it in its own paragraph:

refactor: move X feature to its own crate

Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor
incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis
nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.

Signed-off-by: Gaëtan Lehmann <[email protected]>

and if later we want to add more line to the footer, we want them in the same paragraph:

refactor: move X feature to its own crate

Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor
incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis
nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.

Signed-off-by: Gaëtan Lehmann <[email protected]>
Change-Id: I6a6a6964ebc8c969b282763d28806bfed26bb387
CC: Albert Dupont <[email protected]>

I've never seen a footer paragraph with lines that wouldn't match this format, but I guess that could be possible. In that case, rerunning jj desc would add a new footer paragraph, possibly duplicating the footer lines. We may not want that.

On the UI side, there is a new --add-footer option in jj describe.

jj desc \
    --add-footer '"Signed-off-by: " ++ self.author().name() ++ " <" ++ self.author().email() ++ ">"' \
    --add-footer '"Change-Id: I6a6a6964" ++ self.change_id().normal_hex()' \
    --message "some great stuff"

It works without --message and with --no-edit, so it can be used to add some footer content without changing the
description on the command line or in the editor.

Some predefined templates would probably be useful to make the feature easier to use, for example signed_off_by or
gerrit_change_id.

--add-footer probably needs to be added to other commands (I would say commit, new and split), but then it would
be nice to have a configuration, to define the expected footers in a single place.

ref: #2849
ref: #1399

Checklist

If applicable:

I have updated CHANGELOG.md
I have updated the documentation (README.md, docs/, demos/)
I have updated the config schema (cli/src/config-schema.json)
I have added tests to cover my changes

google-cla · 2025-03-19T16:02:59Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

PhilipMetzger · 2025-03-19T16:45:29Z

Have you seen @thoughtpolice's #2845 patch, where the footer functionality already is implemented?

glehmann · 2025-03-19T16:55:09Z

No, I missed it completely. I'll have a look!

emilazy · 2025-03-19T17:59:37Z

FWIW, Git calls these trailers and has git interpret-trailers, and defines explicit syntax for multiple lines. I believe the Linux kernel log has a lot of examples of such multi‐line trailers.

glehmann · 2025-03-19T19:15:02Z

I'll look at git interpret-trailers too 👍

thoughtpolice · 2025-03-19T19:24:06Z

See also: #5290 which can achieve much of the same thing while making it so that the template language does most of the work. I'd like to migrate things like signed-off-by and Gerrit's Change-Id to this approach...

PhilipMetzger · 2025-03-19T19:45:24Z

See also: #5290 which can achieve much of the same thing while making it so that the template language does most of the work. I'd like to migrate things like signed-off-by and Gerrit's Change-Id to this approach...

We still need someway to work with them in the library though, so taking your footer patch is something to encourage. I also have some downstream work which will profit from it.

glehmann · 2025-03-19T21:12:08Z

See also: #5290 which can achieve much of the same thing while making it so that the template language does most of the work. I'd like to migrate things like signed-off-by and Gerrit's Change-Id to this approach...

Isn't it working only when editing the description in the editor?

I'm almost always writing my commit messages directly in my shell (fish works very well for that), and using the editor only to modify the description. Something like this (a real example from my shell history):

jj desc -m "use hashlib in place of crypt

crypt has been deprecated in python 3.11 and removed in 3.13"

glehmann · 2025-03-23T17:22:00Z

@emilazy thanks for mentioning git interpret-trailers. It's been handy to better understand the git trailer format and parse it.

@PhilipMetzger I've duplicated @thoughtpolice's commit on footer lib in this PR and made the parser more strictly follow git's specification. And of course, I've used it to implement the add footer feature :-)

My next action on this feature would be to add a ui.default_footer = [] configuration, but the settings mechanism is not as clear to me as the command line option, so I'd really like some feedback on that configuration before embarking into the implementation.

martinvonz

How is this feature supposed to be used? Is the user expected to create an alias for adding their common trailers? Or do we expect that they will enter the template on the CLI? Or do we expect that users will usually use the config option? Could we start with just the config option in that case?

lib/src/footer.rs

martinvonz · 2025-03-31T04:23:17Z

cli/src/commands/describe.rs

+    /// If multiple revisions are specified, the same footer line will be used
+    /// for all of them.


clarify that that's only true if the template doesn't depend on the commit (right?)

that's the same template that will be used on all the commit, not the resulting trailer. I'll clarify that.

cli/src/description_util.rs

martinvonz · 2025-03-31T04:26:41Z

cli/src/description_util.rs

+    description: &str,
+    footer_templates: &[String],
+) -> Result<String, CommandError> {
+    if description.is_empty() || footer_templates.is_empty() {


why can you not add trailers without a description?

The description seemed to be trimmed, and thus, the trailer would become the first line of the description.
Also, a commit with no description is treated in a special way in jujutsu — it can be discarded as soon as it's no longer the working copy. Adding a footer to an empty description would break that logic.

martinvonz · 2025-03-31T04:31:02Z

cli/src/commands/describe.rs

+    /// The value is a commit template. The output generated with that template
+    /// must be a valid footer entry.


If the trailer is a template, it means that the user will have to quote string values, which then needs to be shell-quoted to. That may be hard for the user to guess.

martinvonz · 2025-03-31T04:44:38Z

cli/src/description_util.rs

@@ -343,6 +344,16 @@ pub fn add_footer_entries(
    description: &str,
    footer_templates: &[String],
 ) -> Result<String, CommandError> {
+    let default_footer_templates = tx
+        .settings()
+        .get::<Vec<String>>("ui.default-add-footer")


Please add tests

glehmann · 2025-03-31T08:03:02Z

How is this feature supposed to be used? Is the user expected to create an alias for adding their common trailers? Or do we expect that they will enter the template on the CLI? Or do we expect that users will usually use the config option? Could we start with just the config option in that case?

At first, I was thinking of using aliases, but with multiple commands being impacted, that's not very convenient.
The config is much more convenient — that's what I'm using, in combination with a scoped table to only apply it in the project where a DCO is required.

With the addition of the config, I'm not using the command line option anymore. Moreover, I'm not very pleased with how the config and cli option are interacting, so I think it's a good idea to remove the command line option.
Unless someone has a use case where that might be useful.

There is another improvement I'd like to implement: make the footer appear in the description when it is edited. Currently, it's added after the edition. I think it would feel more logical to have it directly in the editor.

glehmann · 2025-03-31T09:31:21Z

There is another improvement I'd like to implement: make the footer appear in the description when it is edited. Currently, it's added after the edition. I think it would feel more logical to have it directly in the editor.

If there is no command line option, and the trailer paragraph is included in the description template, then we don't even need a specific configuration.
It could just be a commit_description_trailers template alias that defaults to an empty string and could be overridden by the user.

[template-aliases]
"commit_description_trailers" = "signed_off_by"

And the template alias would also be used when calling jj desc -m "…" to add the trailer to the commit description.

glehmann · 2025-04-02T08:19:36Z

I've implemented the tailer feature with a template:

[templates]
commit_trailers = "signed_off_by ++ gerrit_change_id"

There are more tests to come. I just wanted to give you a chance to react before spending more time on them.

yuja

I've implemented the tailer feature with a template:
[templates]
commit_trailers = "signed_off_by ++ gerrit_change_id"

cc @thoughtpolice

The idea sounds good to me, thanks. I would port only jj describe in this PR, and split the other command adaptation to separate PR.

cli/src/config/templates.toml

cli/src/commands/describe.rs

lib/src/trailer.rs

To be used for parsing `Change-Id`, `Signed-off-by` and other `Co-authored-by` trailers from commits Co-authored-by: Austin Seipp <[email protected]>

A new `template-aliases.commit_trailer` configuration option allows to define a template for the trailers to add to the descriptn trailer. A new trailer paragraph is created if no trailer paragraph is found in the commit description. The trailer is not added to the trailer paragraph when the trailer is already present, or if the commit description is empty.

glehmann force-pushed the add-footer branch from 7f37f5a to 52d3c2c Compare March 19, 2025 19:16

glehmann force-pushed the add-footer branch 3 times, most recently from 390ab1b to 7971a05 Compare March 23, 2025 17:07

glehmann force-pushed the add-footer branch 2 times, most recently from b6d9c50 to aee3a97 Compare March 29, 2025 23:42

glehmann requested a review from a team as a code owner March 29, 2025 23:42

glehmann force-pushed the add-footer branch from aee3a97 to 0ce383c Compare March 30, 2025 00:08

martinvonz reviewed Mar 31, 2025

View reviewed changes

glehmann force-pushed the add-footer branch 4 times, most recently from e9a36f6 to 90c4591 Compare April 2, 2025 08:15

glehmann force-pushed the add-footer branch from 90c4591 to 504dce3 Compare April 2, 2025 08:23

yuja reviewed Apr 2, 2025

View reviewed changes

cli/src/config/templates.toml Outdated Show resolved Hide resolved

cli/src/commands/describe.rs Outdated Show resolved Hide resolved

lib/src/trailer.rs Outdated Show resolved Hide resolved

glehmann and others added 2 commits April 2, 2025 21:32

lib: add trailer module for commit trailers

4e86f5c

To be used for parsing `Change-Id`, `Signed-off-by` and other `Co-authored-by` trailers from commits Co-authored-by: Austin Seipp <[email protected]>

glehmann force-pushed the add-footer branch from 504dce3 to 329c2d4 Compare April 2, 2025 19:40

glehmann force-pushed the add-footer branch from 329c2d4 to 4d06ac8 Compare April 2, 2025 19:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wip: add footer support #6088

wip: add footer support #6088

glehmann commented Mar 19, 2025

google-cla bot commented Mar 19, 2025

PhilipMetzger commented Mar 19, 2025

glehmann commented Mar 19, 2025

emilazy commented Mar 19, 2025

glehmann commented Mar 19, 2025

thoughtpolice commented Mar 19, 2025

PhilipMetzger commented Mar 19, 2025

glehmann commented Mar 19, 2025

glehmann commented Mar 23, 2025

martinvonz left a comment

martinvonz Mar 31, 2025

glehmann Mar 31, 2025

martinvonz Mar 31, 2025

glehmann Mar 31, 2025

martinvonz Mar 31, 2025

martinvonz Mar 31, 2025

glehmann commented Mar 31, 2025

glehmann commented Mar 31, 2025

glehmann commented Apr 2, 2025

yuja left a comment

		/// If multiple revisions are specified, the same footer line will be used
		/// for all of them.

		/// The value is a commit template. The output generated with that template
		/// must be a valid footer entry.

wip: add footer support #6088

Are you sure you want to change the base?

wip: add footer support #6088

Conversation

glehmann commented Mar 19, 2025

Checklist

google-cla bot commented Mar 19, 2025

PhilipMetzger commented Mar 19, 2025

glehmann commented Mar 19, 2025

emilazy commented Mar 19, 2025

glehmann commented Mar 19, 2025

thoughtpolice commented Mar 19, 2025

PhilipMetzger commented Mar 19, 2025

glehmann commented Mar 19, 2025

glehmann commented Mar 23, 2025

martinvonz left a comment

Choose a reason for hiding this comment

martinvonz Mar 31, 2025

Choose a reason for hiding this comment

glehmann Mar 31, 2025

Choose a reason for hiding this comment

martinvonz Mar 31, 2025

Choose a reason for hiding this comment

glehmann Mar 31, 2025

Choose a reason for hiding this comment

martinvonz Mar 31, 2025

Choose a reason for hiding this comment

martinvonz Mar 31, 2025

Choose a reason for hiding this comment

glehmann commented Mar 31, 2025

glehmann commented Mar 31, 2025

glehmann commented Apr 2, 2025

yuja left a comment

Choose a reason for hiding this comment