| Message ID | 20211202144354.17416-2-chakrabortyabhradeep79@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | making --set-upstream have default arguments | expand |
Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com> writes: > For now, -u in 'push' command requires two arguments (<repository> Drop "For now"; we start our log message by explaining the current system without the proposed change in the present tense, so it is unneeded. Spelling out the option name "--set-upstream" in full, or at least "-u" in quotes, would make it more readable. > and <refspec>) to successfully track upstream branch. In most cases, > users want to set an upstream branch for the local branch they are > currently on and the short names of these two branches are same in > most of the cases. > > There are plenty of configurations to set default > branches for push but again users can't run argumentless pull, rebase > etc. So it will be good to have '-u' having default arguments. Don't judge what's "most" common without a survey. A casual "Often" is acceptable. Taking all together, something like "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 to the branch of the same name at the remote repository that is used by default. > This commit gives ability to '-u' to have default arguments. 'git push cf. Documentation/SubmittingPatches[[imperative-mood]] > -u' runs normally if <repository> and <refspec> are given. But > if those are not given then it tries to get the value of <repository> > from 'branch.<current_branch>.remote'. If not found, it sets 'origin' > as the value of <repository>. <refspec> would be the current branch's > short name. Do not invent an undefined word "short name". The name of the 'main' branch is 'main', and it is not a short name. When people encounter multi-level names, like ac/push-u-default, use of an undefined word "short name" will mislead readers that you meant the leaf level, 'push-u-default', but I do not think that is what you meant (this is not the only instance of "short name" in this submission; all need to be fixed). > However 'git push -u --all' work normally as before. Is this even necessary? --all is to push all branches to the default repository, so clearly it is outside the "we need default because the user did not tell us what to push to where" case. Taking the above together, perhaps something along this line, Teach "git push -u" not to require repository and refspec. When the user did not give what repository to push to, or which branch(es) to push, behave as if the default remote repository and the name of the current branch are given. Note that use of "--all" option, together with "-u", behaves as before, since the user is telling us to push all the branches to the default remote repository and there is no need for this new behaviour to kick in. perhaps? One thing that bothers me is that unlike your assumption, not everybody uses push.default set to simple or upstream. I am not convinced that the "git push -u" that defaults to do the 'current' push with TRANSPORT_PUSH_SET_UPSTREAM for them is an improvement for them. If the new feature does not kick in for them, that should be explained in the proposed log message when you sell the patch to reviewers and documented for the users. > @@ -375,6 +375,12 @@ 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]. > ++ > +If you use -u without any arguments (i.e. no <repository> and <refspec>), Be careful to quote `-u` and things like that by studyng the text around what you are changing. > +it will first try to get the <repository> from current branch's remote > +configuration (i.e. from `branch.<name>.remote`). If not found, it will set > +`origin` as the value of <repository> and <refspec> will be the current > +branch's refspec. This makes it sound as if the push will only affect the current branch even for folks who use the matching push. As I said, I do not know if that is desirable. > diff --git a/builtin/push.c b/builtin/push.c > index 4b026ce6c6..2e417a06ad 100644 > --- a/builtin/push.c > +++ b/builtin/push.c > @@ -527,6 +527,25 @@ static int git_push_config(const char *k, const char *v, void *cb) > return git_default_config(k, v, NULL); > } > > +static struct remote *pushremote_get_remote(const char *repo) > +{ > + struct remote *remote = pushremote_get(repo); > + if (!remote) { > + if (repo) > + die(_("bad repository '%s'"), repo); > + die(_("No configured push destination.\n" > + "Either specify the URL from the command-line or configure a remote repository using\n" > + "\n" > + " git remote add <name> <url>\n" > + "\n" > + "and then push using the remote name\n" > + "\n" > + " git push <name>\n")); > + } > + > + return remote; > +} > + > int cmd_push(int argc, const char **argv, const char *prefix) > { > int flags = 0; > @@ -537,6 +556,7 @@ int cmd_push(int argc, const char **argv, const char *prefix) > struct string_list push_options_cmdline = STRING_LIST_INIT_DUP; > struct string_list *push_options; > const struct string_list_item *item; > + struct remote *default_remote = NULL; > struct remote *remote; > > struct option options[] = { > @@ -603,23 +623,27 @@ int cmd_push(int argc, const char **argv, const char *prefix) > if (tags) > refspec_append(&rs, "refs/tags/*"); > > + if ((argc == 0) && (flags & TRANSPORT_PUSH_SET_UPSTREAM) && !(flags & TRANSPORT_PUSH_ALL)) { > + struct branch *branch = branch_get(NULL); > + if (branch) { > + argc += 2; > + default_remote = pushremote_get_remote(repo); > + argv[0] = default_remote->name; > + argv[1] = branch->name; This does look like it breaks unless the user is a novice without custom configuration. For example, if the current branch has a configuration to integrate with a branch at the default remote of a different name already, this (1) clobbers the tip of a wrong branch by pushing to it, and (2) overrites the upstream configuration. If the user uses push.default set to 'current' or 'simple', this would be OK, but for all other users, I doubt this would be an improvement. > diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh > index fdb4292056..69970b6263 100755 > --- a/t/t5523-push-upstream.sh > +++ b/t/t5523-push-upstream.sh > @@ -60,6 +60,17 @@ test_expect_success 'push -u :topic_2' ' > check_config topic_2 upstream refs/heads/other2 > ' > > +test_expect_success 'push -u' ' We may want to future-proof by checking the current tracking info (or lack of it) before doing "git push -u" here? You cannot control what other developers would do in the future to tests before this one. > + git push -u && > + check_config main upstream refs/heads/main > +' And we make sure "-u" without the repository or branch works in the basic case, which is a good "positive" test. > +test_expect_success 'push -u --dry-run' ' > + git push -u upstream main:other && > + git push -u --dry-run && > + check_config main upstream refs/heads/other > +' This verifies that under '--dry-run' the upstream configuration does not get changed. It is a good "negative" test to have, but there probably are a lot more "negative" tests to ensure that the new feature does not kick in in cases where it should not. Various settings of push.default is probably a good place to start and with or without existing upstream info already set up. Thanks for working on this topic. I suspect that the implementation and design covers too broadly to hurt some users while helping others, and needs tightening up to fix that, but I think the users appreciate the part that helps some users ;-)
Junio C Hamano <gitster@pobox.com> wrote: > Taking all together, something like > > "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 to the branch of the same name at > the remote repository that is used by default. Thanks for the guidance. Improving the cover-letter and commit message now. :) > Do not invent an undefined word "short name". The name of the > 'main' branch is 'main', and it is not a short name. When people > encounter multi-level names, like ac/push-u-default, use of an > undefined word "short name" will mislead readers that you meant > the leaf level, 'push-u-default', but I do not think that is what > you meant (this is not the only instance of "short name" in this > submission; all need to be fixed). Sorry for that, I was referring 'branch->name' as 'short name' (and 'branch->refname' as the 'long name' :| ). Will fix it. > One thing that bothers me is that unlike your assumption, not > everybody uses push.default set to simple or upstream. I am not > convinced that the "git push -u" that defaults to do the 'current' > push with TRANSPORT_PUSH_SET_UPSTREAM for them is an improvement > for them. May be you're right. It may not be an improvement for all. But I think they also would be happy seeing this 'default' case of 'set-upstream'. > If the new feature does not kick in for them, that should > be explained in the proposed log message when you sell the patch to > reviewers and documented for the users. Ok. > This makes it sound as if the push will only affect the current > branch even for folks who use the matching push. As I said, I do > not know if that is desirable. Yeah, this only affects the current branch. In that case they may not use 'git push -u'. As you said previously, this would not be an improvement in this case ( not a deterioration also). > This does look like it breaks unless the user is a novice without > custom configuration. For example, if the current branch has a > configuration to integrate with a branch at the default remote of a > different name already, this (1) clobbers the tip of a wrong branch > by pushing to it, and (2) overrites the upstream configuration. If > the user uses push.default set to 'current' or 'simple', this would > be OK, but for all other users, I doubt this would be an improvement. I don't think so. When an user use '--set-upstream' in push command, it means he/she want to set the upstream to a different branch (if the specified <repository> and <refspec> are not same as the current one) rather than the current upstream branch (if exists). So, I think it would be safe for '--set-upstream' to have default values. Isn't it? If you are fearing for those two points you specified, should we add a safety permission before proceeding? e.g. like this - this would change the upstream branch "%s" to "%s" for this local branch would you like to continue? [y]es [n]no > We may want to future-proof by checking the current tracking info > (or lack of it) before doing "git push -u" here? You cannot control > what other developers would do in the future to tests before this > one. Oh yeah, you're right. Will surely add it. > Various settings of push.default is probably a good place to start and > with or without existing upstream info already set up. Thanks for the suggestion, I will add more tests to address these things. > Thanks for working on this topic. I suspect that the implementation > and design covers too broadly to hurt some users while helping > others, and needs tightening up to fix that, but I think the users > appreciate the part that helps some users ;-) Thanks.
Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com> writes: >> One thing that bothers me is that unlike your assumption, not >> everybody uses push.default set to simple or upstream. I am not >> convinced that the "git push -u" that defaults to do the 'current' >> push with TRANSPORT_PUSH_SET_UPSTREAM for them is an improvement >> for them. > > May be you're right. It may not be an improvement for all. But I > think they also would be happy seeing this 'default' case of > 'set-upstream'. Not at all. The argument for the "good" case is for "simple" or "current" users, they want their "git push" without repo and branch arguments to push their current branch to the branch with the same name at the default remote repository, and if we arrange "git push -u" to do the sameand set up "branch.$current.merge", they will find it convenient. The same reasoning applies for other users who do *not* want their "git push" without repo and branch arguments to push as if they are doing the push.default=current push. If we make "git push -u" push the current (and only the current) branch to the branch of the same name at the default remote repository by overwriting argv[] like the patch under discussion does, we would be giving these users a convenient way to do what they do not want to do. Besides, I think with the current code $ git -c push.default=matching push -u already does the right thing by pushing the matching branches and sets up upstream for the branches that get pushed without the patch in question. With the patch, because it blindly mucks with argv[] to force pushing only the current branch to the default remote, the established expectation by existing users is broken. That of course is not an improvement but actively hurts them. We shouldn't be making it easier for our users to hurt themselves. So, no. The patch in its current form is totally unacceptable. Shouldn't the rule be something like "if 'git push $args' (where $args may be nothing, or any options other than '-u') pushes a branch (or a set of branches) to a repository, 'git push -u $args' (with the same $args) should set the branch.*.{remote,merge} for the branch(es) to the same repository" for the introduction of default to be truly an improvement? Or is it too strict and makes the rule not to trigger even for the intended audience?
Junio C Hamano <gitster@pobox.com> wrote: > That of course is not an improvement but actively hurts them. We > shouldn't be making it easier for our users to hurt themselves. Hmm. In the scenario you mentioned, the proposed change is clearly breaking. Thanks for notifying. > Shouldn't the rule be something like "if 'git push $args' (where > $args may be nothing, or any options other than '-u') pushes a > branch (or a set of branches) to a repository, 'git push -u $args' > (with the same $args) should set the branch.*.{remote,merge} for the > branch(es) to the same repository" for the introduction of default > to be truly an improvement? Or is it too strict and makes the rule > not to trigger even for the intended audience? Sounds good to me. But what if 'push.default' set to 'nothing'? Do the proposed default arguments (I am saying about only the default arguments; not the changes in code) are right fit for that case?
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt index 2f25aa3a29..e1a8b41818 100644 --- a/Documentation/git-push.txt +++ b/Documentation/git-push.txt @@ -375,6 +375,12 @@ 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]. ++ +If you use -u without any arguments (i.e. no <repository> and <refspec>), +it will first try to get the <repository> from current branch's remote +configuration (i.e. from `branch.<name>.remote`). If not found, it will set +`origin` as the value of <repository> and <refspec> will be the current +branch's 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..2e417a06ad 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -527,6 +527,25 @@ static int git_push_config(const char *k, const char *v, void *cb) return git_default_config(k, v, NULL); } +static struct remote *pushremote_get_remote(const char *repo) +{ + struct remote *remote = pushremote_get(repo); + if (!remote) { + if (repo) + die(_("bad repository '%s'"), repo); + die(_("No configured push destination.\n" + "Either specify the URL from the command-line or configure a remote repository using\n" + "\n" + " git remote add <name> <url>\n" + "\n" + "and then push using the remote name\n" + "\n" + " git push <name>\n")); + } + + return remote; +} + int cmd_push(int argc, const char **argv, const char *prefix) { int flags = 0; @@ -537,6 +556,7 @@ int cmd_push(int argc, const char **argv, const char *prefix) struct string_list push_options_cmdline = STRING_LIST_INIT_DUP; struct string_list *push_options; const struct string_list_item *item; + struct remote *default_remote = NULL; struct remote *remote; struct option options[] = { @@ -603,23 +623,27 @@ int cmd_push(int argc, const char **argv, const char *prefix) if (tags) refspec_append(&rs, "refs/tags/*"); + if ((argc == 0) && (flags & TRANSPORT_PUSH_SET_UPSTREAM) && !(flags & TRANSPORT_PUSH_ALL)) { + struct branch *branch = branch_get(NULL); + if (branch) { + argc += 2; + default_remote = pushremote_get_remote(repo); + argv[0] = default_remote->name; + argv[1] = branch->name; + } + } + if (argc > 0) { repo = argv[0]; set_refspecs(argv + 1, argc - 1, repo); } - remote = pushremote_get(repo); - if (!remote) { - if (repo) - die(_("bad repository '%s'"), repo); - die(_("No configured push destination.\n" - "Either specify the URL from the command-line or configure a remote repository using\n" - "\n" - " git remote add <name> <url>\n" - "\n" - "and then push using the remote name\n" - "\n" - " git push <name>\n")); + if (default_remote) { + remote = default_remote; + default_remote = NULL; + } + else { + remote = pushremote_get_remote(repo); } if (remote->mirror) diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh index fdb4292056..69970b6263 100755 --- a/t/t5523-push-upstream.sh +++ b/t/t5523-push-upstream.sh @@ -60,6 +60,17 @@ test_expect_success 'push -u :topic_2' ' check_config topic_2 upstream refs/heads/other2 ' +test_expect_success 'push -u' ' + git push -u && + check_config main upstream refs/heads/main +' + +test_expect_success 'push -u --dry-run' ' + 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 &&
For now, -u in 'push' command requires two arguments (<repository> and <refspec>) to successfully track upstream branch. In most cases, users want to set an upstream branch for the local branch they are currently on and the short names of these two branches are same in most of the cases. There are plenty of configurations to set default branches for push but again users can't run argumentless pull, rebase etc. So it will be good to have '-u' having default arguments. This commit gives ability to '-u' to have default arguments. 'git push -u' runs normally if <repository> and <refspec> are given. But if those are not given then it tries to get the value of <repository> from 'branch.<current_branch>.remote'. If not found, it sets 'origin' as the value of <repository>. <refspec> would be the current branch's short name. However 'git push -u --all' work normally as before. Signed-off-by: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com> --- Documentation/git-push.txt | 6 +++++ builtin/push.c | 48 ++++++++++++++++++++++++++++---------- t/t5523-push-upstream.sh | 11 +++++++++ 3 files changed, 53 insertions(+), 12 deletions(-)