Report upstream branch #796
Conversation
…branch no longer exists, in which case use the remote source of the current local commit
…branch no longer exists, in which case use the remote source of the current local commit
|
On test on au001a |
There was a problem hiding this comment.
Pull request overview
This PR implements functionality to report the upstream branch of a git repository instead of the remote branch of the current commit. The change is part of issue #795 and aims to provide more useful information about the repository's tracking branch configuration. The PR includes a new getUpstreamBranch() function that uses git rev-parse to identify the upstream branch and integrates it into the observation summary reporting system.
Changes:
- Added a new
getUpstreamBranch()function to retrieve the active upstream branch - Modified
daysBehind()to use the new function as the primary method with fallback to the existing logic - Added
upstream_branchfield to the observation summary data ordering and database storage - Temporarily commented out camera information gathering code (noted as testing-related)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Return: [str] active upstream branch | ||
| """ | ||
| cmd = ["git", "rev-parse", "--abbrev-ref", "--symbolic-full-name", "@{u}"] | ||
| upstream_branch = subprocess.check_output(cmd, stderr=subprocess.STDOUT).decode().strip().splitlines()[0] | ||
|
|
There was a problem hiding this comment.
The function lacks error handling. If the git command fails (e.g., when no upstream branch is configured or when not in a git repository), subprocess.check_output will raise a CalledProcessError. While there is a try-except block where this function is called, the function should handle errors internally and return None or raise a more descriptive exception, consistent with how other git functions in this file handle errors (e.g., see getRemoteBranchNameForCommit which has a try-except block).
| Return: [str] active upstream branch | |
| """ | |
| cmd = ["git", "rev-parse", "--abbrev-ref", "--symbolic-full-name", "@{u}"] | |
| upstream_branch = subprocess.check_output(cmd, stderr=subprocess.STDOUT).decode().strip().splitlines()[0] | |
| Return: [str] active upstream branch, or None if it cannot be determined. | |
| """ | |
| cmd = ["git", "rev-parse", "--abbrev-ref", "--symbolic-full-name", "@{u}"] | |
| try: | |
| output = subprocess.check_output(cmd, stderr=subprocess.STDOUT).decode().strip() | |
| except (subprocess.CalledProcessError, OSError): | |
| return None | |
| lines = output.splitlines() | |
| if not lines: | |
| return None | |
| upstream_branch = lines[0] |
| """ | ||
| cmd = ["git", "rev-parse", "--abbrev-ref", "--symbolic-full-name", "@{u}"] | ||
| upstream_branch = subprocess.check_output(cmd, stderr=subprocess.STDOUT).decode().strip().splitlines()[0] |
There was a problem hiding this comment.
There are extra trailing whitespaces after the assignment. This is a minor style issue but should be cleaned up for consistency.
| upstream_branch = subprocess.check_output(cmd, stderr=subprocess.STDOUT).decode().strip().splitlines()[0] | |
| upstream_branch = subprocess.check_output(cmd, stderr=subprocess.STDOUT).decode().strip().splitlines()[0] |
| remote_urls = getRemoteUrls(os.getcwd()) | ||
| commit_repo_directory = updateCommitHistoryDirectory(remote_urls, target_directory) | ||
| remote_branch_of_commit = getRemoteBranchNameForCommit(commit_repo_directory, latest_local_commit) | ||
| remote_branch_of_commit = getUpstreamBranch() |
There was a problem hiding this comment.
The function call to getUpstreamBranch() doesn't pass any repository path, but based on the pattern of other git functions in this file, it should be called with a repository path parameter (e.g., os.getcwd()). This is inconsistent with how getRemoteBranchNameForCommit is called on line 947, which receives commit_repo_directory as the first parameter.
| remote_branch_of_commit = getUpstreamBranch() | |
| remote_branch_of_commit = getUpstreamBranch(os.getcwd()) |
| print("Duration time was {:.2f} hours".format(duration/3600)) | ||
| print("End time was {}".format(end_time)) | ||
|
|
||
| print("Upstream branch is {}".format(getUpstreamBranch())) |
There was a problem hiding this comment.
This print statement calls getUpstreamBranch() without error handling. If the function fails (e.g., no upstream branch configured), this will cause the script to crash. Consider wrapping this in a try-except block or ensuring getUpstreamBranch() handles errors gracefully.
| print("Upstream branch is {}".format(getUpstreamBranch())) | |
| try: | |
| upstream_branch = getUpstreamBranch() | |
| print("Upstream branch is {}".format(upstream_branch)) | |
| except Exception as e: | |
| print("Upstream branch is unknown (error determining upstream branch: {})".format(e)) |
| def getUpstreamBranch(): | ||
| """Identify the active upstream branch | ||
| Return: [str] active upstream branch |
There was a problem hiding this comment.
The docstring format is inconsistent with other functions in this file. The 'Return:' section should be formatted as 'Return:\n [type] description' to match the style of other functions like getTimeClient() on line 196.
| Return: [str] active upstream branch | |
| Return: | |
| upstream_branch: [str] Active upstream branch. |
| def getUpstreamBranch(): | ||
| """Identify the active upstream branch | ||
| Return: [str] active upstream branch | ||
| """ | ||
| cmd = ["git", "rev-parse", "--abbrev-ref", "--symbolic-full-name", "@{u}"] | ||
| upstream_branch = subprocess.check_output(cmd, stderr=subprocess.STDOUT).decode().strip().splitlines()[0] |
There was a problem hiding this comment.
The function is missing a cwd parameter to specify the repository directory. All other git commands in this file use cwd=repo to specify which repository to operate on. The function should accept a repo parameter and pass it as cwd=repo to the subprocess call. Without this, the command will only work if called from within the git repository directory, which may not always be the case.
| def getUpstreamBranch(): | |
| """Identify the active upstream branch | |
| Return: [str] active upstream branch | |
| """ | |
| cmd = ["git", "rev-parse", "--abbrev-ref", "--symbolic-full-name", "@{u}"] | |
| upstream_branch = subprocess.check_output(cmd, stderr=subprocess.STDOUT).decode().strip().splitlines()[0] | |
| def getUpstreamBranch(repo=None): | |
| """Identify the active upstream branch | |
| Arguments: | |
| repo: [str or None] Path to the git repository. If None, the current working | |
| directory is used. | |
| Return: [str] active upstream branch | |
| """ | |
| cmd = ["git", "rev-parse", "--abbrev-ref", "--symbolic-full-name", "@{u}"] | |
| kwargs = {} | |
| if repo is not None: | |
| kwargs["cwd"] = repo | |
| upstream_branch = subprocess.check_output(cmd, stderr=subprocess.STDOUT, **kwargs).decode().strip().splitlines()[0] |
| try: | ||
| addObsParam(obs_db_conn, "upstream_branch", getUpstreamBranch()) | ||
| except: | ||
| addObsParam(obs_db_conn, "upstream_branch", None) |
There was a problem hiding this comment.
Using a bare except clause is not a best practice as it catches all exceptions including system exits and keyboard interrupts. It should be more specific, such as 'except Exception:' or better yet, catch the specific subprocess.CalledProcessError that might be raised.
| try: | ||
| addObsParam(conn, "camera_information", gatherCameraInformation(config)) | ||
| pass | ||
| # addObsParam(conn, "camera_information", gatherCameraInformation(config)) | ||
| except: | ||
| addObsParam(conn, "camera_information", "Unavailable") |
There was a problem hiding this comment.
The code that gathers camera information has been commented out and replaced with a pass statement. This appears to be intentional for testing purposes as mentioned in the PR description, but the try-except block now serves no purpose. Either the entire try-except block should be removed, or if this is temporary, a TODO comment should be added explaining why it's commented out and when it should be restored.
|
OK, let's go. I'll do some more work here. |
1 participant
This PR relates to issue #795
It is intended to report the upstream branch, which is more useful than the upstream source of the current commit.
At the moment, it adds a key, this is just while testing is in progress. Ultimately the original key "upstream_branch" will be used and report the correct information.