#644 allow empty commits
Merged a year ago by onosek. Opened a year ago by msuchy.
msuchy/rpkg empty  into  master

file modified
+1 -1
@@ -1845,7 +1845,7 @@ 

  

          # construct the git command

          # We do this via subprocess because the git module is terrible.

-         cmd = ['git', 'commit']

+         cmd = ['git', 'commit', '--allow-empty']

          if signoff:

              cmd.append('-s')

          if self.quiet:

to be able to add empty commit to satisfy autospec

Signed-off-by: Miroslav Suchý msuchy@redhat.com

Wow, this is simple. But it should solve the issue.

FWIW I consider this dangerous. IMHO it should not be allowed unless rpmautospec is detected.

But OTOH I have never used fedpkg commit, so feel free to disregard my opinion here.

FWIW I consider this dangerous. IMHO it should not be allowed unless rpmautospec is detected.

But OTOH I have never used fedpkg commit, so feel free to disregard my opinion here.

It made me nervous, too, but i think it's actually OK because:

  1. fedpkg commit automatically stages all modified (tracked) files, so there shouldn't be any problems with accidentally making an empty commit because you forgot to git add the changes.
    (You could forget to add a newly-added untracked file. But that's not really any more or less of a danger than it was before. Unless your only change to the repo was to create a new untracked file that you then forgot to add, I suppose. But that seems unlikely, since it'll be ignored anyway without a corresponding specfile edit.)
  2. git itself will still reject an empty commit with no commit message, same as it will a commit that isn't empty but has no commit message.

if we did want to do additional sanity checks, it might make sense to make git --allow-empty mutually exclusive with these fedpkg commit flags that imply a non-empty commit:

           --with-changelog      Get the last changelog from SPEC as commit
                                 message content. This option must be used
                                 with -m together.
           -c, --clog            Generate the commit message from the
                                 Changelog section
           --raw                 Make the clog raw

--allow-empty would also be unnecessary if the files to be committed are supplied as arguments to the fedpkg commit command itself. But it won't do any harm in those cases, so I'm not sure there's any compelling reason to complicate the logic just to avoid including it.

rebased onto 48fdb3e

a year ago

A version of the code that detects rpmautospec would look like this:

pyrpkg/__init__.py:
@@ -1846,6 +1846,8 @@ class Commands(object):
         # construct the git command
         # We do this via subprocess because the git module is terrible.
         cmd = ['git', 'commit']
+        if not self.is_retired() and self.uses_rpmautospec:
+            cmd.append('--allow-empty')
         if signoff:
             cmd.append('-s')
         if self.quiet:

I am going to merge this PR as it is; it was explained, that will be safe. I agree.
If you think that the above version is better, let me know and I will change it.

Just to explain ... self.is_retired() looks unnecessary. But fedpkg retire method internally uses the commit and without this check, it fails.

Commit a2773dd fixes this pull-request

Pull-Request has been merged by onosek

a year ago

Having the "uses_rpmautospec" idea seems like a good idea after all (breaks imports into Copr dist-git): https://pagure.io/rpkg/issue/677

Metadata