diff mbox series

[v2,1/1] push: make '-u' have default arguments

Message ID 20211207182300.4361-2-chakrabortyabhradeep79@gmail.com (mailing list archive)
State Superseded
Headers show
Series making --set-upstream have default arguments | expand

Commit Message

Abhradeep Chakraborty Dec. 7, 2021, 6:23 p.m. UTC
"git push -u" (set-upstream) requires where to push to and what
to push.  Often people push only the current branch to update
the branch of the same name at the 'origin' repository.  For
them, it would be convenient if "git push -u" without repository
or refspec, defaulted to push and set upstream to the branch as
configured by the "push.default" setting, of the remote repository
that is used by default.

Teach "git push -u" not to require repository and refspec.  When
the user do not give what repository to push to, or which
branch(es) to push, behave as if the default remote repository
and a refspec (depending on the "push.default" configuration)
are given.

If "push.default"=matching, push all the branches matched on both
remote and local side and set those remote branches as the upstream
of their respective local matched branches. Otherwise, set the
refspec to the refspec for current branch.

Signed-off-by: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>
---
 Documentation/git-push.txt |  10 ++++
 builtin/push.c             |  11 +++-
 t/t5523-push-upstream.sh   | 114 +++++++++++++++++++++++++++++++++++++
 3 files changed, 133 insertions(+), 2 deletions(-)

Comments

Eric Sunshine Dec. 7, 2021, 10:14 p.m. UTC | #1
On Tue, Dec 7, 2021 at 4:11 PM Abhradeep Chakraborty
<chakrabortyabhradeep79@gmail.com> wrote:
> [...]
> Teach "git push -u" not to require repository and refspec.  When
> the user do not give what repository to push to, or which
> branch(es) to push, behave as if the default remote repository
> and a refspec (depending on the "push.default" configuration)
> are given.
> [...]
> Signed-off-by: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>

This is not a proper review... just some superficial comments from
scanning my eye over the patch...

> diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh
> @@ -60,6 +60,75 @@ test_expect_success 'push -u :topic_2' '
> +default_u_setup() {
> +       git checkout main
> +       remote=$(git config --get branch.main.remote)
> +       if [ ! -z "$remote" ]; then
> +               git branch --unset-upstream
> +       fi
> +       git config push.default $1
> +       git config remote.pushDefault upstream
> +}

A few issues...

* since callers of this function incorporate it into their &&-chains,
the body of the function itself should probably also have an intact
&&-chain

* use `test` rather than `[`

* `then` goes on its own line

* probably want to use test_config() here rather than raw `git config`

* `! -z` can be written more simply as `-n`

Taking the above into account, gives:

    default_u_setup() {
        git checkout main &&
        remote=$(git config --default '' --get branch.main.remote) &&
        if test -n "$remote"
        then
            git branch --unset-upstream
        fi &&
        test_config push.default $1 &&
        test_config remote.pushDefault upstream
    }

The `--default` ensures that `git config` will exit with a success
code which is important now that it's part of the &&-chain.
Alternatively, you could skip the dance of checking for
`branch.main.remote` and just call `git branch --unset-upstream`
unconditionally, but wrap it with test_might_fail() so it can be part
of the &&-chain without worrying about whether that command succeeds
or fails:

    default_u_setup() {
        git checkout main &&
        test_might_fail git branch --unset-upstream &&
        test_config push.default $1 &&
        test_config remote.pushDefault upstream
    }

> +test_expect_success 'push -u with push.default=simple' '
> +       default_u_setup simple &&
> +       git push -u &&
> +       check_config main upstream refs/heads/main &&
> +       git push -u upstream main:other &&
> +       git push -u &&
> +       check_config main upstream refs/heads/main
> +'
> +
> +test_expect_success 'push -u with push.default=current' '
> +       default_u_setup current &&
> +       git push -u &&
> +       check_config main upstream refs/heads/main &&
> +       git push -u upstream main:other &&
> +       git push -u &&
> +       check_config main upstream refs/heads/main
> +'
> +
> +test_expect_success 'push -u with push.default=upstream' '
> +       default_u_setup upstream &&
> +       git push -u &&
> +       check_config main upstream refs/heads/main &&
> +       git push -u upstream main:other &&
> +       git push -u &&
> +       check_config main upstream refs/heads/main
> +'

When a number of tests have nearly identical bodies like this, it is
sometimes clearer and more convenient to turn them into a for-loop
like this:

    for i in simple current upstream
    do
        test_expect_success "push -u with push.default=$i" '
            default_u_setup $i &&
            git push -u &&
            check_config main upstream refs/heads/main &&
            git push -u upstream main:other &&
            git push -u &&
            check_config main upstream refs/heads/main
        '
    done

> +check_empty_config() {
> +       test_expect_code 1 git config "branch.$1.remote"
> +       test_expect_code 1 git config "branch.$1.merge"
> +}

As above, because calls to this function are part of the &&-chain in
test bodies, it is important for the &&-chain to be intact in the
function too. It's especially important in this case since this
function is actually checking for specific conditions. As it's
currently written -- with a broken &&-chain -- if the first
test_expect_code() fails, we'll never know about it since that exit
code gets lost; only the exit code from the second test_expect_code()
has any bearing on the overall result of the test.

> +test_expect_success 'progress messagesdo not go to non-tty (default -u)' '

s/messagesdo/messages do/

> +       ensure_fresh_upstream &&
> +
> +       # skip progress messages, since stderr is non-tty
> +       git push -u >out 2>err &&
> +       test_i18ngrep ! "Writing objects" err
> +'

The captured stdout in `out` doesn't seem to be used, so it's probably
better to drop that redirection.

> +test_expect_success 'progress messages go to non-tty with default -u (forced)' '
> +       ensure_fresh_upstream &&
> +
> +       # force progress messages to stderr, even though it is non-tty
> +       git push -u --progress >out 2>err &&
> +       test_i18ngrep "Writing objects" err
> +'

Ditto. And repeat for the remaining tests.
Abhradeep Chakraborty Dec. 8, 2021, 6:12 a.m. UTC | #2
Eric Sunshine <sunshine@sunshineco.com> wrote:

> As above, because calls to this function are part of the &&-chain in
> test bodies, it is important for the &&-chain to be intact in the
> function too. It's especially important in this case since this
> function is actually checking for specific conditions. As it's
> currently written -- with a broken &&-chain -- if the first
> test_expect_code() fails, we'll never know about it since that exit
> code gets lost; only the exit code from the second test_expect_code()
> has any bearing on the overall result of the test.

Thanks so much for all the suggestions! I am little beginner in shell
scripting. It would help me a lot!

> s/messagesdo/messages do/

oops :|

> The captured stdout in `out` doesn't seem to be used, so it's probably
> better to drop that redirection.

okay.
diff mbox series

Patch

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 2f25aa3a29..048c087e23 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -375,6 +375,16 @@  Specifying `--no-force-if-includes` disables this behavior.
 	upstream (tracking) reference, used by argument-less
 	linkgit:git-pull[1] and other commands. For more information,
 	see `branch.<name>.merge` in linkgit:git-config[1].
++
+`-u` can also work with zero arguments( i.e. no `<repository>` and
+`<refspec>` are given). In that case, it tries to get the `<repository>`
+value from `branch.*.remote` configuration. If not found, it defaults to
+`origin`. If `remote.pushDefault` is set then it uses that instead. The
+value of `<refspec>` depends on the current `push.default` configuration.
+If `push.default` is set to `matching`, all remote branches to which
+local branches pushed, will be set as upstream of respective local
+branches. For all other values of `push.default`, current branch's
+`<refspec>` will be used as the `<refspec>`.
 
 --[no-]thin::
 	These options are passed to linkgit:git-send-pack[1]. A thin transfer
diff --git a/builtin/push.c b/builtin/push.c
index 4b026ce6c6..8bc206c9d8 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -202,11 +202,12 @@  static const char *get_upstream_ref(struct branch *branch, const char *remote_na
 	return branch->merge[0]->src;
 }
 
-static void setup_default_push_refspecs(struct remote *remote)
+static void setup_default_push_refspecs(struct remote *remote, int flags)
 {
 	struct branch *branch;
 	const char *dst;
 	int same_remote;
+	int is_default_u = (flags & TRANSPORT_PUSH_SET_UPSTREAM);
 
 	switch (push_default) {
 	case PUSH_DEFAULT_MATCHING:
@@ -214,6 +215,8 @@  static void setup_default_push_refspecs(struct remote *remote)
 		return;
 
 	case PUSH_DEFAULT_NOTHING:
+		if (is_default_u)
+			break;
 		die(_("You didn't specify any refspecs to push, and "
 		    "push.default is \"nothing\"."));
 		return;
@@ -234,11 +237,15 @@  static void setup_default_push_refspecs(struct remote *remote)
 	case PUSH_DEFAULT_SIMPLE:
 		if (!same_remote)
 			break;
+		if (is_default_u)
+			break;
 		if (strcmp(branch->refname, get_upstream_ref(branch, remote->name)))
 			die_push_simple(branch, remote);
 		break;
 
 	case PUSH_DEFAULT_UPSTREAM:
+		if (is_default_u)
+			break;
 		if (!same_remote)
 			die(_("You are pushing to remote '%s', which is not the upstream of\n"
 			      "your current branch '%s', without telling me what to push\n"
@@ -401,7 +408,7 @@  static int do_push(int flags,
 		if (remote->push.nr) {
 			push_refspec = &remote->push;
 		} else if (!(flags & TRANSPORT_PUSH_MIRROR))
-			setup_default_push_refspecs(remote);
+			setup_default_push_refspecs(remote, flags);
 	}
 	errs = 0;
 	url_nr = push_url_of_remote(remote, &url);
diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh
index fdb4292056..fbcdf303ef 100755
--- a/t/t5523-push-upstream.sh
+++ b/t/t5523-push-upstream.sh
@@ -60,6 +60,75 @@  test_expect_success 'push -u :topic_2' '
 	check_config topic_2 upstream refs/heads/other2
 '
 
+default_u_setup() {
+	git checkout main
+	remote=$(git config --get branch.main.remote)
+	if [ ! -z "$remote" ]; then
+		git branch --unset-upstream
+	fi
+	git config push.default $1
+	git config remote.pushDefault upstream
+}
+
+test_expect_success 'push -u with push.default=simple' '
+	default_u_setup simple &&
+	git push -u &&
+	check_config main upstream refs/heads/main &&
+	git push -u upstream main:other &&
+	git push -u &&
+	check_config main upstream refs/heads/main
+'
+
+test_expect_success 'push -u with push.default=current' '
+	default_u_setup current &&
+	git push -u &&
+	check_config main upstream refs/heads/main &&
+	git push -u upstream main:other &&
+	git push -u &&
+	check_config main upstream refs/heads/main
+'
+
+test_expect_success 'push -u with push.default=upstream' '
+	default_u_setup upstream &&
+	git push -u &&
+	check_config main upstream refs/heads/main &&
+	git push -u upstream main:other &&
+	git push -u &&
+	check_config main upstream refs/heads/main
+'
+
+check_empty_config() {
+	test_expect_code 1 git config "branch.$1.remote"
+	test_expect_code 1 git config "branch.$1.merge"
+}
+
+test_expect_success 'push -u with push.default=matching' '
+	default_u_setup matching &&
+	git branch test_u &&
+	git branch test_u2 &&
+	git push upstream main:test_u2 &&
+	git push -u &&
+	check_config main upstream refs/heads/main &&
+	check_config test_u2 upstream refs/heads/test_u2 &&
+	check_empty_config test_u
+'
+
+test_expect_success 'push -u with push.default=nothing' '
+	default_u_setup nothing &&
+	git push -u &&
+	check_config main upstream refs/heads/main &&
+	git push -u upstream main:other &&
+	git push -u &&
+	check_config main upstream refs/heads/main
+'
+
+test_expect_success 'push -u --dry-run' '
+	git checkout main &&
+	git push -u upstream main:other &&
+	git push -u --dry-run &&
+	check_config main upstream refs/heads/other
+'
+
 test_expect_success 'push -u --all' '
 	git branch all1 &&
 	git branch all2 &&
@@ -81,6 +150,13 @@  test_expect_success TTY 'progress messages go to tty' '
 	test_i18ngrep "Writing objects" err
 '
 
+test_expect_success TTY 'progress messages go to tty with default -u' '
+	ensure_fresh_upstream &&
+
+	test_terminal git push -u >out 2>err &&
+	test_i18ngrep "Writing objects" err
+'
+
 test_expect_success 'progress messages do not go to non-tty' '
 	ensure_fresh_upstream &&
 
@@ -89,6 +165,14 @@  test_expect_success 'progress messages do not go to non-tty' '
 	test_i18ngrep ! "Writing objects" err
 '
 
+test_expect_success 'progress messagesdo not go to non-tty (default -u)' '
+	ensure_fresh_upstream &&
+
+	# skip progress messages, since stderr is non-tty
+	git push -u >out 2>err &&
+	test_i18ngrep ! "Writing objects" err
+'
+
 test_expect_success 'progress messages go to non-tty (forced)' '
 	ensure_fresh_upstream &&
 
@@ -97,6 +181,14 @@  test_expect_success 'progress messages go to non-tty (forced)' '
 	test_i18ngrep "Writing objects" err
 '
 
+test_expect_success 'progress messages go to non-tty with default -u (forced)' '
+	ensure_fresh_upstream &&
+
+	# force progress messages to stderr, even though it is non-tty
+	git push -u --progress >out 2>err &&
+	test_i18ngrep "Writing objects" err
+'
+
 test_expect_success TTY 'push -q suppresses progress' '
 	ensure_fresh_upstream &&
 
@@ -104,6 +196,13 @@  test_expect_success TTY 'push -q suppresses progress' '
 	test_i18ngrep ! "Writing objects" err
 '
 
+test_expect_success TTY 'push -q suppresses progress (with default -u)' '
+	ensure_fresh_upstream &&
+
+	test_terminal git push -u -q >out 2>err &&
+	test_i18ngrep ! "Writing objects" err
+'
+
 test_expect_success TTY 'push --no-progress suppresses progress' '
 	ensure_fresh_upstream &&
 
@@ -112,6 +211,14 @@  test_expect_success TTY 'push --no-progress suppresses progress' '
 	test_i18ngrep ! "Writing objects" err
 '
 
+test_expect_success TTY 'push --no-progress suppresses progress (default -u)' '
+	ensure_fresh_upstream &&
+
+	test_terminal git push -u --no-progress >out 2>err &&
+	test_i18ngrep ! "Unpacking objects" err &&
+	test_i18ngrep ! "Writing objects" err
+'
+
 test_expect_success TTY 'quiet push' '
 	ensure_fresh_upstream &&
 
@@ -126,4 +233,11 @@  test_expect_success TTY 'quiet push -u' '
 	test_must_be_empty output
 '
 
+test_expect_success TTY 'quiet push -u (default -u)' '
+	ensure_fresh_upstream &&
+
+	test_terminal git push --quiet -u --no-progress 2>&1 | tee output &&
+	test_must_be_empty output
+'
+
 test_done