Subject: Re: [PATCH v4 6/6] builtin/clone: teach git-clone(1) the --revision= option

From: Junio C Hamano <[email-masked]>
Date: Fri, 31 Jan 2025 13:05:28 -0800

Toon Claes <[email-masked]> writes:

>  		OPT_STRING('b', "branch", &option_branch, N_("branch"),
>  			   N_("checkout <branch> instead of the remote's HEAD")),
> +		OPT_STRING(0, "revision", &option_rev, N_("rev"),
> +			   N_("clone single revision <rev> and check out")),

OK, this thing comes as a string; we'll parse it down to a commit
later, hopefully?

> -	refspec_appendf(&remote->fetch, "+%s*:%s*", src_ref_prefix,
> -			branch_top.buf);
> +	if (!option_rev)
> +		refspec_appendf(&remote->fetch, "+%s*:%s*", src_ref_prefix,
> +				branch_top.buf);

> +	die_for_incompatible_opt2(!!option_rev, "--revision",
> +				  !!option_branch, "--branch");
> +	die_for_incompatible_opt2(!!option_rev, "--revision",
> +				  option_mirror, "--mirror");

So here is where we mark the new thing incompatible with these two,
and when either of them is given with "--revision", we would bail
out.  OK.

> +	// TODO --no-single-branch

Style.

> @@ -1381,7 +1396,15 @@ int cmd_clone(int argc,
>  	if (transport->smart_options && !deepen && !filter_options.choice)
>  		transport->smart_options->check_self_contained_and_connected = 1;
>  
> -	strvec_push(&transport_ls_refs_options.ref_prefixes, "HEAD");
> +	if (option_rev) {
> +		option_tags = 0;
> +		option_branch = 0;
> +		option_single_branch = 0;
> +		opts.wants_head = 0;
> +		opts.detach = 1;

option_branch is of type "char *" so sparse rightfully complains
that you are assigning an integer 0 to it, which follows stronger
rules than plain vanilla C standard to help us avoid mistakes.

But stepping back a bit, hasn't we already been ruled out earlier
that when option_rev is set, we cannot possibly be affected by the
"--branch" option that was given at the same time?  I do not now
about other assignments we see in this block, but are there others
that are unnecessary?  For example, you do not clear option_mirror
in this block.  Is option_branch so special that it needs clearing,
and if so why?

Thanks.