fix: pass extra options to 'git fetch'

[dwilding]

This PR makes sure that extra options provided after -- are passed through to all git-fetch calls. Without this, providing -- --tags wouldn't work as expected when cloning a fork of an upstream repo.

I'm also adding tests for the option handling, Git error handling, and updating the docs.

[Copilot]

Pull request overview

This PR fixes a bug where extra git options provided after -- were not being passed through to subsequent git-fetch operations, only to the initial git-clone operation. The fix ensures that options like --tags or --recurse-submodules work correctly throughout the entire cloning workflow, including when fetching upstream branches.

Changes:

• Renamed extra_args to fetch_opts and changed it to always include --no-tags by default, allowing users to override with -- --tags
• Refactored error handling to use helper functions is_access_error() and make_access_error() for consistency
• Simplified the Base dataclass by removing the remote_name field and using base.remote.name directly
• Added comprehensive tests for tag fetching behavior and git error handling
• Updated documentation to clarify that extra options are git-fetch options, not git-clone options

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
src/gimmegit/_cli.py	Core implementation changes: renamed parameters, refactored fetch calls to accept fetch_opts, improved error handling with new helper functions, updated update-branch alias generation
src/gimmegit/_help.py	Updated help text to change "git-options" to "fetch-opts" and added description of fetch-opts parameter
README.md	Updated documentation section title and parameter names from "git-options" to "fetch-opts"
tests/functional/test_clone.py	Added three new test functions for tags and submodules, added assertions to verify default no-tags behavior
tests/functional/test_git_errors.py	Added test for invalid fetch option error handling
tests/functional/test_token.py	Added assertion to verify default no-tags behavior
tests/functional/helpers_functional.py	Added get_tags() helper function to check for git tags

[Copilot]

The fetch_opts list should be unpacked when passed to the git.fetch() method. Currently, a list is being passed as a single argument, which will not work correctly. Use the unpacking operator to pass the list elements as separate arguments: cloned.git.fetch(*fetch_opts, "origin", refspec)

[dwilding]

No, this is intended. I'm passing everything in the list as a "refspec", with only the last item being the real refspec. This lets me pass arbitrary options to fetch. Slightly hacky, but needed as fetch in GitPython doesn't support multi_options.

From the docs:

Fetch supports multiple refspecs (as the underlying git-fetch(1) does) - supplying a list rather than a string for 'refspec' will make use of this facility.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (3)

src/gimmegit/_cli.py:335

• When jumbo mode is enabled, the --shallow-since option is prepended to fetch_opts and this modified list is passed to create_local_branch, where it's stored in the update-branch git alias. This means users who run git update-branch weeks or months after the initial clone will be using a stale --shallow-since date that refers to the clone time, not the current time.

To fix this, separate the options used during clone-time fetches from those stored in the alias. The --shallow-since option should only be used for the immediate fetch operations during cloning (lines 296, 316, 322), but not stored in the git alias (line 326). Consider creating two separate lists: one for clone-time fetches that includes --shallow-since, and one for the alias that only includes user-provided options.

        fetch_opts = [f"--shallow-since={shallow_date}", *fetch_opts]
    try:
        cloned = git.Repo.clone_from(
            context.clone_url,
            context.clone_dir,
            single_branch=True,
            multi_options=fetch_opts,
        )
    except git.GitCommandError as e:
        if is_access_error(e):
            raise CloneError(make_access_error(False))
        raise CloneError(make_generic_git_error(e))
    if not context.base_branch:
        context.base_branch = get_default_branch(cloned)
    if context.upstream_url:
        logger.info(f"Setting upstream to {context.upstream_url}")
        upstream = cloned.create_remote("upstream", context.upstream_url)
        create_local_branch(cloned, upstream, context, fetch_opts)
    else:
        create_local_branch(cloned, None, context, fetch_opts)


def compare_usage(status: _status.Status) -> None:
    if not status.has_remote:
        exit_with_error("The review branch has not been created. Push to GitHub first.")
    if not os.isatty(sys.stdout.fileno()):
        logger.log(DATA_LEVEL, status.compare_url)
        return
    # Try xdg-open first, to suppress a Linux/snap/Firefox error message:
    # Gtk-Message: ... Not loading module "atk-bridge"...
    if shutil.which("xdg-open"):
        result = subprocess.run(
            ["xdg-open", status.compare_url],
            stdout=subprocess.DEVNULL,
            stderr=subprocess.DEVNULL,
        )
        if result.returncode:
            logger.log(DATA_LEVEL, status.compare_url)
        return
    try:
        opened = webbrowser.open(status.compare_url, new=2)
    except webbrowser.Error:
        logger.log(DATA_LEVEL, status.compare_url)
    else:
        if not opened:
            logger.log(DATA_LEVEL, status.compare_url)


def configure_logger_data() -> None:
    retval = logging.StreamHandler(sys.stdout)
    retval.setFormatter(logging.Formatter("%(message)s"))
    retval.addFilter(lambda _: _.levelno == DATA_LEVEL)
    logger.addHandler(retval)


def configure_logger_error() -> None:
    error = logging.StreamHandler(sys.stderr)
    if COLOR["stderr"]:
        error.setFormatter(logging.Formatter("\033[1;31mError:\033[0m %(message)s"))
    else:
        error.setFormatter(logging.Formatter("Error: %(message)s"))
    error.addFilter(lambda _: _.levelno == logging.ERROR)
    logger.addHandler(error)


