Is it worth describing what a "sensible commit message" looks like in this context? We might be able to assume that if someone is reading this that they're familiar enough to know...but maybe not?

There should either be a link to further instructions here or a command to run.

Probably better to make the section names xrefs, that way if the titles change later, it's not confusing.

Worth adding a note about including in the PR the exact source location (e.g. GitHub-produced tar.gz file) so that the maintainer who merges the pull request can upload the file with a matching checksum and doesn't have to go hunting for it?

Edit to add: You sort of mention that in the paragraph starting on line 137, but it's probably worth being explicit about it here.

I wonder if this is the place to document expectation setting. I know there's not a Fedora pull request SLA, but some guidance might help. Particularly a "if it's been forever and you've had no reply, here's a link to the non-responsive maintainer policy"

There are two possible workflows. 1) clone the original repository 2) clone the fork. I am strong proponent of the former. The main reason is the I clone whole lot of repositories just to browse the code. Later, I might decide to submit PR and therefore I need a fork. Suggesting to start with fork has also the danger of e.g. fixing issues which are already fixed in the original repository, because there is no space for getting the up2date code.

There is also fedpkg fork command, if the suggestion above was implemented ...

and fedpkg clone --anonymous some-package is also more straight forward

Is it worth describing what a "sensible commit message" looks like in this context?

+1 this should be repeated again, again and again (and I always repeat this also to myself and later I am surprised how bad I still am at it 🙈)

This is rather terse and would deserve some example, e.g. git push HEAD (and if this gets update according to my suggestions about, then probably git push fork HEAD)

Hm, PR against own fork? I understand this is due to cloning fork, but what is this good for?

Is it worth describing what a "sensible commit message" looks like in this context?

+1 this should be repeated again, again and again (and I always repeat this also to myself and later I am surprised how bad I still am at it 🙈)

I think it is a good idea to include some guidance about commit messages. But not in Pull Request Guide, because the same guidance applies equally to commits done in a different way. We already have some guidance starting here:

https://docs.fedoraproject.org/en-US/package-maintainers/Package_Maintenance_Guide/#_generate_git_changelog_from_package_changelog

So I propose the steps forward here would be:

1. Come up with a suitable description (which is probably the hard part, because many people are really attached to their way of writing commit messages, and everybody's way is different)
2. Edit that part of Package Maintenance Guide to include that description
3. Link there from here

I can do the editing, if somebody proposes a text. Also, a separate pull request is probably the better approach, because the commit message guidance can be handled regardless of what happens with this PR.

For me, it would be enough to say that a) Say what changed b) why c) Use as many words as you need to explain things, but not more d) include links to related Bugzilla tickets d) If rpmautospec is in use, follow its syntax.

# Install the package in Rawhide distrobox, test it

There should either be a link to further instructions here or a command to run.

I suppose you are right. This is another instance where we should have supporting material: packaging workflow, where testing packaging changes is explained with the correct amount of detail. Probably trying to squeeze a way (and not the simplest possible. because distrobox is involved) here is not a good idea. Perhaps I replace it with just "Install and test the package".

I feel the correct solution here would be to have testing instructions in Package Maintenance Guide, somewhere around here probably:

https://docs.fedoraproject.org/en-US/package-maintainers/Package_Maintenance_Guide/#_run_some_checks_rpmlint_on_your_package

There are two possible workflows. 1) clone the original repository 2) clone the fork. I am strong proponent of the former. The main reason is the I clone whole lot of repositories just to browse the code. Later, I might decide to submit PR and therefore I need a fork. Suggesting to start with fork has also the danger of e.g. fixing issues which are already fixed in the original repository, because there is no space for getting the up2date code.

There is also fedpkg fork command, if the suggestion above was implemented ...

and fedpkg clone --anonymous some-package is also more straight forward

Thank you, I like this suggestion. Probably it is indeed easier to work when the default remote is dist-git and the fork is a secondary remote. I will update this PR use that approach. I had forgotten about fedpkg fork, very good that even fork creation can be handed in the command line.

remote: Create a pull-request for my-changes
remote: https://src.fedoraproject.org/fork/username/rpms/some-package/diff/rawhide..my-changes

Hm, PR against own fork? I understand this is due to cloning fork, but what is this good for?

Given what I say above, this is not relevant anymore. But just for the record, that URL will be exactly the same in both cases, and Pagure will propose a pull request from fork to upstream.

This is rather terse and would deserve some example, e.g. git push HEAD (and if this gets update according to my suggestions about, then probably git push fork HEAD)

Makes sense. Better to give an explicit command that always does the right thing. I will add that.

you also need to follow sections Installing and Configuration / Mock.

Probably better to make the section names xrefs, that way if the titles change later, it's not confusing.

