Implement hands on hp-push-tags #103

dingZvel · 2025-11-01T10:33:33Z

Exercise Review

Exercise Discussion

Checklist

If you require a new remote repository on the Git-Mastery organization, have you created a request for it?
Have you written unit tests using repo-smith to validate the exercise grading scheme?
Have you tested the download script using test-download.sh?
Have you verified that this exercise does not already exist or is not currently in review?
Did you introduce a new grading mechanism that should belong to git-autograder?
Did you introduce a new dependency that should belong to app?

dingZvel · 2025-11-01T10:46:39Z

I'm not sure what to do when there's a conflict in repo names, i.e., the user already has a repo named samplerepo-preferences. We could ask the user for a new name to rename the fork (using gh repo fork --fork-name).
The gh fork command automatically creates the fork with the name samplerepo-preferences-1 if there's a conflict, and for now I just leave the repo name as is.

damithc · 2025-11-01T11:03:35Z

I'm not sure what to do when there's a conflict in repo names, i.e., the user already has a repo named samplerepo-preferences. We could ask the user for a new name to rename the fork (using gh repo fork --fork-name).
The gh fork command automatically creates the fork with the name samplerepo-preferences-1 if there's a conflict, and for now I just leave the repo name as is.

We can use an alternative fork name such as samplerepo-preferences-1 but in that case we should inform the user about it.
On a related note, GitHub does not allow us to fork the same repo twice. If the user already has a fork of this repo, we may have to abort the operation and inform the user about the conflict e.g., ask the user to delete the fork and run the download operation again.

dingZvel · 2025-11-01T11:51:36Z

We can use an alternative fork name such as samplerepo-preferences-1 but in that case we should inform the user about it. On a related note, GitHub does not allow us to fork the same repo twice. If the user already has a fork of this repo, we may have to abort the operation and inform the user about the conflict e.g., ask the user to delete the fork and run the download operation again.

Sure! Maybe I can submit follow-up commits regarding this later.

dingZvel · 2025-11-02T17:47:07Z

The follow-up commits are up!
Now it aborts the operation when the user already has a fork, and warns the user when there's a conflict in repo name.

woojiahao

This is pending some comments from @damithc on the original hands-on discussion. Let's come back to this once he confirms if we need to fork the repository.

woojiahao · 2025-11-10T04:32:25Z

hands_on/push_tags.py

+    tag_with_options("v1.0", ["HEAD~1"], verbose)
+    annotated_tag_with_options("v0.9", ["HEAD~2", "-m", "First beta release"], verbose)
+
+    pass


Suggested change

pass

woojiahao · 2025-11-10T04:33:18Z

hands_on/push_tags.py

+    check_existing_fork(username, "git-mastery", REPO_NAME)
+    check_same_repo_name(username, REPO_NAME)
+
+    run_command(["gh", "repo", "fork", f"git-mastery/{REPO_NAME}", REPO_NAME, "--clone"], verbose)


I left a comment for @damithc in the original hands-on discussion, but I doubt we actually need to fork the repository for the student. This would avoid polluting the student's Github with excessive repositories they might not be using

woojiahao · 2025-11-20T05:28:51Z

@dingZvel please update this PR as per the linked issue: #119 instructions!

dingZvel · 2025-11-20T11:29:10Z

@woojiahao When I'm implementing the part checking whether the current user has a repo with the same name (but not necessarily a fork), I am a bit confused about the has_repo function in github_cli.py:

def has_repo(repo_name: str, is_fork: bool, verbose: bool) -> bool:
    """Returns if the given repository exists under the current user's repositories."""
    command = ["gh", "repo", "view", repo_name]
    if is_fork:
        command.extend(["--json", "isFork", "--jq", ".isFork"])
    result = run(
        command,
        verbose,
        env={"GH_PAGER": "cat"},
    )
    return result.is_success() and result.stdout == "true"

For this function, if the is_fork argument is False and there is a matching repo, then the gh command will successfully return some details about the repo, but the result.stdout is NOT true, which makes this function return False. However, I believe in this situation the function should return a True? I created a repo with the same name, but not a fork, and using this function, the return value is False.

woojiahao · 2025-11-20T11:35:12Z

@dingZvel Ah good catch, that was a logic bug. I've pushed a fix for it already

dingZvel · 2025-11-20T12:14:58Z

I've refactored the program using the new github_cli util functions! It now does the following:

If the current user already has a fork of git-mastery/samplerepo-preferences, the program aborts and notifies the users to delete the fork and run the download operation again. Else proceed.
If the current user already has a repo named gitmastery-samplerepo-preferences, the program warns the user and proceeds to name the fork as gitmastery-samplerepo-preferences-1. Else proceed.
Clone the fork into a directory named gitmastery-samplerepo-preferences
cd into the directory and do the taggings.

Current issue: If the user already has a repo named gitmastery-samplerepo-preferences AND a repo named gitmastery-samplerepo-preferences-1, the program may fail and exit with code 1.

Possible solution: Add a new util function fork_and_clone_repo with --clone flag appended to the gh fork ... command. The command would then automatically give the fork a new name without any conflict. However, we don't know the new fork name in this case, and we can only warn the user "The fork has been renamed".

woojiahao

Left a comment for discussion, @damithc for your input

woojiahao · 2025-11-21T00:55:58Z

hands_on/push_tags.py

+def check_existing_fork(username: str, fork_owner_name: str, repo_name: str, verbose: bool) -> None:
+    result = run(
+        ["gh",
+        "api",
+        f"repos/{fork_owner_name}/{repo_name}/forks",
+        "-q",
+        f'''.[] | .owner.login | select(. =="{username}")''',
+        ],
+        verbose
+    )
+    if result.is_success():
+        if result.stdout == username:
+            print(f"ERROR: A fork of {fork_owner_name}/{repo_name} already exists! "
+                "Please delete the fork and run this download operation again.\n"
+                "!Aborting...")
+            exit(1)
+
+def check_same_repo_name(username: str, repo_name: str, verbose: bool) -> str:
+    if has_repo(repo_name, False, verbose):
+        print(f"Warning: {username}/{repo_name} already exists, the fork repo will be "
+              f"named as {username}/{repo_name}-1")
+        return repo_name + "-1"
+    return repo_name


Given what @damithc said, if this is used as part of a series of forks, we would be expecting to have already seen the fork before so that we can push to it. So I think it's worth just keeping the repository and avoiding re-forking when a duplicate is found. This is mostly because there is no real way for us to properly abort from this state given the limitations of the tooling

Right! So just to clarify, when an existing fork is found, should I just skip the forking step, obtain the existing fork's repo name, and then clone it? Can I proceed to update the implementation or should I wait for further discussion first?

I believe this is correct.

So, the flow should be like this:

if fork already exists, just clone it

if fork doesn't yet exist, fork it then clone it

You can use the has_repo, clone_repo_with_gh and fork_repo from exercise_utils.

Thank you for the clarification jovnc! Will update later. However, the has_repo function in exercise_utils only checks if a repo with a given name exists. If the user has forked this repo with a random name they created, we would not know the name and thus cannot find out the fork exists. So I believe I still need to stick to my check_existing_fork function.

I agree that checking for existence of fork rather than repo can be more robust, in the event user renamed the repo name of the fork.

Perhaps, we can include a function has_fork inside exercise_utils? This can standardise naming convention and also allows us to reuse it across other exercises in the future.

Sure! I'll then integrate this function into exercise_utils then!

We can wait for @woojiahao input on this, as I'm not 100% certain that exercise_utils can be used for hands on, but from my understanding on the architecture, exercise_utils should be loaded into the namespace for hands on downloads.

VikramGoyal23

Left some comments regarding the new functions introduced, but I think the overall structure of the hands-on makes sense.

exercise_utils/git.py

