Re: [PATCH 3/4] branch: add --dry-run option to branch

[Glen Choo <chooglen@google.com>]

Jonathan Tan <jonathantanmy@google.com> writes:

> Glen Choo <chooglen@google.com> writes:
>> When running "git branch --recurse-submodules topic"
>
> At this point, this argument has not been introduced yet, so better to
> just say "A future patch will introduce branch creation that recurses
> into submodules, so..."
>
>> +-n::
>> +--dry-run::
>> +	Can only be used when creating a branch. If the branch creation
>> +	would fail, show the relevant error message. If the branch
>> +	creation would succeed, show nothing.
>
> Right now we only plan to use this internally so it's not worth using a
> single character argument for this right now, I think. We can always add
> it later if we find it useful.

For the reasons you specified, I didn't intend to add -n. However, -n is
automatically added by OPT__DRY_RUN, so I thought it was appropriate to
document it.

>> -	if (!!delete + !!rename + !!copy + !!new_upstream + !!show_current +
>> -	    list + edit_description + unset_upstream > 1)
>> +	create = 1 - (!!delete + !!rename + !!copy + !!new_upstream +
>> +		      !!show_current + !!list + !!edit_description +
>> +		      !!unset_upstream);
>> +	if (create < 0)
>>  		usage_with_options(builtin_branch_usage, options);
>
> Hmm...I think it would be clearer just to call it noncreate_options and
> print usage if it is greater than 1. Then whenever you want to check if
> it's create, check `!noncreate_options` instead.

Sounds good.

>> @@ -852,10 +862,20 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>>  		if (track == BRANCH_TRACK_OVERRIDE)
>>  			die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead."));
>>  
>> -		create_branch(the_repository,
>> -			      argv[0], (argc == 2) ? argv[1] : head,
>> -			      force, 0, reflog, quiet, track);
>> +		if (dry_run) {
>> +			struct strbuf buf = STRBUF_INIT;
>> +			char *unused_full_ref;
>> +			struct object_id unused_oid;
>>  
>> +			validate_new_branchname(branch_name, &buf, force);
>> +			validate_branch_start(the_repository, start_name, track,
>> +					      &unused_oid, &unused_full_ref);
>> +			strbuf_release(&buf);
>> +			FREE_AND_NULL(unused_full_ref);
>> +			return 0;
>> +		}
>> +		create_branch(the_repository, branch_name, start_name, force, 0,
>> +			      reflog, quiet, track);
>>  	} else
>>  		usage_with_options(builtin_branch_usage, options);
>>  
>
> I don't think we should use separate code paths for the dry run and the
> regular run - could create_branch() take a dry_run parameter instead?
> (If there are too many boolean parameters, it might be time to convert
> some or all to a struct.)

That sounds reasonable, it would be good not to have duplicate code
paths.

> This suggestion would require a reworking of patch 2, which is why I
> didn't comment there. But if we are not going to use this suggestion and
> are going to stick with patch 2, then my comment on it is that it seems
> to be doing too much: I ran "git show --color-moved" on it and saw that
> quite a few lines were newly introduced (not just moved around).

I will do the reworking, but the final result will probably look very
similar to the one in patch 2. Does it look more acceptable with
--color-moved-ws=ignore-all-space? Some text had to be reindented
(because I removed one conditional), but I also replaced some functions
with repo_* versions.