Good idea, I will do that.

If you are in the packager group, you can remove --offline from the command,
so that the source archive is uploaded.
Otherwise, the maintainer who eventually merges the pull request has to do that.

Worth adding a note about including in the PR the exact source location (e.g. GitHub-produced tar.gz file) so that the maintainer who merges the pull request can upload the file with a matching checksum and doesn't have to go hunting for it?

I did not even consider the possibility everything in sources would not have corresponding Source or Patch lines in the specfile, with URLs pointing to originals. But you are right, it is better to add the note you suggest, just in case. I will edit that in.

I think I have now addressed all the items brought up in the reviews, except for commit message and (more extensive) instructions for how to test the package, for which I created #158 and #159 respectively.

Unless there are futher change requests, I will squash the comments and merge soonish.

What the --set-upstream does? Answering to myself, I have found an explanation here. Based on this, I'd still suggest against recommending --set-upstream. It is one additional parameter reader needs to worry about.

I'd say this goes into unnecessary level of detail. Maybe this whole paragraph could be somehow included into previous, just as a short list in the bracket, elaborating about the "pertinent information"

This paragraph is IMHO missing the main information that the CI results should be green. IOW if CI fails, it is good idea to fix that.

git push --set-upstream username HEAD

What the --set-upstream does? Answering to myself, I have found an explanation here. Based on this, I'd still suggest against recommending --set-upstream. It is one additional parameter reader needs to worry about.

I used that form, because that is what Git with default config suggests if you do just git push in that situation:

$ git push
fatal: The current branch test-2 has no upstream branch.
To push the current branch and set the remote as upstream, use
    git push --set-upstream origin test-2
To have this happen automatically for branches without a tracking
upstream, see 'push.autoSetupRemote' in 'git help config'.

But I don't think that matters much. Git is very widely used, so we can safely assume that packagers either already know how to use it, or if not, they can learn if from other sources. So any command that does the right thing, and is part of some sensible workflow, will do. So I edited to command to follow the exact form you suggested.

If you could not populate the lookaside cache in <> step,
include that info and ask for a maintainer to do that.
Also note if you think that your update would make sense for other branches than Rawhide as well

I'd say this goes into unnecessary level of detail. Maybe this whole paragraph could be somehow included into previous, just as a short list in the bracket, elaborating about the "pertinent information"

At least for the lookaside cache part, I disagree. That is exactly the kind of material we should focus here. Almost every open source project out there accepts pull requests, so the basic principle is well known. Mostly, we just have to say "yes, we do pull requests, just like everybody else". But this lookaside cache is specific to Fedora, and needs to be very clearly spelled out.

The second part about which branches should receive the update is somewhere in the middle. Some other projects also do multiple branches and backports, including very well known ones like Linux kernel. So probably most new people already understand the concept. But it is just two sentences, and is something at least I think through for every package change I make, pull requests and direct commits alike. So I figured it is better to ask to help the maintainer by answering, in advance, the question they will anyhow ask in their mind. I would prefer to keep it, unless there is heavy resistance.

== Continuous integration

This paragraph is IMHO missing the main information that the CI results should be green. IOW if CI fails, it is good idea to fix that.

Added as proposed, with the note that Ci is not blocking.

git push --set-upstream username HEAD

What the --set-upstream does? Answering to myself, I have found an explanation here. Based on this, I'd still suggest against recommending --set-upstream. It is one additional parameter reader needs to worry about.

I used that form, because that is what Git with default config suggests if you do just git push in that situation:

```
$ git push
fatal: The current branch test-2 has no upstream branch.
To push the current branch and set the remote as upstream, use

git push --set-upstream origin test-2

To have this happen automatically for branches without a tracking
upstream, see 'push.autoSetupRemote' in 'git help config'.
```

But I don't think that matters much. Git is very widely used, so we can safely assume that packagers either already know how to use it, or if not, they can learn if from other sources. So any command that does the right thing, and is part of some sensible workflow, will do. So I edited to command to follow the exact form you suggested.

👍

If you could not populate the lookaside cache in <> step,
include that info and ask for a maintainer to do that.
Also note if you think that your update would make sense for other branches than Rawhide as well

I'd say this goes into unnecessary level of detail. Maybe this whole paragraph could be somehow included into previous, just as a short list in the bracket, elaborating about the "pertinent information"

At least for the lookaside cache part, I disagree. That is exactly the kind of material we should focus here. Almost every open source project out there accepts pull requests, so the basic principle is well known. Mostly, we just have to say "yes, we do pull requests, just like everybody else". But this lookaside cache is specific to Fedora, and needs to be very clearly spelled out.

If you put it there this way, that would be probably better 😉

