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

From: Patrick Steinhardt <ps@pks.im>
Date: Feb 3, 2025, 07:51 UTC
Message-ID: <Z6B1bM7bbpNUNjpt@pks.im>
In-Reply-To: <20250131-toon-clone-refs-v4-6-2a4ff851498f@iotcl.com>

On Fri, Jan 31, 2025 at 04:30:34PM +0100, Toon Claes wrote:

Show 11 quoted lines

> diff --git a/builtin/clone.c b/builtin/clone.c
> index f92017c751dd31cb25a3ba31667b015d5766ce84..40d6ecfa74608193a88715a29b4ca765687a0c86 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -585,6 +586,10 @@ static void update_head(const struct ref *our, const struct ref *remote,
>  	} else if (our) {
>  		struct commit *c = lookup_commit_reference(the_repository,
>  							   &our->old_oid);
> +
> +		if (!c)
> +			die(_("couldn't look up commit object for '%s'"), our->name);

You can use `lookup_commit_or_die()` instead.

Show 10 quoted lines

> @@ -1500,6 +1534,12 @@ int cmd_clone(int argc,
>  		if (!our_head_points_at)
>  			die(_("Remote branch %s not found in upstream %s"),
>  			    option_branch, remote_name);
> +	} else if (option_rev) {
> +		our_head_points_at = mapped_refs;
> +		if (!our_head_points_at)
> +			die(_("Remote revision %s not found in upstream %s"),
> +			    option_rev, remote_name);
> +		//mapped_refs->name[0] = 0;

This looks like a left-over debug statement?

Show 20 quoted lines

> diff --git a/parse-options.h b/parse-options.h
> index 39f088625494f20dea96b9a9cbe986916773bf60..fca944d9a93d643d984c58de2ead9154c8b16c94 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -436,6 +436,15 @@ static inline void die_for_incompatible_opt3(int opt1, const char *opt1_name,
>  				  0, "");
>  }
>  
> +static inline void die_for_incompatible_opt2(int opt1, const char *opt1_name,
> +					     int opt2, const char *opt2_name)
> +{
> +	die_for_incompatible_opt4(opt1, opt1_name,
> +				  opt2, opt2_name,
> +				  0, "",
> +				  0, "");
> +}
> +
>  /*
>   * Use these assertions for callbacks that expect to be called with NONEG and
>   * NOARG respectively, and do not otherwise handle the "unset" and "arg"

It might make sense to introduce this in a separate commit, ideally with an example callsite. `grep die\(.*incompatible` surfaces a couple of candidates.

Patrick