def configure_logger_info() -> None:
    if INFO_TO == "stdout":
        info = logging.StreamHandler(sys.stdout)
    else:
        info = logging.StreamHandler(sys.stderr)
    info.setFormatter(logging.Formatter("%(message)s"))
    info.addFilter(lambda _: _.levelno == logging.INFO)
    logger.addHandler(info)


def configure_logger_warning() -> None:
    warning = logging.StreamHandler(sys.stderr)
    if COLOR["stderr"]:
        warning.setFormatter(logging.Formatter("\033[33mWarning:\033[0m %(message)s"))
    else:
        warning.setFormatter(logging.Formatter("Warning: %(message)s"))
    warning.addFilter(lambda _: _.levelno == logging.WARNING)
    logger.addHandler(warning)


def create_local_branch(
    cloned: git.Repo, upstream: git.Remote | None, context: Context, fetch_opts: list[str]
):
    """Create the local branch and define the ``update-branch`` alias. ``context.base_branch`` cannot be ``None``."""
    assert context.base_branch
    origin = cloned.remotes.origin
    if upstream:
        assert context.upstream_owner
        base = Base(
            branch=context.base_branch,
            full=f"{context.upstream_owner}:{context.base_branch}",
            owner=context.upstream_owner,
            read_error=make_access_error(True),
            remote=upstream,
        )
    else:
        base = Base(
            branch=context.base_branch,
            full=f"{context.owner}:{context.base_branch}",
            owner=context.owner,
            read_error="Unable to access repo. Try running gimmegit again.",
            remote=origin,
        )
    if context.create_branch:
        # Create a local branch, starting from the base branch.
        logger.info(
            f"Checking out a new branch {f_blue(context.branch)} based on {f_blue(base.full)}"
        )
        if base.branch not in base.remote.refs:
            fetch_base(cloned, base, fetch_opts)
        branch = cloned.create_head(context.branch, base.remote.refs[base.branch])
        # Ensure that on first push, a remote branch is created and set as the tracking branch.
        # The remote branch will be created on origin (the default remote).
        with cloned.config_writer() as config:
            config.set_value(
                "push",
                "default",
                "current",
            )
            config.set_value(
                "push",
                "autoSetupRemote",
                "true",
            )
    else:
        # Create a local branch that tracks the existing branch on origin.
        branch_full = f"{context.owner}:{context.branch}"
        logger.info(f"Checking out {f_blue(branch_full)} with base {f_blue(base.full)}")
        if context.branch not in origin.refs:
            fetch_branch(cloned, context.branch, branch_full, fetch_opts)
        branch = cloned.create_head(context.branch, origin.refs[context.branch])
        branch.set_tracking_branch(origin.refs[context.branch])
        # We don't need the base branch for anything at this stage.
        # Fetch the base branch to ensure that a local tracking branch exists.
        if base.branch not in base.remote.refs:
            fetch_base(cloned, base, fetch_opts)
    branch.checkout()
    # Define the 'update-branch' alias.
    with cloned.config_writer() as config:
        fetch_opts_str = " ".join(fetch_opts)
        update_branch = "!" + " && ".join(
            [
                "branch=$(git config --get gimmegit.branch)",
                "base_remote=$(git config --get gimmegit.baseRemote)",
                "base_branch=$(git config --get gimmegit.baseBranch)",
                'echo \\"$ git checkout $branch\\"',
                "git checkout $branch",
                f'echo \\"$ git fetch {fetch_opts_str} $base_remote $base_branch\\"',
                f"git fetch {fetch_opts_str} $base_remote $base_branch",

src/gimmegit/_help.py:23

• The updated help text mentions that users can provide extra git-fetch options like --tags or --recurse-submodules, but it doesn't explain that --no-tags is used by default. The old help text explicitly documented this: "gimmegit sets --no-tags and --single-branch when cloning the repo. Use '-- ' to provide extra 'git clone' options. For example, use '-- --tags' to cancel out --no-tags."

Consider adding a brief note about the default behavior, such as: "By default, gimmegit uses --no-tags. Use '-- --tags' to fetch tags." This helps users understand why they need to explicitly pass --tags if they want tags.

<fetch-opts>   Extra git-fetch options. For example, --tags or --recurse-submodules.

README.md:338

• The updated documentation mentions that users can provide extra git-fetch options like --tags or --recurse-submodules, but it doesn't explain that --no-tags is used by default. The old documentation explicitly mentioned this: "gimmegit sets --no-tags and --single-branch when cloning the repo."

Consider adding a brief note about the default behavior to help users understand why they need to explicitly pass --tags if they want tags fetched during clone.

<fetch-opts>   Extra git-fetch options. For example, --tags or --recurse-submodules.

[Copilot]

There is a potential command injection vulnerability when constructing the update-branch git alias. The fetch_opts list is joined with spaces and directly interpolated into a shell command without proper escaping. If a user provides malicious options containing shell metacharacters (e.g., -- "; rm -rf /"), these will be executed when the alias is invoked.

To fix this, each option in fetch_opts should be properly shell-escaped before being interpolated into the command string. Consider using shlex.quote() for each element in fetch_opts before joining them.