{"thread":{"id":"42448","subject":"[RFC/PATCH] pull: set-upstream implementation","startedAt":"2016-05-25T15:25:28Z","lastAt":"2016-06-07T13:42:34Z","messageCount":12,"participants":["Erwan Mathoniere","Junio C Hamano","Matthieu Moy","Philip Oakley"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"287495","messageId":"20160525152528.22202-1-erwan.mathoniere@grenoble-inp.org","threadId":"42448","inReplyTo":null,"subject":"[RFC/PATCH] pull: set-upstream implementation","fromName":"Erwan Mathoniere","fromEmail":"erwan.mathoniere@grenoble-inp.org","sentAt":"2016-05-25T15:25:28Z","receivedAt":"2016-05-25T15:25:28Z","isPatch":true,"sender":{"key":"erwan.mathoniere@grenoble-inp.org","avatar":"https://avatars.githubusercontent.com/u/12645778?v=4"},"body":"Implements pull [--set-upstream | -u] which sets the remote tracking\nbranch to the one the user just pulled from.\n\n\tgit pull [--set-upstream | -u] \n\nAfter successfully fetched from , sets branch..remote to\n and branch..merge to follow /\n\nSigned-off-by: Erwan Mathoniere \nSigned-off-by: Jordan De Gea \nSigned-off-by: Matthieu Moy \n---\nHello,\n\n`git push --set-upstream ` sets the remote and merge config\nvariables for after successfully pushed.\n\nThis patch implements an equivalent for `git pull` or\n`git branch --set-upstream-to=/`.\n\nDifficulties:\n\t- I can't figure out what remote branch sould be tracked\n\tin that case: `git pull -u origin :master`\n\t- Is the result of 'resolve_ref_unsafe' should be freed ?\n\nWhat's your opinion ?\n\n\n Documentation/git-pull.txt | 7 +++++\n builtin/pull.c | 61 +++++++++++++++++++++++++++++++++++++\n t/t5544-pull-upstream.sh | 75 ++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 143 insertions(+)\n create mode 100755 t/t5544-pull-upstream.sh\n\ndiff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\nindex d033b25..3a2e0b7 100644\n--- a/Documentation/git-pull.txt\n+++ b/Documentation/git-pull.txt\n@@ -93,6 +93,13 @@ OPTIONS\n \thas to be called afterwards to bring the work tree up to date with the\n \tmerge result.\n \n+-u::\n+--set-upstream::\n+\tFor each branch that is successfully pulled, add\n+\tupstream (tracking) reference, used by argument-less\n+\tlinkgit:git-push[1] and other commands. For more information,\n+\tsee 'branch..merge' in linkgit:git-config[1].\n+\n Options related to merging\n ~~~~~~~~~~~~~~~~~~~~~~~~~~\n \ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 1d7333c..e096858 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -17,6 +17,7 @@\n #include \"revision.h\"\n #include \"tempfile.h\"\n #include \"lockfile.h\"\n+#include \"branch.h\"\n \n enum rebase_type {\n \tREBASE_INVALID = -1,\n@@ -109,6 +110,9 @@ static char *opt_unshallow;\n static char *opt_update_shallow;\n static char *opt_refmap;\n \n+/* Options about upstream */\n+static int opt_set_upstream;\n+\n static struct option pull_options[] = {\n \t/* Shared options */\n \tOPT__VERBOSITY(&opt_verbosity),\n@@ -116,6 +120,9 @@ static struct option pull_options[] = {\n \t\tN_(\"force progress reporting\"),\n \t\tPARSE_OPT_NOARG),\n \n+\t/* Options about upstream */\n+\tOPT_BOOL('u', \"set-upstream\", &opt_set_upstream, N_(\"set upstream for git pull/status\")),\n+\n \t/* Options passed to git-merge or git-rebase */\n \tOPT_GROUP(N_(\"Options related to merging\")),\n \t{ OPTION_CALLBACK, 'r', \"rebase\", &opt_rebase,\n@@ -829,6 +836,57 @@ static int run_rebase(const unsigned char *curr_head,\n \treturn ret;\n }\n \n+static int set_pull_upstream(const char *repo, const char **refspecs_name)\n+{\n+\tunsigned char sha1[GIT_SHA1_RAWSZ];\n+\tconst char *local_branch, *remote_branch;\n+\tstruct strbuf remote_name;\n+\tstruct refspec *refspecs;\n+\tint nr_refspec, i;\n+\n+\tif (repo == NULL) {\n+\t\twarning(N_(\"no remote was specified\"));\n+\t\treturn 1;\n+\t}\n+\n+\tfor (nr_refspec = 0; refspecs_name[nr_refspec] != NULL; nr_refspec++);\n+\n+\tif (nr_refspec == 0) {\n+\t\twarning(N_(\"no refspec was specified\"));\n+\t\treturn 1;\n+\t}\n+\n+\trefspecs = parse_fetch_refspec(nr_refspec, refspecs_name);\n+\n+\tfor (i = 0; i < nr_refspec; i++) {\n+\t\tif (refspecs[i].src != NULL && strlen(refspecs[i].src) > 0) {\n+\t\t\tremote_branch = refspecs[i].src;\n+\t\t} else {\n+\t\t\twarning(N_(\"not yet implemented\"));\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tif (refspecs[i].dst != NULL && strlen(refspecs[i].dst) > 0) {\n+\t\t\tlocal_branch = refspecs[i].dst;\n+\t\t} else {\n+\t\t\t// TODO : Should it be freed ?\n+\t\t\tlocal_branch = resolve_ref_unsafe(\"HEAD\", 0, sha1, NULL);\n+\t\t\tskip_prefix(local_branch, \"refs/heads/\", &local_branch);\n+\t\t}\n+\n+\t\tstrbuf_init(&remote_name, strlen(repo) + strlen(remote_branch) + 1);\n+\t\tstrbuf_addf(&remote_name, \"%s/%s\", repo, remote_branch);\n+\n+\t\tcreate_branch(local_branch, local_branch, remote_name.buf, 0, 0, 0, opt_verbosity < 0, BRANCH_TRACK_OVERRIDE);\n+\n+\t\tstrbuf_release(&remote_name);\n+\t}\n+\n+\tfree_refspec(nr_refspec, refspecs);\n+\n+\treturn 0;\n+}\n+\n int cmd_pull(int argc, const char **argv, const char *prefix)\n {\n \tconst char *repo, **refspecs;\n@@ -884,6 +942,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \tif (opt_dry_run)\n \t\treturn 0;\n \n+\tif (opt_set_upstream)\n+\t\tset_pull_upstream(repo, refspecs);\n+\n \tif (get_sha1(\"HEAD\", curr_head))\n \t\thashclr(curr_head);\n \ndiff --git a/t/t5544-pull-upstream.sh b/t/t5544-pull-upstream.sh\nnew file mode 100755\nindex 0000000..38eb51d\n--- /dev/null\n+++ b/t/t5544-pull-upstream.sh\n@@ -0,0 +1,75 @@\n+#!/bin/sh\n+\n+test_description='pull with --set-upstream'\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-terminal.sh\n+\n+ensure_fresh_upstream() {\n+\trm -rf parent &&\n+\tgit init parent &&\n+\tcd parent &&\n+\t(\n+\t\techo content >file &&\n+\t\tgit add file &&\n+\t\tgit commit -m one &&\n+\t\tgit checkout -b other &&\n+\t\techo content >file2 &&\n+\t\tgit add file2 &&\n+\t\tgit commit -m two &&\n+\t\tgit checkout -b master2 &&\n+\t\tgit checkout master\n+\t) &&\n+\tcd ..\n+}\n+\n+test_expect_success 'setup bare parent' '\n+\tensure_fresh_upstream &&\n+\tgit remote add upstream parent &&\n+\tgit pull upstream master\n+'\n+\n+check_config() {\n+\t(echo $2; echo $3) >expect.$1\n+\t(git config branch.$1.remote\n+\t git config branch.$1.merge) >actual.$1\n+\ttest_cmp expect.$1 actual.$1\n+}\n+\n+test_expect_success 'pull -u master' '\n+\tgit pull -u upstream master &&\n+\tcheck_config master upstream refs/heads/master\n+'\n+\n+test_expect_success 'pull -u master:other' '\n+\tgit pull -u upstream master:other &&\n+\tcheck_config other upstream refs/heads/master\n+'\n+\n+test_expect_success 'pull -u --dry-run other:other' '\n+\tgit pull -u --dry-run upstream other:other &&\n+\tcheck_config other upstream refs/heads/master\n+'\n+\n+test_expect_success 'pull -u master2:master2 master:other' '\n+\tgit branch master2 &&\n+\tgit pull -u upstream master2:master2 master:other &&\n+\tcheck_config master2 upstream refs/heads/master2 &&\n+\tcheck_config other upstream refs/heads/master\n+'\n+\n+test_expect_success 'pull -u HEAD' '\n+\tgit clone parent son &&\n+\tcd son &&\n+\tgit checkout -b headbranch &&\n+\tgit pull -u origin HEAD &&\n+\tcheck_config headbranch origin refs/heads/master\n+'\n+\n+test_expect_success TTY 'quiet pull' '\n+\tensure_fresh_upstream &&\n+\n+\ttest_terminal git pull -u --quiet upstream master 2>&1 | tee output &&\n+\ttest_cmp /dev/null output\n+'\n+\n+test_done\n-- \n2.8.2.562.gdbf6e3e\n"},{"id":"287503","messageId":"xmqqvb22ypot.fsf@gitster.mtv.corp.google.com","threadId":"42448","inReplyTo":"20160525152528.22202-1-erwan.mathoniere@grenoble-inp.org","subject":"Re: [RFC/PATCH] pull: set-upstream implementation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-25T18:09:54Z","receivedAt":"2016-05-25T18:09:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erwan Mathoniere writes:\n\n> Subject: Re: [RFC/PATCH] pull: set-upstream implementation\n\nIf this were multi-patch series and one of the other patches were\n\"pull: set-upstream design\" or something, then it might have been\nunderstandable, but otherwise the word \"implementation\" sits rather\nstrangely in the title of this patch.\n\n> Implements pull [--set-upstream | -u] which sets the remote tracking\n> branch to the one the user just pulled from.\n>\n> \tgit pull [--set-upstream | -u] \n>\n> After successfully fetched from , sets branch..remote to\n> and branch..merge to follow /\n>\n> Signed-off-by: Erwan Mathoniere \n> Signed-off-by: Jordan De Gea \n> Signed-off-by: Matthieu Moy \n> ---\n> Hello,\n>\n> `git push --set-upstream ` sets the remote and merge config\n> variables for after successfully pushed.\n>\n> This patch implements an equivalent for `git pull` or\n> `git branch --set-upstream-to=/`.\n>\n> Difficulties:\n> \t- I can't figure out what remote branch sould be tracked\n> \tin that case: `git pull -u origin :master`\n\nWhat does the command do without \"-u\"?\n\n> \t- Is the result of 'resolve_ref_unsafe' should be freed ?\n\nCheck its function signature--it returns \"const char *\" which is a\nsign that the memory does not belong to you (i.e. the caller), and\nyou should never free it.\n\n> Documentation/git-pull.txt | 7 +++++\n> builtin/pull.c | 61 +++++++++++++++++++++++++++++++++++++\n> t/t5544-pull-upstream.sh | 75 ++++++++++++++++++++++++++++++++++++++++++++++\n> 3 files changed, 143 insertions(+)\n> create mode 100755 t/t5544-pull-upstream.sh\n>\n> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\n> index d033b25..3a2e0b7 100644\n> --- a/Documentation/git-pull.txt\n> +++ b/Documentation/git-pull.txt\n> @@ -93,6 +93,13 @@ OPTIONS\n> \thas to be called afterwards to bring the work tree up to date with the\n> \tmerge result.\n> \n> +-u::\n> +--set-upstream::\n> +\tFor each branch that is successfully pulled, add\n> +\tupstream (tracking) reference, used by argument-less\n> +\tlinkgit:git-push[1] and other commands. For more information,\n> +\tsee 'branch..merge' in linkgit:git-config[1].\n\nTo me, \"For each branch\" hints that I could do this:\n\n\tgit pull --set-upstream git://git.kernel.org/r/e/p/o foo bar\n\nand the command would do something to 'foo' and 'bar'.\n\nBut I suspect that is not what is going on and that is not what you\nwanted to achieve.\n\nA crucial piece of information that is lacking in the above is where\n comes from. The same issue exists in your proposed commit\nlog message.\n\nI think that you meant to add a feature to add branch..remote\nand branch..merge configuration variables for the current\nbranch whose name is , and the values to be recorded for these\ntwo configuration variables are the same as those given on the\ncommand line. For example \"git checkout -b topic master && git pull\norigin that\" would set \"branch.topic.remote\" and\n\"branch.topic.merge\" to \"origin\" and \"that\", respectively.\n\nWithout explaining what is, \"For more information\" that\nrefers to 'branch..merge' does not help the readers much.\n\n\tSide note. It is an understandable mistake. After one\n\tspent a lot of effort designing, implementing and debugging\n\ta feature, by the time one describes what it does, some\n\tassumptions one made earlier becomes so fundamental in one's\n\tmind that one forgets that it is not obvious to others.\n\nThere a few design decisions you must have made that needs to be\neither described fully or at least hinted here, too, such as (not\nexhaustive and in random order):\n\n * What should happen when the current branch already has these two\n configuration variables defined? Should the user get a warning\n when this changes the setting from what was already there?\n\n * What should happen when the remote is specified as a Git URL, not\n as a remote nickname? You want a nickname for the value to place\n in \"branch..remote\".\n\n - Should Git find an existing remote that points at the URL and\n use it? If so, and if the value we are about to place in\n \"branch..merge\" is outside its configured fetch refspec,\n should Git tweak the fetch refspec of the existing remote?\n\n - Should Git create a new remote nickname for the URL? If so,\n what should the fetch refspec be for the newly created remote?\n Should it fetch only the branch we fetched just now?\n\n * What should happen when more than one ref is given?\n branch_get_upstream() can return only one ref; should Git choose\n one of them at random? Should Git warn and turn this into a\n no-op?\n\n * What should happen when the ref given to pull is not a branch\n (e.g. a tag)? A tag, meant to be a stable anchoring point, is\n not something suitable to be set as branch..merge, even\n though it might be technically possible to configure it as such.\n Should Git warn and turn this into a no-op?\n\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index 1d7333c..e096858 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -829,6 +836,57 @@ static int run_rebase(const unsigned char *curr_head,\n> \treturn ret;\n> }\n> \n> +static int set_pull_upstream(const char *repo, const char **refspecs_name)\n> +{\n> +\tunsigned char sha1[GIT_SHA1_RAWSZ];\n> +\tconst char *local_branch, *remote_branch;\n> +\tstruct strbuf remote_name;\n> +\tstruct refspec *refspecs;\n\nStyle. Name a pointer that points at the first element of an array\nas singular, so that \"element[4]\", not \"elements[4]\", becomes the\nway to refer to its fourth element.\n\n> +\tint nr_refspec, i;\n> +\n> +\tif (repo == NULL) {\n> +\t\twarning(N_(\"no remote was specified\"));\n> +\t\treturn 1;\n> +\t}\n> +\n> +\tfor (nr_refspec = 0; refspecs_name[nr_refspec] != NULL; nr_refspec++);\n\nStyle. Give an empty statement its own line to make it stand out,\ni.e.\n\n\tfor (....)\n \t; /* just counting */\n\n> +\tif (nr_refspec == 0) {\n> +\t\twarning(N_(\"no refspec was specified\"));\n> +\t\treturn 1;\n> +\t}\n\nOK, this is \"git pull -u \" without any explicit branch name;\nthe pull may have already used configured one (or it may have fetched\nnothing); there is nothing to do here either way.\n\n> +\trefspecs = parse_fetch_refspec(nr_refspec, refspecs_name);\n> +\n> +\tfor (i = 0; i < nr_refspec; i++) {\n> +\t\tif (refspecs[i].src != NULL && strlen(refspecs[i].src) > 0) {\n\nStyle. Lose the \" != NULL\" and useless strlen(). I.e.\n\n\tif (refspec[i].src && *refspec[i].src)\n\nMore importantly, what would you see here in .src when your command\nline were:\n\n\t$ git pull -u \"refs/heads/*:refs/remotes/foo/*\"\n\nHint: check .pattern to make sure it is false. Skip them at the top\nof the loop body, perhaps.\n\n> +\t\t\tremote_branch = refspecs[i].src;\n> +\t\t} else {\n> +\t\t\twarning(N_(\"not yet implemented\"));\n\nWhat do you plan to implement here?\n\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tif (refspecs[i].dst != NULL && strlen(refspecs[i].dst) > 0) {\n> +\t\t\tlocal_branch = refspecs[i].dst;\n> +\t\t} else {\n> +\t\t\t// TODO : Should it be freed ?\n\nStyle. No \"// comment\".\n\n> +\t\t\tlocal_branch = resolve_ref_unsafe(\"HEAD\", 0, sha1, NULL);\n> +\t\t\tskip_prefix(local_branch, \"refs/heads/\", &local_branch);\n\nWhat happens here if your user did this?\n\n\t$ git checkout HEAD^0 && git pull -u ...\n\nYou do not have \"local_branch\" here. Check the condition, warn and\nturn it into no-op (I do not think \"pull -u\" is important enough to\nfail the entire command).\n\n> +\t\t}\n> +\n> +\t\tstrbuf_init(&remote_name, strlen(repo) + strlen(remote_branch) + 1);\n> +\t\tstrbuf_addf(&remote_name, \"%s/%s\", repo, remote_branch);\n\nWhat happens here if your user did this?\n\n\t$ git pull -u git://git.kernel.org/r/e/p/o master\n\nSpecifically, what is \"repo\" at this point?\n\n> +\t\tcreate_branch(local_branch, local_branch, remote_name.buf, 0, 0, 0, opt_verbosity < 0, BRANCH_TRACK_OVERRIDE);\n\nI thought you were only setting up configurations for existing\nbranch. Why do you call create_branch() on the existing\nlocal_branch, which is supposed to be the current one?\n\nPerhaps create_branch() API is too crappy and it needs to be\nproperly refactored in preparation for this patch. I dunno.\n\nWhat does it mean for this loop to execute more than once, flipping\nthe configuration for the current branch number of times? The last\none wins? Does it even make sense from the end-user's point of\nview?\n\n> +\n> +\t\tstrbuf_release(&remote_name);\n> +\t}\n> +\n> +\tfree_refspec(nr_refspec, refspecs);\n> +\n> +\treturn 0;\n> +}\n> +\n> int cmd_pull(int argc, const char **argv, const char *prefix)\n> {\n> \tconst char *repo, **refspecs;\n> @@ -884,6 +942,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n> \tif (opt_dry_run)\n> \t\treturn 0;\n> \n> +\tif (opt_set_upstream)\n> +\t\tset_pull_upstream(repo, refspecs);\n> +\n> \tif (get_sha1(\"HEAD\", curr_head))\n> \t\thashclr(curr_head);\n> \n> diff --git a/t/t5544-pull-upstream.sh b/t/t5544-pull-upstream.sh\n> new file mode 100755\n> index 0000000..38eb51d\n> --- /dev/null\n> +++ b/t/t5544-pull-upstream.sh\n> @@ -0,0 +1,75 @@\n> +#!/bin/sh\n> +\n> +test_description='pull with --set-upstream'\n> +. ./test-lib.sh\n> +. \"$TEST_DIRECTORY\"/lib-terminal.sh\n> +\n> +ensure_fresh_upstream() {\n\nStyle:\n\n\tensure_fresh_upstream () {\n\n> +\trm -rf parent &&\n> +\tgit init parent &&\n> +\tcd parent &&\n> +\t(\n> +\t\techo content >file &&\n> +\t\tgit add file &&\n> +\t\tgit commit -m one &&\n> +\t\tgit checkout -b other &&\n> +\t\techo content >file2 &&\n> +\t\tgit add file2 &&\n> +\t\tgit commit -m two &&\n> +\t\tgit checkout -b master2 &&\n> +\t\tgit checkout master\n> +\t) &&\n> +\tcd ..\n\nThe tests typically do\n\n\t... some stuff ... &&\n (\n\t\tcd elsewhere &&\n ... more stuff ...\n\t) &&\n ... yet more stuff ...\n\nto make sure that even after any of \"... more stuff ...\" fails, the\ncaller of this sequence will find it in an expected and stable\nplace. As written in your patch, if any of the \"... more stuff ...\"\nfails, the caller will be in \"parent\" directory, not in the original\ndirectory, because your \"cd ..\" will not be exectued.\n\nI wonder if you are completely missing the point of using a subshell\nhere?\n\n> +}\n> +\n> +test_expect_success 'setup bare parent' '\n> +\tensure_fresh_upstream &&\n> +\tgit remote add upstream parent &&\n> +\tgit pull upstream master\n> +'\n> +\n> +check_config() {\n> +\t(echo $2; echo $3) >expect.$1\n\n\ttest_write_lines \"$2\" \"$3\" >\"expect.$1\"\n\nUnless you want your variable reference is split at $IFS, quote\nyour variables.\n\n> +\t(git config branch.$1.remote\n> +\t git config branch.$1.merge) >actual.$1\n> +\ttest_cmp expect.$1 actual.$1\n\nIt is not _wrong_ per-se, but I do not think \".$1\" suffix is adding\nany value. I'd drop it if I were doing this patch.\n\n> +}\n> +\n> +test_expect_success 'pull -u master' '\n> +\tgit pull -u upstream master &&\n> +\tcheck_config master upstream refs/heads/master\n> +'\n> +\n> +test_expect_success 'pull -u master:other' '\n> +\tgit pull -u upstream master:other &&\n> +\tcheck_config other upstream refs/heads/master\n> +'\n> +\n> +test_expect_success 'pull -u --dry-run other:other' '\n> +\tgit pull -u --dry-run upstream other:other &&\n> +\tcheck_config other upstream refs/heads/master\n> +'\n> +\n> +test_expect_success 'pull -u master2:master2 master:other' '\n> +\tgit branch master2 &&\n> +\tgit pull -u upstream master2:master2 master:other &&\n> +\tcheck_config master2 upstream refs/heads/master2 &&\n> +\tcheck_config other upstream refs/heads/master\n> +'\n> +\n> +test_expect_success 'pull -u HEAD' '\n> +\tgit clone parent son &&\n> +\tcd son &&\n> +\tgit checkout -b headbranch &&\n> +\tgit pull -u origin HEAD &&\n> +\tcheck_config headbranch origin refs/heads/master\n\nDo not \"chdir\" outside a subshell.\n\nI think the next test will not work if this test, specifically the\nfirst two steps that creates a new repository \"son\" and moves into\nit, fails. Don't create such an interdependency between tests when\nyou can avoid it.\n\nHint. Immediately after creating \"parent\", in the same test, clone\nit into \"son\". Everybody needs to depend on that \"setup bare\nparent\" test anyway, so that is not making things worse. And then\nupdate this test like so:\n\n\ttest_expect_success 'title' '\n \t(\n \tcd son &&\n git checkout -b headbranch &&\n ...\n\t\t)\n\t'\n\nmaking sure that outside test_expect_success block, the process will\nstay at the original directory, no matter which tests fail or get\nskipped.\n\n> +'\n> +\n> +test_expect_success TTY 'quiet pull' '\n> +\tensure_fresh_upstream &&\n> +\n> +\ttest_terminal git pull -u --quiet upstream master 2>&1 | tee output &&\n> +\ttest_cmp /dev/null output\n> +'\n\nThere is no test that makes sure that the new feature does not kick\nin when it should not. E.g. pulling from somewhere that is not a\nconfigured remote. For example, \n\n> +test_done\n"},{"id":"287765","messageId":"5639de7f-5a76-7038-ca13-867aaca3de64@grenoble-inp.org","threadId":"42448","inReplyTo":"xmqqvb22ypot.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH] pull: set-upstream implementation","fromName":"Erwan Mathoniere","fromEmail":"erwan.mathoniere@grenoble-inp.org","sentAt":"2016-05-29T20:00:50Z","receivedAt":"2016-05-29T20:00:50Z","isPatch":true,"sender":{"key":"erwan.mathoniere@grenoble-inp.org","avatar":"https://avatars.githubusercontent.com/u/12645778?v=4"},"body":"\n\nOn 25/05/2016 20:09, Junio C Hamano wrote:\n > Erwan Mathoniere writes:\n >> Difficulties:\n >> - I can't figure out what remote branch sould be tracked\n >> in that case: `git pull -u origin :master`\n >\n > What does the command do without \"-u\"?\n\nAfter some research, I think it creates a new branch if the ref doesn't \nexist. Otherwise it does nothing. So no branch should be tracked in that \ncase.\n\n >> +-u::\n >> +--set-upstream::\n >> + For each branch that is successfully pulled, add\n >> + upstream (tracking) reference, used by argument-less\n >> + linkgit:git-push[1] and other commands. For more information,\n >> + see 'branch..merge' in linkgit:git-config[1].\n >\n > A crucial piece of information that is lacking in the above is where\n > comes from. The same issue exists in your proposed commit\n > log message.\n\nYou're right, documentation is unclear. I'll sort it out.\n\n > There a few design decisions you must have made that needs to be\n > either described fully or at least hinted here, too, such as (not\n > exhaustive and in random order):\n >\n > * What should happen when the current branch already has these two\n > configuration variables defined? Should the user get a warning\n > when this changes the setting from what was already there?\n\nNeither `git pull --set-upstream` nor `git branch --set-upstream-to` \nwarns the user in such case. So `git pull --set-upstream` should simply \noverride the previous configuration the same way these commands do.\n\n\n > * What should happen when the remote is specified as a Git URL, not\n > as a remote nickname? You want a nickname for the value to place\n > in \"branch..remote\".\n >\n > - Should Git find an existing remote that points at the URL and\n > use it? If so, and if the value we are about to place in\n > \"branch..merge\" is outside its configured fetch refspec,\n > should Git tweak the fetch refspec of the existing remote?\n >\n > - Should Git create a new remote nickname for the URL? If so,\n > what should the fetch refspec be for the newly created remote?\n > Should it fetch only the branch we fetched just now?\n\nStill in comparison with `git push --set-upstream`, when using a Git URL \nas , \"push\" just sets \"branch..remote\" to the URL given in \nargument (even if there is a configured remote with this URL).\n`git pull --set-upstream` should work the same way.\n\n > * What should happen when more than one ref is given?\n > branch_get_upstream() can return only one ref; should Git choose\n > one of them at random? Should Git warn and turn this into a\n > no-op?\n\nIt depends whether refspecs given in arguments refers to the same branch \nor not. I discuss this point later in this email.\n\n > * What should happen when the ref given to pull is not a branch\n > (e.g. a tag)? A tag, meant to be a stable anchoring point, is\n > not something suitable to be set as branch..merge, even\n > though it might be technically possible to configure it as such.\n > Should Git warn and turn this into a no-op?\n\n`pull --set-upstream` should only set \"branch..merge\" to remote \nbranches. So yes a check must be done with eventually a no-op.\n\n >> + refspecs = parse_fetch_refspec(nr_refspec, refspecs_name);\n >> +\n >> + for (i = 0; i < nr_refspec; i++) {\n >> + if (refspecs[i].src != NULL && strlen(refspecs[i].src) > 0) {\n >\n > More importantly, what would you see here in .src when your command\n > line were:\n >\n > $ git pull -u \"refs/heads/*:refs/remotes/foo/*\"\n >\n > Hint: check .pattern to make sure it is false. Skip them at the top\n > of the loop body, perhaps.\n\nThis case wasn't handled at all, thanks for the hint.\n\n >> + remote_branch = refspecs[i].src;\n >> + } else {\n >> + warning(N_(\"not yet implemented\"));\n >\n > What do you plan to implement here?\n\nNow that I understand `git pull -u origin :master`, there's nothing to \nimplement here, except maybe a warning?\n\n >> + local_branch = resolve_ref_unsafe(\"HEAD\", 0, sha1, NULL);\n >> + skip_prefix(local_branch, \"refs/heads/\", &local_branch);\n >\n > What happens here if your user did this?\n >\n > $ git checkout HEAD^0 && git pull -u ...\n >\n > You do not have \"local_branch\" here. Check the condition, warn and\n > turn it into no-op (I do not think \"pull -u\" is important enough to\n > fail the entire command).\n\n\"pull -u\" should indeed only work for branches, I'll put a check on this.\nBut I'm not really sure what procedure I should use to fully resolve \nrefs. In git code, sometimes \"dwim_ref\" is used, sometimes it's \n\"resolve_ref_unsafe\" and haven't found any documentation on this.\n\n >> + }\n >> +\n >> + strbuf_init(&remote_name, strlen(repo) + \nstrlen(remote_branch) + 1);\n >> + strbuf_addf(&remote_name, \"%s/%s\", repo, remote_branch);\n >\n > What happens here if your user did this?\n >\n > $ git pull -u git://git.kernel.org/r/e/p/o master\n >\n > Specifically, what is \"repo\" at this point?\n >\n >> + create_branch(local_branch, local_branch, remote_name.buf, \n0, 0, 0, opt_verbosity < 0, BRANCH_TRACK_OVERRIDE);\n >\n > I thought you were only setting up configurations for existing\n > branch. Why do you call create_branch() on the existing\n > local_branch, which is supposed to be the current one?\n\nWe took the example of `git branch --set-upstream-to=/` \nfor this one. Perhaps using directly install_branch_config() is more \nrelevant here.\nIt allows to set a non-configured remote (I mean in the .git/config) as \nthe remote for the branch (with the full URL), fixing the example you \ngave me and working like `git push -u git://git.kernel.org/r/e/p/o master`.\n\n\n > What does it mean for this loop to execute more than once, flipping\n > the configuration for the current branch number of times? The last\n > one wins? Does it even make sense from the end-user's point of\n > view?\n\nThe loop is here to handle when the user uses multiple \n'remote_branch:local_branch' notation.\n(e.g. `git pull -u master:master other:other`)\n\nFor now, the last one wins. But I agree, it doesn't really make sense.\nThe simplest solution from the technical point of view would be to no-op \nwhen multiple refs are passed in arguments.\nHowever `git pull -u master:master other:other` seems legit.\n\nSo another solution would be to warn and no-op if the same branch config \nis modified twice.\n\n >> + rm -rf parent &&\n >> + git init parent &&\n >> + cd parent &&\n >> + (\n >> + echo content >file &&\n >> + git add file &&\n >> + git commit -m one &&\n >> + git checkout -b other &&\n >> + echo content >file2 &&\n >> + git add file2 &&\n >> + git commit -m two &&\n >> + git checkout -b master2 &&\n >> + git checkout master\n >> + ) &&\n >> + cd ..\n >\n > The tests typically do\n >\n > ... some stuff ... &&\n > (\n > cd elsewhere &&\n > ... more stuff ...\n > ) &&\n > ... yet more stuff ...\n >\n > to make sure that even after any of \"... more stuff ...\" fails, the\n > caller of this sequence will find it in an expected and stable\n > place. As written in your patch, if any of the \"... more stuff ...\"\n > fails, the caller will be in \"parent\" directory, not in the original\n > directory, because your \"cd ..\" will not be exectued.\n >\n > I wonder if you are completely missing the point of using a subshell\n > here?\n\nYou quoted the same issue later and indeed I didn't use it correctly.\n\n >> +check_config() {\n >> + (echo $2; echo $3) >expect.$1\n >\n > test_write_lines \"$2\" \"$3\" >\"expect.$1\"\n >\n > Unless you want your variable reference is split at $IFS, quote\n > your variables.\n >\n\n >> + (git config branch.$1.remote\n >> + git config branch.$1.merge) >actual.$1\n >> + test_cmp expect.$1 actual.$1\n >\n > It is not _wrong_ per-se, but I do not think \".$1\" suffix is adding\n > any value. I'd drop it if I were doing this patch.\n\nI took these functions from \"t/t5523-push-upstream.sh\" and I didn't \nclosely look at it. I'll clean it up.\n\n >> +'\n >> +\n >> +test_expect_success TTY 'quiet pull' '\n >> + ensure_fresh_upstream &&\n >> +\n >> + test_terminal git pull -u --quiet upstream master 2>&1 | tee \noutput &&\n >> + test_cmp /dev/null output\n >> +'\n >\n > There is no test that makes sure that the new feature does not kick\n > in when it should not. E.g. pulling from somewhere that is not a\n > configured remote. For example,\n >\n\nYes you're right. I'll reshape tests and create new ones to cover more \nuse cases.\n\nThanks for the time you spent for this huge and very useful feedback.\n"},{"id":"288409","messageId":"20160606093437.1992-1-erwan.mathoniere@grenoble-inp.org","threadId":"42448","inReplyTo":"20160525152528.22202-1-erwan.mathoniere@grenoble-inp.org","subject":"[RFC/PATCH v2] pull: add --set-upstream","fromName":"Erwan Mathoniere","fromEmail":"erwan.mathoniere@grenoble-inp.org","sentAt":"2016-06-06T09:34:37Z","receivedAt":"2016-06-06T09:34:37Z","isPatch":true,"sender":{"key":"erwan.mathoniere@grenoble-inp.org","avatar":"https://avatars.githubusercontent.com/u/12645778?v=4"},"body":"Implement `git pull [--set-upstream | -u] ` that set\ntracking to the remote branch the user just pulled from.\n\nAfter successfully pulling from ``, for each ``\ndescribed in format `:`, set\n`branch..remote` to `` and\n`branch..merge` to `refs/heads/`. If\n`` lacks `` in the previous format or directly\nrefers to a branch, use the current branch as `` in the\nabove configuration setting.\n\n`git push` has already its `--set-upstream`, it makes sense to have its\nsymmetrical for `git pull`.\n\nFor a beginner, when trying to use argumentless `git pull` without\ntracking information set, advising to use \n`git branch --set-upstream-to` to set upstream can be quite confusing.\nUsing this `git pull --set-upstream` is easier and more natural.\n\nSigned-off-by: Erwan Mathoniere \nSigned-off-by: Jordan De Gea \nSigned-off-by: Matthieu Moy \n---\n\nChanges from v1:\n- Code reshaped to :\n * warn + no-op when pulling from or to something that isn't a branch\nor a configured remote\n * set upstream only after successfully merging/rebasing\n- More relevant documentation\n- Tests reshaped to be more independent from each others\n- More tests (tags, detached heads, non-configured remote...)\n\n\nFor now, the documentation is quite hard to understand, but I didn't\nfigure how to explain without using too technical words. Should it stay\nas it is or should I write something similar the above commit message?\n\nAllowing to set non-configured repository as upstream isn't easy to\nhandle since the type of refspec must be checked and this is done by\nverifying the existence of the remote-tracking branch at\n`refs/remotes//`.\n\n\n Documentation/git-pull.txt | 18 +++++\n builtin/pull.c | 106 ++++++++++++++++++++++++++++-\n t/t5544-pull-upstream.sh | 164 +++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 285 insertions(+), 3 deletions(-)\n create mode 100755 t/t5544-pull-upstream.sh\n\ndiff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\nindex d033b25..6ae5e58 100644\n--- a/Documentation/git-pull.txt\n+++ b/Documentation/git-pull.txt\n@@ -93,6 +93,24 @@ OPTIONS\n \thas to be called afterwards to bring the work tree up to date with the\n \tmerge result.\n \n+-u::\n+--set-upstream::\n+\tAfter successfully pulling from explicitly given and\n+\t, set the configuration of the local branches pulled on, so\n+\tthat each one tracks the remote branch pulled from. If a configuration\n+\talready exists, it is overwriten. For example, with `git pull -u origin\n+\tbranch` the current branch will track `branch` from `origin`.\n++\n+If two or more branches are pulled on the same local branch, only the last one\n+in arguments will be tracked.\n++\n+The given must be a configured remote. Can only set tracking to\n+remote branches (e.g. can't set upstream to remote HEAD).\n++\n+Works symmetrically as `--set-upstream` for linkgit:git-push[1]. Allow using\n+argumentless linkgit:git-pull[1] and other commands. For more information, see\n+`branch..merge` in linkgit:git-config[1].\n+\n Options related to merging\n ~~~~~~~~~~~~~~~~~~~~~~~~~~\n \ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 1d7333c..d9823d5 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -17,6 +17,7 @@\n #include \"revision.h\"\n #include \"tempfile.h\"\n #include \"lockfile.h\"\n+#include \"branch.h\"\n \n enum rebase_type {\n \tREBASE_INVALID = -1,\n@@ -109,6 +110,9 @@ static char *opt_unshallow;\n static char *opt_update_shallow;\n static char *opt_refmap;\n \n+/* Options about upstream */\n+static int opt_set_upstream;\n+\n static struct option pull_options[] = {\n \t/* Shared options */\n \tOPT__VERBOSITY(&opt_verbosity),\n@@ -116,6 +120,9 @@ static struct option pull_options[] = {\n \t\tN_(\"force progress reporting\"),\n \t\tPARSE_OPT_NOARG),\n \n+\t/* Options about upstream */\n+\tOPT_BOOL('u', \"set-upstream\", &opt_set_upstream, N_(\"set upstream for git pull/status\")),\n+\n \t/* Options passed to git-merge or git-rebase */\n \tOPT_GROUP(N_(\"Options related to merging\")),\n \t{ OPTION_CALLBACK, 'r', \"rebase\", &opt_rebase,\n@@ -497,6 +504,10 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs\n \t\tfprintf(stderr, \"\\n\");\n \t\tfprintf_ln(stderr, _(\"If you wish to set tracking information for this branch you can do so with:\"));\n \t\tfprintf(stderr, \"\\n\");\n+\t\tfprintf_ln(stderr, \" git pull -u %s %s\", _(\"\"), _(\"\"));\n+\t\tfprintf(stderr, \"\\n\");\n+\t\tfprintf_ln(stderr, _(\"or\"));\n+\t\tfprintf(stderr, \"\\n\");\n \t\tfprintf_ln(stderr, \" git branch --set-upstream-to=%s/%s %s\\n\",\n \t\t\t\tremote_name, _(\"\"), curr_branch->name);\n \t} else\n@@ -829,8 +840,92 @@ static int run_rebase(const unsigned char *curr_head,\n \treturn ret;\n }\n \n+static void set_pull_upstream(const char *repo, const char **refspecs_name)\n+{\n+\tunsigned char sha1[GIT_SHA1_RAWSZ];\n+\tstruct refspec *refspec;\n+\tstruct branch *branch;\n+\tstruct remote *remote;\n+\tstruct strbuf buf;\n+\tstruct refspec tracking_refspec;\n+\tint nr_refspec, i, flags;\n+\n+\tif (!repo) {\n+\t\twarning(_(\"a remote must be specified to set the upstream\"));\n+\t\treturn;\n+\t}\n+\n+\tremote = remote_get(repo);\n+\tif (!remote) {\n+\t\twarning(_(\"cannot set upstream: \"\n+\t\t\t\"'%s' is not a configured remote\"), repo);\n+\t}\n+\n+\tfor (nr_refspec = 0; refspecs_name[nr_refspec] != NULL; nr_refspec++)\n+\t\t; /* just counting */\n+\n+\tif (nr_refspec == 0) {\n+\t\twarning(_(\"a remote branch must be specified to set the upstream\"));\n+\t\treturn;\n+\t}\n+\n+\tstrbuf_init(&buf, 0);\n+\trefspec = parse_fetch_refspec(nr_refspec, refspecs_name);\n+\n+\tfor (i = 0; i < nr_refspec; i++) {\n+\t\tif (refspec[i].pattern) {\n+\t\t\twarning(_(\"upstream cannot be set for patterns\"));\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tbranch = branch_get(refspec[i].dst);\n+\t\tif (!branch || !ref_exists(branch->refname)) {\n+\t\t\tif (!refspec[i].dst || !*refspec[i].dst)\n+\t\t\t\twarning(_(\"could not set upstream of HEAD when \"\n+\t\t\t\t\t\"it does not point to any branch.\"));\n+\t\t\telse\n+\t\t\t\twarning(_(\"cannot set upstream: \"\n+\t\t\t\t\t\"'%s' is not a branch\"), refspec[i].dst);\n+\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tif (!refspec[i].src || !*refspec[i].src) {\n+\t\t\twarning(_(\"remote branch must be specified \"\n+\t\t\t\t\"to set upstream\"));\n+\t\t\tcontinue;\n+\t\t}\n+\t\t\n+\t\tstrbuf_reset(&buf);\n+\t\tstrbuf_addf(&buf, \"refs/heads/%s\", refspec[i].src);\n+\t\tmemset(&tracking_refspec, 0, sizeof(struct refspec));\n+\t\ttracking_refspec.src = buf.buf;\n+\n+\t\tif (remote_find_tracking(remote, &tracking_refspec)) {\n+\t\t\twarning(_(\"cannot set upstream: \"\n+\t\t\t\t\"no such remote branch '%s'\"), refspec[i].src);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tif (!resolve_ref_unsafe(tracking_refspec.dst, RESOLVE_REF_READING,\n+\t\t\t\t\tsha1, &flags)\n+\t\t\t\t|| (flags & REF_ISSYMREF)) {\n+\t\t\twarning(_(\"cannot set upstream: \"\n+\t\t\t\t\"no such remote branch '%s'\"), refspec[i].src);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tinstall_branch_config((opt_verbosity >= 0 ? BRANCH_CONFIG_VERBOSE : 0),\n+\t\t branch->name, repo, buf.buf);\n+\t}\n+\n+\tfree_refspec(nr_refspec, refspec);\n+\tstrbuf_release(&buf);\n+}\n+\n int cmd_pull(int argc, const char **argv, const char *prefix)\n {\n+\tint ret;\n \tconst char *repo, **refspecs;\n \tstruct sha1_array merge_heads = SHA1_ARRAY_INIT;\n \tunsigned char orig_head[GIT_SHA1_RAWSZ], curr_head[GIT_SHA1_RAWSZ];\n@@ -918,11 +1013,16 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \tif (is_null_sha1(orig_head)) {\n \t\tif (merge_heads.nr > 1)\n \t\t\tdie(_(\"Cannot merge multiple branches into empty head.\"));\n-\t\treturn pull_into_void(*merge_heads.sha1, curr_head);\n+\t\tret = pull_into_void(*merge_heads.sha1, curr_head);\n \t} else if (opt_rebase) {\n \t\tif (merge_heads.nr > 1)\n \t\t\tdie(_(\"Cannot rebase onto multiple branches.\"));\n-\t\treturn run_rebase(curr_head, *merge_heads.sha1, rebase_fork_point);\n+\t\tret = run_rebase(curr_head, *merge_heads.sha1, rebase_fork_point);\n \t} else\n-\t\treturn run_merge();\n+\t\tret = run_merge();\n+\n+\tif (opt_set_upstream && ret < 128)\n+\t\tset_pull_upstream(repo, refspecs);\n+\n+\treturn ret;\n }\ndiff --git a/t/t5544-pull-upstream.sh b/t/t5544-pull-upstream.sh\nnew file mode 100755\nindex 0000000..59f009d\n--- /dev/null\n+++ b/t/t5544-pull-upstream.sh\n@@ -0,0 +1,164 @@\n+#!/bin/sh\n+\n+test_description='pull with --set-upstream'\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-terminal.sh\n+\n+test_config_unchanged () {\n+\tgit config --list --local >original\n+\t\"$@\"\n+\tgit config --list --local >modified\n+\ttest_cmp original modified\n+}\n+\n+check_config () {\n+\t(echo \"$2\"; echo \"$3\") >expect\n+\t(git config branch.$1.remote\n+\t git config branch.$1.merge) >actual\n+\ttest_cmp expect actual\n+}\n+\n+test_expect_success 'setup repos' '\n+\tgit init parent &&\n+\t(\n+\t\tcd parent &&\n+\t\techo content >file &&\n+\t\tgit add file &&\n+\t\tgit commit -am one &&\n+\t\tgit tag initial_tag &&\n+\t\tgit checkout -b master2 &&\n+\t\techo content_modified >file &&\n+\t\tgit commit -am \"file modification\"\n+\t\tgit checkout -b other master &&\n+\t\techo content >file2 &&\n+\t\tgit add file2 &&\n+\t\tgit commit -am two &&\n+\t\tgit checkout -b other2\n+\t) &&\n+\tgit init step_parent &&\n+\t(\n+\t\tcd step_parent &&\n+\t\techo step_content >step_file &&\n+\t\tgit add step_file &&\n+\t\tgit commit -m step_one\n+\t) &&\n+\tgit remote add upstream parent &&\n+\tgit remote add step_upstream step_parent &&\n+\tgit pull upstream master &&\n+\tgit branch other\n+'\n+\n+test_expect_success 'pull -u master' '\n+\tgit pull -u upstream master &&\n+\tcheck_config master upstream refs/heads/master\n+'\n+\n+test_expect_success 'pull -u takes the last branch as upstream' '\n+\ttest_might_fail git config --unset branch.master.merge &&\n+\ttest_might_fail git config --unset branch.master.remote &&\n+\tgit pull -u upstream master master2 &&\n+\tcheck_config master upstream refs/heads/master2\n+'\n+\n+test_expect_success 'pull -u master:other' '\n+\tgit pull -u upstream master:other &&\n+\tcheck_config other upstream refs/heads/master\n+'\n+\n+\n+test_expect_success 'pull -u tracking non-local branch' '\n+\tgit checkout -b master2_local &&\n+\tgit pull -u upstream master2 &&\n+\tcheck_config master2_local upstream refs/heads/master2\n+'\n+\n+\n+test_expect_success 'pull -u --dry-run other:other' '\n+\tgit config branch.other.merge refs/heads/master &&\n+\tgit config branch.other.remote upstream &&\n+\tgit pull -u --dry-run upstream other:other &&\n+\tcheck_config other upstream refs/heads/master\n+'\n+\n+test_expect_success 'pull -u master2:master2 master:other' '\n+\tgit branch master2 &&\n+\tgit pull -u upstream master2:master2 master:other &&\n+\tcheck_config master2 upstream refs/heads/master2 &&\n+\tcheck_config other upstream refs/heads/master\n+'\n+\n+test_expect_success 'pull -u HEAD does not track origin/HEAD nor remote HEAD on origin' '\n+\tgit checkout -b other_head master &&\n+\tgit fetch upstream other &&\n+\tgit remote set-head upstream other &&\n+\ttest_config_unchanged git pull -u upstream HEAD\n+'\n+\n+test_expect_success 'pull -u sets upstream when merge conflicts occur' '\n+\tgit checkout -b master_edited master &&\n+\techo conflict >file2 &&\n+\tgit add file2 &&\n+\tgit commit -am conflict &&\n+\ttest_must_fail git pull -u upstream other &&\n+\tgit rm file2 &&\n+\tgit commit &&\n+\tcheck_config master_edited upstream refs/heads/other\n+'\n+\n+test_expect_success 'pull -u should not work when merging unrelated histories' '\n+\tgit checkout master &&\n+\ttest_config_unchanged test_must_fail git pull -u step_parent master\n+'\n+\n+test_expect_success 'pull -u sets upstream after rebasing' '\n+\tgit checkout -b other_rebased other &&\n+\tgit config branch.other_rebased.merge master &&\n+\tls .git/refs/remotes/upstream &&\n+\tgit pull -r -u upstream master2 &&\n+\tgit branch --set-upstream-to=upstream/master2 &&\n+\tls .git/refs/remotes/upstream &&\n+\tcheck_config other_rebased upstream refs/heads/master2\n+'\n+\n+test_expect_success 'pull -u refs/heads/*:refs/remotes/origin/* should not work' '\n+\tgit checkout master &&\n+\ttest_config_unchanged git pull -u upstream \"refs/heads/*:refs/remotes/upstream/*\"\n+'\n+\n+test_expect_success 'pull -u master:refs/remotes/origin/master should not work' '\n+\ttest_config_unchanged git pull -u upstream master:refs/remotes/upstream/master\n+'\n+\n+test_expect_success 'pull -u with a tag should not work' '\n+\tgit checkout master &&\n+\ttest_config_unchanged git pull -u upstream initial_tag\n+'\n+\n+test_expect_success 'pull -u on detached head should not work' '\n+\tgit checkout HEAD^0 &&\n+\ttest_config_unchanged git pull -u upstream master2 &&\n+\tgit checkout -\n+'\n+\n+test_expect_success 'pull -u with an unconfigured remote should not work' '\n+\tgit checkout master &&\n+\tgit clone parent unconfigured_parent &&\n+\ttest_config_unchanged git pull -u \"$(pwd)/unconfigured_parent\" other2\n+'\n+\n+test_expect_success 'pull -u with a modified remote.fetch' '\n+\tgit remote add origin_modified parent &&\n+\tgit push upstream master:custom_branch &&\n+\tgit config remote.origin_modified.fetch \"+refs/heads/*:refs/remotes/custom/*\" &&\n+\tgit checkout -b lonely_branch master &&\n+\tgit pull -u origin_modified custom_branch &&\n+\tcheck_config lonely_branch origin_modified refs/heads/custom_branch\n+'\n+\n+test_expect_success TTY 'quiet pull' '\n+\tgit checkout master &&\n+\ttest_terminal git pull -u --quiet upstream master 2>&1 | tee output &&\n+\ttest_cmp /dev/null output\n+'\n+\n+test_done\n-- \n2.8.2.662.g22d3535.dirty\n"},{"id":"288443","messageId":"vpqvb1mgvn5.fsf@anie.imag.fr","threadId":"42448","inReplyTo":"20160606093437.1992-1-erwan.mathoniere@grenoble-inp.org","subject":"Re: [RFC/PATCH v2] pull: add --set-upstream","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-06-06T15:54:54Z","receivedAt":"2016-06-06T15:54:54Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Erwan Mathoniere writes:\n\n> @@ -497,6 +504,10 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs\n> \t\tfprintf(stderr, \"\\n\");\n> \t\tfprintf_ln(stderr, _(\"If you wish to set tracking information for this branch you can do so with:\"));\n> \t\tfprintf(stderr, \"\\n\");\n> +\t\tfprintf_ln(stderr, \" git pull -u %s %s\", _(\"\"), _(\"\"));\n\nI'd rather use the long syntax \"--set-upstream\" in the advice. It gives\na hint to the user about what the command is actually going to do.\n\n> +static void set_pull_upstream(const char *repo, const char **refspecs_name)\n> +{\n> +\tunsigned char sha1[GIT_SHA1_RAWSZ];\n\nThe trend in the codebase is to use object_id instead of these char\nsha1[] arrays. See the output of \"git log --grep object_id\" for details.\n\n> +\tstrbuf_init(&buf, 0);\n> +\trefspec = parse_fetch_refspec(nr_refspec, refspecs_name);\n> +\n> +\tfor (i = 0; i < nr_refspec; i++) {\n> +\t\tif (refspec[i].pattern) {\n> +\t\t\twarning(_(\"upstream cannot be set for patterns\"));\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tbranch = branch_get(refspec[i].dst);\n> +\t\tif (!branch || !ref_exists(branch->refname)) {\n> +\t\t\tif (!refspec[i].dst || !*refspec[i].dst)\n> +\t\t\t\twarning(_(\"could not set upstream of HEAD when \"\n> +\t\t\t\t\t\"it does not point to any branch.\"));\n> +\t\t\telse\n> +\t\t\t\twarning(_(\"cannot set upstream: \"\n> +\t\t\t\t\t\"'%s' is not a branch\"), refspec[i].dst);\n\nInconsistent message: \"could not\"/\"cannot\".\n\nFor this kind of code, it greatly helps to have comments refering to the\ninput syntax for each branch of each conditionals. I'm not familiar with\nthis part of the code, but if I understood correctly, the above would be\n\nif ()\n\t/* refspec: : */\n warning(...);\nelse\n\t/* refspec: : */\n warning(...);\n\nI think it would make sense to actually raise an error (i.e. set the\nexit status of the \"git pull\" process to a non-zero value) when such\nissue occur. OTOH, \"git push --set-upstream\" does not even issue a\nwarning when trying to --set-upstream to a tag for example, so not\nraising an error is consistant.\n\n> int cmd_pull(int argc, const char **argv, const char *prefix)\n> {\n> +\tint ret;\n> \tconst char *repo, **refspecs;\n> \tstruct sha1_array merge_heads = SHA1_ARRAY_INIT;\n> \tunsigned char orig_head[GIT_SHA1_RAWSZ], curr_head[GIT_SHA1_RAWSZ];\n> @@ -918,11 +1013,16 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n> \tif (is_null_sha1(orig_head)) {\n> \t\tif (merge_heads.nr > 1)\n> \t\t\tdie(_(\"Cannot merge multiple branches into empty head.\"));\n> -\t\treturn pull_into_void(*merge_heads.sha1, curr_head);\n> +\t\tret = pull_into_void(*merge_heads.sha1, curr_head);\n> \t} else if (opt_rebase) {\n> \t\tif (merge_heads.nr > 1)\n> \t\t\tdie(_(\"Cannot rebase onto multiple branches.\"));\n> -\t\treturn run_rebase(curr_head, *merge_heads.sha1, rebase_fork_point);\n> +\t\tret = run_rebase(curr_head, *merge_heads.sha1, rebase_fork_point);\n> \t} else\n> -\t\treturn run_merge();\n> +\t\tret = run_merge();\n> +\n> +\tif (opt_set_upstream && ret < 128)\n\nShouldn't this be \"ret == 0\"?\n\n> --- /dev/null\n> +++ b/t/t5544-pull-upstream.sh\n> @@ -0,0 +1,164 @@\n> +#!/bin/sh\n> +\n> +test_description='pull with --set-upstream'\n> +. ./test-lib.sh\n> +. \"$TEST_DIRECTORY\"/lib-terminal.sh\n> +\n> +test_config_unchanged () {\n> +\tgit config --list --local >original\n> +\t\"$@\"\n> +\tgit config --list --local >modified\n> +\ttest_cmp original modified\n> +}\n\nThe test passes if \"$@\" fails. You should &&-chain the lines here to\ncatch things like crashes or unexpected \"exit 1\" in git.\n\n> +check_config () {\n\nPerhaps \"check_upstream\" is more expressive.\n\n> +\t(echo \"$2\"; echo \"$3\") >expect\n\nWhat happened to Junio's suggestion with test_write_lines?\n\n> +test_expect_success 'setup repos' '\n> +\tgit init parent &&\n> +\t(\n> +\t\tcd parent &&\n> +\t\techo content >file &&\n> +\t\tgit add file &&\n> +\t\tgit commit -am one &&\n\ntest_commit can make this code simpler.\n\n> +\t\techo content_modified >file &&\n> +\t\tgit commit -am \"file modification\"\n\nLikewise.\n\n> +test_expect_success 'pull -u master' '\n> +\tgit pull -u upstream master &&\n> +\tcheck_config master upstream refs/heads/master\n> +'\n> +\n> +test_expect_success 'pull -u takes the last branch as upstream' '\n> +\ttest_might_fail git config --unset branch.master.merge &&\n> +\ttest_might_fail git config --unset branch.master.remote &&\n> +\tgit pull -u upstream master master2 &&\n> +\tcheck_config master upstream refs/heads/master2\n> +'\n> +\n> +test_expect_success 'pull -u master:other' '\n> +\tgit pull -u upstream master:other &&\n> +\tcheck_config other upstream refs/heads/master\n> +'\n> +\n> +\n\nNit: two blank lines instead of one.\n\n> +test_expect_success 'pull -u refs/heads/*:refs/remotes/origin/* should not work' '\n> +\tgit checkout master &&\n> +\ttest_config_unchanged git pull -u upstream \"refs/heads/*:refs/remotes/upstream/*\"\n> +'\n> +\n> +test_expect_success 'pull -u master:refs/remotes/origin/master should not work' '\n> +\ttest_config_unchanged git pull -u upstream master:refs/remotes/upstream/master\n> +'\n> +\n> +test_expect_success 'pull -u with a tag should not work' '\n> +\tgit checkout master &&\n> +\ttest_config_unchanged git pull -u upstream initial_tag\n> +'\n> +\n> +test_expect_success 'pull -u on detached head should not work' '\n> +\tgit checkout HEAD^0 &&\n> +\ttest_config_unchanged git pull -u upstream master2 &&\n> +\tgit checkout -\n> +'\n\nFor all these \"test_config_unchanged\", it would be nice to check that\nthe error message is the right one too.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"288450","messageId":"2DE0829216C940168DCD804378CD18CC@PhilipOakley","threadId":"42448","inReplyTo":"20160606093437.1992-1-erwan.mathoniere@grenoble-inp.org","subject":"Re: [RFC/PATCH v2] pull: add --set-upstream","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2016-06-06T16:29:39Z","receivedAt":"2016-06-06T16:29:39Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Erwan Mathoniere\" \n> Implement `git pull [--set-upstream | -u] ` that set\n> tracking to the remote branch the user just pulled from.\n>\n> After successfully pulling from ``, for each ``\n> described in format `:`, set\n> `branch..remote` to `` and\n> `branch..merge` to `refs/heads/`. If\n> `` lacks `` in the previous format or directly\n> refers to a branch, use the current branch as `` in the\n> above configuration setting.\n>\n> `git push` has already its `--set-upstream`, it makes sense to have its\n> symmetrical for `git pull`.\n>\n> For a beginner, when trying to use argumentless `git pull` without\n> tracking information set, advising to use\n> `git branch --set-upstream-to` to set upstream can be quite confusing.\n> Using this `git pull --set-upstream` is easier and more natural.\n>\n> Signed-off-by: Erwan Mathoniere \n> Signed-off-by: Jordan De Gea \n> Signed-off-by: Matthieu Moy \n> ---\n>\n> Changes from v1:\n> - Code reshaped to :\n> * warn + no-op when pulling from or to something that isn't a branch\n> or a configured remote\n> * set upstream only after successfully merging/rebasing\n> - More relevant documentation\n> - Tests reshaped to be more independent from each others\n> - More tests (tags, detached heads, non-configured remote...)\n>\n>\n> For now, the documentation is quite hard to understand, but I didn't\n> figure how to explain without using too technical words. Should it stay\n> as it is or should I write something similar the above commit message?\n>\n> Allowing to set non-configured repository as upstream isn't easy to\n> handle since the type of refspec must be checked and this is done by\n> verifying the existence of the remote-tracking branch at\n> `refs/remotes//`.\n>\n>\n> Documentation/git-pull.txt | 18 +++++\n> builtin/pull.c | 106 ++++++++++++++++++++++++++++-\n> t/t5544-pull-upstream.sh | 164 \n> +++++++++++++++++++++++++++++++++++++++++++++\n> 3 files changed, 285 insertions(+), 3 deletions(-)\n> create mode 100755 t/t5544-pull-upstream.sh\n>\n> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\n> index d033b25..6ae5e58 100644\n> --- a/Documentation/git-pull.txt\n> +++ b/Documentation/git-pull.txt\n> @@ -93,6 +93,24 @@ OPTIONS\n> has to be called afterwards to bring the work tree up to date with the\n> merge result.\n>\n> +-u::\n> +--set-upstream::\n> + After successfully pulling from explicitly given and\n\ns/from explicitly/from an explicitly/\n\n> + , set the configuration of the local branches pulled on, so\n\ns/branches pulled on/branches that were pulled/\n\n> + that each one tracks the remote branch pulled from. If a configuration\n> + already exists, it is overwriten. For example, with `git pull -u origin\n> + branch` the current branch will track `branch` from `origin`.\n> ++\n> +If two or more branches are pulled on the same local branch, only the \n> last one\n> +in arguments will be tracked.\n\nIs this specific to this pull --setupstream or a general worning ? i.e. that \na second entry is created in the config file, or that only the last branch \nrefspec will be added?\n\n> ++\n> +The given must be a configured remote. Can only set tracking \n> to\n> +remote branches (e.g. can't set upstream to remote HEAD).\n> ++\n> +Works symmetrically as `--set-upstream` for linkgit:git-push[1]. Allow \n> using\n> +argumentless linkgit:git-pull[1] and other commands. For more \n> information, see\n> +`branch..merge` in linkgit:git-config[1].\n> +\n> Options related to merging\n> ~~~~~~~~~~~~~~~~~~~~~~~~~~\n>\n> diff --git a/builtin/pull.c b/builtin/pull.c\n[snip] \n"},{"id":"288472","messageId":"xmqqoa7ew319.fsf@gitster.mtv.corp.google.com","threadId":"42448","inReplyTo":"vpqvb1mgvn5.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH v2] pull: add --set-upstream","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-06T19:06:10Z","receivedAt":"2016-06-06T19:06:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n>> +test_config_unchanged () {\n>> +\tgit config --list --local >original\n>> +\t\"$@\"\n>> +\tgit config --list --local >modified\n>> +\ttest_cmp original modified\n>> +}\n>\n> The test passes if \"$@\" fails. You should &&-chain the lines here to\n> catch things like crashes or unexpected \"exit 1\" in git.\n\nThat is true, but allowing \"$@\" failure may be deliberate. After\nall, \"git pull -u there that\", when pulling that from there fails,\nmay not want to update the upstream tracking information.\n\nBut I am unhappy with a more serious problem with the tests in this\npatch. They assume that \"-u\" option will forever be the only thing\nthat is allowed to modify the configuration during \"git pull -u\".\nIt should never make such an assumption.\n\nThe only thing these additional tests later in the patch (ommitted)\nwant to check, if I understand them correctly, is that when -u is\nused on a ref that shouldn't be tracked from the given remote then \nremote..merge etc. are not updated. Make a list of the\nconfiguration variables the feature cares about, and check them and\nignore changes to any other variable. Somebody else's feature that\nwill be added to \"git pull\" may have legitimate reason to update\nconfiguration variables that are not releated to this feature, and\nyou shouldn't be writing your test for your feature in such a way\nto forbid such a new feature by others from being added.\n"},{"id":"288529","messageId":"vpqeg89bhqt.fsf@anie.imag.fr","threadId":"42448","inReplyTo":"xmqqoa7ew319.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH v2] pull: add --set-upstream","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-06-07T07:06:18Z","receivedAt":"2016-06-07T07:06:18Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> Matthieu Moy writes:\n>\n>>> +test_config_unchanged () {\n>>> +\tgit config --list --local >original\n>>> +\t\"$@\"\n>>> +\tgit config --list --local >modified\n>>> +\ttest_cmp original modified\n>>> +}\n>>\n>> The test passes if \"$@\" fails. You should &&-chain the lines here to\n>> catch things like crashes or unexpected \"exit 1\" in git.\n>\n> That is true, but allowing \"$@\" failure may be deliberate. \n\nI don't think so:\n\n+test_expect_success 'pull -u should not work when merging unrelated histories' '\n+\tgit checkout master &&\n+\ttest_config_unchanged test_must_fail git pull -u step_parent master\n+'\n\n;-)\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"288589","messageId":"8c74e096-f573-66b1-5f00-e4bb771720ee@grenoble-inp.org","threadId":"42448","inReplyTo":"vpqvb1mgvn5.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH v2] pull: add --set-upstream","fromName":"Erwan Mathoniere","fromEmail":"erwan.mathoniere@grenoble-inp.org","sentAt":"2016-06-07T12:43:06Z","receivedAt":"2016-06-07T12:43:06Z","isPatch":true,"sender":{"key":"erwan.mathoniere@grenoble-inp.org","avatar":"https://avatars.githubusercontent.com/u/12645778?v=4"},"body":"\n\nOn 06/06/2016 17:54, Matthieu Moy wrote:\n> Erwan Mathoniere writes:\n>\n>> @@ -497,6 +504,10 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs\n>> \t\tfprintf(stderr, \"\\n\");\n>> \t\tfprintf_ln(stderr, _(\"If you wish to set tracking information for this branch you can do so with:\"));\n>> \t\tfprintf(stderr, \"\\n\");\n>> +\t\tfprintf_ln(stderr, \" git pull -u %s %s\", _(\"\"), _(\"\"));\n>\n> I'd rather use the long syntax \"--set-upstream\" in the advice. It gives\n> a hint to the user about what the command is actually going to do.\n\nYou're right, I'll change it.\n\n>> +static void set_pull_upstream(const char *repo, const char **refspecs_name)\n>> +{\n>> +\tunsigned char sha1[GIT_SHA1_RAWSZ];\n>\n> The trend in the codebase is to use object_id instead of these char\n> sha1[] arrays. See the output of \"git log --grep object_id\" for details.\n\nI'll dig into it, thanks.\n\n>> +\tstrbuf_init(&buf, 0);\n>> +\trefspec = parse_fetch_refspec(nr_refspec, refspecs_name);\n>> +\n>> +\tfor (i = 0; i < nr_refspec; i++) {\n>> +\t\tif (refspec[i].pattern) {\n>> +\t\t\twarning(_(\"upstream cannot be set for patterns\"));\n>> +\t\t\tcontinue;\n>> +\t\t}\n>> +\n>> +\t\tbranch = branch_get(refspec[i].dst);\n>> +\t\tif (!branch || !ref_exists(branch->refname)) {\n>> +\t\t\tif (!refspec[i].dst || !*refspec[i].dst)\n>> +\t\t\t\twarning(_(\"could not set upstream of HEAD when \"\n>> +\t\t\t\t\t\"it does not point to any branch.\"));\n>> +\t\t\telse\n>> +\t\t\t\twarning(_(\"cannot set upstream: \"\n>> +\t\t\t\t\t\"'%s' is not a branch\"), refspec[i].dst);\n>\n> Inconsistent message: \"could not\"/\"cannot\".\n\nI copied/pasted the first warning from another portion of code in order \nto avoid useless translation work, but it isn't really relevant in that \ncase.\n\n> For this kind of code, it greatly helps to have comments refering to the\n> input syntax for each branch of each conditionals. I'm not familiar with\n> this part of the code, but if I understood correctly, the above would be\n>\n> if ()\n> \t/* refspec: : */\n> warning(...);\n> else\n> \t/* refspec: : */\n> warning(...);\n\nGood idea, some part aren't so easy to link to the input.\n\n> I think it would make sense to actually raise an error (i.e. set the\n> exit status of the \"git pull\" process to a non-zero value) when such\n> issue occur. OTOH, \"git push --set-upstream\" does not even issue a\n> warning when trying to --set-upstream to a tag for example, so not\n> raising an error is consistant.\n\nSince the idea is to have a similar behavior to `git push \n--set-upstream`, I think it's not relevant to raise an error when `git \npush` doesn't. And for a minor feature, it seems disproportionate to \nconsider a `git pull -u ...` as a failure when the entire process \nsucceeded except for the upstream part.\n\n>> int cmd_pull(int argc, const char **argv, const char *prefix)\n>> {\n>> +\tint ret;\n>> \tconst char *repo, **refspecs;\n>> \tstruct sha1_array merge_heads = SHA1_ARRAY_INIT;\n>> \tunsigned char orig_head[GIT_SHA1_RAWSZ], curr_head[GIT_SHA1_RAWSZ];\n>> @@ -918,11 +1013,16 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n>> \tif (is_null_sha1(orig_head)) {\n>> \t\tif (merge_heads.nr > 1)\n>> \t\t\tdie(_(\"Cannot merge multiple branches into empty head.\"));\n>> -\t\treturn pull_into_void(*merge_heads.sha1, curr_head);\n>> +\t\tret = pull_into_void(*merge_heads.sha1, curr_head);\n>> \t} else if (opt_rebase) {\n>> \t\tif (merge_heads.nr > 1)\n>> \t\t\tdie(_(\"Cannot rebase onto multiple branches.\"));\n>> -\t\treturn run_rebase(curr_head, *merge_heads.sha1, rebase_fork_point);\n>> +\t\tret = run_rebase(curr_head, *merge_heads.sha1, rebase_fork_point);\n>> \t} else\n>> -\t\treturn run_merge();\n>> +\t\tret = run_merge();\n>> +\n>> +\tif (opt_set_upstream && ret < 128)\n>\n> Shouldn't this be \"ret == 0\"?\n\nThe feature activates after rebasing/merging. When there are merge \nconflicts, `run_merge` doesn't return a non-null error code and we want \nto set upstream even in that case.\nOn the other hand, when merge fails (e.g. histories aren't related) it \nstops itself by invoking `die` and returning a >= 128 error code. We \ndon't want to proceed in that case.\n\n>> +check_config () {\n>\n> Perhaps \"check_upstream\" is more expressive.\n\nI took this procedure from t5523-push-upstream, but you're right it's \nmore relevant.\n\n>> +\t(echo \"$2\"; echo \"$3\") >expect\n>\n> What happened to Junio's suggestion with test_write_lines?\n\nOups, small oversight.\n\n>> +test_expect_success 'setup repos' '\n>> +\tgit init parent &&\n>> +\t(\n>> +\t\tcd parent &&\n>> +\t\techo content >file &&\n>> +\t\tgit add file &&\n>> +\t\tgit commit -am one &&\n>\n> test_commit can make this code simpler.\n>\n>> +\t\techo content_modified >file &&\n>> +\t\tgit commit -am \"file modification\"\n>\n> Likewise.\n>\n\nThanks, I'll take a look at it.\n\n>> +test_expect_success 'pull -u with a tag should not work' '\n>> +\tgit checkout master &&\n>> +\ttest_config_unchanged git pull -u upstream initial_tag\n>> +'\n>> +\n>> +test_expect_success 'pull -u on detached head should not work' '\n>> +\tgit checkout HEAD^0 &&\n>> +\ttest_config_unchanged git pull -u upstream master2 &&\n>> +\tgit checkout -\n>> +'\n>\n> For all these \"test_config_unchanged\", it would be nice to check that\n> the error message is the right one too.\n\nOk, I'll add checks on that.\n"},{"id":"288590","messageId":"22613f02-06b8-9f5f-fd92-15df3ca3a34f@grenoble-inp.org","threadId":"42448","inReplyTo":"vpqeg89bhqt.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH v2] pull: add --set-upstream","fromName":"Erwan Mathoniere","fromEmail":"erwan.mathoniere@grenoble-inp.org","sentAt":"2016-06-07T12:54:20Z","receivedAt":"2016-06-07T12:54:20Z","isPatch":true,"sender":{"key":"erwan.mathoniere@grenoble-inp.org","avatar":"https://avatars.githubusercontent.com/u/12645778?v=4"},"body":"\n\nOn 07/06/2016 09:06, Matthieu Moy wrote:\n> Junio C Hamano writes:\n>\n>> Matthieu Moy writes:\n>>\n>>>> +test_config_unchanged () {\n>>>> +\tgit config --list --local >original\n>>>> +\t\"$@\"\n>>>> +\tgit config --list --local >modified\n>>>> +\ttest_cmp original modified\n>>>> +}\n>>>\n>>> The test passes if \"$@\" fails. You should &&-chain the lines here to\n>>> catch things like crashes or unexpected \"exit 1\" in git.\n>>\n>> That is true, but allowing \"$@\" failure may be deliberate.\n>\n> I don't think so:\n>\n> +test_expect_success 'pull -u should not work when merging unrelated histories' '\n> +\tgit checkout master &&\n> +\ttest_config_unchanged test_must_fail git pull -u step_parent master\n> +'\n>\n> ;-)\n>\n\nWhen writing `test_config_unchanged` procedure, I wanted to return the \ncomparison even if \"$@\" failed. But it's indeed not consistent with the \nway I wrote the tests.\n\nAdding \"&&\" between instructions and using `test_must_fail` when calling \nthe procedure is clearer and more logical.\n"},{"id":"288598","messageId":"af8762b1-3865-5d10-296c-db9bf9e8aa53@grenoble-inp.org","threadId":"42448","inReplyTo":"xmqqoa7ew319.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH v2] pull: add --set-upstream","fromName":"Erwan Mathoniere","fromEmail":"erwan.mathoniere@grenoble-inp.org","sentAt":"2016-06-07T13:15:02Z","receivedAt":"2016-06-07T13:15:02Z","isPatch":true,"sender":{"key":"erwan.mathoniere@grenoble-inp.org","avatar":"https://avatars.githubusercontent.com/u/12645778?v=4"},"body":"\n\nOn 06/06/2016 21:06, Junio C Hamano wrote:\n>\n> But I am unhappy with a more serious problem with the tests in this\n> patch. They assume that \"-u\" option will forever be the only thing\n> that is allowed to modify the configuration during \"git pull -u\".\n> It should never make such an assumption.\n>\n> The only thing these additional tests later in the patch (ommitted)\n> want to check, if I understand them correctly, is that when -u is\n> used on a ref that shouldn't be tracked from the given remote then\n> remote..merge etc. are not updated. Make a list of the\n> configuration variables the feature cares about, and check them and\n> ignore changes to any other variable. Somebody else's feature that\n> will be added to \"git pull\" may have legitimate reason to update\n> configuration variables that are not releated to this feature, and\n> you shouldn't be writing your test for your feature in such a way\n> to forbid such a new feature by others from being added.\n\nI asked myself these questions but I came to the wrong conclusion since \nI considered that testing if `--set-upstream` doesn't alter any \nconfiguration var it shouldn't was also important. But there is no \nreason \"git pull -u\" modify the configuration in such a chaotic way.\n\nI'll apply your suggestions, thanks.\n"},{"id":"288600","messageId":"eb423083-2574-45a7-ed78-4ebb75363141@grenoble-inp.org","threadId":"42448","inReplyTo":"2DE0829216C940168DCD804378CD18CC@PhilipOakley","subject":"Re: [RFC/PATCH v2] pull: add --set-upstream","fromName":"Erwan Mathoniere","fromEmail":"erwan.mathoniere@grenoble-inp.org","sentAt":"2016-06-07T13:42:34Z","receivedAt":"2016-06-07T13:42:34Z","isPatch":true,"sender":{"key":"erwan.mathoniere@grenoble-inp.org","avatar":"https://avatars.githubusercontent.com/u/12645778?v=4"},"body":"\n\nOn 06/06/2016 18:29, Philip Oakley wrote:\n>> + that each one tracks the remote branch pulled from. If a configuration\n>> + already exists, it is overwriten. For example, with `git pull -u origin\n>> + branch` the current branch will track `branch` from `origin`.\n>> ++\n>> +If two or more branches are pulled on the same local branch, only the\n>> last one\n>> +in arguments will be tracked.\n>\n> Is this specific to this pull --setupstream or a general worning ? i.e.\n> that a second entry is created in the config file, or that only the last\n> branch refspec will be added?\n\nOnly the last branch will be added. More precisely, its behavior is just \nlike `git push --set-upstream`. If you do `git push --set-upstream \nmaster:master master:other`, git will change its configuration twice and \nwill print out:\n\n$ git push --set-upstream origin master:master master:other\n[...]\nBranch master set up to track remote branch master from origin.\nBranch master set up to track remote branch other from origin.\n\nAnd at the end, \"master\" will only track \"other\" from origin and both \n\"branch.master.{merge, remote}\" will be set once.\nSo for now, `git pull --set-upstream` does the same and for example, on \nmaster:\n\n$ git pull --set-upstream origin master other\n[...]\nBranch master set up to track remote branch master from origin.\nBranch master set up to track remote branch other from origin.\n"}]}