Thanks for looking into this :)

Have you tested this with altering the local html file? It seems like this would only change the text, but not make the button actually not disabled.

I have not managed to start up, but the PR removes the disabled keyword.

I have tested to create a PR after I used firefox developer mode to remove the disabled keyword, and then I was able to create this PR: https://src.fedoraproject.org/rpms/petsc/pull-request/8

I am not sure we should allow empty pull requests by default. In almost every case except with RPMAutoSpec, it is considered an error, and I think that's the right thing to do by default.

I am not sure we should allow empty pull requests by default. In almost every case except with RPMAutoSpec, it is considered an error, and I think that's the right thing to do by default.

Shouldn't that be handled by the person reviewing the PR? David showed in the example PR that the PR is not automatically merged when it is empty, so there is no issue on that front. Also creating an empty commit is not easy and it shows that the author intentionally wanted to create it.

For the record, both gitlab and github allow empty PRs:
https://github.com/dschwoerer/test/pull/1

Alternatively I could write a plugin for greasemonkey or similar, to allow empty PRs, but that feels like an ugly workaround.

I do agree in most cases it is not wanted, but it also makes sense outside of RPMAutoSpec, for example if you want to merge the history of two branches.

@ngompa

I am not sure we should allow empty pull requests by default. In almost every case except with RPMAutoSpec, it is considered an error, and I think that's the right thing to do by default.

A pull request isn't empty if it contains commits that aren't in the target branch. An empty diff shouldn't be conflated with an empty PR, they're not the same thing.

Well, this rips out the conditional that checks for an empty diff.

Well, this rips out the conditional that checks for an empty diff.

Right. Which I think is correct, because Pagure won't allow you to create a PR with no commits (a truly empty PR). The "Create Pull Request" button is disabled on the creation page.

So if a PR is created, it must have some commits, and even if their diff reduces to nothing those commits may have some value in the upstream repo — at least, that's something for the maintainers to decide, not have the system decide for them.

(The other thing is, even if it is a mistake, merging a PR with no diff isn't going to hurt anything. By definition it has no effect on the code itself, only the commit history.)

If there's an easy way to do it in the UI code, it might be worth instead doing a check for a commit diff, for cases where a PR branch gets force-pushed to the HEAD of the target branch. (IOW, becomes a PR with no commits.) If that can be merged again it'll do even less damage to the repo, but I don't know if Pagure would balk when asked to do such a thing. Git doesn't care, git merge <other_branch_with_same_HEAD> just reports, "Already up to date." and exits successfully.

Pagure won't allow you to create a PR with no commits (a truly empty PR). The "Create Pull Request" button is disabled on the creation page.

Oh, duh, never mind. This is about the Create PR button (which doesn't allow no-diff PRs even with commits), NOT about the Merge button. Sorry.

(Does the Merge button already allow merging PRs with no diff?)

Now that I've caught up to the rest of the class, maybe instead of removing the disabled check, it should be changed to...

<input type="submit" class="btn btn-primary" value="Create Pull Request"{%
if diff_commits | length < 1 %} disabled
title="This appears to be an empty PR, no commits to request pulling"%{ endif %}>

...Since we apparently have that info as well (for the tabs down below), and a PR with no commits to merge really IS useless.

And maybe the logic for summarizing the changes, up higher:

      <div class="btn btn-outline-secondary border-0 disabled opacity-100 font-weight-bold">
          {% if diff %}
            {{diff_commits | length}} commit{{'s' if diff_commits | length > 1 }},
            {{diff.stats.files_changed}} file{{'s' if diff.stats.files_changed > 1 }} changed
          {% else %}
            no changes
          {% endif %}
      </div>

...Should have the {% if diff %} wrapper removed from it, as well.

There are also real-live examples for empty merge requests, outside of dist git:

A no-diff PR can resolve merge-conflicts. Here is an example: https://github.com/boutproject/BOUT-dev/pull/2964

I think without a commit I am not even able to get to this page, so I don't think we need to special handle that. Anyway, an empty PR does not do anything anyway so I do not think we need to put to much effort into preventing that ...

Sorry, I misspoke. It is possible to open an empty PR: https://pagure.io/pagure/pull-request/5504

I have update the PR ...

@ferdnyc I have addressed your concerns. Are you happy with the changes?

@ngompa Are you still against this PR? If so, could you explain why it is so bad to allow to open such PRs? What is the downside of allowing it? If you do not find them useful for your project, is it not sufficient to just not merge such PRs? It is already possible to open such PRs (either by changing the html in the browser or buy changing the branch after the PR has been opened). I do see the idea behind preventing it, but as has been shown there are real use cases that makes such PRs useful.

@davidsch Sorry for the tardy response.

This looks sensible to me, assuming it works and doesn't break. My main concern would be this line here -- previously, it was inside an {% if diff %}, so we knew diff had to be defined. But if a comparison does have NO diff, is it safe to dereference it as diff.stats.files_changed? Will it even have those properties, or will it be None (and the diff.stats.files_changed reference therefore an error)?

If that won't cause any problems (or if it can't happen), then LGTM!

This project has moved to https://forge.fedoraproject.org/apps/pagure. As part of the migration, all open pull requests on pagure.io have been closed. If you'd like to continue working on this, please fork the repository on the new forge and re-submit your PR there.