VikramGoyal23 · 2025-12-06T15:06:56Z

exercise_utils/git.py

+def tag_with_options(tag_name: str, options: List[str], verbose: bool) -> None:
+    """Tags with the given tag_name with specified options."""
+    run_command(["git", "tag", tag_name, *options], verbose)
+
+
+def annotated_tag_with_options(tag_name: str, options: List[str], verbose: bool) -> None:
+    """Adds an annotated tag with the given tag_name with specified options."""
+    run_command(["git", "tag", "-a", tag_name, *options], verbose)


It is technically not necessary to have both of these functions, as the -a flag is unnecessary to make an annotated tag if the -m flag is passed with its message. However, since it increases code readability, I am not opposed to keeping both of these in.

exercise_utils/github_cli.py

dingZvel · 2025-12-08T08:01:57Z

Fixes are up for review! Sorry for the delay, I was between travels.

VikramGoyal23

Yup, looks alright from my end now, just waiting for @woojiahao's input on the additions to exercise_utils.

Implement hands on hp-push-tags

7c554f2

Fix the cloned repo directory name to REPO_NAME

598a3b8

damithc linked an issue Nov 2, 2025 that may be closed by this pull request

[Hands-On Discussion] T4L2/hp-push-tags (Pushing tags to a remote) #94

Open

dingZvel added 2 commits November 3, 2025 00:23

Notify user when fork name is changed

5dc73ff

Abort script when a fork already exists

d851e1d

woojiahao added the hands-on review label Nov 10, 2025

woojiahao requested changes Nov 10, 2025

View reviewed changes

woojiahao added the discussing label Nov 10, 2025

woojiahao mentioned this pull request Nov 16, 2025

[NOTICE] Aligning on how to fork a repository #119

Open

7 tasks

Merge branch 'git-mastery:main' into main

3aece8a

dingZvel and others added 4 commits November 20, 2025 19:43

Refactor using core util functions for gh

0b48ebf

Merge branch 'git-mastery:main' into main

d4457fe

Refactor using has_repo util function

f003982

Fix the name of directory to "gitmastery-samplerepo-preferences"

e1ac310

dingZvel requested a review from woojiahao November 20, 2025 12:15

woojiahao reviewed Nov 21, 2025

View reviewed changes

dingZvel and others added 2 commits December 5, 2025 10:44

Merge branch 'git-mastery:main' into main

c63cbc2

Skip and clone when an existing fork is found

bac4807

dingZvel requested a review from woojiahao December 5, 2025 03:37

VikramGoyal23 suggested changes Dec 6, 2025

View reviewed changes

Fix based on VikramGoyal23's review

246e7c3

dingZvel requested a review from VikramGoyal23 December 8, 2025 07:58

VikramGoyal23 approved these changes Dec 8, 2025

View reviewed changes

Implement hands on hp-push-tags #103

Are you sure you want to change the base?

Implement hands on hp-push-tags #103

Uh oh!

Conversation

dingZvel commented Nov 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Exercise Review

Exercise Discussion

Checklist

Uh oh!

dingZvel commented Nov 1, 2025

Uh oh!

damithc commented Nov 1, 2025

Uh oh!

dingZvel commented Nov 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dingZvel commented Nov 2, 2025

Uh oh!

woojiahao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

woojiahao commented Nov 20, 2025

Uh oh!

dingZvel commented Nov 20, 2025

Uh oh!

woojiahao commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dingZvel commented Nov 20, 2025

Uh oh!

woojiahao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dingZvel Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jovnc Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VikramGoyal23 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dingZvel commented Dec 8, 2025

Uh oh!

VikramGoyal23 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dingZvel commented Nov 1, 2025 •

edited

Loading

dingZvel commented Nov 1, 2025 •

edited

Loading

woojiahao commented Nov 20, 2025 •

edited

Loading

dingZvel Nov 21, 2025 •

edited

Loading

jovnc Dec 5, 2025 •

edited

Loading