Skip to content

Update PrePush Modified Files and Lines with Multiple Branches Push #548

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

Merged
merged 1 commit into from
Feb 10, 2018
Merged

Update PrePush Modified Files and Lines with Multiple Branches Push #548

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions lib/overcommit/hook_context/pre_push.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,14 @@ def pushed_refs
end

def modified_files
Copy link
Contributor Author

@taufek taufek Feb 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@sds ,

I want to ask your opinion on this one. If I push changes on the same file on separate branch/local_ref:

  1. Should we convert the output to unique list of files.
[‘file_one’]

Or

  1. Should we distinguish the file from different local_ref by appending sort of prefix. For example:
[‘<local_ref_1>:file_one’, ‘<local_ref_2>:file_one’]

Or

  1. Do nothing. Let there be duplicate of file from different local_ref.
[‘file_one’, ‘file_one’]

Copy link
Owner

@sds sds Feb 9, 2018

Choose a reason for hiding this comment

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

Good question, @taufek. If I try to put myself in the shoes of a developer writing a pre-push hook, the least-surprising behavior to me would be net set of differences between the local and remote branches, and thus the unique list of files (option 1) seems reasonable to me.

If you have other ideas about why others would be better, let me know!

Copy link
Contributor Author

@taufek taufek Feb 9, 2018

Choose a reason for hiding this comment

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

Yup. Unique list of files make sense to me. Thanks.

@modified_files ||= Overcommit::GitRepo.modified_files(refs: ref_range)
@modified_files ||= pushed_refs.map(&:modified_files).flatten.uniq
Copy link
Contributor Author

@taufek taufek Feb 10, 2018

Choose a reason for hiding this comment

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

require 'benchmark'
require 'set'

iterations = 10
data = 100.times.map do
  Array.new(1_000) { rand(0..1_000) }
end

Benchmark.bmbm do |x|
  x.report('array.flatten.uniq') do
    iterations.times do
      data.flatten.uniq
    end
  end

  x.report('array.each_with_object{ merge }.to_a') do
    iterations.times do
      data.each_with_object(Set.new) do |datum, set|
        set.merge(datum)
      end.to_a
    end
  end
end

puts 'Are they equivalent?'
puts data.flatten.uniq.sort == data.each_with_object(Set.new) { |datum, set| set.merge(datum) }.sort

Rehearsal ------------------------------------------------------------------------
array.flatten.uniq                     0.100000   0.010000   0.110000 (  0.097410)
array.each_with_object{ merge }.to_a   0.140000   0.000000   0.140000 (  0.148073)
--------------------------------------------------------------- total: 0.250000sec

                                           user     system      total        real
array.flatten.uniq                     0.090000   0.000000   0.090000 (  0.100320)
array.each_with_object{ merge }.to_a   0.140000   0.000000   0.140000 (  0.151008)
Are they equivalent?
true

Seems like for this case, calling flatten.uniq is fast.

end

def modified_lines_in_file(file)
@modified_lines ||= {}
@modified_lines[file] =
Overcommit::GitRepo.extract_modified_lines(file, refs: ref_range)
end

def ref_range
"#{pushed_refs[0].remote_sha1}..#{pushed_refs[0].local_sha1}"
@modified_lines[file] = pushed_refs.each_with_object(Set.new) do |pushed_ref, set|
set.merge(pushed_ref.modified_lines_in_file(file))
end
end

PushedRef = Struct.new(:local_ref, :local_sha1, :remote_ref, :remote_sha1) do
Expand All @@ -48,12 +45,24 @@ def destructive?
deleted? || forced?
end

def modified_files
Overcommit::GitRepo.modified_files(refs: ref_range)
end

def modified_lines_in_file(file)
Overcommit::GitRepo.extract_modified_lines(file, refs: ref_range)
end

def to_s
"#{local_ref} #{local_sha1} #{remote_ref} #{remote_sha1}"
end

private

def ref_range
"#{remote_sha1}..#{local_sha1}"
end

