Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix appending trailers to git commits #33

Merged
merged 1 commit into from
Jun 17, 2022

Conversation

bjh83
Copy link
Collaborator

@bjh83 bjh83 commented Jan 20, 2021

Previously trailers were not properly added to end of git commit
messages. Apparently, trailers were overwriting the entire message
because the file was not opened and flushed properly.

@dlatypov
Copy link
Collaborator

Commit message has a change-id in it?
Is that intentional?

src/git.py Outdated
@@ -121,8 +121,10 @@ def _set_trailers(self, patch: Patch) -> str:
# Add trailers, changing them if they exist.
# It's _highly_ unlikely that they'd exist, but this seems to be the
# most sane way of handling that edge case.
with tempfile.NamedTemporaryFile(mode='wt') as f:
with tempfile.NamedTemporaryFile(mode='w+') as f:
f.seek(0, os.SEEK_END)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what this seek does/if it's necessary.
Perhaps a comment is in order?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could have sworn that it didn't work without it, but I just tried it without the seek and it appears to work.

Will fix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

src/git.py Outdated
f.write(original_message)
f.flush()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have a comment like

  # flush before we write to the file externally below

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Previously trailers were not properly added to end of git commit
messages. Apparently, trailers were overwriting the entire message
because the file was not opened and flushed properly.

Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
@codecov-commenter
Copy link

Codecov Report

Merging #33 (10f7bab) into master (fdd1830) will decrease coverage by 0.07%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master      #33      +/-   ##
==========================================
- Coverage   81.25%   81.18%   -0.08%     
==========================================
  Files          13       13              
  Lines        1115     1116       +1     
==========================================
  Hits          906      906              
- Misses        209      210       +1     
Flag Coverage Δ
unittests 81.18% <0.00%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/git.py 36.19% <0.00%> (-0.35%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fdd1830...10f7bab. Read the comment docs.

@bjh83 bjh83 requested a review from dlatypov June 17, 2022 02:28
Copy link
Collaborator

@dlatypov dlatypov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
I also forgot this wasn't submitted.

@dlatypov dlatypov merged commit dac3754 into google:master Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants