Re: [PATCH v4 2/3] worktree add: add --orphan flag

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Dec 12 2022, Jacob Abel wrote:

> +static int make_worktree_orphan(const char * ref, const struct add_opts *opts,
> +				struct strvec *child_env)
> +{
> +	int ret;

You can avoid this variable entirely....

> +	struct strbuf symref = STRBUF_INIT;
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	cp.git_cmd = 1;

(aside: We usually split up variables & decls, I think this is better
right before the run_command() line).
> +
> +	validate_new_branchname(ref, &symref, 0);
> +	strvec_pushl(&cp.args, "symbolic-ref", "HEAD", symref.buf, NULL);

...by just calling strbuf_release(&symref); right after this line, we'll
never need it again, and the strvec will have its own copy.

> +	if (opts->quiet)
> +		strvec_push(&cp.args, "--quiet");
> +	strvec_pushv(&cp.env, child_env->v);

So:

> +	ret = run_command(&cp);
> +	strbuf_release(&symref);
> +	return ret;

We don't have to carry the "ret" here, and can just do:

	return run_command(&cp);

> +}
> +
>  static int add_worktree(const char *path, const char *refname,
>  			const struct add_opts *opts)
>  {
> @@ -393,8 +415,9 @@ static int add_worktree(const char *path, const char *refname,
>  			die_if_checked_out(symref.buf, 0);
>  	}
>  	commit = lookup_commit_reference_by_name(refname);
> -	if (!commit)
> +	if (!commit && !opts->orphan) {
>  		die(_("invalid reference: %s"), refname);
> +	}

We don't add {}'s for one-statement if's like this, see
CodingGuidelines. So skip the {}'s.

>
>  	name = worktree_basename(path, &len);
>  	strbuf_add(&sb, name, path + len - name);
> @@ -482,10 +505,10 @@ static int add_worktree(const char *path, const char *refname,
>  	strvec_pushf(&child_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);
>  	cp.git_cmd = 1;
>
> -	if (!is_branch)
> +	if (!is_branch && commit) {
>  		strvec_pushl(&cp.args, "update-ref", "HEAD",
>  			     oid_to_hex(&commit->object.oid), NULL);
> -	else {
> +	} else {

Here that style change is good, even if it inflates the diff size a
litte bit with the while-at-it fixu-up.

> +	/*
> +	 * When creating a new branch, new_branch now contains the branch to
> +	 * create.
> +	 *
> +	 * Past this point, new_branch_force can be treated solely as a
> +	 * boolean flag to indicate whether `-B` was selected.
> +	 */
>  	if (new_branch_force) {
>  		struct strbuf symref = STRBUF_INIT;
>

I think I commented on this commentary in an earlier round. IMO it could
just be omitted, as the code is rather self-explanatory.

To the extent that it isn't this commentary just makes things more
confusing, at least to my reading. It's not explaining what the code is
doing now, because the very next line after this context (omitted here) is:

	new_branch = new_branch_force

So we're saying it "can be treated solely as a boolean flag", but it
isn't being treated as such by the code now.

And the "new_branch now contains the branch to create" is also
inaccurate, we're about to make it true with that assignment, but (and
again, I don't think a comment is needed at all) *if* we think that's
worth commenting on then surely the first paragraph of the comment
should be split off, and come just before that assignment.

> -	if (new_branch) {
> +	if (opts.orphan) {
> +		branch = new_branch;
> +	} else if (!lookup_commit_reference_by_name(branch)) {
> +		/*
> +		 * If `branch` does not reference a valid commit, a new
> +		 * worktree (and/or branch) cannot be created based off of it.
> +		 */

I think with the advice added in 3/3 this comment can also just be
omitted here, as the end result is that the comment will be
re-explaining something which should be obvious from the inline advice
string (and if it isn't, that inline string needs improving).

> -test_expect_success '"add" -b/-B mutually exclusive' '
> -	test_must_fail git worktree add -b poodle -B poodle bamboo main
> -'
> -
> -test_expect_success '"add" -b/--detach mutually exclusive' '
> -	test_must_fail git worktree add -b poodle --detach bamboo main
> -'
> +# Helper function to test mutually exclusive options.
> +test_wt_add_excl() {
> +	local opts="$@" &&
> +	test_expect_success "'worktree add' with '$opts' has mutually exclusive options" '
> +		test_must_fail git worktree add $opts
> +	'
> +}
>
> -test_expect_success '"add" -B/--detach mutually exclusive' '
> -	test_must_fail git worktree add -B poodle --detach bamboo main
> -'
> +test_wt_add_excl -b poodle -B poodle bamboo main
> +test_wt_add_excl -b poodle --orphan poodle bamboo
> +test_wt_add_excl -b poodle --detach bamboo main
> +test_wt_add_excl -B poodle --detach bamboo main
> +test_wt_add_excl -B poodle --detach bamboo main
> +test_wt_add_excl -B poodle --orphan poodle bamboo
> +test_wt_add_excl --orphan poodle --detach bamboo
> +test_wt_add_excl --orphan poodle --no-checkout bamboo
> +test_wt_add_excl --orphan poodle bamboo main

It's good to see this as a helper function, but I think it would be nice
to have this split up into its own pre-refactoring commit.

As here we're changing some existing tests that are per-se unrelated,
just so that they can use this new helper.

This commit could then add tests that use the helper, and which are new
for --orphan.