Skip to content

Commit

Permalink
cmd/util: fix error handling for parseArgsWithGitBranchMR
Browse files Browse the repository at this point in the history
The parse logic to get the MR ID from remote and/or branch name was kinda
confusing:

1) the variable names were non-obvious
2) the error code was being ignored because it was being used as part of the
logic itself: it was fine to fail.

However, after the commit 69ae19e ("cmd: fix return value checks and
statements") was introduced, the error handling broke the logic.

To make things clearer and also to fix the breakage, this patch changed the
variable names and considered the error returning value to make sure we can
safely return with a valid ID value (in case the actual MR ID was used and
not the remote and/or branch name).

It's a soft revert of the commit 69ae19e ("cmd: fix return value checks
and statements") in this specific codepath, but still avoids golint
complaining about an error returning value being ignored.

Signed-off-by: Bruno Meneguele <bmeneg@redhat.com>
  • Loading branch information
bmeneg committed Jun 15, 2021
1 parent ecd57aa commit b9e0de0
Showing 1 changed file with 22 additions and 29 deletions.
51 changes: 22 additions & 29 deletions cmd/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,12 +175,10 @@ func parseArgsRemoteAndBranch(args []string) (string, string, error) {
}

remote, branch, err := parseArgsRemoteAndString(args)
if branch == "" && err == nil {
branch, err = git.CurrentBranch()
}

if err != nil {
return "", "", err
} else if branch == "" {
branch, err = git.CurrentBranch()
}

remoteBranch, _ := git.UpstreamBranch(branch)
Expand Down Expand Up @@ -311,38 +309,33 @@ func parseArgsRemoteAndString(args []string) (string, string, error) {
// If no number is specified, the MR id associated with the given branch
// is returned, using the current branch as fallback.
func parseArgsWithGitBranchMR(args []string) (string, int64, error) {
var (
s string
branch string
err error
)
s, i, err := parseArgsRemoteAndID(args)
rn, id, err := parseArgsRemoteAndID(args)
if err == nil && id != 0 {
return rn, id, nil
}

rn, branch, err := parseArgsRemoteAndString(args)
if err != nil {
return "", 0, err
}

if i == 0 {
s, branch, err = parseArgsRemoteAndString(args)
if err != nil {
return "", 0, err
}
rn, err = getRemoteName(rn)
if err != nil {
return "", 0, err
}

s, err = getRemoteName(s)
if err != nil {
return "", 0, err
}
if branch == "" {
id = int64(getCurrentBranchMR(rn))
} else {
id = int64(getBranchMR(rn, branch))
}

if branch == "" {
i = int64(getCurrentBranchMR(s))
} else {
i = int64(getBranchMR(s, branch))
}
if i == 0 {
fmt.Println("Error: Cannot determine MR id.")
os.Exit(1)
}
if id == 0 {
err = fmt.Errorf("cannot determine MR id")
return "", 0, err
}
return s, i, nil

return rn, id, nil
}

// filterCommentArg separate the case where a command can have both the
Expand Down

0 comments on commit b9e0de0

Please sign in to comment.