def overwritten_commits
return @overwritten_commits if defined? @overwritten_commits
result = Overcommit::Subprocess.spawn(%W[git rev-list #{remote_sha1} ^#{local_sha1}])
Expand Down
109 changes: 92 additions & 17 deletions spec/overcommit/hook_context/pre_push_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,55 @@
`git add . 2>&1 > #{File::NULL}`
`git commit -m "Update Branch 2" 2>&1 > #{File::NULL}`

should include(*%w[added-1 update-me added-2].map { |file| File.expand_path(file) })
should == %w[added-1 added-2 update-me].map { |file| File.expand_path(file) }
should_not include(*%w[delete-me].map { |file| File.expand_path(file) })
end
end
end

context 'when pushing multiple branches at once' do
let(:local_ref_1) { 'refs/heads/project-branch-1' }
let(:local_sha1_1) { get_sha1(local_ref_1) }
let(:local_ref_2) { 'refs/heads/project-branch-2' }
let(:local_sha1_2) { get_sha1(local_ref_2) }
let(:remote_ref) { 'refs/remotes/origin/master' }
let(:remote_sha1) { get_sha1(remote_ref) }
let(:input) do
double('input', read: ref_ranges)
end
let(:ref_ranges) do
[
"#{local_ref_1} #{local_sha1_1} #{remote_ref} #{remote_sha1}\n",
"#{local_ref_2} #{local_sha1_2} #{remote_ref} #{remote_sha1}\n"
].join
end

it 'has modified files based on multiple tracking branches' do
repo do
`git remote add origin file://#{remote_repo}`
`git fetch origin 2>&1 > #{File::NULL} && git reset --hard origin/master`

`git checkout -b project-branch-1 2>&1 > #{File::NULL}`
`git push -u origin project-branch-1 2>&1 > #{File::NULL}`

touch 'added-1'
echo 'add', 'added-1'
echo 'append', 'update-me'
FileUtils.rm 'delete-me'
`git add . 2>&1 > #{File::NULL}`
`git commit -m "Update Branch 1" 2>&1 > #{File::NULL}`

`git checkout master 2>&1 > #{File::NULL}`
`git checkout -b project-branch-2 2>&1 > #{File::NULL}`
`git push -u origin project-branch-2 2>&1 > #{File::NULL}`

echo 'append', 'update-me'
touch 'added-2'
echo 'add', 'added-2'
`git add . 2>&1 > #{File::NULL}`
`git commit -m "Update Branch 2" 2>&1 > #{File::NULL}`

should == %w[added-1 update-me added-2].map { |file| File.expand_path(file) }
should_not include(*%w[delete-me].map { |file| File.expand_path(file) })
end
end
Expand Down Expand Up @@ -121,7 +169,7 @@
`git add . 2>&1 > #{File::NULL}`
`git commit -m "Update Branch 2" 2>&1 > #{File::NULL}`

should include(*%w[added-1 update-me added-2].map { |file| File.expand_path(file) })
should == %w[added-1 added-2 update-me].map { |file| File.expand_path(file) }
Copy link
Owner

@sds sds Feb 10, 2018

Choose a reason for hiding this comment

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

One thing worth noting is that using == here is slightly more brittle, as it prevents us from changing our implementation (which could return the file names in a different order) without breaking tests.

At the end of the day, our goal is to make sure we're returning the correct set of files, not files in a specified order, which is why we used include. However, perhaps you changed this because we weren't checking for unintended files to be included. In that case, you could use a combination of include along with verifying the count to make sure no additional files are in the set.

This is not a big deal, so I'm fine merging this as-is, but it's important we're not overly-specific in tests.

Copy link
Contributor Author

@taufek taufek Feb 11, 2018

Choose a reason for hiding this comment

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

I’ll take a note for future reference. Thanks.

should_not include(*%w[delete-me].map { |file| File.expand_path(file) })
end
end
Expand All @@ -130,12 +178,20 @@

describe '#modified_lines_in_file' do
subject { context.modified_lines_in_file(file) }
let(:local_ref) { 'refs/heads/project-branch' }
let(:local_sha1) { get_sha1(local_ref) }
let(:local_ref_1) { 'refs/heads/project-branch-1' }
let(:local_sha1_1) { get_sha1(local_ref_1) }
let(:local_ref_2) { 'refs/heads/project-branch-2' }
let(:local_sha1_2) { get_sha1(local_ref_2) }
let(:remote_ref) { 'refs/remotes/origin/master' }
let(:remote_sha1) { get_sha1(remote_ref) }
let(:input) do
double('input', read: "#{local_ref} #{local_sha1} #{remote_ref} #{remote_sha1}\n")
double('input', read: ref_ranges)
end
let(:ref_ranges) do
[
"#{local_ref_1} #{local_sha1_1} #{remote_ref} #{remote_sha1}\n",
"#{local_ref_2} #{local_sha1_2} #{remote_ref} #{remote_sha1}\n"
].join
end
let(:remote_repo) do
repo do
Expand All @@ -154,20 +210,28 @@
`git remote add origin file://#{remote_repo}`
`git fetch origin 2>&1 > #{File::NULL} && git reset --hard origin/master`

`git checkout -b project-branch 2>&1 > #{File::NULL}`
`git push -u origin project-branch 2>&1 > #{File::NULL}`
`git checkout -b project-branch-1 2>&1 > #{File::NULL}`
`git push -u origin project-branch-1 2>&1 > #{File::NULL}`

echo 'append-1', 'initial_file', append: true

`git add . 2>&1 > #{File::NULL}`
`git commit -m "Update Branch 1" 2>&1 > #{File::NULL}`
`git commit -m "Update Branch 1 Commit 1" 2>&1 > #{File::NULL}`

echo 'append-2', 'initial_file', append: true

`git add . 2>&1 > #{File::NULL}`
`git commit -m "Update Branch 2" 2>&1 > #{File::NULL}`
`git commit -m "Update Branch 1 Commit 2" 2>&1 > #{File::NULL}`

should == [2, 3].to_set
`git checkout -b project-branch-2 2>&1 > #{File::NULL}`
`git push -u origin project-branch-2 2>&1 > #{File::NULL}`

echo 'append-3', 'initial_file', append: true

`git add . 2>&1 > #{File::NULL}`
`git commit -m "Update Branch 2 Commit 1" 2>&1 > #{File::NULL}`

should == [2, 3, 4].to_set
end
end
end
Expand All @@ -180,36 +244,47 @@
`git remote add origin file://#{remote_repo}`
`git fetch origin 2>&1 > #{File::NULL} && git reset --hard origin/master`

`git checkout -b project-branch 2>&1 > #{File::NULL}`
`git push -u origin project-branch 2>&1 > #{File::NULL}`
`git checkout -b project-branch-1 2>&1 > #{File::NULL}`
`git push -u origin project-branch-1 2>&1 > #{File::NULL}`

touch 'new_file'

echo 'append-1', 'new_file', append: true

`git add . 2>&1 > #{File::NULL}`
`git commit -m "Update Branch 1" 2>&1 > #{File::NULL}`
`git commit -m "Update Branch 1 Commit 1" 2>&1 > #{File::NULL}`

echo 'append-2', 'new_file', append: true

`git add . 2>&1 > #{File::NULL}`
`git commit -m "Update Branch 2" 2>&1 > #{File::NULL}`
`git commit -m "Update Branch 1 Commit 2" 2>&1 > #{File::NULL}`

`git checkout -b project-branch-2 2>&1 > #{File::NULL}`
`git push -u origin project-branch-2 2>&1 > #{File::NULL}`

should == [1, 2].to_set
echo 'append-3', 'new_file', append: true

`git add . 2>&1 > #{File::NULL}`
`git commit -m "Update Branch 2 Commit 1" 2>&1 > #{File::NULL}`

should == [1, 2, 3].to_set
end
end
end

context 'when deleting a file' do
let(:file) { File.expand_path('initial_file') }
let(:ref_ranges) do
"#{local_ref_1} #{local_sha1_1} #{remote_ref} #{remote_sha1}\n"
end

it 'has modified lines in file' do
repo do
`git remote add origin file://#{remote_repo}`
`git fetch origin 2>&1 > #{File::NULL} && git reset --hard origin/master`

`git checkout -b project-branch 2>&1 > #{File::NULL}`
`git push -u origin project-branch 2>&1 > #{File::NULL}`
`git checkout -b project-branch-1 2>&1 > #{File::NULL}`
`git push -u origin project-branch-1 2>&1 > #{File::NULL}`

FileUtils.rm 'initial_file'

Expand Down