Jonathan Tan <jonathantanmy@xxxxxxxxxx> writes:
> Glen Choo <chooglen@xxxxxxxxxx> 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.
