| Message ID | 20211122223252.19922-4-chooglen@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | implement branch --recurse-submodules | expand |
On Mon, Nov 22 2021, Glen Choo wrote: > Add a --dry-run option to branch creation that can check whether or not > a branch name and start point would be valid for a repository without > creating a branch. Refactor cmd_branch() to make the chosen action more > obvious. > [...] > -'git branch' [--track | --no-track] [-f] <branchname> [<start-point>] > +'git branch' [--track | --no-track] [-f] [--dry-run | -n] <branchname> [<start-point>] > 'git branch' (--set-upstream-to=<upstream> | -u <upstream>) [<branchname>] > 'git branch' --unset-upstream [<branchname>] > 'git branch' (-m | -M) [<oldbranch>] <newbranch> > @@ -205,6 +205,12 @@ This option is only applicable in non-verbose mode. > --no-abbrev:: > Display the full sha1s in the output listing rather than abbreviating them. > > +-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. > + The usage & test show that we've got --dry-run for branch creation, but not the "creation" we do on --copy or --move. The former is just a "create from source", but "move" maybe not. In any case, any reason to leave those out?
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Mon, Nov 22 2021, Glen Choo wrote: > >> Add a --dry-run option to branch creation that can check whether or not >> a branch name and start point would be valid for a repository without >> creating a branch. Refactor cmd_branch() to make the chosen action more >> obvious. >> [...] >> -'git branch' [--track | --no-track] [-f] <branchname> [<start-point>] >> +'git branch' [--track | --no-track] [-f] [--dry-run | -n] <branchname> [<start-point>] >> 'git branch' (--set-upstream-to=<upstream> | -u <upstream>) [<branchname>] >> 'git branch' --unset-upstream [<branchname>] >> 'git branch' (-m | -M) [<oldbranch>] <newbranch> >> @@ -205,6 +205,12 @@ This option is only applicable in non-verbose mode. >> --no-abbrev:: >> Display the full sha1s in the output listing rather than abbreviating them. >> >> +-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. >> + > > The usage & test show that we've got --dry-run for branch creation, but > not the "creation" we do on --copy or --move. Perhaps this is more of a wording issue i.e. 'creating a branch' is too unspecific. Maybe Can only be used when creating a new branch (without copying or moving an existing branch). If the branch creation would fail, show the relevant error message. If the branch creation would succeed, show nothing. > In any case, any reason to leave those out? No long term reason. I left those out because "create brand new branch with --dry-run" was needed right now, but the others are not. For consistency, we'd want --dry-run for all other actions, including copy, move, delete, etc.
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. > - 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. > @@ -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).
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.
diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt index 5449767121..8cdc33c097 100644 --- a/Documentation/git-branch.txt +++ b/Documentation/git-branch.txt @@ -16,7 +16,7 @@ SYNOPSIS [--points-at <object>] [--format=<format>] [(-r | --remotes) | (-a | --all)] [--list] [<pattern>...] -'git branch' [--track | --no-track] [-f] <branchname> [<start-point>] +'git branch' [--track | --no-track] [-f] [--dry-run | -n] <branchname> [<start-point>] 'git branch' (--set-upstream-to=<upstream> | -u <upstream>) [<branchname>] 'git branch' --unset-upstream [<branchname>] 'git branch' (-m | -M) [<oldbranch>] <newbranch> @@ -205,6 +205,12 @@ This option is only applicable in non-verbose mode. --no-abbrev:: Display the full sha1s in the output listing rather than abbreviating them. +-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. + -t:: --track:: When creating a new branch, set up `branch.<name>.remote` and diff --git a/branch.c b/branch.c index f8b755513f..528cb2d639 100644 --- a/branch.c +++ b/branch.c @@ -206,9 +206,9 @@ N_("\n" "will track its remote counterpart, you may want to use\n" "\"git push -u\" to set the upstream config as you push."); -static void validate_branch_start(struct repository *r, const char *start_name, - enum branch_track track, - struct object_id *oid, char **full_ref) +void validate_branch_start(struct repository *r, const char *start_name, + enum branch_track track, struct object_id *oid, + char **full_ref) { struct commit *commit; int explicit_tracking = 0; diff --git a/branch.h b/branch.h index 75cefcdcbd..d8e5ff4e28 100644 --- a/branch.h +++ b/branch.h @@ -3,6 +3,7 @@ struct repository; struct strbuf; +struct object_id; enum branch_track { BRANCH_TRACK_UNSPECIFIED = -1, @@ -17,6 +18,27 @@ extern enum branch_track git_branch_track; /* Functions for acting on the information about branches. */ +/* + * Validates whether a ref is a valid starting point for a branch, where: + * + * - r is the repository to validate the branch for + * + * - start_name is the ref that we would like to test + * + * - track is the tracking mode of the new branch. If tracking is + * explicitly requested, start_name must be a branch (because + * otherwise start_name cannot be tracked) + * + * - oid is an out parameter containing the object_id of start_name + * + * - full_ref is an out parameter containing the 'full' form of + * start_name e.g. refs/heads/main instead of main + * + */ +void validate_branch_start(struct repository *r, const char *start_name, + enum branch_track track, struct object_id *oid, + char **full_ref); + /* * This sets the branch.<new_ref>.{remote,merge} config settings so that * branch 'new_ref' tracks 'orig_ref'. This is called when branches are diff --git a/builtin/branch.c b/builtin/branch.c index eb5c117a6e..5d4b9c82b4 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -27,7 +27,8 @@ static const char * const builtin_branch_usage[] = { N_("git branch [<options>] [-r | -a] [--merged] [--no-merged]"), - N_("git branch [<options>] [-l] [-f] <branch-name> [<start-point>]"), + N_("git branch [<options>] [-l] [<pattern>...]"), + N_("git branch [<options>] [-f] [--dry-run | -n] <branch-name> [<start-point>]"), N_("git branch [<options>] [-r] (-d | -D) <branch-name>..."), N_("git branch [<options>] (-m | -M) [<old-branch>] <new-branch>"), N_("git branch [<options>] (-c | -C) [<old-branch>] <new-branch>"), @@ -616,14 +617,14 @@ static int edit_branch_description(const char *branch_name) int cmd_branch(int argc, const char **argv, const char *prefix) { - int delete = 0, rename = 0, copy = 0, force = 0, list = 0; - int show_current = 0; - int reflog = 0, edit_description = 0; - int quiet = 0, unset_upstream = 0; + /* possible actions */ + int delete = 0, rename = 0, copy = 0, force = 0, list = 0, create = 0, + unset_upstream = 0, show_current = 0, edit_description = 0; + /* possible options */ + int reflog = 0, quiet = 0, dry_run = 0, icase = 0; const char *new_upstream = NULL; enum branch_track track; struct ref_filter filter; - int icase = 0; static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting; struct ref_format format = REF_FORMAT_INIT; @@ -670,6 +671,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) N_("print only branches of the object"), parse_opt_object_name), OPT_BOOL('i', "ignore-case", &icase, N_("sorting and filtering are case insensitive")), OPT_STRING( 0 , "format", &format.format, N_("format"), N_("format to use for the output")), + OPT__DRY_RUN(&dry_run, N_("show whether the branch would be created")), OPT_END(), }; @@ -705,10 +707,15 @@ int cmd_branch(int argc, const char **argv, const char *prefix) filter.reachable_from || filter.unreachable_from || filter.points_at.nr) list = 1; - 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); + if (dry_run && !create) + die(_("--dry-run can only be used when creating branches")); + if (filter.abbrev == -1) filter.abbrev = DEFAULT_ABBREV; filter.ignore_case = icase; @@ -844,7 +851,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix) strbuf_addf(&buf, "branch.%s.merge", branch->name); git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE); strbuf_release(&buf); - } else if (argc > 0 && argc <= 2) { + } else if (create && argc > 0 && argc <= 2) { + const char *branch_name = argv[0]; + const char *start_name = (argc == 2) ? argv[1] : head; + if (filter.kind != FILTER_REFS_BRANCHES) die(_("The -a, and -r, options to 'git branch' do not take a branch name.\n" "Did you mean to use: -a|-r --list <pattern>?")); @@ -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); diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index 6bf95a1707..653891736a 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -59,6 +59,19 @@ test_expect_success 'git branch --force abc should succeed when abc exists' ' test_cmp expect actual ' +test_expect_success 'git branch --dry-run abc should fail when abc exists' ' + test_must_fail git branch --dry-run abc +' + +test_expect_success 'git branch --dry-run --force abc should succeed when abc exists' ' + git branch --dry-run --force abc +' + +test_expect_success 'git branch --dry-run def should not create a branch' ' + git branch --dry-run def && + test_must_fail git rev-parse def +' + test_expect_success 'git branch a/b/c should create a branch' ' git branch a/b/c && test_path_is_file .git/refs/heads/a/b/c '
When running "git branch --recurse-submodules topic", it would be useful to know whether or not 'topic' is a valid branch for all repositories. Currently there is no way to test this without actually creating the branch. Add a --dry-run option to branch creation that can check whether or not a branch name and start point would be valid for a repository without creating a branch. Refactor cmd_branch() to make the chosen action more obvious. Incidentally, fix an incorrect usage string that combined the 'list' usage of git branch (-l) with the 'create' usage; this string has been incorrect since its inception, a8dfd5eac4 (Make builtin-branch.c use parse_options., 2007-10-07). Signed-off-by: Glen Choo <chooglen@google.com> --- The --dry-run option is motivated mainly by --recurse-submodules. To my knowledge, there isn't a strong existing demand, but this might be mildly useful to some users. Documentation/git-branch.txt | 8 ++++++- branch.c | 6 ++--- branch.h | 22 ++++++++++++++++++ builtin/branch.c | 44 ++++++++++++++++++++++++++---------- t/t3200-branch.sh | 13 +++++++++++ 5 files changed, 77 insertions(+), 16 deletions(-)