| Message ID | 20220101143748.2582-2-chakrabortyabhradeep79@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | making --set-upstream have default arguments | expand |
Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com> writes: > Teach "git push -u" not to require repository and refspec. When > the user do not give what repository to push to, or which > branch(es) to push, behave as if the default remote repository > and a refspec (depending on the "push.default" configuration) > are given. That means if the user says push.default==nothing, we should error out "git push -u" as before, but that is not what the change to setup_default_push_refspecs() function does, is it? > -static void setup_default_push_refspecs(struct remote *remote) > +static void setup_default_push_refspecs(struct remote *remote, int flags) > { > struct branch *branch; > const char *dst; > int same_remote; > + int is_default_u = (flags & TRANSPORT_PUSH_SET_UPSTREAM); > > switch (push_default) { > case PUSH_DEFAULT_MATCHING: > @@ -214,6 +215,8 @@ static void setup_default_push_refspecs(struct remote *remote) > return; > > case PUSH_DEFAULT_NOTHING: > + if (is_default_u) > + break; > die(_("You didn't specify any refspecs to push, and " > "push.default is \"nothing\".")); > return; > @@ -234,11 +237,15 @@ static void setup_default_push_refspecs(struct remote *remote) > case PUSH_DEFAULT_SIMPLE: > if (!same_remote) > break; > + if (is_default_u) > + break; > if (strcmp(branch->refname, get_upstream_ref(branch, remote->name))) > die_push_simple(branch, remote); > break; > > case PUSH_DEFAULT_UPSTREAM: > + if (is_default_u) > + break; > if (!same_remote) > die(_("You are pushing to remote '%s', which is not the upstream of\n" > "your current branch '%s', without telling me what to push\n" So, I am not sure if many of the above changes are sensible. The first one certainly does not sound like sensible. > diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh > index fdb4292056..c2d11c3f2a 100755 > --- a/t/t5523-push-upstream.sh > +++ b/t/t5523-push-upstream.sh > @@ -60,6 +60,86 @@ test_expect_success 'push -u :topic_2' ' > check_config topic_2 upstream refs/heads/other2 > ' > > +default_u_setup() { Style. (cf. Documentation/CodingGuildelines). > + git checkout main && > + test_might_fail git branch --unset-upstream && > + test_config push.default $1 && > + test_config remote.pushDefault upstream > +} > + > +check_empty_config() { Likewise. > + test_expect_code 1 git config "branch.$1.remote" && > + test_expect_code 1 git config "branch.$1.merge" > +} > + > +for i in simple current upstream nothing > +do > + test_expect_success 'push -u with push.default=$i' ' > + default_u_setup $i && > + git push -u && > + check_config main upstream refs/heads/main && > + git push -u upstream main:other && > + git push -u && > + check_config main upstream refs/heads/main > + ' > + > + test_expect_success 'push -u -f with push.default=$i' ' > + default_u_setup $i && > + git push -u -f && > + check_config main upstream refs/heads/main > + ' > +done > + > +for i in simple current upstream nothing matching > +do > + test_expect_success 'push -u --prune with push.default=$i' ' > + default_u_setup $i && > + git push upstream main:test_u215 && > + git push -u --prune >out && > + check_config main upstream refs/heads/main && > + test_i18ngrep "[deleted]" out && > + test_i18ngrep ! "Branch '"'"'test_u215'"'"' set up to track" out > + ' > + > + test_expect_success 'push -u --mirror with push.default=$i' ' > + default_u_setup $i && > + test_might_fail git branch mirror1 && > + test_might_fail git branch mirror2 && > + git push -u --mirror && > + check_config main upstream refs/heads/main && > + check_config mirror1 upstream refs/heads/mirror1 && > + check_config mirror2 upstream refs/heads/mirror2 > + ' > +done > + > +for i in '' '-f' > +do > + > + test_expect_success 'push -u $i with push.default=matching' ' Doesn't $i show in the output as-is here? Quote the test title in double-quotes, while using single-qoutes around the test body. > + default_u_setup matching && > + test_might_fail git branch test_u && > + test_might_fail git branch test_u2 && > + git push upstream main:test_u2 && > + git push -u $i && > + check_config main upstream refs/heads/main && > + check_config test_u2 upstream refs/heads/test_u2 && > + check_empty_config test_u > + ' > +done > +
Junio C Hamano <gitster@pobox.com> wrote: > That means if the user says push.default==nothing, we should error > out "git push -u" as before, but that is not what the change to > setup_default_push_refspecs() function does, is it? Yeah. You're right. The current change does not throw error for push.default=nothing. Because I thought that for all values of `push.default` (except matching), 'git push -u' should create a new branch with the same name as the current local branch. Now, It seems that I was wrong. > So, I am not sure if many of the above changes are sensible. The > first one certainly does not sound like sensible. Actually, I didn't think deeply while commiting the changes. Today, I think about it deeply and I realized the following points. * if push.default='simple' or unspecified then it should not create a new branch on the remote. So, my proposed change of 'git push -u' for push.default='simple' is badly affecting the reason why push.default='simple' was built for. * if push.default='nothing', It should throw error if no <refspec> is provided. Again, my proposed change is hurting it. * For push.default=upstream, If an upstream is already defined then 'git push -u' should only set that branch as the upstream of the local branch. This already works in git. But if an upstream is not provied, it should throw error. So, I am not sure whether 'git push -u' (with no upstream information) should create a new branch with the same name or not. What do you think about that? * For push.default=matching, 'git push -u' should set all the existing matching branches as upstream of their respective matching local branches. It also already works. Same for 'push.default'=current also. So, to put all in a nutshell, I think that the current behaviour of 'git push -u' is okay. It also seems that he/she who built the setup_default_push_refspecs() was aware of this. Sorry for the patch request and thanks for reviewing. > Doesn't $i show in the output as-is here? Quote the test title in > double-quotes, while using single-qoutes around the test body. Yeah. I observed this while testing. But had no idea why this happend ( as I am very beginner in shell scripting). I was waiting for the review comment for it. Thanks again for reviewing my patch request.
Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com> writes: > * For push.default=upstream, If an upstream is already defined then > 'git push -u' should only set that branch as the upstream of the > local branch. This already works in git. But if an upstream is not > provied, it should throw error. So, I am not sure whether 'git push > -u' (with no upstream information) should create a new branch with > the same name or not. What do you think about that? I think erring on the side of caution is more sensible than blindly assuming that the user wants a new branch with the same name. Thank you for working on the topic and thinking its ramifications through.
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt index 2f25aa3a29..6fd474441f 100644 --- a/Documentation/git-push.txt +++ b/Documentation/git-push.txt @@ -375,6 +375,16 @@ Specifying `--no-force-if-includes` disables this behavior. upstream (tracking) reference, used by argument-less linkgit:git-pull[1] and other commands. For more information, see `branch.<name>.merge` in linkgit:git-config[1]. ++ +`-u` can also work with zero arguments( i.e. no `<repository>` and +`<refspec>` are given). In that case, it tries to get the `<repository>` +value from `branch.*.remote` configuration. If not found, it defaults to +`origin`. If `remote.pushDefault` is set then it uses that instead. The +value of `<refspec>` depends on the current `push.default` configuration. +If `push.default` is set to `matching`, all remote branches to which +local branches pushed, will be set as upstream of respective local +branches. For all other values of `push.default`, current branch's +refspec will be used as the `<refspec>`. --[no-]thin:: These options are passed to linkgit:git-send-pack[1]. A thin transfer diff --git a/builtin/push.c b/builtin/push.c index 4b026ce6c6..8bc206c9d8 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -202,11 +202,12 @@ static const char *get_upstream_ref(struct branch *branch, const char *remote_na return branch->merge[0]->src; } -static void setup_default_push_refspecs(struct remote *remote) +static void setup_default_push_refspecs(struct remote *remote, int flags) { struct branch *branch; const char *dst; int same_remote; + int is_default_u = (flags & TRANSPORT_PUSH_SET_UPSTREAM); switch (push_default) { case PUSH_DEFAULT_MATCHING: @@ -214,6 +215,8 @@ static void setup_default_push_refspecs(struct remote *remote) return; case PUSH_DEFAULT_NOTHING: + if (is_default_u) + break; die(_("You didn't specify any refspecs to push, and " "push.default is \"nothing\".")); return; @@ -234,11 +237,15 @@ static void setup_default_push_refspecs(struct remote *remote) case PUSH_DEFAULT_SIMPLE: if (!same_remote) break; + if (is_default_u) + break; if (strcmp(branch->refname, get_upstream_ref(branch, remote->name))) die_push_simple(branch, remote); break; case PUSH_DEFAULT_UPSTREAM: + if (is_default_u) + break; if (!same_remote) die(_("You are pushing to remote '%s', which is not the upstream of\n" "your current branch '%s', without telling me what to push\n" @@ -401,7 +408,7 @@ static int do_push(int flags, if (remote->push.nr) { push_refspec = &remote->push; } else if (!(flags & TRANSPORT_PUSH_MIRROR)) - setup_default_push_refspecs(remote); + setup_default_push_refspecs(remote, flags); } errs = 0; url_nr = push_url_of_remote(remote, &url); diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh index fdb4292056..c2d11c3f2a 100755 --- a/t/t5523-push-upstream.sh +++ b/t/t5523-push-upstream.sh @@ -60,6 +60,86 @@ test_expect_success 'push -u :topic_2' ' check_config topic_2 upstream refs/heads/other2 ' +default_u_setup() { + git checkout main && + test_might_fail git branch --unset-upstream && + test_config push.default $1 && + test_config remote.pushDefault upstream +} + +check_empty_config() { + test_expect_code 1 git config "branch.$1.remote" && + test_expect_code 1 git config "branch.$1.merge" +} + +for i in simple current upstream nothing +do + test_expect_success 'push -u with push.default=$i' ' + default_u_setup $i && + git push -u && + check_config main upstream refs/heads/main && + git push -u upstream main:other && + git push -u && + check_config main upstream refs/heads/main + ' + + test_expect_success 'push -u -f with push.default=$i' ' + default_u_setup $i && + git push -u -f && + check_config main upstream refs/heads/main + ' +done + +for i in simple current upstream nothing matching +do + test_expect_success 'push -u --prune with push.default=$i' ' + default_u_setup $i && + git push upstream main:test_u215 && + git push -u --prune >out && + check_config main upstream refs/heads/main && + test_i18ngrep "[deleted]" out && + test_i18ngrep ! "Branch '"'"'test_u215'"'"' set up to track" out + ' + + test_expect_success 'push -u --mirror with push.default=$i' ' + default_u_setup $i && + test_might_fail git branch mirror1 && + test_might_fail git branch mirror2 && + git push -u --mirror && + check_config main upstream refs/heads/main && + check_config mirror1 upstream refs/heads/mirror1 && + check_config mirror2 upstream refs/heads/mirror2 + ' +done + +for i in '' '-f' +do + + test_expect_success 'push -u $i with push.default=matching' ' + default_u_setup matching && + test_might_fail git branch test_u && + test_might_fail git branch test_u2 && + git push upstream main:test_u2 && + git push -u $i && + check_config main upstream refs/heads/main && + check_config test_u2 upstream refs/heads/test_u2 && + check_empty_config test_u + ' +done + +test_expect_success 'push -u -d will fail' ' + git checkout main && + test_might_fail git branch --unset-upstream && + test_must_fail git push -u -d +' + +test_expect_success 'push -u --dry-run' ' + git checkout main && + git push -u upstream main:other && + git push -u --dry-run && + check_config main upstream refs/heads/other +' + test_expect_success 'push -u --all' ' git branch all1 && git branch all2 && @@ -81,6 +161,13 @@ test_expect_success TTY 'progress messages go to tty' ' test_i18ngrep "Writing objects" err ' +test_expect_success TTY 'progress messages go to tty with default -u' ' + ensure_fresh_upstream && + + test_terminal git push -u 2>err && + test_i18ngrep "Writing objects" err +' + test_expect_success 'progress messages do not go to non-tty' ' ensure_fresh_upstream && @@ -89,6 +176,14 @@ test_expect_success 'progress messages do not go to non-tty' ' test_i18ngrep ! "Writing objects" err ' +test_expect_success 'progress messages do not go to non-tty (default -u)' ' + ensure_fresh_upstream && + + # skip progress messages, since stderr is non-tty + git push -u 2>err && + test_i18ngrep ! "Writing objects" err +' + test_expect_success 'progress messages go to non-tty (forced)' ' ensure_fresh_upstream && @@ -97,6 +192,14 @@ test_expect_success 'progress messages go to non-tty (forced)' ' test_i18ngrep "Writing objects" err ' +test_expect_success 'progress messages go to non-tty with default -u (forced)' ' + ensure_fresh_upstream && + + # force progress messages to stderr, even though it is non-tty + git push -u --progress 2>err && + test_i18ngrep "Writing objects" err +' + test_expect_success TTY 'push -q suppresses progress' ' ensure_fresh_upstream && @@ -104,6 +207,13 @@ test_expect_success TTY 'push -q suppresses progress' ' test_i18ngrep ! "Writing objects" err ' +test_expect_success TTY 'push -q suppresses progress (with default -u)' ' + ensure_fresh_upstream && + + test_terminal git push -u -q 2>err && + test_i18ngrep ! "Writing objects" err +' + test_expect_success TTY 'push --no-progress suppresses progress' ' ensure_fresh_upstream && @@ -112,6 +222,14 @@ test_expect_success TTY 'push --no-progress suppresses progress' ' test_i18ngrep ! "Writing objects" err ' +test_expect_success TTY 'push --no-progress suppresses progress (default -u)' ' + ensure_fresh_upstream && + + test_terminal git push -u --no-progress 2>err && + test_i18ngrep ! "Unpacking objects" err && + test_i18ngrep ! "Writing objects" err +' + test_expect_success TTY 'quiet push' ' ensure_fresh_upstream && @@ -126,4 +244,11 @@ test_expect_success TTY 'quiet push -u' ' test_must_be_empty output ' +test_expect_success TTY 'quiet push -u (default -u)' ' + ensure_fresh_upstream && + + test_terminal git push --quiet -u --no-progress 2>&1 | tee output && + test_must_be_empty output +' + test_done
"git push -u" (set-upstream) requires where to push to and what to push. Often people push only the current branch to update the branch of the same name at the 'origin' repository. For them, it would be convenient if "git push -u" without repository or refspec, defaulted to push and set upstream to the branch as configured by the "push.default" setting, of the remote repository that is used by default. Teach "git push -u" not to require repository and refspec. When the user do not give what repository to push to, or which branch(es) to push, behave as if the default remote repository and a refspec (depending on the "push.default" configuration) are given. If "push.default"=matching, push all the branches matched on both remote and local side and set those remote branches as the upstream of their respective local matched branches. Otherwise, set the refspec to the refspec for current branch. Signed-off-by: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com> --- Documentation/git-push.txt | 10 +++ builtin/push.c | 11 +++- t/t5523-push-upstream.sh | 125 +++++++++++++++++++++++++++++++++++++ 3 files changed, 144 insertions(+), 2 deletions(-)