Re: [PATCH 3/4] branch: add --dry-run option to branch
From: Jonathan Tan <hidden>
Date: 2021-11-23 23:10:41
Glen Choo [off-list ref] 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..."
quoted hunk ↗ jump to hunk
+-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.
quoted hunk ↗ jump to hunk
- 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.
quoted hunk ↗ jump to hunk
@@ -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.) 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).