The second part about which branches should receive the update is somewhere in the middle. Some other projects also do multiple branches and backports, including very well known ones like Linux kernel. So probably most new people already understand the concept. But it is just two sentences, and is something at least I think through for every package change I make, pull requests and direct commits alike. So I figured it is better to ask to help the maintainer by answering, in advance, the question they will anyhow ask in their mind. I would prefer to keep it, unless there is heavy resistance.

I might be biased, because I do most updates just for Rawhide. And also most of the PRs I have seen were against Rawhide. Therefore to me wanting the change somewhere else would be exceptional and likely natural to ask explicitly without any additional suggestion. But up to you or if somebody else will chime in ;)

BTW speaking of Rawhide and skimming through the PR, I am missing some note there that Rawhide should be primary target. It comes from the workflow, but maybe it would be worth of spelling it explicitly.

If you could not populate the lookaside cache in <> step,
include that info and ask for a maintainer to do that.
Also note if you think that your update would make sense for other branches than Rawhide as well

I'd say this goes into unnecessary level of detail. Maybe this whole paragraph could be somehow included into previous, just as a short list in the bracket, elaborating about the "pertinent information"

At least for the lookaside cache part, I disagree. That is exactly the kind of material we should focus here. Almost every open source project out there accepts pull requests, so the basic principle is well known. Mostly, we just have to say "yes, we do pull requests, just like everybody else". But this lookaside cache is specific to Fedora, and needs to be very clearly spelled out.

If you put it there this way, that would be probably better 😉

I am not sure what you are asking for here. There is a whole section Updating sources that explains how this is done for pull requests, and this part just links there and asks to add a note to PR description if lookaside cache needs maintainer action. Could you propose alternative wording if you really do not think the current one works?

The second part about which branches should receive the update is somewhere in the middle. Some other projects also do multiple branches and backports, including very well known ones like Linux kernel. So probably most new people already understand the concept. But it is just two sentences, and is something at least I think through for every package change I make, pull requests and direct commits alike. So I figured it is better to ask to help the maintainer by answering, in advance, the question they will anyhow ask in their mind. I would prefer to keep it, unless there is heavy resistance.

I might be biased, because I do most updates just for Rawhide. And also most of the PRs I have seen were against Rawhide. Therefore to me wanting the change somewhere else would be exceptional and likely natural to ask explicitly without any additional suggestion. But up to you or if somebody else will chime in ;)

Also in my experience Rawhide is by far the most common target for new commits, via pull requests or direct pushes. I just wanted to somehow cover the case where it is needed for release branches as well. On the other hand, it is not that uncommon for a bug stays under maintainer radar for sizeable period of time, until somebody finally reports it, and the fix needs to be applied to all release branches.

Anyhow, I removed the lines now. It is implicit in "pertinent information", not very common case, and can be added later — I don't think other pages in Package Maintainer Docs cover that topic in any detail either.

BTW speaking of Rawhide and skimming through the PR, I am missing some note there that Rawhide should be primary target. It comes from the workflow, but maybe it would be worth of spelling it explicitly.

You are, it is in the opening section. I made it a bit more explicit now: The target branch for the pull request is assumed to be rawhide, as Rawhide is much more common change target than release branches.

I think I have addressed all the feedback now. Unless somebody requests more changes, I will merge this soon.

If you could not populate the lookaside cache in <> step,
include that info and ask for a maintainer to do that.
Also note if you think that your update would make sense for other branches than Rawhide as well

I'd say this goes into unnecessary level of detail. Maybe this whole paragraph could be somehow included into previous, just as a short list in the bracket, elaborating about the "pertinent information"

At least for the lookaside cache part, I disagree. That is exactly the kind of material we should focus here. Almost every open source project out there accepts pull requests, so the basic principle is well known. Mostly, we just have to say "yes, we do pull requests, just like everybody else". But this lookaside cache is specific to Fedora, and needs to be very clearly spelled out.

If you put it there this way, that would be probably better 😉

I am not sure what you are asking for here. There is a whole section Updating sources that explains how this is done for pull requests, and this part just links there and asks to add a note to PR description if lookaside cache needs maintainer action. Could you propose alternative wording if you really do not think the current one works?

I still think the note is not needed at all.

Actually, speaking about the Updating sources chapter, maybe it would make sense to move this note there? E.g. include something like: If you could not populate the lookaside cache, please note this later when <>

But in any case, this is minor nit.

Thank you, I appreciate that you want to keep the instructions concise and to the point. I also understand that the same information can be conveyed in multiple different ways. But since we agree that this is a minor issue, I will merge this pull request now without further changes. Thank you Vít and Ben, this is the most comprehesive pull request review I have received for Package Maintainer Docs this far!