Skip to content

ci: avoid running git diff after patching #29461

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.

Sign up for GitHub

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

Jump to bottom
Merged
fanquake merged 1 commit into from
Feb 26, 2024
Merged

ci: avoid running git diff after patching #29461

fanquake merged 1 commit into from
Feb 26, 2024
+4 −4

Conversation

@ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Feb 21, 2024 •
edited
Loading

Drop git diff command so it is easier to run CI locally if git checkout is a worktree. Currently it fails because the directory is not recognized as a git repository.

The git diff command was added recently in #28359 commit fa07ac4 and can be avoided just by teeing the patch to stdout

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 21, 2024 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK maflcko, TheCharlatan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@DrahtBot DrahtBot added the Tests label Feb 21, 2024
@maflcko
Copy link
Member

maflcko commented Feb 21, 2024

It is there to clarify the CI is using different code than the one in the source dir.

lgtm ACK c835176

@ryanofsky
Copy link
Contributor Author

ryanofsky commented Feb 21, 2024

It is there to clarify the CI is using different code than the one in the source dir.

Oh that makes sense. If you think it would be an improvement, I could change the patch command to echo '...' | tee >(patch -p1) so the patch is copied to stdout

@maflcko
Copy link
Member

maflcko commented Feb 21, 2024

Sure, anything is fine here.

@ryanofsky
ci: avoid running git diff after patching
84388c9 
Drop `git diff` command so it is easier to run CI locally if git checkout is a
worktree. Currently it fails because the directory is not recognized as a git
repository.

The `git diff` command was added recently in bitcoin#28359 commit
fa07ac4 and can be avoided just by teeing the
patch to stdout
@ryanofsky ryanofsky changed the title ci: remove unnecessary git diff after applying patch ci: avoid running git diff after patching Feb 21, 2024
@ryanofsky
Copy link
Contributor Author

ryanofsky commented Feb 21, 2024

Updated c835176 -> 84388c9 (pr/nodiff.1 -> pr/nodiff.2, compare) restoring diff output with tee

@maflcko maflcko added this to the 27.0 milestone Feb 22, 2024
@maflcko
Copy link
Member

maflcko commented Feb 22, 2024

lgtm ACK 84388c9

I can't review this, since it is written in bash, but given that CI passed and something is printed in the log, I guess this is fine 🤷‍♂️

+ export HOST=i686-pc-linux-gnu
+ HOST=i686-pc-linux-gnu
+ tee /dev/fd/63
++ patch -p1
--- a/src/leveldb/db/db_impl.cc
+++ b/src/leveldb/db/db_impl.cc
@@ -1028,9 +1028,6 @@ Status DBImpl::DoCompactionWork(CompactionState* compact) {
       stats.bytes_read += compact->compaction->input(which, i)->file_size;
     }
   }
-  for (size_t i = 0; i < compact->outputs.size(); i++) {
-    stats.bytes_written += compact->outputs[i].file_size;
-  }
   mutex_.Lock();
   stats_[compact->compaction->level() + 1].Add(stats);
+ '[' false = true ']'
+ '[' true = true ']'

https://cirrus-ci.com/task/4707541565833216?logs=ci#L326

@maflcko
Copy link
Member

maflcko commented Feb 22, 2024

Added milestone, since this is a bugfix

sedited
sedited approved these changes Feb 26, 2024
View reviewed changes
Copy link
Contributor

@sedited sedited left a comment

Choose a reason for hiding this comment

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

ACK 84388c9

@fanquake fanquake merged commit bd1c66f into bitcoin:master Feb 26, 2024
@bitcoin bitcoin locked and limited conversation to collaborators Feb 25, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sedited sedited sedited approved these changes

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

27.0

Development

Successfully merging this pull request may close these issues.

5 participants

@ryanofsky @DrahtBot @maflcko @sedited @fanquake