From 0b87b3f4ac7498d17a3fee573e386465160d7aff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Cohen=20Arazi?= Date: Tue, 7 Oct 2025 15:58:10 -0300 Subject: [PATCH] [#1] Fix attachment upload order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Attachments were being uploaded in reverse chronological order (newest first), which breaks sequential patch application. Added --reverse flag to git rev-list to ensure commits are returned in chronological order (oldest first). - GitBz::Git->rev_list(): Add --reverse flag for chronological ordering - t/attach_order.t: Test demonstrating issue and expected behavior Signed-off-by: Tomás Cohen Arazi --- lib/GitBz/Git.pm | 3 +- t/attach_order.t | 143 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 145 insertions(+), 1 deletion(-) create mode 100755 t/attach_order.t diff --git a/lib/GitBz/Git.pm b/lib/GitBz/Git.pm index d87cff3..67a7e18 100644 --- a/lib/GitBz/Git.pm +++ b/lib/GitBz/Git.pm @@ -82,8 +82,9 @@ sub rev_list { my ( $class, @args ) = @_; unshift( @args, '--pretty=format:%s' ); + unshift( @args, '--reverse' ); # Add --reverse to get chronological order (oldest first) unshift( @args, '--max-count=1' ) - if $args[1] eq 'HEAD'; + if $args[2] eq 'HEAD'; # Adjust index due to --reverse insertion my $output = $class->run( 'rev-list', @args ); diff --git a/t/attach_order.t b/t/attach_order.t new file mode 100755 index 0000000..7d82304 --- /dev/null +++ b/t/attach_order.t @@ -0,0 +1,143 @@ +#!/usr/bin/perl + +use Modern::Perl; +use Test::More; +use FindBin; +use lib "$FindBin::Bin/../lib"; + +=head1 NAME + +t/attach_order.t - Test that attachments are uploaded in correct chronological order + +=head1 DESCRIPTION + +Tests that when attaching multiple commits, they are uploaded in chronological +order (oldest first) so they can be applied correctly later. + +=cut + +# Simple mock for testing without external dependencies +{ + package MockBug; + sub new { + my ($class, %args) = @_; + bless { %args, _attachments => [] }, $class; + } + sub id { $_[0]->{id} } + sub summary { $_[0]->{summary} } + sub status { $_[0]->{status} } + sub attachments { $_[0]->{_attachments} } + sub add_attachment { + my ($self, $patch, $filename, $description, %opts) = @_; + push @{$self->{_attachment_order}}, $description; + return 1; + } +} + +{ + package MockCommands; + sub new { + my ($class, %args) = @_; + bless \%args, $class; + } +} + +subtest 'current behavior - attachments uploaded in reverse chronological order' => sub { + plan tests => 4; + + # Track attachment order + my @attachment_order; + + # Create mock bug that captures attachment order + my $bug = MockBug->new( + id => 12345, + summary => 'Test bug', + status => 'NEW', + _attachment_order => \@attachment_order + ); + + # Simulate current GitBz::Git->get_commits behavior (reverse chronological) + my @commits = ( + { id => 'abc123', subject => 'Third commit (newest)' }, + { id => 'def456', subject => 'Second commit (middle)' }, + { id => 'ghi789', subject => 'First commit (oldest)' }, + ); + + # Simulate current attach_patches behavior + for my $commit (@commits) { + $bug->add_attachment( + "patch content", + $commit->{id} . ".patch", + $commit->{subject} + ); + } + + # Verify current (wrong) behavior + is(scalar @attachment_order, 3, 'Three attachments were created'); + is($attachment_order[0], 'Third commit (newest)', 'First attachment is newest commit (WRONG)'); + is($attachment_order[1], 'Second commit (middle)', 'Second attachment is middle commit'); + is($attachment_order[2], 'First commit (oldest)', 'Third attachment is oldest commit (WRONG)'); + + note("CURRENT BEHAVIOR (BROKEN): " . join(' -> ', @attachment_order)); + note("This is backwards! Patches should be applied oldest-first for correct git apply sequence."); +}; + +subtest 'expected correct behavior - chronological order' => sub { + plan tests => 4; + + my @attachment_order; + my $bug = MockBug->new( + id => 12345, + summary => 'Test bug', + status => 'NEW', + _attachment_order => \@attachment_order + ); + + # What the corrected behavior should return (chronological order) + my @commits = ( + { id => 'ghi789', subject => 'First commit (oldest)' }, + { id => 'def456', subject => 'Second commit (middle)' }, + { id => 'abc123', subject => 'Third commit (newest)' }, + ); + + # Simulate corrected attach_patches behavior + for my $commit (@commits) { + $bug->add_attachment( + "patch content", + $commit->{id} . ".patch", + $commit->{subject} + ); + } + + # Verify correct behavior + is(scalar @attachment_order, 3, 'Three attachments were created'); + is($attachment_order[0], 'First commit (oldest)', 'First attachment is oldest commit (CORRECT)'); + is($attachment_order[1], 'Second commit (middle)', 'Second attachment is middle commit'); + is($attachment_order[2], 'Third commit (newest)', 'Third attachment is newest commit (CORRECT)'); + + note("EXPECTED BEHAVIOR (CORRECT): " . join(' -> ', @attachment_order)); + note("This is the correct order for sequential git apply."); +}; + +subtest 'git rev-list default behavior analysis' => sub { + plan tests => 1; + + # Document the root cause: git rev-list returns newest-first by default + my $explanation = <<'EOF'; +ROOT CAUSE ANALYSIS: +- git rev-list returns commits in reverse chronological order (newest first) +- GitBz::Git->get_commits() uses git rev-list without --reverse flag +- attach_patches() processes commits in the order returned by get_commits() +- Result: attachments are uploaded newest-first (backwards for applying) + +SOLUTION: +- Add --reverse flag to git rev-list in GitBz::Git->rev_list() +- This will return commits in chronological order (oldest first) +- Attachments will then be uploaded in correct sequence for git apply +EOF + + pass("Analysis documented"); + note($explanation); +}; + +done_testing(); -- 2.51.0