Re: [RFC/PATCH v2] pull: add --set-upstream
- From
Matthieu Moy <matthieu.moy@grenoble-inp.fr>
- Date
- Jun 6, 2016, 15:54 UTC
- Message-ID
- <vpqvb1mgvn5.fsf@anie.imag.fr>
- In-Reply-To
- <20160606093437.1992-1-erwan.mathoniere@grenoble-inp.org>
Erwan Mathoniere <erwan.mathoniere@grenoble-inp.org> writes:
Show 5 quoted lines
> @@ -497,6 +504,10 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs
> fprintf(stderr, "\n");
> fprintf_ln(stderr, _("If you wish to set tracking information for this branch you can do so with:"));
> fprintf(stderr, "\n");
> + fprintf_ln(stderr, " git pull -u %s %s", _("<remote>"), _("<branch>"));I'd rather use the long syntax "--set-upstream" in the advice. It gives a hint to the user about what the command is actually going to do.
> +static void set_pull_upstream(const char *repo, const char **refspecs_name)
> +{
> + unsigned char sha1[GIT_SHA1_RAWSZ];The trend in the codebase is to use object_id instead of these char sha1[] arrays. See the output of "git log --grep object_id" for details.
Show 17 quoted lines
> + strbuf_init(&buf, 0);
> + refspec = parse_fetch_refspec(nr_refspec, refspecs_name);
> +
> + for (i = 0; i < nr_refspec; i++) {
> + if (refspec[i].pattern) {
> + warning(_("upstream cannot be set for patterns"));
> + continue;
> + }
> +
> + branch = branch_get(refspec[i].dst);
> + if (!branch || !ref_exists(branch->refname)) {
> + if (!refspec[i].dst || !*refspec[i].dst)
> + warning(_("could not set upstream of HEAD when "
> + "it does not point to any branch."));
> + else
> + warning(_("cannot set upstream: "
> + "'%s' is not a branch"), refspec[i].dst);Inconsistent message: "could not"/"cannot".
For this kind of code, it greatly helps to have comments refering to the input syntax for each branch of each conditionals. I'm not familiar with this part of the code, but if I understood correctly, the above would be
if ()
/* refspec: <branch>: */
warning(...);
else
/* refspec: <branch>:<no-such-branch> */
warning(...);I think it would make sense to actually raise an error (i.e. set the exit status of the "git pull" process to a non-zero value) when such issue occur. OTOH, "git push --set-upstream" does not even issue a warning when trying to --set-upstream to a tag for example, so not raising an error is consistant.
Show 22 quoted lines
> int cmd_pull(int argc, const char **argv, const char *prefix)
> {
> + int ret;
> const char *repo, **refspecs;
> struct sha1_array merge_heads = SHA1_ARRAY_INIT;
> unsigned char orig_head[GIT_SHA1_RAWSZ], curr_head[GIT_SHA1_RAWSZ];
> @@ -918,11 +1013,16 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
> if (is_null_sha1(orig_head)) {
> if (merge_heads.nr > 1)
> die(_("Cannot merge multiple branches into empty head."));
> - return pull_into_void(*merge_heads.sha1, curr_head);
> + ret = pull_into_void(*merge_heads.sha1, curr_head);
> } else if (opt_rebase) {
> if (merge_heads.nr > 1)
> die(_("Cannot rebase onto multiple branches."));
> - return run_rebase(curr_head, *merge_heads.sha1, rebase_fork_point);
> + ret = run_rebase(curr_head, *merge_heads.sha1, rebase_fork_point);
> } else
> - return run_merge();
> + ret = run_merge();
> +
> + if (opt_set_upstream && ret < 128)Shouldn't this be "ret == 0"?
Show 15 quoted lines
> --- /dev/null
> +++ b/t/t5544-pull-upstream.sh
> @@ -0,0 +1,164 @@
> +#!/bin/sh
> +
> +test_description='pull with --set-upstream'
> +. ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-terminal.sh
> +
> +test_config_unchanged () {
> + git config --list --local >original
> + "$@"
> + git config --list --local >modified
> + test_cmp original modified
> +}The test passes if "$@" fails. You should &&-chain the lines here to catch things like crashes or unexpected "exit 1" in git.
> +check_config () {Perhaps "check_upstream" is more expressive.
> + (echo "$2"; echo "$3") >expect
What happened to Junio's suggestion with test_write_lines?
Show 7 quoted lines
> +test_expect_success 'setup repos' ' > + git init parent && > + ( > + cd parent && > + echo content >file && > + git add file && > + git commit -am one &&
test_commit can make this code simpler.
> + echo content_modified >file && > + git commit -am "file modification"
Likewise.
Show 18 quoted lines
> +test_expect_success 'pull -u master' ' > + git pull -u upstream master && > + check_config master upstream refs/heads/master > +' > + > +test_expect_success 'pull -u takes the last branch as upstream' ' > + test_might_fail git config --unset branch.master.merge && > + test_might_fail git config --unset branch.master.remote && > + git pull -u upstream master master2 && > + check_config master upstream refs/heads/master2 > +' > + > +test_expect_success 'pull -u master:other' ' > + git pull -u upstream master:other && > + check_config other upstream refs/heads/master > +' > + > +
Nit: two blank lines instead of one.
Show 19 quoted lines
> +test_expect_success 'pull -u refs/heads/*:refs/remotes/origin/* should not work' ' > + git checkout master && > + test_config_unchanged git pull -u upstream "refs/heads/*:refs/remotes/upstream/*" > +' > + > +test_expect_success 'pull -u master:refs/remotes/origin/master should not work' ' > + test_config_unchanged git pull -u upstream master:refs/remotes/upstream/master > +' > + > +test_expect_success 'pull -u with a tag should not work' ' > + git checkout master && > + test_config_unchanged git pull -u upstream initial_tag > +' > + > +test_expect_success 'pull -u on detached head should not work' ' > + git checkout HEAD^0 && > + test_config_unchanged git pull -u upstream master2 && > + git checkout - > +'
For all these "test_config_unchanged", it would be nice to check that the error message is the right one too.
-- Matthieu Moy http://www-verimag.imag.fr/~moy/