Improve commit message #934
Conversation
thachlp
requested review from
ikhoon,
jrhee17,
minwoox and
trustin
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #934 +/- ##
============================================
- Coverage 66.86% 66.80% -0.06%
+ Complexity 3529 3517 -12
============================================
Files 370 370
Lines 14531 14482 -49
Branches 1563 1554 -9
============================================
- Hits 9716 9675 -41
+ Misses 3936 3931 -5
+ Partials 879 876 -3 ☔ View full report in Codecov by Sentry. |
...or-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/AbstractGitMirror.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Ikhun Um <ih.pert@gmail.com>
| @@ -257,7 +257,8 @@ void mirrorRemoteToLocal( | |||
| changes.put(mirrorStatePath, Change.ofJsonUpsert( | |||
| mirrorStatePath, "{ \"sourceRevision\": \"" + headCommitId.name() + "\" }")); | |||
| // Construct the log message and log. | |||
| summary = "Mirror " + abbrId + ", " + remoteRepoUri() + '#' + remoteBranch() + | |||
| summary = "Mirror " + abbrId + ", " + remoteRepoUri() + | |||
| (remoteBranch() != null ? '#' + remoteBranch() : "") + | |||
There was a problem hiding this comment.
Question) In this case, the head branch exists but the user just didn't specify the remote branch.
Would it make sense to use headBranchRef instead?
There was a problem hiding this comment.
Hi @jrhee17, this is just removing #null in the message in case the user doesn't specify the branch.
There was a problem hiding this comment.
Sure, but isn't a branch still used?
There was a problem hiding this comment.
@jrhee17 agree, we use headBranchRef will be more clear. Please check my latest commit 🙇
| if (headBranchRef.getName().startsWith(Constants.R_HEADS)) { | ||
| return headBranchRef.getName().substring(Constants.R_HEADS.length()); | ||
| } | ||
| return ""; |
There was a problem hiding this comment.
Let's just return headBranchRef.getName() because Miiror abbrId, remote-uri# to the repository is a bit awkward.
There was a problem hiding this comment.
Hi @minwoox, you mean line 734 🤔
@jrhee17 Just update some test case to cover the case that having |
Motivation
When unspecified a branch name on mirroring setup, the message show with
#null, we should omit #null when using the default branch to avoid confuse.Result
Close: #933