{"thread":{"id":"33477","subject":"[PATCH] pull: fail early if we know we can't merge from upstream","startedAt":"2013-04-11T13:26:41Z","lastAt":"2013-04-12T16:35:10Z","messageCount":4,"participants":["Carlos Martín Nieto","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"213957","messageId":"1365686801-17206-1-git-send-email-cmn@elego.de","threadId":"33477","inReplyTo":null,"subject":"[PATCH] pull: fail early if we know we can't merge from upstream","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2013-04-11T13:26:41Z","receivedAt":"2013-04-11T13:26:41Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"A 'git pull' without specifying a remote is asked to take the current\nbranch's upstream as the branch to merge from. This cannot work\nwithout an upstream configuration nor with HEAD detached, but we only\ncheck for this after fetching.\n\nPerform the check beforehand, as we already know whether we have\nenough information to merge and can fail immediately otherwise.\n\nSigned-off-by: Carlos Martín Nieto <cmn@elego.de>\n---\n git-pull.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++-----------------\n 1 file changed, 45 insertions(+), 17 deletions(-)\n\nI can't quite decide whether the behaviour of 'git pull' with no\nupstream configured but a default remote with no fetch refspecs\nmerging the remote's HEAD is a feature, a bug or something in between,\nbut it's used by t7409 so maybe someone else is using it and we\nshouldn't break it.\n\nThere's another check that could be made earlier ('git pull\nsomeremote' when that's not the branch's upstream remote), but then\nyou have to start figuring out what the flags to fetch are. I'll\nrevisit this at some point, but I wanted to get this out since it's\nworking.\n\ndiff --git a/git-pull.sh b/git-pull.sh\nindex 266e682..b62f5d3 100755\n--- a/git-pull.sh\n+++ b/git-pull.sh\n@@ -43,6 +43,8 @@ log_arg= verbosity= progress= recurse_submodules=\n merge_args= edit=\n curr_branch=$(git symbolic-ref -q HEAD)\n curr_branch_short=\"${curr_branch#refs/heads/}\"\n+upstream=$(git config \"branch.$curr_branch_short.merge\")\n+remote=$(git config \"branch.$curr_branch_short.remote\")\n rebase=$(git config --bool branch.$curr_branch_short.rebase)\n if test -z \"$rebase\"\n then\n@@ -138,6 +140,47 @@ do\n \tesac\n \tshift\n done\n+if test true = \"$rebase\"\n+then\n+    op_type=rebase\n+    op_prep=against\n+else\n+    op_type=merge\n+    op_prep=with\n+fi\n+\n+check_args_against_config () {\n+\t# If fetch gets user-provided arguments, the user is\n+\t# overriding the upstream configuration, so we have to wait\n+\t# for fetch to do its work to know if we can merge.\n+\tif [ $# -gt 0 ]; then\n+\t\treturn\n+\tfi\n+\n+\t# Figure out what remote we're going to be fetching from\n+\tuse_remote=origin\n+\tif [ -n \"$remote\" ]; then\n+\t\tuse_remote=\"$remote\"\n+\tfi\n+\n+\t# If the remote doesn't have a fetch refspec, then we'll merge\n+\t# whatever fetch marks for-merge, same as above.\n+\tfetch=$(git config --get-all \"remote.$use_remote.fetch\")\n+\tif [ -z \"$fetch\" ]; then\n+\t\treturn\n+\tfi\n+\n+\t# The typical 'git pull' case where it should merge from the\n+\t# current branch's upstream. We can already check whether we\n+\t# we can do it. If HEAD is detached or there is no upstream\n+\t# branch, complain now.\n+\tif [ -z \"$curr_branch_short\" -o -z \"$upstream\" ]; then\n+\t\t. git-parse-remote\n+\t\terror_on_missing_default_upstream \"pull\" $op_type $op_prep \\\n+\t\t    \"git pull <remote> <branch>\"\n+\t\texit 1\n+\tfi\n+}\n \n error_on_no_merge_candidates () {\n \texec >&2\n@@ -151,19 +194,6 @@ error_on_no_merge_candidates () {\n \t\tesac\n \tdone\n \n-\tif test true = \"$rebase\"\n-\tthen\n-\t\top_type=rebase\n-\t\top_prep=against\n-\telse\n-\t\top_type=merge\n-\t\top_prep=with\n-\tfi\n-\n-\tcurr_branch=${curr_branch#refs/heads/}\n-\tupstream=$(git config \"branch.$curr_branch.merge\")\n-\tremote=$(git config \"branch.$curr_branch.remote\")\n-\n \tif [ $# -gt 1 ]; then\n \t\tif [ \"$rebase\" = true ]; then\n \t\t\tprintf \"There is no candidate for rebasing against \"\n@@ -177,10 +207,6 @@ error_on_no_merge_candidates () {\n \t\techo \"You asked to pull from the remote '$1', but did not specify\"\n \t\techo \"a branch. Because this is not the default configured remote\"\n \t\techo \"for your current branch, you must specify a branch on the command line.\"\n-\telif [ -z \"$curr_branch\" -o -z \"$upstream\" ]; then\n-\t\t. git-parse-remote\n-\t\terror_on_missing_default_upstream \"pull\" $op_type $op_prep \\\n-\t\t\t\"git pull <remote> <branch>\"\n \telse\n \t\techo \"Your configuration specifies to $op_type $op_prep the ref '${upstream#refs/heads/}'\"\n \t\techo \"from the remote, but no such ref was fetched.\"\n@@ -213,6 +239,8 @@ test true = \"$rebase\" && {\n \t\tfi\n \tdone\n }\n+\n+check_args_against_config \"$@\"\n orig_head=$(git rev-parse -q --verify HEAD)\n git fetch $verbosity $progress $dry_run $recurse_submodules --update-head-ok \"$@\" || exit 1\n test -z \"$dry_run\" || exit 0\n-- \n1.8.2.524.g8f8def7\n"},{"id":"213977","messageId":"7v1uahj7do.fsf@alter.siamese.dyndns.org","threadId":"33477","inReplyTo":"1365686801-17206-1-git-send-email-cmn@elego.de","subject":"Re: [PATCH] pull: fail early if we know we can't merge from upstream","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-11T17:37:39Z","receivedAt":"2013-04-11T17:37:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto <cmn@elego.de> writes:\n\n> I can't quite decide whether the behaviour of 'git pull' with no\n> upstream configured but a default remote with no fetch refspecs\n> merging the remote's HEAD is a feature, a bug or something in between,\n> but it's used by t7409 so maybe someone else is using it and we\n> shouldn't break it.\n\nIsn't it the simplest \"works without any configuration\" from the\noriginal days? \n\n> There's another check that could be made earlier ('git pull\n> someremote' when that's not the branch's upstream remote), but then\n> you have to start figuring out what the flags to fetch are.\n\nWhen the user gave us explicitly the name of the remote, it does not\nsound too bad to fetch from there.  \"git pull someremote thatbranch\"\ncan be given after seeing a failure and succeed without retransfer,\nno?\n\nI am not sure if it is worth the added complexity and potential to\nintroduce new bugs in general by trying to outsmart the for-merge\nlogic that kicks in only after we learn what the other side offers\nand fetch from it, but anyway, let's see what we got here...\n\n> diff --git a/git-pull.sh b/git-pull.sh\n> index 266e682..b62f5d3 100755\n> --- a/git-pull.sh\n> +++ b/git-pull.sh\n> @@ -43,6 +43,8 @@ log_arg= verbosity= progress= recurse_submodules=\n>  merge_args= edit=\n>  curr_branch=$(git symbolic-ref -q HEAD)\n>  curr_branch_short=\"${curr_branch#refs/heads/}\"\n> +upstream=$(git config \"branch.$curr_branch_short.merge\")\n> +remote=$(git config \"branch.$curr_branch_short.remote\")\n>  rebase=$(git config --bool branch.$curr_branch_short.rebase)\n\nLearning these upfront sounds sensible.\n\n>  if test -z \"$rebase\"\n>  then\n> @@ -138,6 +140,47 @@ do\n>  \tesac\n>  \tshift\n>  done\n> +if test true = \"$rebase\"\n> +then\n> +    op_type=rebase\n> +    op_prep=against\n> +else\n> +    op_type=merge\n> +    op_prep=with\n> +fi\n> +\n> +check_args_against_config () {\n> +\t# If fetch gets user-provided arguments, the user is\n> +\t# overriding the upstream configuration, so we have to wait\n> +\t# for fetch to do its work to know if we can merge.\n> +\tif [ $# -gt 0 ]; then\n> +\t\treturn\n> +\tfi\n\n> +\t# Figure out what remote we're going to be fetching from\n> +\tuse_remote=origin\n> +\tif [ -n \"$remote\" ]; then\n> +\t\tuse_remote=\"$remote\"\n> +\tfi\n> +\n> +\t# If the remote doesn't have a fetch refspec, then we'll merge\n> +\t# whatever fetch marks for-merge, same as above.\n\nThe \"above\" in this sentence refers to...?\n\nI guess \"we have to wait\", but it wasn't very clear.\n\n> +\tfetch=$(git config --get-all \"remote.$use_remote.fetch\")\n> +\tif [ -z \"$fetch\" ]; then\n> +\t\treturn\n> +\tfi\n\nHmm, it is probably correct to punt on this case, but it defeats\nlarge part of the effect of your effort, doesn't it? We fetch what\nis covered by remote.$name.fetch _and_ what need to complete the\nmerge operation (otherwise branch.$name.merge that is not covered by\nremote.$there.fetch will not work).  So\n\n    [remote \"origin\"]\n            url = $over_there\n    [branch \"master\"]\n            remote = origin\n            merge = refs/heads/master\n\nwould still fetch refs/heads/master from there and merge it.\n\n> +\t# The typical 'git pull' case where it should merge from the\n> +\t# current branch's upstream. We can already check whether we\n> +\t# we can do it. If HEAD is detached or there is no upstream\n> +\t# branch, complain now.\n\nDrop \"typical\", and rephrase \"merge from\" to also cover \"rebase\" (I\noften say \"integrate with\").\n\nTo return to your original description:\n\n    A 'git pull' without specifying a remote is asked to take the\n    current branch's upstream as the branch to merge from. This\n    cannot work without an upstream configuration nor with HEAD\n    detached, but we only check for this after fetching.\n\nWouldn't it be sufficient to add something like this before fetch\nhappens:\n\n\tif test $# != 0 || # args explicitly specified\n           test -n \"$curr_branch\" || # not detached\n\t   test -n \"$upstream\" # what to integrate with is known\n\tthen\n\t\treturn ;# then no problem\n\tfi\n\tdie \"underspecified 'git pull'\"\n\nwithout changing anything else?  For that matter, $upstream is\nlikely to be empty when detached, so the second test may not even be\nnecessary.\n"},{"id":"214051","messageId":"1365761838.2468.21.camel@flaca.cmartin.tk","threadId":"33477","inReplyTo":"7v1uahj7do.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] pull: fail early if we know we can't merge from upstream","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2013-04-12T10:17:18Z","receivedAt":"2013-04-12T10:17:18Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On Thu, 2013-04-11 at 10:37 -0700, Junio C Hamano wrote:\n> Carlos Martín Nieto <cmn@elego.de> writes:\n> \n> > I can't quite decide whether the behaviour of 'git pull' with no\n> > upstream configured but a default remote with no fetch refspecs\n> > merging the remote's HEAD is a feature, a bug or something in between,\n> > but it's used by t7409 so maybe someone else is using it and we\n> > shouldn't break it.\n> \n> Isn't it the simplest \"works without any configuration\" from the\n> original days? \n\nI don't recall remotes not having refspecs when they're int he config,\nthough I guess it's equivalent to running 'git pull\ngit://example.org/myrepo.git'.\n\n> \n> > There's another check that could be made earlier ('git pull\n> > someremote' when that's not the branch's upstream remote), but then\n> > you have to start figuring out what the flags to fetch are.\n> \n> When the user gave us explicitly the name of the remote, it does not\n> sound too bad to fetch from there.  \"git pull someremote thatbranch\"\n> can be given after seeing a failure and succeed without retransfer,\n> no?\n\nIt's not too bad, though you're paying for connection and ref\nadvertisement twice which breaks the otherwise quick pace of git\ncommands.\n\nWhat I find bad from a UI point of view is that after fetching (which\ncould even be from the wrong remote for 'git pull' w/o upstream info)\ngit turns around and says \"I was never going to merge/rebase that\" for\nthings that we can know before fetching because they depend solely on\nthe configuration.\n\n> \n> I am not sure if it is worth the added complexity and potential to\n> introduce new bugs in general by trying to outsmart the for-merge\n> logic that kicks in only after we learn what the other side offers\n> and fetch from it, but anyway, let's see what we got here...\n> \n> > diff --git a/git-pull.sh b/git-pull.sh\n> > index 266e682..b62f5d3 100755\n> > --- a/git-pull.sh\n> > +++ b/git-pull.sh\n> > @@ -43,6 +43,8 @@ log_arg= verbosity= progress= recurse_submodules=\n> >  merge_args= edit=\n> >  curr_branch=$(git symbolic-ref -q HEAD)\n> >  curr_branch_short=\"${curr_branch#refs/heads/}\"\n> > +upstream=$(git config \"branch.$curr_branch_short.merge\")\n> > +remote=$(git config \"branch.$curr_branch_short.remote\")\n> >  rebase=$(git config --bool branch.$curr_branch_short.rebase)\n> \n> Learning these upfront sounds sensible.\n> \n> >  if test -z \"$rebase\"\n> >  then\n> > @@ -138,6 +140,47 @@ do\n> >  \tesac\n> >  \tshift\n> >  done\n> > +if test true = \"$rebase\"\n> > +then\n> > +    op_type=rebase\n> > +    op_prep=against\n> > +else\n> > +    op_type=merge\n> > +    op_prep=with\n> > +fi\n> > +\n> > +check_args_against_config () {\n> > +\t# If fetch gets user-provided arguments, the user is\n> > +\t# overriding the upstream configuration, so we have to wait\n> > +\t# for fetch to do its work to know if we can merge.\n> > +\tif [ $# -gt 0 ]; then\n> > +\t\treturn\n> > +\tfi\n> \n> > +\t# Figure out what remote we're going to be fetching from\n> > +\tuse_remote=origin\n> > +\tif [ -n \"$remote\" ]; then\n> > +\t\tuse_remote=\"$remote\"\n> > +\tfi\n> > +\n> > +\t# If the remote doesn't have a fetch refspec, then we'll merge\n> > +\t# whatever fetch marks for-merge, same as above.\n> \n> The \"above\" in this sentence refers to...?\n> \n> I guess \"we have to wait\", but it wasn't very clear.\n> \n\nYes, it refers to having to wait for fetch to complete before we can\nknow if we'll be able to merge.\n\n> > +\tfetch=$(git config --get-all \"remote.$use_remote.fetch\")\n> > +\tif [ -z \"$fetch\" ]; then\n> > +\t\treturn\n> > +\tfi\n> \n> Hmm, it is probably correct to punt on this case, but it defeats\n> large part of the effect of your effort, doesn't it? We fetch what\n> is covered by remote.$name.fetch _and_ what need to complete the\n> merge operation (otherwise branch.$name.merge that is not covered by\n> remote.$there.fetch will not work).  So\n> \n>     [remote \"origin\"]\n>             url = $over_there\n>     [branch \"master\"]\n>             remote = origin\n>             merge = refs/heads/master\n> \n> would still fetch refs/heads/master from there and merge it.\n\nIf you run 'git pull' in this situation, then everything's fine and the\nright thing gets merged.\n\n> \n> > +\t# The typical 'git pull' case where it should merge from the\n> > +\t# current branch's upstream. We can already check whether we\n> > +\t# we can do it. If HEAD is detached or there is no upstream\n> > +\t# branch, complain now.\n> \n> Drop \"typical\", and rephrase \"merge from\" to also cover \"rebase\" (I\n> often say \"integrate with\").\n\nSounds good.\n\n> \n> To return to your original description:\n> \n>     A 'git pull' without specifying a remote is asked to take the\n>     current branch's upstream as the branch to merge from. This\n>     cannot work without an upstream configuration nor with HEAD\n>     detached, but we only check for this after fetching.\n> \n> Wouldn't it be sufficient to add something like this before fetch\n> happens:\n> \n> \tif test $# != 0 || # args explicitly specified\n>            test -n \"$curr_branch\" || # not detached\n> \t   test -n \"$upstream\" # what to integrate with is known\n> \tthen\n> \t\treturn ;# then no problem\n> \tfi\n> \tdie \"underspecified 'git pull'\"\n> \n> without changing anything else?  For that matter, $upstream is\n> likely to be empty when detached, so the second test may not even be\n> necessary.\n> \n\nI'm not sure if this allows us to print out the help message about\nmissing upstream configuration in the right case. I'll test.\n\n   cmn\n"},{"id":"214075","messageId":"7v38uvg11d.fsf@alter.siamese.dyndns.org","threadId":"33477","inReplyTo":"1365761838.2468.21.camel@flaca.cmartin.tk","subject":"Re: [PATCH] pull: fail early if we know we can't merge from upstream","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-12T16:35:10Z","receivedAt":"2013-04-12T16:35:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto <cmn@elego.de> writes:\n\n> On Thu, 2013-04-11 at 10:37 -0700, Junio C Hamano wrote:\n>\n>> > +\tfetch=$(git config --get-all \"remote.$use_remote.fetch\")\n>> > +\tif [ -z \"$fetch\" ]; then\n>> > +\t\treturn\n>> > +\tfi\n>> \n>> Hmm, it is probably correct to punt on this case, but it defeats\n>> large part of the effect of your effort, doesn't it? We fetch what\n>> is covered by remote.$name.fetch _and_ what need to complete the\n>> merge operation (otherwise branch.$name.merge that is not covered by\n>> remote.$there.fetch will not work).  So\n>> \n>>     [remote \"origin\"]\n>>             url = $over_there\n>>     [branch \"master\"]\n>>             remote = origin\n>>             merge = refs/heads/master\n>> \n>> would still fetch refs/heads/master from there and merge it.\n>\n> If you run 'git pull' in this situation, then everything's fine and the\n> right thing gets merged.\n\nMy mistake.  You are trying to reject an obviously bad case early,\nand because this is an obviously good case, you just let it be\nhandled in the original codeflow (which should not find any issues\nin this set-up).\n"}]}