fix: fetch syncedRevision in UpdateRevisionForPaths (#21014) #21015
Conversation
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
toyamagu-2021
changed the title
toyamagu-2021
force-pushed
the
fix-21014
branch
from
625c163 to
ce4cc7b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #21015 +/- ##
==========================================
+ Coverage 55.58% 55.62% +0.03%
==========================================
Files 339 339
Lines 56874 56908 +34
==========================================
+ Hits 31616 31653 +37
+ Misses 22609 22605 -4
- Partials 2649 2650 +1 ☔ View full report in Codecov by Sentry. |
toyamagu-2021
force-pushed
the
fix-21014
branch
from
6abf24d to
a7d0947
Compare
toyamagu-2021
changed the title
| @@ -2851,6 +2884,10 @@ func (s *Service) UpdateRevisionForPaths(_ context.Context, request *apiclient.U | |||
| } | |||
| defer io.Close(closer) | |||
|
|
|||
| if err := s.fetch(gitClient, []string{syncedRevision, revision}); err != nil { | |||
| return nil, status.Errorf(codes.Internal, "unable to fetch git repo %s with revisions (%s, %s): %v", repo.Repo, syncedRevision, revision, err) | |||
| } | |||
There was a problem hiding this comment.
How would locking above work with both revisions being used here? Can we lock for multiple revisions?
There was a problem hiding this comment.
Should we take a lock to fetch from the remote? If we checkout we need to take a lock, but for fetch I think we don't need to lock.
There was a problem hiding this comment.
I don't know
| @@ -2851,6 +2895,10 @@ func (s *Service) UpdateRevisionForPaths(_ context.Context, request *apiclient.U | |||
| } | |||
| defer io.Close(closer) | |||
|
|
|||
| if err := s.fetch(gitClient, []string{syncedRevision}); err != nil { | |||
There was a problem hiding this comment.
We lock on revision above, but use syncedRevision here. Should it be consistent?
There was a problem hiding this comment.
To exec git diff syncRevision..targetRevision we only need to fetch from remote, so I think we needn't take a lock.
There was a problem hiding this comment.
I see. Maybe we can release the lock earlier then.
toyamagu-2021
changed the title
| return err | ||
| } | ||
|
|
||
| func fetch(gitClient git.Client, targetRevisions []string) error { |
There was a problem hiding this comment.
| func fetch(gitClient git.Client, targetRevisions []string) error { | |
| func fetchRevision(gitClient git.Client, targetRevisions []string) error { |
Is there a specific reason behind using the same func name as fetch? If not, we can probably change the func name to avoid confusion.
There was a problem hiding this comment.
I'm not being particular, but it is the same naming convention as checkoutRevision.
Signed-off-by: toyamagu2021 <toyamagu2021@gmail.com>
Signed-off-by: toyamagu-2021 <toyamagu2021@gmail.com>
Signed-off-by: toyamagu-2021 <toyamagu2021@gmail.com>
Signed-off-by: toyamagu-2021 <toyamagu2021@gmail.com>
Signed-off-by: toyamagu-2021 <toyamagu2021@gmail.com>
Signed-off-by: toyamagu2021 <toyamagu2021@gmail.com>
toyamagu-2021
force-pushed
the
fix-21014
branch
from
514df2c to
05e2df0
Compare
|
@toyamagu-2021 Are you still working on this PR? |
Signed-off-by: toyamagu2021 <toyamagu2021@gmail.com>
Signed-off-by: toyamagu2021 <toyamagu2021@gmail.com>
|
/cherry-pick release-2.14 |
…rgoproj#21015) Signed-off-by: toyamagu2021 <toyamagu2021@gmail.com> Signed-off-by: toyamagu-2021 <toyamagu2021@gmail.com> Signed-off-by: kingbj0429 <kingbj0429@lunit.io>
…rgoproj#21015) Signed-off-by: toyamagu2021 <toyamagu2021@gmail.com> Signed-off-by: toyamagu-2021 <toyamagu2021@gmail.com>
…rgoproj#21015) Signed-off-by: toyamagu2021 <toyamagu2021@gmail.com> Signed-off-by: toyamagu-2021 <toyamagu2021@gmail.com> Signed-off-by: kingbj0429 <kingbj0429@lunit.io>
Signed-off-by: toyamagu2021 toyamagu2021@gmail.com
Closes #21014
Check if syncedRevision exists in a local git repo.
Can be cherry-picked into v2.12 and v2.13.
Checklist: