* [PATCH v5 0/8] ambiguous checkout UI & checkout.defaultRemote
2018-05-31 19:52 ` [PATCH v4 0/9] ambiguous checkout UI & checkout.defaultRemote Ævar Arnfjörð Bjarmason
@ 2018-06-01 21:10 ` Ævar Arnfjörð Bjarmason
2018-06-02 11:50 ` [PATCH v6 " Ævar Arnfjörð Bjarmason
` (8 more replies)
2018-06-01 21:10 ` [PATCH v5 1/8] checkout tests: index should be clean after dwim checkout Ævar Arnfjörð Bjarmason
` (7 subsequent siblings)
8 siblings, 9 replies; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-06-01 21:10 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer,
Eric Sunshine, Ævar Arnfjörð Bjarmason
This v5 should address all the comments to v4. Thanks all! It's one
patch less because the struct isn't being moved around anymore.
tbdiff:
1: 16d656ee3b ! 1: ab4529d9f5 checkout tests: index should be clean after dwim checkout
@@ -29,6 +29,10 @@
"checkout", that's being done with "-uno" because there's going to be
some untracked files related to the test itself which we don't care
about.
+
+ In all these tests (DWIM or otherwise) we start with a clean index, so
+ these tests are asserting that that's still the case after the
+ "checkout", failed or otherwise.
Then if we ever run into this sort of regression, either in the
existing code or with a new feature, we'll know.
@@ -65,12 +69,8 @@
test_must_fail git checkout foo &&
+ status_uno_is_clean &&
test_must_fail git rev-parse --verify refs/heads/foo &&
-- test_branch master
-+ test_branch master &&
-+ status_uno_is_clean
- '
-
- test_expect_success 'checkout of branch from a single remote succeeds #1' '
+ test_branch master
+ '
@@
test_might_fail git branch -D bar &&
2: 159cc0634b = 2: c8bbece403 checkout.h: wrap the arguments to unique_tracking_name()
3: 3df4594e2d < -: ------- checkout.[ch]: move struct declaration into *.h
4: 35c6481208 < -: ------- checkout.[ch]: introduce an *_INIT macro
-: ------- > 3: 4fc5ab27fa checkout.[ch]: introduce an *_INIT macro
5: 69a834f010 ! 4: fbce6df584 checkout.[ch]: change "unique" member to "num_matches"
@@ -11,6 +11,19 @@
diff --git a/checkout.c b/checkout.c
--- a/checkout.c
+++ b/checkout.c
+@@
+ /* const */ char *src_ref;
+ char *dst_ref;
+ struct object_id *dst_oid;
+- int unique;
++ int num_matches;
+ };
+
+-#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 1 }
++#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 0 }
+
+ static int check_tracking_name(struct remote *remote, void *cb_data)
+ {
@@
free(query.dst);
return 0;
@@ -31,20 +44,3 @@
return cb_data.dst_ref;
free(cb_data.dst_ref);
return NULL;
-
-diff --git a/checkout.h b/checkout.h
---- a/checkout.h
-+++ b/checkout.h
-@@
- /* const */ char *src_ref;
- char *dst_ref;
- struct object_id *dst_oid;
-- int unique;
-+ int num_matches;
- };
-
--#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 1 }
-+#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 0 }
-
- /*
- * Check if the branch name uniquely matches a branch name on a remote
6: 13547824dc ! 5: 6e016d43d7 checkout: pass the "num_matches" up to callers
@@ -11,16 +11,6 @@
diff --git a/builtin/checkout.c b/builtin/checkout.c
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
-@@
- }
-
- static int checkout_paths(const struct checkout_opts *opts,
-- const char *revision)
-+ const char *revision,
-+ int *dwim_remotes_matched)
- {
- int pos;
- struct checkout state = CHECKOUT_INIT;
@@
int dwim_new_local_branch_ok,
struct branch_info *new_branch_info,
@@ -59,16 +49,6 @@
argv += n;
argc -= n;
}
-@@
-
- UNLEAK(opts);
- if (opts.patch_mode || opts.pathspec.nr)
-- return checkout_paths(&opts, new_branch_info.name);
-+ return checkout_paths(&opts, new_branch_info.name,
-+ &dwim_remotes_matched);
- else
- return checkout_branch(&opts, &new_branch_info);
- }
diff --git a/builtin/worktree.c b/builtin/worktree.c
--- a/builtin/worktree.c
7: 6895b5c903 ! 6: 07b11b133d builtin/checkout.c: use "ret" variable for return
@@ -16,12 +16,10 @@
UNLEAK(opts);
- if (opts.patch_mode || opts.pathspec.nr)
-- return checkout_paths(&opts, new_branch_info.name,
-- &dwim_remotes_matched);
+- return checkout_paths(&opts, new_branch_info.name);
- else
+ if (opts.patch_mode || opts.pathspec.nr) {
-+ int ret = checkout_paths(&opts, new_branch_info.name,
-+ &dwim_remotes_matched);
++ int ret = checkout_paths(&opts, new_branch_info.name);
+ return ret;
+ } else {
return checkout_branch(&opts, &new_branch_info);
8: 5cfc0896e5 ! 7: 97e84f6e1c checkout: add advice for ambiguous "checkout <branch>"
@@ -8,9 +8,9 @@
exactly one remote (call it <remote>) with a matching name, treat
as equivalent to [...] <remote>/<branch.
- This is a really useful feature, the problem is that when you another
- remote (e.g. a fork) git won't find a unique branch name anymore, and
- will instead print this nondescript message:
+ This is a really useful feature. The problem is that when you and
+ another remote (e.g. a fork) git won't find a unique branch name
+ anymore, and will instead print this nondescript message:
$ git checkout master
error: pathspec 'master' did not match any file(s) known to git
@@ -23,8 +23,10 @@
hint: We found 26 remotes with a reference that matched. So we fell back
hint: on trying to resolve the argument as a path, but failed there too!
hint:
- hint: Perhaps you meant fully qualify the branch name? E.g. origin/<name>
- hint: instead of <name>?
+ hint: If you meant to check out a remote tracking branch on e.g. 'origin'
+ hint: you can do so by fully-qualifying the name with the --track option:
+ hint:
+ hint: git checkout --track origin/<name>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
@@ -90,17 +92,19 @@
static const char * const checkout_usage[] = {
N_("git checkout [<options>] <branch>"),
@@
+ UNLEAK(opts);
if (opts.patch_mode || opts.pathspec.nr) {
- int ret = checkout_paths(&opts, new_branch_info.name,
- &dwim_remotes_matched);
+ int ret = checkout_paths(&opts, new_branch_info.name);
+ if (ret && dwim_remotes_matched > 1 &&
+ advice_checkout_ambiguous_remote_branch_name)
+ advise(_("The argument '%s' matched more than one remote tracking branch.\n"
+ "We found %d remotes with a reference that matched. So we fell back\n"
+ "on trying to resolve the argument as a path, but failed there too!\n"
+ "\n"
-+ "Perhaps you meant fully qualify the branch name? E.g. origin/<name>\n"
-+ "instead of <name>?"),
++ "If you meant to check out a remote tracking branch on e.g. 'origin'\n"
++ "you can do so by fully-qualifying the name with the --track option:\n"
++ "\n"
++ " git checkout --track origin/<name>"),
+ argv[0],
+ dwim_remotes_matched);
return ret;
@@ -111,7 +115,7 @@
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@
- status_uno_is_clean
+ test_branch master
'
+test_expect_success 'checkout of branch from multiple remotes fails with advice' '
9: fad1df1edd ! 8: a5cc070ebf checkout & worktree: introduce checkout.defaultRemote
@@ -57,12 +57,14 @@
hint: We found 26 remotes with a reference that matched. So we fell back
hint: on trying to resolve the argument as a path, but failed there too!
hint:
- hint: Perhaps you meant fully qualify the branch name? E.g. origin/<name>
- hint: instead of <name>?
+ hint: If you meant to check out a remote tracking branch on e.g. 'origin'
+ hint: you can do so by fully-qualifying the name with the --track option:
hint:
- hint: If you'd like to always have checkouts of 'master' prefer one remote,
- hint: e.g. the 'origin' remote, consider setting checkout.defaultRemote=origin
- hint: in your config. See the 'git-config' manual page for details.
+ hint: git checkout --track origin/<name>
+ hint:
+ hint: If you'd like to always have checkouts of an ambiguous <name> prefer
+ hint: one remote, e.g. the 'origin' remote, consider setting
+ hint: checkout.defaultRemote=origin in your config.
I considered splitting this into checkout.defaultRemote and
worktree.defaultRemote, but it's probably less confusing to break our
@@ -128,7 +130,7 @@
+one for the purposes of disambiguation, even if the `<branch>` isn't
+unique across all remotes. Set it to
+e.g. `checkout.defaultRemote=origin` to always checkout remote
-+branches from there if `<branch> is ambiguous but exists on the
++branches from there if `<branch>` is ambiguous but exists on the
+'origin' remote. See also `checkout.defaultRemote` in
+linkgit:git-config[1].
++
@@ -148,7 +150,7 @@
+one for the purposes of disambiguation, even if the `<branch>` isn't
+unique across all remotes. Set it to
+e.g. `checkout.defaultRemote=origin` to always checkout remote
-+branches from there if `<branch> is ambiguous but exists on the
++branches from there if `<branch>` is ambiguous but exists on the
+'origin' remote. See also `checkout.defaultRemote` in
+linkgit:git-config[1].
++
@@ -173,22 +175,18 @@
* (c) Otherwise, if "--" is present, treat it like case (1).
*
@@
- "on trying to resolve the argument as a path, but failed there too!\n"
+ "If you meant to check out a remote tracking branch on e.g. 'origin'\n"
+ "you can do so by fully-qualifying the name with the --track option:\n"
"\n"
- "Perhaps you meant fully qualify the branch name? E.g. origin/<name>\n"
-- "instead of <name>?"),
-+ "instead of <name>?\n"
+- " git checkout --track origin/<name>"),
++ " git checkout --track origin/<name>\n"
+ "\n"
-+ "If you'd like to always have checkouts of '%s' prefer one remote,\n"
-+ "e.g. the 'origin' remote, consider setting checkout.defaultRemote=origin\n"
-+ "in your config. See the 'git-config' manual page for details."),
++ "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
++ "one remote, e.g. the 'origin' remote, consider setting\n"
++ "checkout.defaultRemote=origin in your config."),
argv[0],
-- dwim_remotes_matched);
-+ dwim_remotes_matched,
-+ argv[0]);
+ dwim_remotes_matched);
return ret;
- } else {
- return checkout_branch(&opts, &new_branch_info);
diff --git a/checkout.c b/checkout.c
--- a/checkout.c
@@ -198,6 +196,19 @@
#include "refspec.h"
#include "checkout.h"
+#include "config.h"
+
+ struct tracking_name_data {
+ /* const */ char *src_ref;
+ char *dst_ref;
+ struct object_id *dst_oid;
+ int num_matches;
++ const char *default_remote;
++ char *default_dst_ref;
++ struct object_id *default_dst_oid;
+ };
+
+-#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 0 }
++#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 0, NULL, NULL, NULL }
static int check_tracking_name(struct remote *remote, void *cb_data)
{
@@ -243,31 +254,21 @@
return NULL;
}
-diff --git a/checkout.h b/checkout.h
---- a/checkout.h
-+++ b/checkout.h
-@@
- char *dst_ref;
- struct object_id *dst_oid;
- int num_matches;
-+ const char *default_remote;
-+ char *default_dst_ref;
-+ struct object_id *default_dst_oid;
- };
-
--#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 0 }
-+#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 0, NULL, NULL, NULL }
-
- /*
- * Check if the branch name uniquely matches a branch name on a remote
-
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@
- test_i18ngrep ! "^hint: " stderr
- '
-
+ checkout foo 2>stderr &&
+ test_branch master &&
+ status_uno_is_clean &&
+- test_i18ngrep ! "^hint: " stderr
++ test_i18ngrep ! "^hint: " stderr &&
++ # Make sure the likes of checkout -p don not print this hint
++ git checkout -p foo 2>stderr &&
++ test_i18ngrep ! "^hint: " stderr &&
++ status_uno_is_clean
++'
++
+test_expect_success 'checkout of branch from multiple remotes succeeds with checkout.defaultRemote #1' '
+ git checkout -B master &&
+ status_uno_is_clean &&
@@ -278,11 +279,9 @@
+ test_branch foo &&
+ test_cmp_rev remotes/repo_a/foo HEAD &&
+ test_branch_upstream foo repo_a foo
-+'
-+
+ '
+
test_expect_success 'checkout of branch from a single remote succeeds #1' '
- git checkout -B master &&
- test_might_fail git branch -D bar &&
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
--- a/t/t2025-worktree-add.sh
Ævar Arnfjörð Bjarmason (8):
checkout tests: index should be clean after dwim checkout
checkout.h: wrap the arguments to unique_tracking_name()
checkout.[ch]: introduce an *_INIT macro
checkout.[ch]: change "unique" member to "num_matches"
checkout: pass the "num_matches" up to callers
builtin/checkout.c: use "ret" variable for return
checkout: add advice for ambiguous "checkout <branch>"
checkout & worktree: introduce checkout.defaultRemote
Documentation/config.txt | 26 +++++++++++++++
Documentation/git-checkout.txt | 9 ++++++
Documentation/git-worktree.txt | 9 ++++++
advice.c | 2 ++
advice.h | 1 +
builtin/checkout.c | 41 ++++++++++++++++++-----
builtin/worktree.c | 4 +--
checkout.c | 37 ++++++++++++++++++---
checkout.h | 4 ++-
t/t2024-checkout-dwim.sh | 59 ++++++++++++++++++++++++++++++++++
t/t2025-worktree-add.sh | 21 ++++++++++++
11 files changed, 197 insertions(+), 16 deletions(-)
--
2.17.0.290.gded63e768a
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH v6 0/8] ambiguous checkout UI & checkout.defaultRemote
2018-06-01 21:10 ` [PATCH v5 0/8] " Ævar Arnfjörð Bjarmason
@ 2018-06-02 11:50 ` Ævar Arnfjörð Bjarmason
2018-06-05 14:40 ` [PATCH v7 " Ævar Arnfjörð Bjarmason
` (8 more replies)
2018-06-02 11:50 ` [PATCH v6 1/8] checkout tests: index should be clean after dwim checkout Ævar Arnfjörð Bjarmason
` (7 subsequent siblings)
8 siblings, 9 replies; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-06-02 11:50 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer,
Eric Sunshine, Ævar Arnfjörð Bjarmason
Typo & grammar fixes suggested by Eric Sunshine. tbdiff from v5:
1: ab4529d9f5 = 1: ab4529d9f5 checkout tests: index should be clean after dwim checkout
2: c8bbece403 = 2: c8bbece403 checkout.h: wrap the arguments to unique_tracking_name()
3: 4fc5ab27fa ! 3: 881fe63f4f checkout.c: introduce an *_INIT macro
@@ -1,6 +1,6 @@
Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- checkout.[ch]: introduce an *_INIT macro
+ checkout.c: introduce an *_INIT macro
Add an *_INIT macro for the tracking_name_data similar to what exists
elsewhere in the codebase, e.g. OID_ARRAY_INIT in sha1-array.h. This
4: fbce6df584 ! 4: 72ddaeddd3 checkout.c]: change "unique" member to "num_matches"
@@ -1,6 +1,6 @@
Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- checkout.[ch]: change "unique" member to "num_matches"
+ checkout.c]: change "unique" member to "num_matches"
Internally track how many matches we find in the check_tracking_name()
callback. Nothing uses this now, but it will be made use of in a later
5: 6e016d43d7 = 5: 5e8c82680b checkout: pass the "num_matches" up to callers
6: 07b11b133d = 6: 07e667f80a builtin/checkout.c: use "ret" variable for return
7: 97e84f6e1c ! 7: 0a148182e6 checkout: add advice for ambiguous "checkout <branch>"
@@ -8,9 +8,9 @@
exactly one remote (call it <remote>) with a matching name, treat
as equivalent to [...] <remote>/<branch.
- This is a really useful feature. The problem is that when you and
- another remote (e.g. a fork) git won't find a unique branch name
- anymore, and will instead print this nondescript message:
+ This is a really useful feature. The problem is that when you add
+ another remote (e.g. a fork), git won't find a unique branch name
+ anymore, and will instead print this unhelpful message:
$ git checkout master
error: pathspec 'master' did not match any file(s) known to git
@@ -19,12 +19,12 @@
$ ./git --exec-path=$PWD checkout master
error: pathspec 'master' did not match any file(s) known to git.
- hint: The argument 'master' matched more than one remote tracking branch.
+ hint: 'master' matched more than one remote tracking branch.
hint: We found 26 remotes with a reference that matched. So we fell back
hint: on trying to resolve the argument as a path, but failed there too!
hint:
- hint: If you meant to check out a remote tracking branch on e.g. 'origin'
- hint: you can do so by fully-qualifying the name with the --track option:
+ hint: If you meant to check out a remote tracking branch on, e.g. 'origin',
+ hint: you can do so by fully qualifying the name with the --track option:
hint:
hint: git checkout --track origin/<name>
@@ -97,12 +97,12 @@
int ret = checkout_paths(&opts, new_branch_info.name);
+ if (ret && dwim_remotes_matched > 1 &&
+ advice_checkout_ambiguous_remote_branch_name)
-+ advise(_("The argument '%s' matched more than one remote tracking branch.\n"
++ advise(_("'%s' matched more than one remote tracking branch.\n"
+ "We found %d remotes with a reference that matched. So we fell back\n"
+ "on trying to resolve the argument as a path, but failed there too!\n"
+ "\n"
-+ "If you meant to check out a remote tracking branch on e.g. 'origin'\n"
-+ "you can do so by fully-qualifying the name with the --track option:\n"
++ "If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
++ "you can do so by fully qualifying the name with the --track option:\n"
+ "\n"
+ " git checkout --track origin/<name>"),
+ argv[0],
8: a5cc070ebf ! 8: f3a52a26a2 checkout & worktree: introduce checkout.defaultRemote
@@ -175,8 +175,8 @@
* (c) Otherwise, if "--" is present, treat it like case (1).
*
@@
- "If you meant to check out a remote tracking branch on e.g. 'origin'\n"
- "you can do so by fully-qualifying the name with the --track option:\n"
+ "If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
+ "you can do so by fully qualifying the name with the --track option:\n"
"\n"
- " git checkout --track origin/<name>"),
+ " git checkout --track origin/<name>\n"
Ævar Arnfjörð Bjarmason (8):
checkout tests: index should be clean after dwim checkout
checkout.h: wrap the arguments to unique_tracking_name()
checkout.c: introduce an *_INIT macro
checkout.c]: change "unique" member to "num_matches"
checkout: pass the "num_matches" up to callers
builtin/checkout.c: use "ret" variable for return
checkout: add advice for ambiguous "checkout <branch>"
checkout & worktree: introduce checkout.defaultRemote
Documentation/config.txt | 26 +++++++++++++++
Documentation/git-checkout.txt | 9 ++++++
Documentation/git-worktree.txt | 9 ++++++
advice.c | 2 ++
advice.h | 1 +
builtin/checkout.c | 41 ++++++++++++++++++-----
builtin/worktree.c | 4 +--
checkout.c | 37 ++++++++++++++++++---
checkout.h | 4 ++-
t/t2024-checkout-dwim.sh | 59 ++++++++++++++++++++++++++++++++++
t/t2025-worktree-add.sh | 21 ++++++++++++
11 files changed, 197 insertions(+), 16 deletions(-)
--
2.17.0.290.gded63e768a
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH v7 0/8] ambiguous checkout UI & checkout.defaultRemote
2018-06-02 11:50 ` [PATCH v6 " Ævar Arnfjörð Bjarmason
@ 2018-06-05 14:40 ` Ævar Arnfjörð Bjarmason
2018-06-05 14:40 ` [PATCH v7 1/8] checkout tests: index should be clean after dwim checkout Ævar Arnfjörð Bjarmason
` (7 subsequent siblings)
8 siblings, 0 replies; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-06-05 14:40 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer,
Eric Sunshine, Ævar Arnfjörð Bjarmason
Fixes issues noted with v6, hopefully ready for queuing. A tbdiff with
v6:
1: ab4529d9f5 = 1: 2ca81c76fc checkout tests: index should be clean after dwim checkout
2: c8bbece403 = 2: 19b14a1c75 checkout.h: wrap the arguments to unique_tracking_name()
3: 881fe63f4f = 3: 8bc6a9c052 checkout.c: introduce an *_INIT macro
4: 72ddaeddd3 ! 4: 34f3b67f9b checkout.c: change "unique" member to "num_matches"
@@ -1,6 +1,6 @@
Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- checkout.c]: change "unique" member to "num_matches"
+ checkout.c: change "unique" member to "num_matches"
Internally track how many matches we find in the check_tracking_name()
callback. Nothing uses this now, but it will be made use of in a later
5: 5e8c82680b = 5: 7d81c06a23 checkout: pass the "num_matches" up to callers
6: 07e667f80a = 6: e86636ad2c builtin/checkout.c: use "ret" variable for return
7: 0a148182e6 ! 7: c2130b347c checkout: add advice for ambiguous "checkout <branch>"
@@ -27,6 +27,28 @@
hint: you can do so by fully qualifying the name with the --track option:
hint:
hint: git checkout --track origin/<name>
+
+ Note that the "error: pathspec[...]" message is still printed. This is
+ because whatever else checkout may have tried earlier, its final
+ fallback is to try to resolve the argument as a path. E.g. in this
+ case:
+
+ $ ./git --exec-path=$PWD checkout master pu
+ error: pathspec 'master' did not match any file(s) known to git.
+ error: pathspec 'pu' did not match any file(s) known to git.
+
+ There we don't print the "hint:" implicitly due to earlier logic
+ around the DWIM fallback. That fallback is only used if it looks like
+ we have one argument that might be a branch.
+
+ I can't think of an intrinsic reason for why we couldn't in some
+ future change skip printing the "error: pathspec[...]" error. However,
+ to do so we'd need to pass something down to checkout_paths() to make
+ it suppress printing an error on its own, and for us to be confident
+ that we're not silencing cases where those errors are meaningful.
+
+ I don't think that's worth it since determining whether that's the
+ case could easily change due to future changes in the checkout logic.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
8: f3a52a26a2 ! 8: f1ac0f7351 checkout & worktree: introduce checkout.defaultRemote
@@ -53,12 +53,12 @@
$ ./git --exec-path=$PWD checkout master
error: pathspec 'master' did not match any file(s) known to git.
- hint: The argument 'master' matched more than one remote tracking branch.
+ hint: 'master' matched more than one remote tracking branch.
hint: We found 26 remotes with a reference that matched. So we fell back
hint: on trying to resolve the argument as a path, but failed there too!
hint:
- hint: If you meant to check out a remote tracking branch on e.g. 'origin'
- hint: you can do so by fully-qualifying the name with the --track option:
+ hint: If you meant to check out a remote tracking branch on, e.g. 'origin',
+ hint: you can do so by fully qualifying the name with the --track option:
hint:
hint: git checkout --track origin/<name>
hint:
@@ -263,7 +263,7 @@
status_uno_is_clean &&
- test_i18ngrep ! "^hint: " stderr
+ test_i18ngrep ! "^hint: " stderr &&
-+ # Make sure the likes of checkout -p don not print this hint
++ # Make sure the likes of checkout -p do not print this hint
+ git checkout -p foo 2>stderr &&
+ test_i18ngrep ! "^hint: " stderr &&
+ status_uno_is_clean
Ævar Arnfjörð Bjarmason (8):
checkout tests: index should be clean after dwim checkout
checkout.h: wrap the arguments to unique_tracking_name()
checkout.c: introduce an *_INIT macro
checkout.c: change "unique" member to "num_matches"
checkout: pass the "num_matches" up to callers
builtin/checkout.c: use "ret" variable for return
checkout: add advice for ambiguous "checkout <branch>"
checkout & worktree: introduce checkout.defaultRemote
Documentation/config.txt | 26 +++++++++++++++
Documentation/git-checkout.txt | 9 ++++++
Documentation/git-worktree.txt | 9 ++++++
advice.c | 2 ++
advice.h | 1 +
builtin/checkout.c | 41 ++++++++++++++++++-----
builtin/worktree.c | 4 +--
checkout.c | 37 ++++++++++++++++++---
checkout.h | 4 ++-
t/t2024-checkout-dwim.sh | 59 ++++++++++++++++++++++++++++++++++
t/t2025-worktree-add.sh | 21 ++++++++++++
11 files changed, 197 insertions(+), 16 deletions(-)
--
2.17.0.290.gded63e768a
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH v7 1/8] checkout tests: index should be clean after dwim checkout
2018-06-02 11:50 ` [PATCH v6 " Ævar Arnfjörð Bjarmason
2018-06-05 14:40 ` [PATCH v7 " Ævar Arnfjörð Bjarmason
@ 2018-06-05 14:40 ` Ævar Arnfjörð Bjarmason
2018-06-05 15:45 ` SZEDER Gábor
2018-06-05 14:40 ` [PATCH v7 2/8] checkout.h: wrap the arguments to unique_tracking_name() Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
8 siblings, 1 reply; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-06-05 14:40 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer,
Eric Sunshine, Ævar Arnfjörð Bjarmason
Assert that whenever there's a DWIM checkout that the index should be
clean afterwards, in addition to the correct branch being checked-out.
The way the DWIM checkout code in checkout.[ch] works is by looping
over all remotes, and for each remote trying to find if a given
reference name only exists on that remote, or if it exists anywhere
else.
This is done by starting out with a `unique = 1` tracking variable in
a struct shared by the entire loop, which will get set to `0` if the
data reference is not unique.
Thus if we find a match we know the dst_oid member of
tracking_name_data must be correct, since it's associated with the
only reference on the only remote that could have matched our query.
But if there was ever a mismatch there for some reason we might end up
with the correct branch checked out, but at the wrong oid, which would
show whatever the difference between the two staged in the
index (checkout branch A, stage changes from the state of branch B).
So let's amend the tests (mostly added in) 399e4a1c56 ("t2024: Add
tests verifying current DWIM behavior of 'git checkout <branch>'",
2013-04-21) to always assert that "status" is clean after we run
"checkout", that's being done with "-uno" because there's going to be
some untracked files related to the test itself which we don't care
about.
In all these tests (DWIM or otherwise) we start with a clean index, so
these tests are asserting that that's still the case after the
"checkout", failed or otherwise.
Then if we ever run into this sort of regression, either in the
existing code or with a new feature, we'll know.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/t2024-checkout-dwim.sh | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index 3e5ac81bd2..ed32828105 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -23,6 +23,12 @@ test_branch_upstream () {
test_cmp expect.upstream actual.upstream
}
+status_uno_is_clean() {
+ >status.expect &&
+ git status -uno --porcelain >status.actual &&
+ test_cmp status.expect status.actual
+}
+
test_expect_success 'setup' '
test_commit my_master &&
git init repo_a &&
@@ -55,6 +61,7 @@ test_expect_success 'checkout of non-existing branch fails' '
test_might_fail git branch -D xyzzy &&
test_must_fail git checkout xyzzy &&
+ status_uno_is_clean &&
test_must_fail git rev-parse --verify refs/heads/xyzzy &&
test_branch master
'
@@ -64,6 +71,7 @@ test_expect_success 'checkout of branch from multiple remotes fails #1' '
test_might_fail git branch -D foo &&
test_must_fail git checkout foo &&
+ status_uno_is_clean &&
test_must_fail git rev-parse --verify refs/heads/foo &&
test_branch master
'
@@ -73,6 +81,7 @@ test_expect_success 'checkout of branch from a single remote succeeds #1' '
test_might_fail git branch -D bar &&
git checkout bar &&
+ status_uno_is_clean &&
test_branch bar &&
test_cmp_rev remotes/repo_a/bar HEAD &&
test_branch_upstream bar repo_a bar
@@ -83,6 +92,7 @@ test_expect_success 'checkout of branch from a single remote succeeds #2' '
test_might_fail git branch -D baz &&
git checkout baz &&
+ status_uno_is_clean &&
test_branch baz &&
test_cmp_rev remotes/other_b/baz HEAD &&
test_branch_upstream baz repo_b baz
@@ -90,6 +100,7 @@ test_expect_success 'checkout of branch from a single remote succeeds #2' '
test_expect_success '--no-guess suppresses branch auto-vivification' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D bar &&
test_must_fail git checkout --no-guess bar &&
@@ -99,6 +110,7 @@ test_expect_success '--no-guess suppresses branch auto-vivification' '
test_expect_success 'setup more remotes with unconventional refspecs' '
git checkout -B master &&
+ status_uno_is_clean &&
git init repo_c &&
(
cd repo_c &&
@@ -128,27 +140,33 @@ test_expect_success 'setup more remotes with unconventional refspecs' '
test_expect_success 'checkout of branch from multiple remotes fails #2' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D bar &&
test_must_fail git checkout bar &&
+ status_uno_is_clean &&
test_must_fail git rev-parse --verify refs/heads/bar &&
test_branch master
'
test_expect_success 'checkout of branch from multiple remotes fails #3' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D baz &&
test_must_fail git checkout baz &&
+ status_uno_is_clean &&
test_must_fail git rev-parse --verify refs/heads/baz &&
test_branch master
'
test_expect_success 'checkout of branch from a single remote succeeds #3' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D spam &&
git checkout spam &&
+ status_uno_is_clean &&
test_branch spam &&
test_cmp_rev refs/remotes/extra_dir/repo_c/extra_dir/spam HEAD &&
test_branch_upstream spam repo_c spam
@@ -156,9 +174,11 @@ test_expect_success 'checkout of branch from a single remote succeeds #3' '
test_expect_success 'checkout of branch from a single remote succeeds #4' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D eggs &&
git checkout eggs &&
+ status_uno_is_clean &&
test_branch eggs &&
test_cmp_rev refs/repo_d/eggs HEAD &&
test_branch_upstream eggs repo_d eggs
@@ -166,32 +186,38 @@ test_expect_success 'checkout of branch from a single remote succeeds #4' '
test_expect_success 'checkout of branch with a file having the same name fails' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D spam &&
>spam &&
test_must_fail git checkout spam &&
+ status_uno_is_clean &&
test_must_fail git rev-parse --verify refs/heads/spam &&
test_branch master
'
test_expect_success 'checkout of branch with a file in subdir having the same name fails' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D spam &&
>spam &&
mkdir sub &&
mv spam sub/spam &&
test_must_fail git -C sub checkout spam &&
+ status_uno_is_clean &&
test_must_fail git rev-parse --verify refs/heads/spam &&
test_branch master
'
test_expect_success 'checkout <branch> -- succeeds, even if a file with the same name exists' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D spam &&
>spam &&
git checkout spam -- &&
+ status_uno_is_clean &&
test_branch spam &&
test_cmp_rev refs/remotes/extra_dir/repo_c/extra_dir/spam HEAD &&
test_branch_upstream spam repo_c spam
@@ -200,6 +226,7 @@ test_expect_success 'checkout <branch> -- succeeds, even if a file with the same
test_expect_success 'loosely defined local base branch is reported correctly' '
git checkout master &&
+ status_uno_is_clean &&
git branch strict &&
git branch loose &&
git commit --allow-empty -m "a bit more" &&
@@ -210,7 +237,9 @@ test_expect_success 'loosely defined local base branch is reported correctly' '
test_config branch.loose.merge master &&
git checkout strict | sed -e "s/strict/BRANCHNAME/g" >expect &&
+ status_uno_is_clean &&
git checkout loose | sed -e "s/loose/BRANCHNAME/g" >actual &&
+ status_uno_is_clean &&
test_cmp expect actual
'
--
2.17.0.290.gded63e768a
^ permalink raw reply related [flat|nested] 95+ messages in thread
* Re: [PATCH v7 1/8] checkout tests: index should be clean after dwim checkout
2018-06-05 14:40 ` [PATCH v7 1/8] checkout tests: index should be clean after dwim checkout Ævar Arnfjörð Bjarmason
@ 2018-06-05 15:45 ` SZEDER Gábor
2018-07-27 17:48 ` [PATCH] tests: make use of the test_must_be_empty function Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 95+ messages in thread
From: SZEDER Gábor @ 2018-06-05 15:45 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: SZEDER Gábor, git, Junio C Hamano, Jeff King,
Johannes Schindelin, Nguyễn Thái Ngọc Duy,
Thomas Gummerer, Eric Sunshine
> diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
> index 3e5ac81bd2..ed32828105 100755
> --- a/t/t2024-checkout-dwim.sh
> +++ b/t/t2024-checkout-dwim.sh
> @@ -23,6 +23,12 @@ test_branch_upstream () {
> test_cmp expect.upstream actual.upstream
> }
>
> +status_uno_is_clean() {
> + >status.expect &&
> + git status -uno --porcelain >status.actual &&
> + test_cmp status.expect status.actual
This function could be written a tad simpler as:
git status -uno --porcelain >status.actual &&
test_must_be_empty status.actual
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH] tests: make use of the test_must_be_empty function
2018-06-05 15:45 ` SZEDER Gábor
@ 2018-07-27 17:48 ` Ævar Arnfjörð Bjarmason
2018-07-27 21:50 ` Junio C Hamano
` (2 more replies)
0 siblings, 3 replies; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-07-27 17:48 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, SZEDER Gábor,
Ævar Arnfjörð Bjarmason
Change various tests that use an idiom of the form:
>expect &&
test_cmp expect actual
To instead use:
test_must_be_empty actual
The test_must_be_empty() wrapper was introduced in ca8d148daf ("test:
test_must_be_empty helper", 2013-06-09). Many of these tests have been
added after that time. This was mostly found with, and manually pruned
from:
git grep '^\s+>.*expect.* &&$' t
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
As promised in
https://public-inbox.org/git/CACBZZX4yG5h5kk4NFQz_NzAweMa+Nh3H-39OHtcH4XWsA6FGpg@mail.gmail.com/
here's a series to refactor various test_cmp invocations to use
test_must_be_empty.
This patch is on top of "next" since one of the things being fixed up
is a test in my in-flight ab/checkout-default-remote series. It also
applies cleanly to "pu", and the only conflicts with "master" are due
to that series of mine.
t/t0008-ignores.sh | 9 ++----
t/t0030-stripspace.sh | 17 +++++-----
t/t0300-credentials.sh | 3 +-
t/t1011-read-tree-sparse-checkout.sh | 3 +-
t/t1306-xdg-files.sh | 6 ++--
t/t1403-show-ref.sh | 46 ++++++++++------------------
t/t1507-rev-parse-upstream.sh | 3 +-
t/t2024-checkout-dwim.sh | 3 +-
t/t2025-worktree-add.sh | 3 +-
t/t2202-add-addremove.sh | 3 +-
t/t3001-ls-files-others-exclude.sh | 15 +++------
t/t3070-wildmatch.sh | 3 +-
t/t3201-branch-contains.sh | 15 +++------
t/t3700-add.sh | 3 +-
t/t3910-mac-os-precompose.sh | 3 +-
t/t4010-diff-pathspec.sh | 3 +-
t/t4015-diff-whitespace.sh | 17 +++++-----
t/t4039-diff-assume-unchanged.sh | 3 +-
t/t4200-rerere.sh | 9 ++----
t/t4202-log.sh | 6 ++--
t/t4210-log-i18n.sh | 6 ++--
t/t5310-pack-bitmaps.sh | 9 ++----
t/t5313-pack-bounds-checks.sh | 3 +-
t/t5505-remote.sh | 6 ++--
t/t5512-ls-remote.sh | 6 ++--
t/t5514-fetch-multiple.sh | 3 +-
t/t5533-push-cas.sh | 6 ++--
t/t5612-clone-refspec.sh | 9 ++----
t/t6000-rev-list-misc.sh | 3 +-
t/t6009-rev-list-parent.sh | 6 ++--
t/t6018-rev-list-glob.sh | 12 +++-----
t/t6019-rev-list-ancestry-path.sh | 3 +-
t/t6022-merge-rename.sh | 3 +-
t/t6060-merge-index.sh | 3 +-
t/t6300-for-each-ref.sh | 3 +-
t/t7004-tag.sh | 22 +++++--------
| 3 +-
t/t7030-verify-tag.sh | 3 +-
t/t7063-status-untracked-cache.sh | 3 +-
t/t7102-reset.sh | 6 ++--
t/t7106-reset-unborn-branch.sh | 9 ++----
t/t7401-submodule-summary.sh | 3 +-
t/t7502-commit.sh | 3 +-
t/t7610-mergetool.sh | 3 +-
t/t7810-grep.sh | 3 +-
t/t9001-send-email.sh | 3 +-
t/t9300-fast-import.sh | 7 ++---
47 files changed, 113 insertions(+), 209 deletions(-)
diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
index c03f155a35..1744cee5e9 100755
--- a/t/t0008-ignores.sh
+++ b/t/t0008-ignores.sh
@@ -807,10 +807,9 @@ test_expect_success 'trailing whitespace is ignored' '
cat >expect <<EOF &&
whitespace/untracked
EOF
- : >err.expect &&
git ls-files -o -X ignore whitespace >actual 2>err &&
test_cmp expect actual &&
- test_cmp err.expect err
+ test_must_be_empty err
'
test_expect_success !MINGW 'quoting allows trailing whitespace' '
@@ -820,10 +819,9 @@ test_expect_success !MINGW 'quoting allows trailing whitespace' '
>whitespace/untracked &&
echo "whitespace/trailing\\ \\ " >ignore &&
echo whitespace/untracked >expect &&
- : >err.expect &&
git ls-files -o -X ignore whitespace >actual 2>err &&
test_cmp expect actual &&
- test_cmp err.expect err
+ test_must_be_empty err
'
test_expect_success !MINGW,!CYGWIN 'correct handling of backslashes' '
@@ -845,10 +843,9 @@ test_expect_success !MINGW,!CYGWIN 'correct handling of backslashes' '
whitespace/trailing 6 \\a\\Z
EOF
echo whitespace/untracked >expect &&
- >err.expect &&
git ls-files -o -X ignore whitespace >actual 2>err &&
test_cmp expect actual &&
- test_cmp err.expect err
+ test_must_be_empty err
'
test_expect_success 'info/exclude trumps core.excludesfile' '
diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
index bbf3e39e3d..b77948c618 100755
--- a/t/t0030-stripspace.sh
+++ b/t/t0030-stripspace.sh
@@ -110,31 +110,30 @@ test_expect_success \
test_expect_success \
'only consecutive blank lines should be completely removed' '
- > expect &&
printf "\n" | git stripspace >actual &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
printf "\n\n\n" | git stripspace >actual &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
printf "$sss\n$sss\n$sss\n" | git stripspace >actual &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
printf "$sss$sss\n$sss\n\n" | git stripspace >actual &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
printf "\n$sss\n$sss$sss\n" | git stripspace >actual &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
printf "$sss$sss$sss$sss\n\n\n" | git stripspace >actual &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
printf "\n$sss$sss$sss$sss\n\n" | git stripspace >actual &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
printf "\n\n$sss$sss$sss$sss\n" | git stripspace >actual &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success \
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index 03bd31e9f2..82eaaea0f4 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -294,8 +294,7 @@ test_expect_success 'helpers can abort the process' '
-c credential.helper="!f() { echo quit=1; }; f" \
-c credential.helper="verbatim foo bar" \
credential fill >stdout &&
- >expect &&
- test_cmp expect stdout
+ test_must_be_empty stdout
'
test_expect_success 'empty helper spec resets helper list' '
diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index 0c6f48f302..ba71b159ba 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -227,12 +227,11 @@ test_expect_success 'index removal and worktree narrowing at the same time' '
'
test_expect_success 'read-tree --reset removes outside worktree' '
- >empty &&
echo init.t >.git/info/sparse-checkout &&
git checkout -f top &&
git reset --hard removed &&
git ls-files sub/added >result &&
- test_cmp empty result
+ test_must_be_empty result
'
test_expect_success 'print errors when failed to update worktree' '
diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
index 8b14ab187c..21e139a313 100755
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -114,11 +114,10 @@ test_expect_success 'Exclusion in a non-XDG global ignore file' '
'
test_expect_success 'Checking XDG ignore file when HOME is unset' '
- >expected &&
(sane_unset HOME &&
git config --unset core.excludesfile &&
git ls-files --exclude-standard --ignored >actual) &&
- test_cmp expected actual
+ test_must_be_empty actual
'
test_expect_success 'Checking attributes in the XDG attributes file' '
@@ -132,10 +131,9 @@ test_expect_success 'Checking attributes in the XDG attributes file' '
'
test_expect_success 'Checking XDG attributes when HOME is unset' '
- >expected &&
(sane_unset HOME &&
git check-attr -a f >actual) &&
- test_cmp expected actual
+ test_must_be_empty actual
'
test_expect_success '$XDG_CONFIG_HOME overrides $HOME/.config/git/attributes' '
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index 30354fd26c..5d955c3bff 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -26,26 +26,22 @@ test_expect_success 'show-ref' '
git show-ref refs/tags/A >actual &&
test_cmp expect actual &&
- >expect &&
-
test_must_fail git show-ref D >actual &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'show-ref -q' '
- >expect &&
-
git show-ref -q A >actual &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
git show-ref -q tags/A >actual &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
git show-ref -q refs/tags/A >actual &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
test_must_fail git show-ref -q D >actual &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'show-ref --verify' '
@@ -54,32 +50,28 @@ test_expect_success 'show-ref --verify' '
git show-ref --verify refs/tags/A >actual &&
test_cmp expect actual &&
- >expect &&
-
test_must_fail git show-ref --verify A >actual &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
test_must_fail git show-ref --verify tags/A >actual &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
test_must_fail git show-ref --verify D >actual &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'show-ref --verify -q' '
- >expect &&
-
git show-ref --verify -q refs/tags/A >actual &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
test_must_fail git show-ref --verify -q A >actual &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
test_must_fail git show-ref --verify -q tags/A >actual &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
test_must_fail git show-ref --verify -q D >actual &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'show-ref -d' '
@@ -113,19 +105,17 @@ test_expect_success 'show-ref -d' '
git show-ref -d --verify refs/heads/master >actual &&
test_cmp expect actual &&
- >expect &&
-
test_must_fail git show-ref -d --verify master >actual &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
test_must_fail git show-ref -d --verify heads/master >actual &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
test_must_fail git show-ref --verify -d A C >actual &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
test_must_fail git show-ref --verify -d tags/A tags/C >actual &&
- test_cmp expect actual
+ test_must_be_empty actual
'
@@ -178,10 +168,8 @@ test_expect_success 'show-ref --verify HEAD' '
git show-ref --verify HEAD >actual &&
test_cmp expect actual &&
- >expect &&
-
git show-ref --verify -q HEAD >actual &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'show-ref --verify with dangling ref' '
diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
index 349f6e10af..fa3e499641 100755
--- a/t/t1507-rev-parse-upstream.sh
+++ b/t/t1507-rev-parse-upstream.sh
@@ -138,8 +138,7 @@ test_expect_success 'branch -d other@{u}' '
git checkout -t -b other master &&
git branch -d @{u} &&
git for-each-ref refs/heads/master >actual &&
- >expect &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'checkout other@{u}' '
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index 26dc3f1fc0..f79dfbbdd6 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -24,9 +24,8 @@ test_branch_upstream () {
}
status_uno_is_clean () {
- >status.expect &&
git status -uno --porcelain >status.actual &&
- test_cmp status.expect status.actual
+ test_must_be_empty status.actual
}
test_expect_success 'setup' '
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index be6e093142..166942c1bd 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -412,9 +412,8 @@ test_expect_success '"add" <path> <branch> dwims with checkout.defaultRemote' '
git fetch repo_upstream2 &&
test_must_fail git worktree add ../foo foo &&
git -c checkout.defaultRemote=repo_upstream worktree add ../foo foo &&
- >status.expect &&
git status -uno --porcelain >status.actual &&
- test_cmp status.expect status.actual
+ test_must_be_empty status.actual
) &&
(
cd foo &&
diff --git a/t/t2202-add-addremove.sh b/t/t2202-add-addremove.sh
index 17744e8c57..9ee659098c 100755
--- a/t/t2202-add-addremove.sh
+++ b/t/t2202-add-addremove.sh
@@ -48,8 +48,7 @@ test_expect_success 'Just "git add" is a no-op' '
>will-not-be-added &&
git add &&
git diff-index --name-status --cached HEAD >actual &&
- >expect &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_done
diff --git a/t/t3001-ls-files-others-exclude.sh b/t/t3001-ls-files-others-exclude.sh
index 3fc484e8c3..3b47647ed5 100755
--- a/t/t3001-ls-files-others-exclude.sh
+++ b/t/t3001-ls-files-others-exclude.sh
@@ -210,8 +210,7 @@ test_expect_success 'subdirectory ignore (toplevel)' '
cd top &&
git ls-files -o --exclude-standard
) >actual &&
- >expect &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'subdirectory ignore (l1/l2)' '
@@ -219,8 +218,7 @@ test_expect_success 'subdirectory ignore (l1/l2)' '
cd top/l1/l2 &&
git ls-files -o --exclude-standard
) >actual &&
- >expect &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'subdirectory ignore (l1)' '
@@ -228,8 +226,7 @@ test_expect_success 'subdirectory ignore (l1)' '
cd top/l1 &&
git ls-files -o --exclude-standard
) >actual &&
- >expect &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'show/hide empty ignored directory (setup)' '
@@ -251,8 +248,7 @@ test_expect_success 'hide empty ignored directory with --no-empty-directory' '
cd top &&
git ls-files -o -i --exclude l1 --directory --no-empty-directory
) >actual &&
- >expect &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'show/hide empty ignored sub-directory (setup)' '
@@ -277,8 +273,7 @@ test_expect_success 'hide empty ignored sub-directory with --no-empty-directory'
cd top &&
git ls-files -o -i --exclude l1 --directory --no-empty-directory
) >actual &&
- >expect &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'pattern matches prefix completely' '
diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh
index dce102130f..46aca0af10 100755
--- a/t/t3070-wildmatch.sh
+++ b/t/t3070-wildmatch.sh
@@ -101,8 +101,7 @@ match_with_ls_files() {
match_stdout_stderr_cmp="
tr -d '\0' <actual.raw >actual &&
- >expect.err &&
- test_cmp expect.err actual.err &&
+ test_must_be_empty actual.err &&
test_cmp expect actual"
if test "$match_expect" = 'E'
diff --git a/t/t3201-branch-contains.sh b/t/t3201-branch-contains.sh
index 0ef1b6fdcc..0ea4fc4694 100755
--- a/t/t3201-branch-contains.sh
+++ b/t/t3201-branch-contains.sh
@@ -48,16 +48,14 @@ test_expect_success 'branch --contains master' '
test_expect_success 'branch --no-contains=master' '
git branch --no-contains=master >actual &&
- >expect &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'branch --no-contains master' '
git branch --no-contains master >actual &&
- >expect &&
- test_cmp expect actual
+ test_must_be_empty actual
'
@@ -94,8 +92,7 @@ test_expect_success 'branch --contains with pattern implies --list' '
test_expect_success 'branch --no-contains with pattern implies --list' '
git branch --no-contains=master master >actual &&
- >expect &&
- test_cmp expect actual
+ test_must_be_empty actual
'
@@ -123,8 +120,7 @@ test_expect_success 'branch --merged with pattern implies --list' '
test_expect_success 'side: branch --no-merged' '
git branch --no-merged >actual &&
- >expect &&
- test_cmp expect actual
+ test_must_be_empty actual
'
@@ -152,8 +148,7 @@ test_expect_success 'master: branch --no-merged' '
test_expect_success 'branch --no-merged with pattern implies --list' '
git branch --no-merged=master master >actual &&
- >expect &&
- test_cmp expect actual
+ test_must_be_empty actual
'
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 618750167a..37729ba258 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -188,9 +188,8 @@ test_expect_success 'git add --refresh with pathspec' '
git add foo bar baz && H=$(git rev-parse :foo) && git rm -f foo &&
echo "100644 $H 3 foo" | git update-index --index-info &&
test-tool chmtime -60 bar baz &&
- >expect &&
git add --refresh bar >actual &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
git diff-files --name-only >actual &&
! grep bar actual&&
diff --git a/t/t3910-mac-os-precompose.sh b/t/t3910-mac-os-precompose.sh
index 26dd5b7f78..54ce19e353 100755
--- a/t/t3910-mac-os-precompose.sh
+++ b/t/t3910-mac-os-precompose.sh
@@ -187,9 +187,8 @@ test_expect_failure 'handle existing decomposed filenames' '
echo content >"verbatim.$Adiarnfd" &&
git -c core.precomposeunicode=false add "verbatim.$Adiarnfd" &&
git commit -m "existing decomposed file" &&
- >expect &&
git ls-files --exclude-standard -o "verbatim*" >untracked &&
- test_cmp expect untracked
+ test_must_be_empty untracked
'
# Test if the global core.precomposeunicode stops autosensing
diff --git a/t/t4010-diff-pathspec.sh b/t/t4010-diff-pathspec.sh
index b7f25071cf..281f8fad0c 100755
--- a/t/t4010-diff-pathspec.sh
+++ b/t/t4010-diff-pathspec.sh
@@ -74,8 +74,7 @@ test_expect_success 'diff-tree pathspec' '
tree2=$(git write-tree) &&
echo "$tree2" &&
git diff-tree -r --name-only $tree $tree2 -- pa path1/a >current &&
- >expected &&
- test_cmp expected current
+ test_must_be_empty current
'
test_expect_success 'diff-tree with wildcard shows dir also matches' '
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 41facf7abf..62ed0232d7 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -93,21 +93,20 @@ test_expect_success 'another test, without options' '
git diff >out &&
test_cmp expect out &&
- >expect &&
git diff -w >out &&
- test_cmp expect out &&
+ test_must_be_empty out &&
git diff -w -b >out &&
- test_cmp expect out &&
+ test_must_be_empty out &&
git diff -w --ignore-space-at-eol >out &&
- test_cmp expect out &&
+ test_must_be_empty out &&
git diff -w -b --ignore-space-at-eol >out &&
- test_cmp expect out &&
+ test_must_be_empty out &&
git diff -w --ignore-cr-at-eol >out &&
- test_cmp expect out &&
+ test_must_be_empty out &&
tr "Q_" "\015 " <<-\EOF >expect &&
diff --git a/x b/x
@@ -182,8 +181,7 @@ test_expect_success 'ignore-blank-lines: only new lines' '
test_seq 5 | sed "/3/i\\
" >x &&
git diff --ignore-blank-lines >out &&
- >expect &&
- test_cmp expect out
+ test_must_be_empty out
'
test_expect_success 'ignore-blank-lines: only new lines with space' '
@@ -192,8 +190,7 @@ test_expect_success 'ignore-blank-lines: only new lines with space' '
test_seq 5 | sed "/3/i\\
" >x &&
git diff -w --ignore-blank-lines >out &&
- >expect &&
- test_cmp expect out
+ test_must_be_empty out
'
test_expect_success 'ignore-blank-lines: after change' '
diff --git a/t/t4039-diff-assume-unchanged.sh b/t/t4039-diff-assume-unchanged.sh
index 23c0e357a7..53ac44b0f0 100755
--- a/t/t4039-diff-assume-unchanged.sh
+++ b/t/t4039-diff-assume-unchanged.sh
@@ -34,9 +34,8 @@ test_expect_success POSIXPERM 'find-copies-harder is not confused by mode bits'
git add exec &&
git commit -m exec &&
git update-index --assume-unchanged exec &&
- >expect &&
git diff-files --find-copies-harder -- exec >actual &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_done
diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index 8417e5a4b1..65da74c766 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -267,8 +267,7 @@ rerere_gc_custom_expiry_test () {
git -c "gc.rerereresolved=$right_now" \
-c "gc.rerereunresolved=$right_now" rerere gc &&
find .git/rr-cache -type f | sort >actual &&
- >expect &&
- test_cmp expect actual
+ test_must_be_empty actual
'
}
@@ -536,9 +535,8 @@ test_expect_success 'multiple identical conflicts' '
# We resolved file1 and file2
git rerere &&
- >expect &&
git rerere remaining >actual &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
# We must have recorded both of them
count_pre_post 2 2 &&
@@ -548,9 +546,8 @@ test_expect_success 'multiple identical conflicts' '
test_must_fail git merge six.1 &&
git rerere &&
- >expect &&
git rerere remaining >actual &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
concat_insert short 6.1 6.2 >file1.expect &&
concat_insert long 6.1 6.2 >file2.expect &&
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 25b1f8cc73..0b1cd3408b 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -340,10 +340,9 @@ test_expect_success PCRE 'log -F -E --perl-regexp --grep=<pcre> uses PCRE' '
'
test_expect_success 'log with grep.patternType configuration' '
- >expect &&
git -c grep.patterntype=fixed \
log -1 --pretty=tformat:%s --grep=s.c.nd >actual &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'log with grep.patternType configuration and command line' '
@@ -1625,9 +1624,8 @@ test_expect_success 'log diagnoses bogus HEAD' '
'
test_expect_success 'log does not default to HEAD when rev input is given' '
- >expect &&
git log --branches=does-not-exist >actual &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'set up --source tests' '
diff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh
index e585fe6129..7c519436ef 100755
--- a/t/t4210-log-i18n.sh
+++ b/t/t4210-log-i18n.sh
@@ -44,15 +44,13 @@ test_expect_success !MINGW 'log --grep searches in log output encoding (latin1)'
'
test_expect_success !MINGW 'log --grep does not find non-reencoded values (utf8)' '
- >expect &&
git log --encoding=utf8 --format=%s --grep=$latin1_e >actual &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'log --grep does not find non-reencoded values (latin1)' '
- >expect &&
git log --encoding=ISO-8859-1 --format=%s --grep=$utf8_e >actual &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_done
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 2d22a17c4a..6ee4d3f2d9 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -309,9 +309,8 @@ test_expect_success 'pack reuse respects --honor-pack-keep' '
done &&
reusable_pack --honor-pack-keep >empty.pack &&
git index-pack empty.pack &&
- >expect &&
git show-index <empty.idx >actual &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'pack reuse respects --local' '
@@ -319,17 +318,15 @@ test_expect_success 'pack reuse respects --local' '
test_when_finished "mv alt.git/objects/pack/* .git/objects/pack/" &&
reusable_pack --local >empty.pack &&
git index-pack empty.pack &&
- >expect &&
git show-index <empty.idx >actual &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'pack reuse respects --incremental' '
reusable_pack --incremental >empty.pack &&
git index-pack empty.pack &&
- >expect &&
git show-index <empty.idx >actual &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'truncated bitmap fails gracefully' '
diff --git a/t/t5313-pack-bounds-checks.sh b/t/t5313-pack-bounds-checks.sh
index 4fe4ad9d61..f1708d415e 100755
--- a/t/t5313-pack-bounds-checks.sh
+++ b/t/t5313-pack-bounds-checks.sh
@@ -90,9 +90,8 @@ test_expect_success 'matched bogus object count' '
# Unlike above, we should notice early that the .idx is totally
# bogus, and not even enumerate its contents.
- >expect &&
git cat-file --batch-all-objects --batch-check >actual &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
# But as before, we can do the same object-access checks.
test_must_fail git cat-file blob $object &&
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 11e14a1e0f..6edec8fed7 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -74,8 +74,7 @@ test_expect_success 'add another remote' '
git for-each-ref "--format=%(refname)" refs/remotes |
sed -e "/^refs\/remotes\/origin\//d" \
-e "/^refs\/remotes\/second\//d" >actual &&
- >expect &&
- test_cmp expect actual
+ test_must_be_empty actual
)
'
@@ -112,8 +111,7 @@ test_expect_success C_LOCALE_OUTPUT 'remove remote' '
check_remote_track origin master side &&
git for-each-ref "--format=%(refname)" refs/remotes |
sed -e "/^refs\/remotes\/origin\//d" >actual &&
- >expect &&
- test_cmp expect actual
+ test_must_be_empty actual
)
'
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index ea020040e8..bc5703ff9b 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -155,14 +155,12 @@ test_expect_success 'die with non-2 for wrong repository even with --exit-code'
test_expect_success 'Report success even when nothing matches' '
git ls-remote other.git "refs/nsn/*" >actual &&
- >expect &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'Report no-match with --exit-code' '
test_expect_code 2 git ls-remote --exit-code other.git "refs/nsn/*" >actual &&
- >expect &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'Report match with --exit-code' '
diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh
index 4b4b6673b8..0030c92e1a 100755
--- a/t/t5514-fetch-multiple.sh
+++ b/t/t5514-fetch-multiple.sh
@@ -152,7 +152,6 @@ test_expect_success 'git fetch --multiple (ignoring skipFetchAll)' '
'
test_expect_success 'git fetch --all --no-tags' '
- >expect &&
git clone one test5 &&
git clone test5 test6 &&
(cd test5 && git tag test-tag) &&
@@ -161,7 +160,7 @@ test_expect_success 'git fetch --all --no-tags' '
git fetch --all --no-tags &&
git tag >output
) &&
- test_cmp expect test6/output
+ test_must_be_empty test6/output
'
test_expect_success 'git fetch --all --tags' '
diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
index d38ecee217..0b0eb1d025 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -142,9 +142,8 @@ test_expect_success 'push to delete (protected, forced)' '
cd dst &&
git push --force --force-with-lease=master:master^ origin :master
) &&
- >expect &&
git ls-remote src refs/heads/master >actual &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'push to delete (allowed)' '
@@ -154,9 +153,8 @@ test_expect_success 'push to delete (allowed)' '
git push --force-with-lease=master origin :master 2>err &&
grep deleted err
) &&
- >expect &&
git ls-remote src refs/heads/master >actual &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'cover everything with default force-with-lease (protected)' '
diff --git a/t/t5612-clone-refspec.sh b/t/t5612-clone-refspec.sh
index fac5a73851..5582b3d5fd 100755
--- a/t/t5612-clone-refspec.sh
+++ b/t/t5612-clone-refspec.sh
@@ -97,8 +97,7 @@ test_expect_success 'clone with --no-tags' '
git fetch &&
git for-each-ref refs/tags >../actual
) &&
- >expect &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success '--single-branch while HEAD pointing at master' '
@@ -140,8 +139,7 @@ test_expect_success '--single-branch while HEAD pointing at master and --no-tags
git fetch &&
git for-each-ref refs/tags >../actual
) &&
- >expect &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
test_line_count = 0 actual &&
# get tags with --tags overrides tagOpt
(
@@ -230,8 +228,7 @@ test_expect_success '--single-branch with detached' '
-e "s|/remotes/origin/|/heads/|" >../actual
) &&
# nothing
- >expect &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_done
diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index 969e4e9e52..fb4d295aa0 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh
@@ -58,8 +58,7 @@ test_expect_success 'rev-list A..B and rev-list ^A B are the same' '
test_expect_success 'propagate uninteresting flag down correctly' '
git rev-list --objects ^HEAD^{tree} HEAD^{tree} >actual &&
- >expect &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'symleft flag bit is propagated down from tag' '
diff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh
index 20e3e2554a..916d9692bc 100755
--- a/t/t6009-rev-list-parent.sh
+++ b/t/t6009-rev-list-parent.sh
@@ -31,8 +31,7 @@ test_expect_success setup '
test_expect_success 'one is ancestor of others and should not be shown' '
git rev-list one --not four >result &&
- >expect &&
- test_cmp expect result
+ test_must_be_empty result
'
@@ -144,8 +143,7 @@ test_expect_success 'ancestors with the same commit time' '
test_commit t$i
done &&
git rev-list t1^! --not t$i >result &&
- >expect &&
- test_cmp expect result
+ test_must_be_empty result
'
test_done
diff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh
index d3453c583c..02936c2f24 100755
--- a/t/t6018-rev-list-glob.sh
+++ b/t/t6018-rev-list-glob.sh
@@ -256,31 +256,27 @@ test_expect_success 'rev-list accumulates multiple --exclude' '
'
test_expect_failure 'rev-list should succeed with empty output on empty stdin' '
- >expect &&
git rev-list --stdin <expect >actual &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'rev-list should succeed with empty output with all refs excluded' '
- >expect &&
git rev-list --exclude=* --all >actual &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'rev-list should succeed with empty output with empty --all' '
(
test_create_repo empty &&
cd empty &&
- >expect &&
git rev-list --all >actual &&
- test_cmp expect actual
+ test_must_be_empty actual
)
'
test_expect_success 'rev-list should succeed with empty output with empty glob' '
- >expect &&
git rev-list --glob=does-not-match-anything >actual &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'shortlog accepts --glob/--tags/--remotes' '
diff --git a/t/t6019-rev-list-ancestry-path.sh b/t/t6019-rev-list-ancestry-path.sh
index dabebaee0b..beadaf6cca 100755
--- a/t/t6019-rev-list-ancestry-path.sh
+++ b/t/t6019-rev-list-ancestry-path.sh
@@ -95,10 +95,9 @@ test_expect_success 'rev-list --ancestry-path F...I' '
# G.t is dropped in an "-s ours" merge
test_expect_success 'rev-list G..M -- G.t' '
- >expect &&
git rev-list --format=%s G..M -- G.t |
sed -e "/^commit /d" >actual &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'rev-list --ancestry-path G..M -- G.t' '
diff --git a/t/t6022-merge-rename.sh b/t/t6022-merge-rename.sh
index b760c223c6..53cc9b2ffb 100755
--- a/t/t6022-merge-rename.sh
+++ b/t/t6022-merge-rename.sh
@@ -893,8 +893,7 @@ test_expect_success 'do not follow renames for empty files' '
git mv empty1 empty2 &&
git commit -m rename &&
test_must_fail git merge empty-base &&
- >expect &&
- test_cmp expect empty2
+ test_must_be_empty empty2
'
test_done
diff --git a/t/t6060-merge-index.sh b/t/t6060-merge-index.sh
index debadbd299..ddf34f0115 100755
--- a/t/t6060-merge-index.sh
+++ b/t/t6060-merge-index.sh
@@ -44,8 +44,7 @@ test_expect_success 'read-tree does not resolve content merge' '
test_expect_success 'git merge-index git-merge-one-file resolves' '
git merge-index git-merge-one-file -a &&
git diff-files --name-only --diff-filter=U >unmerged &&
- >expect &&
- test_cmp expect unmerged &&
+ test_must_be_empty unmerged &&
test_cmp expect-merged file &&
git cat-file blob :file >file-index &&
test_cmp expect-merged file-index
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index e0496da812..024f8c06f7 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -796,9 +796,8 @@ test_expect_success ':remotename and :remoteref' '
'
test_expect_success 'for-each-ref --ignore-case ignores case' '
- >expect &&
git for-each-ref --format="%(refname)" refs/heads/MASTER >actual &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
echo refs/heads/master >expect &&
git for-each-ref --format="%(refname)" --ignore-case \
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index d7b319e919..93a6694f0e 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -693,9 +693,8 @@ test_expect_success \
'
test_expect_success 'The -n 100 invocation means -n --list 100, not -n100' '
- >expect &&
git tag -n 100 >actual &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
git tag -m "A msg" 100 &&
echo "100 A msg" >expect &&
@@ -974,9 +973,8 @@ test_expect_success GPG 'verifying a proper tag with --format pass and format ac
'
test_expect_success GPG 'verifying a forged tag with --format should fail silently' '
- >expect &&
test_must_fail git tag -v --format="tagname : %(tag)" "forged-tag" >actual &&
- test_cmp expect actual
+ test_must_be_empty actual
'
# blank and empty messages for signed tags:
@@ -1382,9 +1380,8 @@ test_expect_success 'message in editor has initial comment: first line' '
test_expect_success \
'message in editor has initial comment: remainder' '
# remove commented lines from the remainder -- should be empty
- >rest.expect &&
sed -e 1d -e "/^#/d" <actual >rest.actual &&
- test_cmp rest.expect rest.actual
+ test_must_be_empty rest.actual
'
get_tag_header reuse $commit commit $time >expect
@@ -1466,19 +1463,18 @@ test_expect_success 'checking that first commit is in all tags (relative)' "
# All the --contains tests above, but with --no-contains
test_expect_success 'checking that first commit is not listed in any tag with --no-contains (hash)' "
- >expected &&
git tag -l --no-contains $hash1 v* >actual &&
- test_cmp expected actual
+ test_must_be_empty actual
"
test_expect_success 'checking that first commit is in all tags (tag)' "
git tag -l --no-contains v1.0 v* >actual &&
- test_cmp expected actual
+ test_must_be_empty actual
"
test_expect_success 'checking that first commit is in all tags (relative)' "
git tag -l --no-contains HEAD~2 v* >actual &&
- test_cmp expected actual
+ test_must_be_empty actual
"
cat > expected <<EOF
@@ -1606,9 +1602,8 @@ test_expect_success 'checking that --contains can be used in non-list mode' '
'
test_expect_success 'checking that initial commit is in all tags with --no-contains' "
- >expected &&
git tag -l --no-contains $hash1 v* >actual &&
- test_cmp expected actual
+ test_must_be_empty actual
"
# mixing modes and options:
@@ -1905,7 +1900,6 @@ test_expect_success 'version sort with very long prerelease suffix' '
'
test_expect_success ULIMIT_STACK_SIZE '--contains and --no-contains work in a deep repo' '
- >expect &&
i=1 &&
while test $i -lt 8000
do
@@ -1920,7 +1914,7 @@ EOF"
git checkout master &&
git tag far-far-away HEAD^ &&
run_with_limited_stack git tag --contains HEAD >actual &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
run_with_limited_stack git tag --no-contains HEAD >actual &&
test_line_count "-gt" 10 actual
'
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 7541ba5edb..00e09a375c 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -626,12 +626,11 @@ test_expect_success TTY 'sub-commands of externals use their own pager' '
test_expect_success TTY 'external command pagers override sub-commands' '
sane_unset PAGER GIT_PAGER &&
- >expect &&
>actual &&
test_config pager.external false &&
test_config pager.log "sed s/^/log:/ >actual" &&
test_terminal git --exec-path=. external log --format=%s -1 &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'command with underscores does not complain' '
diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
index 291a1e2b07..1f068714c5 100755
--- a/t/t7030-verify-tag.sh
+++ b/t/t7030-verify-tag.sh
@@ -134,9 +134,8 @@ test_expect_success GPG 'verifying tag with --format' '
'
test_expect_success GPG 'verifying a forged tag with --format should fail silently' '
- >expect &&
test_must_fail git verify-tag --format="tagname : %(tag)" $(cat forged1.tag) >actual-forged &&
- test_cmp expect actual-forged
+ test_must_be_empty actual-forged
'
test_done
diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
index c61e304e97..6d8256a424 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -26,9 +26,8 @@ avoid_racy() {
}
status_is_clean() {
- >../status.expect &&
git status --porcelain >../status.actual &&
- test_cmp ../status.expect ../status.actual
+ test_must_be_empty ../status.actual
}
test_lazy_prereq UNTRACKED_CACHE '
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 95653a08ca..97be0d968d 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -549,8 +549,7 @@ test_expect_success 'reset -N keeps removed files as intent-to-add' '
tree=$(git write-tree) &&
git ls-tree $tree new-file >actual &&
- >expect &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
git diff --name-only >actual &&
echo new-file >expect &&
@@ -563,9 +562,8 @@ test_expect_success 'reset --mixed sets up work tree' '
cd mixed_worktree &&
test_commit dummy
) &&
- : >expect &&
git --git-dir=mixed_worktree/.git --work-tree=mixed_worktree reset >actual &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_done
diff --git a/t/t7106-reset-unborn-branch.sh b/t/t7106-reset-unborn-branch.sh
index 0f95f00477..ecb85c3b82 100755
--- a/t/t7106-reset-unborn-branch.sh
+++ b/t/t7106-reset-unborn-branch.sh
@@ -12,9 +12,8 @@ test_expect_success 'reset' '
git add a b &&
git reset &&
- >expect &&
git ls-files >actual &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'reset HEAD' '
@@ -39,9 +38,8 @@ test_expect_success PERL 'reset -p' '
echo y >yes &&
git reset -p <yes >output &&
- >expect &&
git ls-files >actual &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
test_i18ngrep "Unstage" output
'
@@ -61,9 +59,8 @@ test_expect_success 'reset --hard' '
test_when_finished "echo a >a" &&
git reset --hard &&
- >expect &&
git ls-files >actual &&
- test_cmp expect actual &&
+ test_must_be_empty actual &&
test_path_is_missing a
'
diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
index 4e4c455502..1cd12b38c5 100755
--- a/t/t7401-submodule-summary.sh
+++ b/t/t7401-submodule-summary.sh
@@ -64,8 +64,7 @@ test_expect_success 'added submodule (subdirectory only)' "
cd sub &&
git submodule summary . >../actual
) &&
- >expected &&
- test_cmp expected actual
+ test_must_be_empty actual
"
test_expect_success 'added submodule (subdirectory with explicit path)' "
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index d33a3cb331..ca4a740da0 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -393,7 +393,6 @@ EOF
test_expect_success !AUTOIDENT 'do not fire editor when committer is bogus' '
>.git/result &&
- >expect &&
echo >>negative &&
(
@@ -403,7 +402,7 @@ test_expect_success !AUTOIDENT 'do not fire editor when committer is bogus' '
export GIT_EDITOR &&
test_must_fail git commit -e -m sample -a
) &&
- test_cmp expect .git/result
+ test_must_be_empty .git/result
'
test_expect_success 'do not fire editor if -m <msg> was given' '
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 047156e9d5..b18503de81 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -620,8 +620,7 @@ test_expect_success 'file with no base' '
git checkout -b test$test_count branch1 &&
test_must_fail git merge master &&
git mergetool --no-prompt --tool mybase -- both &&
- >expected &&
- test_cmp expected both
+ test_must_be_empty both
'
test_expect_success 'custom commands override built-ins' '
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index dcaab1557b..d826e24b45 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -691,8 +691,7 @@ test_expect_success 'log grep (5)' '
test_expect_success 'log grep (6)' '
git log --author=-0700 --pretty=tformat:%s >actual &&
- >expect &&
- test_cmp expect actual
+ test_must_be_empty actual
'
test_expect_success 'log grep (7)' '
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index b8e919e25d..1ef1a19003 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -253,10 +253,9 @@ test_suppress_self () {
mv msgtxt1 msgtxt1-$3 &&
sed -e '/^$/q' msgtxt1-$3 >"msghdr1-$3" &&
- >"expected-no-cc-$3" &&
(grep '^Cc:' msghdr1-$3 >"actual-no-cc-$3";
- test_cmp expected-no-cc-$3 actual-no-cc-$3)
+ test_must_be_empty actual-no-cc-$3)
}
test_suppress_self_unquoted () {
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 9e7f96223d..f8f869e26a 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -2191,12 +2191,11 @@ test_expect_success 'R: --import-marks-if-exists' '
test_expect_success 'R: feature import-marks-if-exists' '
rm -f io.marks &&
- >expect &&
git fast-import --export-marks=io.marks <<-\EOF &&
feature import-marks-if-exists=not_io.marks
EOF
- test_cmp expect io.marks &&
+ test_must_be_empty io.marks &&
blob=$(echo hi | git hash-object --stdin) &&
@@ -2227,13 +2226,11 @@ test_expect_success 'R: feature import-marks-if-exists' '
EOF
test_cmp expect io.marks &&
- >expect &&
-
git fast-import --import-marks-if-exists=not_io.marks \
--export-marks=io.marks <<-\EOF &&
feature import-marks-if-exists=io.marks
EOF
- test_cmp expect io.marks
+ test_must_be_empty io.marks
'
test_expect_success 'R: import to output marks works without any content' '
--
2.18.0.345.g5c9ce644c3
^ permalink raw reply related [flat|nested] 95+ messages in thread
* Re: [PATCH] tests: make use of the test_must_be_empty function
2018-07-27 17:48 ` [PATCH] tests: make use of the test_must_be_empty function Ævar Arnfjörð Bjarmason
@ 2018-07-27 21:50 ` Junio C Hamano
2018-07-31 0:17 ` SZEDER Gábor
2018-08-22 17:48 ` [PATCH] t6018-rev-list-glob: fix 'empty stdin' test SZEDER Gábor
2 siblings, 0 replies; 95+ messages in thread
From: Junio C Hamano @ 2018-07-27 21:50 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, SZEDER Gábor
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Change various tests that use an idiom of the form:
>
> >expect &&
> test_cmp expect actual
>
> To instead use:
>
> test_must_be_empty actual
I trust that you eyeballed the result to make sure steps after these
converted parts do *not* depend on (or expect) the presence of an
empty 'expect' file (e.g. ".gitignore" has 'expect' listed, or the
expected output from "status" has expected in the untracked section)?
I'd imagine that kind of sanity checking the result would have taken
10x more time than the actual conversion itself.
Very much appreciated.
> This patch is on top of "next" since one of the things being fixed up
> is a test in my in-flight ab/checkout-default-remote series. It also
> applies cleanly to "pu", and the only conflicts with "master" are due
> to that series of mine.
A patch that truly depends on all of 'next' won't have a chance to
graduate, but it seems you only need one topic out of it.
If I were you in such a situation I would have prepared two patches,
i.e. one to apply on 'master', and the remainder to apply on tip of
ab/checkout-default-remote, and made sure that applying the former
on 'master' and merging the updated ab/checkout-default-remote would
match the tree you would get by merging ab/checkout-default-remote
to 'master' and applying _this_ patch.
Let's see if I can tease them apart myself first.
Thanks.
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [PATCH] tests: make use of the test_must_be_empty function
2018-07-27 17:48 ` [PATCH] tests: make use of the test_must_be_empty function Ævar Arnfjörð Bjarmason
2018-07-27 21:50 ` Junio C Hamano
@ 2018-07-31 0:17 ` SZEDER Gábor
2018-08-22 17:48 ` [PATCH] t6018-rev-list-glob: fix 'empty stdin' test SZEDER Gábor
2 siblings, 0 replies; 95+ messages in thread
From: SZEDER Gábor @ 2018-07-31 0:17 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Git mailing list, Junio C Hamano
On Fri, Jul 27, 2018 at 7:48 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> Change various tests that use an idiom of the form:
>
> >expect &&
> test_cmp expect actual
>
> To instead use:
>
> test_must_be_empty actual
Thanks for working on this.
> The test_must_be_empty() wrapper was introduced in ca8d148daf ("test:
> test_must_be_empty helper", 2013-06-09). Many of these tests have been
> added after that time. This was mostly found with, and manually pruned
> from:
>
> git grep '^\s+>.*expect.* &&$' t
This command doesn't output anything as it is, it should be 'git grep
-E ...'.
Furthermore, it doesn't catch the cases when the empty expected file
is written as ': > expect'. I see that you noticed and converted a
few of these, too, but running
git grep ': >.*exp.*&&' t/
turns up some more.
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH] t6018-rev-list-glob: fix 'empty stdin' test
2018-07-27 17:48 ` [PATCH] tests: make use of the test_must_be_empty function Ævar Arnfjörð Bjarmason
2018-07-27 21:50 ` Junio C Hamano
2018-07-31 0:17 ` SZEDER Gábor
@ 2018-08-22 17:48 ` SZEDER Gábor
2018-08-22 17:53 ` Eric Sunshine
` (2 more replies)
2 siblings, 3 replies; 95+ messages in thread
From: SZEDER Gábor @ 2018-08-22 17:48 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Ævar Arnfjörð Bjarmason, SZEDER Gábor
Prior to d3c6751b18 (tests: make use of the test_must_be_empty
function, 2018-07-27), in the test 'rev-list should succeed with empty
output on empty stdin' in 't6018-rev-list-glob' the empty 'expect'
file served dual purpose: besides specifying the expected output, as
usual, it also served as empty input for 'git rev-list --stdin'.
Then d3c6751b18 came along, and, as part of the conversion to
'test_must_be_empty', removed this empty 'expect' file, not realizing
its secondary purpose. Redirecting stdin from the now non-existing
file failed the test, but since this test expects failure in the first
place, this issue went unnoticed.
Redirect 'git rev-list's stdin explicitly from /dev/null to provide
empty input. (Strictly speaking we don't need this redirection,
because the test script's stdin is already redirected from /dev/null
anyway, but I think it's better to be explicit about it.)
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/t6018-rev-list-glob.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh
index 02936c2f24..0bf10d0686 100755
--- a/t/t6018-rev-list-glob.sh
+++ b/t/t6018-rev-list-glob.sh
@@ -256,7 +256,7 @@ test_expect_success 'rev-list accumulates multiple --exclude' '
'
test_expect_failure 'rev-list should succeed with empty output on empty stdin' '
- git rev-list --stdin <expect >actual &&
+ git rev-list --stdin </dev/null >actual &&
test_must_be_empty actual
'
--
2.19.0.rc0.136.gd2dd172e64
^ permalink raw reply related [flat|nested] 95+ messages in thread
* Re: [PATCH] t6018-rev-list-glob: fix 'empty stdin' test
2018-08-22 17:48 ` [PATCH] t6018-rev-list-glob: fix 'empty stdin' test SZEDER Gábor
@ 2018-08-22 17:53 ` Eric Sunshine
2018-08-22 18:59 ` SZEDER Gábor
2018-08-22 18:01 ` Junio C Hamano
2018-08-22 18:50 ` Junio C Hamano
2 siblings, 1 reply; 95+ messages in thread
From: Eric Sunshine @ 2018-08-22 17:53 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Junio C Hamano, Git List, Ævar Arnfjörð Bjarmason
On Wed, Aug 22, 2018 at 1:48 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> Prior to d3c6751b18 (tests: make use of the test_must_be_empty
> function, 2018-07-27), in the test 'rev-list should succeed with empty
> output on empty stdin' in 't6018-rev-list-glob' the empty 'expect'
> file served dual purpose: besides specifying the expected output, as
> usual, it also served as empty input for 'git rev-list --stdin'.
>
> Then d3c6751b18 came along, and, as part of the conversion to
> 'test_must_be_empty', removed this empty 'expect' file, not realizing
> its secondary purpose. Redirecting stdin from the now non-existing
> file failed the test, but since this test expects failure in the first
> place, this issue went unnoticed.
Can you say a word or two (here in the email thread) about how you're
finding these failures (across the various test fixes you've posted
recently)? Are you instrumenting the code in some fashion? Or, finding
them by visual inspection?
Thanks.
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [PATCH] t6018-rev-list-glob: fix 'empty stdin' test
2018-08-22 17:53 ` Eric Sunshine
@ 2018-08-22 18:59 ` SZEDER Gábor
2018-08-22 20:30 ` Eric Sunshine
0 siblings, 1 reply; 95+ messages in thread
From: SZEDER Gábor @ 2018-08-22 18:59 UTC (permalink / raw)
To: Eric Sunshine
Cc: Junio C Hamano, Git mailing list,
Ævar Arnfjörð Bjarmason
On Wed, Aug 22, 2018 at 7:53 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> Can you say a word or two (here in the email thread) about how you're
> finding these failures (across the various test fixes you've posted
> recently)? Are you instrumenting the code in some fashion? Or, finding
> them by visual inspection?
Errors from system commands in our tests look like these:
grep: file3: No such file or directory
sed: -e expression #1, char 2: extra characters after command
diff: sub1/.git: No such file or directory
tar: rmtlseek not stopped at a record boundary
tar: Error is not recoverable: exiting now
while errors from the shell running the test like these:
t0020-crlf.sh: 8: eval: cannot open two: No such file
t6018-rev-list-glob.sh: 4: eval: cannot open expect: No such file
t7408-submodule-reference.sh: 615: test: =: unexpected operator
i.e. lines starting with various system commands' or test scripts'
names, followed by ': '.
So I've modified t/Makefile to not remove the 'test-results' directory
after a successful 'make test':
diff --git a/t/Makefile b/t/Makefile
index ea36cf7ac7..c7b1655593 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -54,10 +54,11 @@ pre-clean:
$(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)'
clean-except-prove-cache:
- $(RM) -r 'trash directory'.* '$(TEST_RESULTS_DIRECTORY_SQ)'
+ $(RM) -r 'trash directory'.*
$(RM) -r valgrind/bin
clean: clean-except-prove-cache
+ $(RM) '$(TEST_RESULTS_DIRECTORY_SQ)'
distclean: clean
$(RM) .prove
And then scanned the results of a '--verbose-log -x' test run with:
grep -E '^(awk|basename|cat|cd|chmod|cmp|cp|cut|diff|dirname|egrep|find|fgrep|grep|gunzip|gzip|ln|mkdir|mkfifo|mktemp|mv|readlink|rmdir|sed|sort|tar|touch|tr|ulimit|umask|uniq|unzip|wc|zipinfo|t[0-9][0-9][0-9][0-9]-[^:]*\.sh):
' test-results/*.out
and then, for lack of something better to do ;), I started looking at
the simpler looking errors.
I've though about how a check like this could be automated, but
haven't had any workable idea yet. There are commands that can
legitimately print errors, e.g. when checking for a prereq which the
system doesn't have (e.g. the 'tar' errors above, I think). And the
list of system commands in the grep pattern above is surely incomplete
and will likely change in the future...
^ permalink raw reply related [flat|nested] 95+ messages in thread
* Re: [PATCH] t6018-rev-list-glob: fix 'empty stdin' test
2018-08-22 18:59 ` SZEDER Gábor
@ 2018-08-22 20:30 ` Eric Sunshine
0 siblings, 0 replies; 95+ messages in thread
From: Eric Sunshine @ 2018-08-22 20:30 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Junio C Hamano, Git List, Ævar Arnfjörð Bjarmason
On Wed, Aug 22, 2018 at 2:59 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> On Wed, Aug 22, 2018 at 7:53 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > Can you say a word or two (here in the email thread) about how you're
> > finding these failures (across the various test fixes you've posted
> > recently)? Are you instrumenting the code in some fashion? Or, finding
> > them by visual inspection?
>
> Errors from system commands in our tests look like these:
> grep: file3: No such file or directory
> i.e. lines starting with various system commands' or test scripts'
> names, followed by ': '.
>
> So I've modified t/Makefile to not remove the 'test-results' directory
> after a successful 'make test':
> And then scanned the results of a '--verbose-log -x' test run with:
> grep -E [...]
> and then, for lack of something better to do ;), I started looking at
> the simpler looking errors.
Thanks for the explanation. That makes a lot of sense.
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [PATCH] t6018-rev-list-glob: fix 'empty stdin' test
2018-08-22 17:48 ` [PATCH] t6018-rev-list-glob: fix 'empty stdin' test SZEDER Gábor
2018-08-22 17:53 ` Eric Sunshine
@ 2018-08-22 18:01 ` Junio C Hamano
2018-08-22 18:50 ` Junio C Hamano
2 siblings, 0 replies; 95+ messages in thread
From: Junio C Hamano @ 2018-08-22 18:01 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, Ævar Arnfjörð Bjarmason
SZEDER Gábor <szeder.dev@gmail.com> writes:
> Prior to d3c6751b18 (tests: make use of the test_must_be_empty
> function, 2018-07-27), in the test 'rev-list should succeed with empty
> output on empty stdin' in 't6018-rev-list-glob' the empty 'expect'
> file served dual purpose: besides specifying the expected output, as
> usual, it also served as empty input for 'git rev-list --stdin'.
Thanks for a correction to a breakage on 'master' that was
introduced during this cycle.
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [PATCH] t6018-rev-list-glob: fix 'empty stdin' test
2018-08-22 17:48 ` [PATCH] t6018-rev-list-glob: fix 'empty stdin' test SZEDER Gábor
2018-08-22 17:53 ` Eric Sunshine
2018-08-22 18:01 ` Junio C Hamano
@ 2018-08-22 18:50 ` Junio C Hamano
2018-08-22 19:23 ` Jeff King
2 siblings, 1 reply; 95+ messages in thread
From: Junio C Hamano @ 2018-08-22 18:50 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, Ævar Arnfjörð Bjarmason, Jeff King
SZEDER Gábor <szeder.dev@gmail.com> writes:
> Redirect 'git rev-list's stdin explicitly from /dev/null to provide
> empty input. (Strictly speaking we don't need this redirection,
> because the test script's stdin is already redirected from /dev/null
> anyway, but I think it's better to be explicit about it.)
Yes.
> test_expect_failure 'rev-list should succeed with empty output on empty stdin' '
> - git rev-list --stdin <expect >actual &&
> + git rev-list --stdin </dev/null >actual &&
> test_must_be_empty actual
> '
By the way, it may be about time to turn that expect-failure into
expect-success. It is somewhat unfortunate that 0c5dc743 ("t6018:
flesh out empty input/output rev-list tests", 2017-08-02) removed
the comment that said "we _might_ want to change the behaviour in
these cases" and explained the tests as reminders, anticipating that
the series will change the behaviour for three cases where the
pending list ends up empty to make the discussion moot, but it
changed the behaviour of only two of them, leaving the "--stdin
reads empty" case behind.
It may be just the matter of doing something like the attached
patch. I won't be committing such a behaviour change during the
pre-release feature freeze, but we may want to consider doing this
early in the next cycle.
revision.c | 13 +++++++++++++
t/t6018-rev-list-glob.sh | 4 ++--
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/revision.c b/revision.c
index d12e6d8a4a..21fb413511 100644
--- a/revision.c
+++ b/revision.c
@@ -2441,6 +2441,19 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
object = get_reference(revs, revs->def, &oid, 0);
add_pending_object_with_mode(revs, object, revs->def, oc.mode);
}
+ /*
+ * Even if revs->pending is empty after all the above, if we
+ * handled "--stdin", then the caller really meant to give us
+ * an empty commit range. Just let the traversal give an
+ * empty result without causing a "no input? do you know how
+ * to use this command?" failure.
+ *
+ * NOTE!!! Because "--stdin </dev/null --default HEAD" should
+ * default to HEAD, this must come _after_ the above block
+ * that deals with revs->ref fallback.
+ */
+ if (read_from_stdin)
+ revs->rev_input_given = 1;
/* Did the user ask for any diff output? Run the diff! */
if (revs->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT)
diff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh
index 02936c2f24..db8a7834d8 100755
--- a/t/t6018-rev-list-glob.sh
+++ b/t/t6018-rev-list-glob.sh
@@ -255,8 +255,8 @@ test_expect_success 'rev-list accumulates multiple --exclude' '
compare rev-list "--exclude=refs/remotes/* --exclude=refs/tags/* --all" --branches
'
-test_expect_failure 'rev-list should succeed with empty output on empty stdin' '
+test_expect_success 'rev-list should succeed with empty output on empty stdin' '
git rev-list --stdin </dev/null >actual &&
test_must_be_empty actual
'
^ permalink raw reply related [flat|nested] 95+ messages in thread
* Re: [PATCH] t6018-rev-list-glob: fix 'empty stdin' test
2018-08-22 18:50 ` Junio C Hamano
@ 2018-08-22 19:23 ` Jeff King
2018-08-22 19:50 ` [PATCH] rev-list: make empty --stdin not an error Jeff King
0 siblings, 1 reply; 95+ messages in thread
From: Jeff King @ 2018-08-22 19:23 UTC (permalink / raw)
To: Junio C Hamano
Cc: SZEDER Gábor, git, Ævar Arnfjörð Bjarmason
On Wed, Aug 22, 2018 at 11:50:46AM -0700, Junio C Hamano wrote:
> > test_expect_failure 'rev-list should succeed with empty output on empty stdin' '
> > - git rev-list --stdin <expect >actual &&
> > + git rev-list --stdin </dev/null >actual &&
> > test_must_be_empty actual
> > '
>
> By the way, it may be about time to turn that expect-failure into
> expect-success. It is somewhat unfortunate that 0c5dc743 ("t6018:
> flesh out empty input/output rev-list tests", 2017-08-02) removed
> the comment that said "we _might_ want to change the behaviour in
> these cases" and explained the tests as reminders, anticipating that
> the series will change the behaviour for three cases where the
> pending list ends up empty to make the discussion moot, but it
> changed the behaviour of only two of them, leaving the "--stdin
> reads empty" case behind.
Yeah, I think I had intended to make --stdin work there, and when I
realized at the end of the series it did not, I failed to go back and
modify that initial commit touching the test.
More discussion in the original thread:
https://public-inbox.org/git/20170802223416.gwiezhbuxbdmbjzx@sigill.intra.peff.net/
> It may be just the matter of doing something like the attached
> patch. I won't be committing such a behaviour change during the
> pre-release feature freeze, but we may want to consider doing this
> early in the next cycle.
Yes, definitely this is tricky enough to avoid doing during the freeze.
> diff --git a/revision.c b/revision.c
> index d12e6d8a4a..21fb413511 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2441,6 +2441,19 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
> object = get_reference(revs, revs->def, &oid, 0);
> add_pending_object_with_mode(revs, object, revs->def, oc.mode);
> }
> + /*
> + * Even if revs->pending is empty after all the above, if we
> + * handled "--stdin", then the caller really meant to give us
> + * an empty commit range. Just let the traversal give an
> + * empty result without causing a "no input? do you know how
> + * to use this command?" failure.
> + *
> + * NOTE!!! Because "--stdin </dev/null --default HEAD" should
> + * default to HEAD, this must come _after_ the above block
> + * that deals with revs->ref fallback.
> + */
> + if (read_from_stdin)
> + revs->rev_input_given = 1;
I think this should do the right thing. All of the issues discussed in
the earlier thread were about using revs->def, and this neatly sidesteps
that by touching the flag afterwards. It's a little funny that the flag
now means two things (earlier in the function it is "we should use the
default" and later it becomes "the caller may complain").
It may be worth splitting it into two flags: rev_input_given and
rev_read_stdin. That puts the responsibility on the caller to check both
flags. But really, rev-list.c is the only caller that checks it anyway.
And it would mean this scary comment can go away. ;)
It also _could_ be useful to other callers to distinguish the two cases
(e.g., if they wanted to know whether they were free to use stdin
themselves). But I don't offhand know of any callers that need that (I
have a vague recollection that it might have come up once over the
years).
-Peff
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH] rev-list: make empty --stdin not an error
2018-08-22 19:23 ` Jeff King
@ 2018-08-22 19:50 ` Jeff King
2018-08-22 20:42 ` Junio C Hamano
0 siblings, 1 reply; 95+ messages in thread
From: Jeff King @ 2018-08-22 19:50 UTC (permalink / raw)
To: Junio C Hamano
Cc: SZEDER Gábor, git, Ævar Arnfjörð Bjarmason
On Wed, Aug 22, 2018 at 03:23:08PM -0400, Jeff King wrote:
> > + /*
> > + * Even if revs->pending is empty after all the above, if we
> > + * handled "--stdin", then the caller really meant to give us
> > + * an empty commit range. Just let the traversal give an
> > + * empty result without causing a "no input? do you know how
> > + * to use this command?" failure.
> > + *
> > + * NOTE!!! Because "--stdin </dev/null --default HEAD" should
> > + * default to HEAD, this must come _after_ the above block
> > + * that deals with revs->ref fallback.
> > + */
> > + if (read_from_stdin)
> > + revs->rev_input_given = 1;
>
> I think this should do the right thing. All of the issues discussed in
> the earlier thread were about using revs->def, and this neatly sidesteps
> that by touching the flag afterwards. It's a little funny that the flag
> now means two things (earlier in the function it is "we should use the
> default" and later it becomes "the caller may complain").
Here's my "two flags" approach as a real patch. After giving it some
thought, I really do think it's cleaner.
I know we're in a freeze. If you want to pick this up for pu or maybe
next, that's fine. If not, I'll re-send it after the release. It applies
on top of Gábor's text fixup patch.
-- >8 --
Subject: [PATCH] rev-list: make empty --stdin not an error
When we originally did the series that contains 7ba826290a
(revision: add rev_input_given flag, 2017-08-02) the intent
was that "git rev-list --stdin </dev/null" would similarly
become a successful noop. However, an attempt at the time to
do that did not work[1]. The problem is that rev_input_given
serves two roles:
- it tells rev-list.c that it should not error out
- it tells revision.c that it should not have the "default"
ref kick (e.g., "HEAD" in "git log")
We want to trigger the former, but not the latter. This is
technically possible with a single flag, if we set the flag
only after revision.c's revs->def check. But this introduces
a rather subtle ordering dependency.
Instead, let's keep two flags: one to denote when we got
actual input (which triggers both roles) and one for when we
read stdin (which triggers only the first).
This does mean a caller interested in the first role has to
check both flags, but there's only one such caller. And any
future callers might want to make the distinction anyway
(e.g., if they care less about erroring out, and more about
whether revision.c soaked up our stdin).
[1] https://public-inbox.org/git/20170802223416.gwiezhbuxbdmbjzx@sigill.intra.peff.net/
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/rev-list.c | 2 +-
revision.c | 1 +
revision.h | 5 +++++
t/t6018-rev-list-glob.sh | 2 +-
4 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 5b07f3f4a2..ed0ea7dc5b 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -493,7 +493,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
if ((!revs.commits && reflog_walk_empty(revs.reflog_info) &&
(!(revs.tag_objects || revs.tree_objects || revs.blob_objects) &&
!revs.pending.nr) &&
- !revs.rev_input_given) ||
+ !revs.rev_input_given && !revs.read_from_stdin) ||
revs.diff)
usage(rev_list_usage);
diff --git a/revision.c b/revision.c
index de4dce600d..4d53102cf4 100644
--- a/revision.c
+++ b/revision.c
@@ -2369,6 +2369,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
}
if (read_from_stdin++)
die("--stdin given twice?");
+ revs->read_from_stdin = 1;
read_revisions_from_stdin(revs, &prune_data);
continue;
}
diff --git a/revision.h b/revision.h
index 007278cc11..1225957927 100644
--- a/revision.h
+++ b/revision.h
@@ -82,6 +82,11 @@ struct rev_info {
*/
int rev_input_given;
+ /*
+ * Whether we read from stdin due to the --stdin option.
+ */
+ int read_from_stdin;
+
/* topo-sort */
enum rev_sort_order sort_order;
diff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh
index 0bf10d0686..db8a7834d8 100755
--- a/t/t6018-rev-list-glob.sh
+++ b/t/t6018-rev-list-glob.sh
@@ -255,7 +255,7 @@ test_expect_success 'rev-list accumulates multiple --exclude' '
compare rev-list "--exclude=refs/remotes/* --exclude=refs/tags/* --all" --branches
'
-test_expect_failure 'rev-list should succeed with empty output on empty stdin' '
+test_expect_success 'rev-list should succeed with empty output on empty stdin' '
git rev-list --stdin </dev/null >actual &&
test_must_be_empty actual
'
--
2.19.0.rc0.412.g7005db4e88
^ permalink raw reply related [flat|nested] 95+ messages in thread
* Re: [PATCH] rev-list: make empty --stdin not an error
2018-08-22 19:50 ` [PATCH] rev-list: make empty --stdin not an error Jeff King
@ 2018-08-22 20:42 ` Junio C Hamano
2018-08-22 21:37 ` Jeff King
2018-08-22 21:41 ` Junio C Hamano
0 siblings, 2 replies; 95+ messages in thread
From: Junio C Hamano @ 2018-08-22 20:42 UTC (permalink / raw)
To: Jeff King; +Cc: SZEDER Gábor, git, Ævar Arnfjörð Bjarmason
Jeff King <peff@peff.net> writes:
> Instead, let's keep two flags: one to denote when we got
> actual input (which triggers both roles) and one for when we
> read stdin (which triggers only the first).
>
> This does mean a caller interested in the first role has to
> check both flags, but there's only one such caller. And any
> future callers might want to make the distinction anyway
> (e.g., if they care less about erroring out, and more about
> whether revision.c soaked up our stdin).
>
> [1] https://public-inbox.org/git/20170802223416.gwiezhbuxbdmbjzx@sigill.intra.peff.net/
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/rev-list.c | 2 +-
> revision.c | 1 +
> revision.h | 5 +++++
> t/t6018-rev-list-glob.sh | 2 +-
> 4 files changed, 8 insertions(+), 2 deletions(-)
I think this makes sense, but if we were to give a dedicated field
in the revs structure, can we lose the local variable at the same
time, I wonder?
Thanks.
> diff --git a/revision.c b/revision.c
> index de4dce600d..4d53102cf4 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2369,6 +2369,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
> }
> if (read_from_stdin++)
> die("--stdin given twice?");
> + revs->read_from_stdin = 1;
> read_revisions_from_stdin(revs, &prune_data);
> continue;
> }
> diff --git a/revision.h b/revision.h
> index 007278cc11..1225957927 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -82,6 +82,11 @@ struct rev_info {
> */
> int rev_input_given;
>
> + /*
> + * Whether we read from stdin due to the --stdin option.
> + */
> + int read_from_stdin;
> +
> /* topo-sort */
> enum rev_sort_order sort_order;
>
> diff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh
> index 0bf10d0686..db8a7834d8 100755
> --- a/t/t6018-rev-list-glob.sh
> +++ b/t/t6018-rev-list-glob.sh
> @@ -255,7 +255,7 @@ test_expect_success 'rev-list accumulates multiple --exclude' '
> compare rev-list "--exclude=refs/remotes/* --exclude=refs/tags/* --all" --branches
> '
>
> -test_expect_failure 'rev-list should succeed with empty output on empty stdin' '
> +test_expect_success 'rev-list should succeed with empty output on empty stdin' '
> git rev-list --stdin </dev/null >actual &&
> test_must_be_empty actual
> '
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [PATCH] rev-list: make empty --stdin not an error
2018-08-22 20:42 ` Junio C Hamano
@ 2018-08-22 21:37 ` Jeff King
2018-08-22 21:50 ` Junio C Hamano
2018-08-22 21:41 ` Junio C Hamano
1 sibling, 1 reply; 95+ messages in thread
From: Jeff King @ 2018-08-22 21:37 UTC (permalink / raw)
To: Junio C Hamano
Cc: SZEDER Gábor, git, Ævar Arnfjörð Bjarmason
On Wed, Aug 22, 2018 at 01:42:26PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Instead, let's keep two flags: one to denote when we got
> > actual input (which triggers both roles) and one for when we
> > read stdin (which triggers only the first).
> >
> > This does mean a caller interested in the first role has to
> > check both flags, but there's only one such caller. And any
> > future callers might want to make the distinction anyway
> > (e.g., if they care less about erroring out, and more about
> > whether revision.c soaked up our stdin).
> >
> > [1] https://public-inbox.org/git/20170802223416.gwiezhbuxbdmbjzx@sigill.intra.peff.net/
> >
> > Helped-by: Junio C Hamano <gitster@pobox.com>
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > builtin/rev-list.c | 2 +-
> > revision.c | 1 +
> > revision.h | 5 +++++
> > t/t6018-rev-list-glob.sh | 2 +-
> > 4 files changed, 8 insertions(+), 2 deletions(-)
>
> I think this makes sense, but if we were to give a dedicated field
> in the revs structure, can we lose the local variable at the same
> time, I wonder?
Yes. I was thinking it had more purpose than this, but it really is just
a flag to check "did we do this already?". Which is one of the main
purposes I claimed for the new flag in my commit message. :)
Here it is with that squashed in (this is the whole patch, since I
updated the commit message to mention it).
-- >8 --
Subject: [PATCH] rev-list: make empty --stdin not an error
When we originally did the series that contains 7ba826290a
(revision: add rev_input_given flag, 2017-08-02) the intent
was that "git rev-list --stdin </dev/null" would similarly
become a successful noop. However, an attempt at the time to
do that did not work[1]. The problem is that rev_input_given
serves two roles:
- it tells rev-list.c that it should not error out
- it tells revision.c that it should not have the "default"
ref kick (e.g., "HEAD" in "git log")
We want to trigger the former, but not the latter. This is
technically possible with a single flag, if we set the flag
only after revision.c's revs->def check. But this introduces
a rather subtle ordering dependency.
Instead, let's keep two flags: one to denote when we got
actual input (which triggers both roles) and one for when we
read stdin (which triggers only the first).
This does mean a caller interested in the first role has to
check both flags, but there's only one such caller. And any
future callers might want to make the distinction anyway
(e.g., if they care less about erroring out, and more about
whether revision.c soaked up our stdin).
In fact, we already keep such a flag internally in
revision.c for this purpose, so this is really just exposing
that to the caller (and the old function-local flag can go
away in favor of our new one).
[1] https://public-inbox.org/git/20170802223416.gwiezhbuxbdmbjzx@sigill.intra.peff.net/
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/rev-list.c | 2 +-
revision.c | 5 ++---
revision.h | 5 +++++
t/t6018-rev-list-glob.sh | 2 +-
4 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 5b07f3f4a2..ed0ea7dc5b 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -493,7 +493,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
if ((!revs.commits && reflog_walk_empty(revs.reflog_info) &&
(!(revs.tag_objects || revs.tree_objects || revs.blob_objects) &&
!revs.pending.nr) &&
- !revs.rev_input_given) ||
+ !revs.rev_input_given && !revs.read_from_stdin) ||
revs.diff)
usage(rev_list_usage);
diff --git a/revision.c b/revision.c
index de4dce600d..46228f82ee 100644
--- a/revision.c
+++ b/revision.c
@@ -2318,7 +2318,7 @@ static void NORETURN diagnose_missing_default(const char *def)
*/
int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct setup_revision_opt *opt)
{
- int i, flags, left, seen_dashdash, read_from_stdin, got_rev_arg = 0, revarg_opt;
+ int i, flags, left, seen_dashdash, got_rev_arg = 0, revarg_opt;
struct argv_array prune_data = ARGV_ARRAY_INIT;
const char *submodule = NULL;
@@ -2348,7 +2348,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
revarg_opt = opt ? opt->revarg_opt : 0;
if (seen_dashdash)
revarg_opt |= REVARG_CANNOT_BE_FILENAME;
- read_from_stdin = 0;
for (left = i = 1; i < argc; i++) {
const char *arg = argv[i];
if (*arg == '-') {
@@ -2367,7 +2366,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
argv[left++] = arg;
continue;
}
- if (read_from_stdin++)
+ if (revs->read_from_stdin++)
die("--stdin given twice?");
read_revisions_from_stdin(revs, &prune_data);
continue;
diff --git a/revision.h b/revision.h
index 007278cc11..1225957927 100644
--- a/revision.h
+++ b/revision.h
@@ -82,6 +82,11 @@ struct rev_info {
*/
int rev_input_given;
+ /*
+ * Whether we read from stdin due to the --stdin option.
+ */
+ int read_from_stdin;
+
/* topo-sort */
enum rev_sort_order sort_order;
diff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh
index 0bf10d0686..db8a7834d8 100755
--- a/t/t6018-rev-list-glob.sh
+++ b/t/t6018-rev-list-glob.sh
@@ -255,7 +255,7 @@ test_expect_success 'rev-list accumulates multiple --exclude' '
compare rev-list "--exclude=refs/remotes/* --exclude=refs/tags/* --all" --branches
'
-test_expect_failure 'rev-list should succeed with empty output on empty stdin' '
+test_expect_success 'rev-list should succeed with empty output on empty stdin' '
git rev-list --stdin </dev/null >actual &&
test_must_be_empty actual
'
--
2.19.0.rc0.412.g7005db4e88
^ permalink raw reply related [flat|nested] 95+ messages in thread
* Re: [PATCH] rev-list: make empty --stdin not an error
2018-08-22 21:37 ` Jeff King
@ 2018-08-22 21:50 ` Junio C Hamano
2018-08-22 21:55 ` Jeff King
0 siblings, 1 reply; 95+ messages in thread
From: Junio C Hamano @ 2018-08-22 21:50 UTC (permalink / raw)
To: Jeff King; +Cc: SZEDER Gábor, git, Ævar Arnfjörð Bjarmason
Jeff King <peff@peff.net> writes:
> Yes. I was thinking it had more purpose than this, but it really is just
> a flag to check "did we do this already?". Which is one of the main
> purposes I claimed for the new flag in my commit message. :)
OK.
The reason I was on the fence was primarily because read_from_stdin
field in the structure observable from outside can be a boolean
(that is, "unsigned :1"), but internally this may want to count up
to two.
Or with "unsigned read_from_stdin:1", would this
if (revs->read_from_stdin++)
die("twice???");
still be usable? As the value of post-increment would be 1 even
when the resulting field would have wrapped-around already, it
should be OK, but it just felt strange to me.
But that is something we do not have to worry about until somebody
tries to shrink the structure by making these flags into bitfields.
Thanks for an updated patch.
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [PATCH] rev-list: make empty --stdin not an error
2018-08-22 21:50 ` Junio C Hamano
@ 2018-08-22 21:55 ` Jeff King
0 siblings, 0 replies; 95+ messages in thread
From: Jeff King @ 2018-08-22 21:55 UTC (permalink / raw)
To: Junio C Hamano
Cc: SZEDER Gábor, git, Ævar Arnfjörð Bjarmason
On Wed, Aug 22, 2018 at 02:50:05PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Yes. I was thinking it had more purpose than this, but it really is just
> > a flag to check "did we do this already?". Which is one of the main
> > purposes I claimed for the new flag in my commit message. :)
>
> OK.
>
> The reason I was on the fence was primarily because read_from_stdin
> field in the structure observable from outside can be a boolean
> (that is, "unsigned :1"), but internally this may want to count up
> to two.
>
> Or with "unsigned read_from_stdin:1", would this
>
> if (revs->read_from_stdin++)
> die("twice???");
>
> still be usable? As the value of post-increment would be 1 even
> when the resulting field would have wrapped-around already, it
> should be OK, but it just felt strange to me.
I agree it would work in practice, though I also agree it is funny and
should be avoided.
> But that is something we do not have to worry about until somebody
> tries to shrink the structure by making these flags into bitfields.
Also agreed. I'd probably resolve it then by writing:
if (revs->read_from_stdin)
die("twice");
revs->read_from_stdin = 1;
I guess we could even do that now. Or add a test to make sure "--stdin
--stdin" barfs. But I am perfectly happy to punt until somebody actually
wants to use a bitfield.
-Peff
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [PATCH] rev-list: make empty --stdin not an error
2018-08-22 20:42 ` Junio C Hamano
2018-08-22 21:37 ` Jeff King
@ 2018-08-22 21:41 ` Junio C Hamano
1 sibling, 0 replies; 95+ messages in thread
From: Junio C Hamano @ 2018-08-22 21:41 UTC (permalink / raw)
To: Jeff King; +Cc: SZEDER Gábor, git, Ævar Arnfjörð Bjarmason
Junio C Hamano <gitster@pobox.com> writes:
> I think this makes sense, but if we were to give a dedicated field
> in the revs structure, can we lose the local variable at the same
> time, I wonder?
>
> Thanks.
Well, the answer to "can we" is always "yes"; what I was truly
wondering was if it makes sense to do so. I am on the fence.
>
>> diff --git a/revision.c b/revision.c
>> index de4dce600d..4d53102cf4 100644
>> --- a/revision.c
>> +++ b/revision.c
>> @@ -2369,6 +2369,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>> }
>> if (read_from_stdin++)
>> die("--stdin given twice?");
>> + revs->read_from_stdin = 1;
>> read_revisions_from_stdin(revs, &prune_data);
>> continue;
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH v7 2/8] checkout.h: wrap the arguments to unique_tracking_name()
2018-06-02 11:50 ` [PATCH v6 " Ævar Arnfjörð Bjarmason
2018-06-05 14:40 ` [PATCH v7 " Ævar Arnfjörð Bjarmason
2018-06-05 14:40 ` [PATCH v7 1/8] checkout tests: index should be clean after dwim checkout Ævar Arnfjörð Bjarmason
@ 2018-06-05 14:40 ` Ævar Arnfjörð Bjarmason
2018-06-05 14:40 ` [PATCH v7 3/8] checkout.c: introduce an *_INIT macro Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
8 siblings, 0 replies; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-06-05 14:40 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer,
Eric Sunshine, Ævar Arnfjörð Bjarmason
The line was too long already, and will be longer still when a later
change adds another argument.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
checkout.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/checkout.h b/checkout.h
index 9980711179..4cd4cd1c23 100644
--- a/checkout.h
+++ b/checkout.h
@@ -8,6 +8,7 @@
* tracking branch. Return the name of the remote if such a branch
* exists, NULL otherwise.
*/
-extern const char *unique_tracking_name(const char *name, struct object_id *oid);
+extern const char *unique_tracking_name(const char *name,
+ struct object_id *oid);
#endif /* CHECKOUT_H */
--
2.17.0.290.gded63e768a
^ permalink raw reply related [flat|nested] 95+ messages in thread
* [PATCH v7 3/8] checkout.c: introduce an *_INIT macro
2018-06-02 11:50 ` [PATCH v6 " Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2018-06-05 14:40 ` [PATCH v7 2/8] checkout.h: wrap the arguments to unique_tracking_name() Ævar Arnfjörð Bjarmason
@ 2018-06-05 14:40 ` Ævar Arnfjörð Bjarmason
2018-06-05 14:40 ` [PATCH v7 4/8] checkout.c: change "unique" member to "num_matches" Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
8 siblings, 0 replies; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-06-05 14:40 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer,
Eric Sunshine, Ævar Arnfjörð Bjarmason
Add an *_INIT macro for the tracking_name_data similar to what exists
elsewhere in the codebase, e.g. OID_ARRAY_INIT in sha1-array.h. This
will make it more idiomatic in later changes to add more fields to the
struct & its initialization macro.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
checkout.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/checkout.c b/checkout.c
index bdefc888ba..80e430cda8 100644
--- a/checkout.c
+++ b/checkout.c
@@ -10,6 +10,8 @@ struct tracking_name_data {
int unique;
};
+#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 1 }
+
static int check_tracking_name(struct remote *remote, void *cb_data)
{
struct tracking_name_data *cb = cb_data;
@@ -32,7 +34,7 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
const char *unique_tracking_name(const char *name, struct object_id *oid)
{
- struct tracking_name_data cb_data = { NULL, NULL, NULL, 1 };
+ struct tracking_name_data cb_data = TRACKING_NAME_DATA_INIT;
cb_data.src_ref = xstrfmt("refs/heads/%s", name);
cb_data.dst_oid = oid;
for_each_remote(check_tracking_name, &cb_data);
--
2.17.0.290.gded63e768a
^ permalink raw reply related [flat|nested] 95+ messages in thread
* [PATCH v7 4/8] checkout.c: change "unique" member to "num_matches"
2018-06-02 11:50 ` [PATCH v6 " Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2018-06-05 14:40 ` [PATCH v7 3/8] checkout.c: introduce an *_INIT macro Ævar Arnfjörð Bjarmason
@ 2018-06-05 14:40 ` Ævar Arnfjörð Bjarmason
2018-06-05 14:40 ` [PATCH v7 5/8] checkout: pass the "num_matches" up to callers Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
8 siblings, 0 replies; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-06-05 14:40 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer,
Eric Sunshine, Ævar Arnfjörð Bjarmason
Internally track how many matches we find in the check_tracking_name()
callback. Nothing uses this now, but it will be made use of in a later
change.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
checkout.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/checkout.c b/checkout.c
index 80e430cda8..7662a39a62 100644
--- a/checkout.c
+++ b/checkout.c
@@ -7,10 +7,10 @@ struct tracking_name_data {
/* const */ char *src_ref;
char *dst_ref;
struct object_id *dst_oid;
- int unique;
+ int num_matches;
};
-#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 1 }
+#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 0 }
static int check_tracking_name(struct remote *remote, void *cb_data)
{
@@ -23,9 +23,9 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
free(query.dst);
return 0;
}
+ cb->num_matches++;
if (cb->dst_ref) {
free(query.dst);
- cb->unique = 0;
return 0;
}
cb->dst_ref = query.dst;
@@ -39,7 +39,7 @@ const char *unique_tracking_name(const char *name, struct object_id *oid)
cb_data.dst_oid = oid;
for_each_remote(check_tracking_name, &cb_data);
free(cb_data.src_ref);
- if (cb_data.unique)
+ if (cb_data.num_matches == 1)
return cb_data.dst_ref;
free(cb_data.dst_ref);
return NULL;
--
2.17.0.290.gded63e768a
^ permalink raw reply related [flat|nested] 95+ messages in thread
* [PATCH v7 5/8] checkout: pass the "num_matches" up to callers
2018-06-02 11:50 ` [PATCH v6 " Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2018-06-05 14:40 ` [PATCH v7 4/8] checkout.c: change "unique" member to "num_matches" Ævar Arnfjörð Bjarmason
@ 2018-06-05 14:40 ` Ævar Arnfjörð Bjarmason
2018-06-05 14:40 ` [PATCH v7 6/8] builtin/checkout.c: use "ret" variable for return Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
8 siblings, 0 replies; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-06-05 14:40 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer,
Eric Sunshine, Ævar Arnfjörð Bjarmason
Pass the previously added "num_matches" struct value up to the callers
of unique_tracking_name(). This will allow callers to optionally print
better error messages in a later change.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/checkout.c | 10 +++++++---
builtin/worktree.c | 4 ++--
checkout.c | 5 ++++-
checkout.h | 3 ++-
4 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2e1d2376d2..72457fb7d5 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -878,7 +878,8 @@ static int parse_branchname_arg(int argc, const char **argv,
int dwim_new_local_branch_ok,
struct branch_info *new_branch_info,
struct checkout_opts *opts,
- struct object_id *rev)
+ struct object_id *rev,
+ int *dwim_remotes_matched)
{
struct tree **source_tree = &opts->source_tree;
const char **new_branch = &opts->new_branch;
@@ -972,7 +973,8 @@ static int parse_branchname_arg(int argc, const char **argv,
recover_with_dwim = 0;
if (recover_with_dwim) {
- const char *remote = unique_tracking_name(arg, rev);
+ const char *remote = unique_tracking_name(arg, rev,
+ dwim_remotes_matched);
if (remote) {
*new_branch = arg;
arg = remote;
@@ -1109,6 +1111,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
struct branch_info new_branch_info;
char *conflict_style = NULL;
int dwim_new_local_branch = 1;
+ int dwim_remotes_matched = 0;
struct option options[] = {
OPT__QUIET(&opts.quiet, N_("suppress progress reporting")),
OPT_STRING('b', NULL, &opts.new_branch, N_("branch"),
@@ -1219,7 +1222,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
opts.track == BRANCH_TRACK_UNSPECIFIED &&
!opts.new_branch;
int n = parse_branchname_arg(argc, argv, dwim_ok,
- &new_branch_info, &opts, &rev);
+ &new_branch_info, &opts, &rev,
+ &dwim_remotes_matched);
argv += n;
argc -= n;
}
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 5c7d2bb180..a763dbdccb 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -412,7 +412,7 @@ static const char *dwim_branch(const char *path, const char **new_branch)
if (guess_remote) {
struct object_id oid;
const char *remote =
- unique_tracking_name(*new_branch, &oid);
+ unique_tracking_name(*new_branch, &oid, NULL);
return remote;
}
return NULL;
@@ -484,7 +484,7 @@ static int add(int ac, const char **av, const char *prefix)
commit = lookup_commit_reference_by_name(branch);
if (!commit) {
- remote = unique_tracking_name(branch, &oid);
+ remote = unique_tracking_name(branch, &oid, NULL);
if (remote) {
new_branch = branch;
branch = remote;
diff --git a/checkout.c b/checkout.c
index 7662a39a62..ee3a7e9c05 100644
--- a/checkout.c
+++ b/checkout.c
@@ -32,12 +32,15 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
return 0;
}
-const char *unique_tracking_name(const char *name, struct object_id *oid)
+const char *unique_tracking_name(const char *name, struct object_id *oid,
+ int *dwim_remotes_matched)
{
struct tracking_name_data cb_data = TRACKING_NAME_DATA_INIT;
cb_data.src_ref = xstrfmt("refs/heads/%s", name);
cb_data.dst_oid = oid;
for_each_remote(check_tracking_name, &cb_data);
+ if (dwim_remotes_matched)
+ *dwim_remotes_matched = cb_data.num_matches;
free(cb_data.src_ref);
if (cb_data.num_matches == 1)
return cb_data.dst_ref;
diff --git a/checkout.h b/checkout.h
index 4cd4cd1c23..6b2073310c 100644
--- a/checkout.h
+++ b/checkout.h
@@ -9,6 +9,7 @@
* exists, NULL otherwise.
*/
extern const char *unique_tracking_name(const char *name,
- struct object_id *oid);
+ struct object_id *oid,
+ int *dwim_remotes_matched);
#endif /* CHECKOUT_H */
--
2.17.0.290.gded63e768a
^ permalink raw reply related [flat|nested] 95+ messages in thread
* [PATCH v7 6/8] builtin/checkout.c: use "ret" variable for return
2018-06-02 11:50 ` [PATCH v6 " Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2018-06-05 14:40 ` [PATCH v7 5/8] checkout: pass the "num_matches" up to callers Ævar Arnfjörð Bjarmason
@ 2018-06-05 14:40 ` Ævar Arnfjörð Bjarmason
2018-06-05 14:40 ` [PATCH v7 7/8] checkout: add advice for ambiguous "checkout <branch>" Ævar Arnfjörð Bjarmason
2018-06-05 14:40 ` [PATCH v7 8/8] checkout & worktree: introduce checkout.defaultRemote Ævar Arnfjörð Bjarmason
8 siblings, 0 replies; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-06-05 14:40 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer,
Eric Sunshine, Ævar Arnfjörð Bjarmason
There is no point in doing this right now, but in later change the
"ret" variable will be inspected. This change makes that meaningful
change smaller.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/checkout.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 72457fb7d5..8c93c55cbc 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1265,8 +1265,10 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
}
UNLEAK(opts);
- if (opts.patch_mode || opts.pathspec.nr)
- return checkout_paths(&opts, new_branch_info.name);
- else
+ if (opts.patch_mode || opts.pathspec.nr) {
+ int ret = checkout_paths(&opts, new_branch_info.name);
+ return ret;
+ } else {
return checkout_branch(&opts, &new_branch_info);
+ }
}
--
2.17.0.290.gded63e768a
^ permalink raw reply related [flat|nested] 95+ messages in thread
* [PATCH v7 7/8] checkout: add advice for ambiguous "checkout <branch>"
2018-06-02 11:50 ` [PATCH v6 " Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2018-06-05 14:40 ` [PATCH v7 6/8] builtin/checkout.c: use "ret" variable for return Ævar Arnfjörð Bjarmason
@ 2018-06-05 14:40 ` Ævar Arnfjörð Bjarmason
2018-06-05 14:40 ` [PATCH v7 8/8] checkout & worktree: introduce checkout.defaultRemote Ævar Arnfjörð Bjarmason
8 siblings, 0 replies; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-06-05 14:40 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer,
Eric Sunshine, Ævar Arnfjörð Bjarmason
As the "checkout" documentation describes:
If <branch> is not found but there does exist a tracking branch in
exactly one remote (call it <remote>) with a matching name, treat
as equivalent to [...] <remote>/<branch.
This is a really useful feature. The problem is that when you add
another remote (e.g. a fork), git won't find a unique branch name
anymore, and will instead print this unhelpful message:
$ git checkout master
error: pathspec 'master' did not match any file(s) known to git
Now it will, on my git.git checkout, print:
$ ./git --exec-path=$PWD checkout master
error: pathspec 'master' did not match any file(s) known to git.
hint: 'master' matched more than one remote tracking branch.
hint: We found 26 remotes with a reference that matched. So we fell back
hint: on trying to resolve the argument as a path, but failed there too!
hint:
hint: If you meant to check out a remote tracking branch on, e.g. 'origin',
hint: you can do so by fully qualifying the name with the --track option:
hint:
hint: git checkout --track origin/<name>
Note that the "error: pathspec[...]" message is still printed. This is
because whatever else checkout may have tried earlier, its final
fallback is to try to resolve the argument as a path. E.g. in this
case:
$ ./git --exec-path=$PWD checkout master pu
error: pathspec 'master' did not match any file(s) known to git.
error: pathspec 'pu' did not match any file(s) known to git.
There we don't print the "hint:" implicitly due to earlier logic
around the DWIM fallback. That fallback is only used if it looks like
we have one argument that might be a branch.
I can't think of an intrinsic reason for why we couldn't in some
future change skip printing the "error: pathspec[...]" error. However,
to do so we'd need to pass something down to checkout_paths() to make
it suppress printing an error on its own, and for us to be confident
that we're not silencing cases where those errors are meaningful.
I don't think that's worth it since determining whether that's the
case could easily change due to future changes in the checkout logic.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Documentation/config.txt | 7 +++++++
advice.c | 2 ++
advice.h | 1 +
builtin/checkout.c | 13 +++++++++++++
t/t2024-checkout-dwim.sh | 14 ++++++++++++++
5 files changed, 37 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index ab641bf5a9..dfc0413a84 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -344,6 +344,13 @@ advice.*::
Advice shown when you used linkgit:git-checkout[1] to
move to the detach HEAD state, to instruct how to create
a local branch after the fact.
+ checkoutAmbiguousRemoteBranchName::
+ Advice shown when the argument to
+ linkgit:git-checkout[1] ambiguously resolves to a
+ remote tracking branch on more than one remote in
+ situations where an unambiguous argument would have
+ otherwise caused a remote-tracking branch to be
+ checked out.
amWorkDir::
Advice that shows the location of the patch file when
linkgit:git-am[1] fails to apply it.
diff --git a/advice.c b/advice.c
index 370a56d054..75e7dede90 100644
--- a/advice.c
+++ b/advice.c
@@ -21,6 +21,7 @@ int advice_add_embedded_repo = 1;
int advice_ignored_hook = 1;
int advice_waiting_for_editor = 1;
int advice_graft_file_deprecated = 1;
+int advice_checkout_ambiguous_remote_branch_name = 1;
static int advice_use_color = -1;
static char advice_colors[][COLOR_MAXLEN] = {
@@ -72,6 +73,7 @@ static struct {
{ "ignoredhook", &advice_ignored_hook },
{ "waitingforeditor", &advice_waiting_for_editor },
{ "graftfiledeprecated", &advice_graft_file_deprecated },
+ { "checkoutambiguousremotebranchname", &advice_checkout_ambiguous_remote_branch_name },
/* make this an alias for backward compatibility */
{ "pushnonfastforward", &advice_push_update_rejected }
diff --git a/advice.h b/advice.h
index 9f5064e82a..4d11d51d43 100644
--- a/advice.h
+++ b/advice.h
@@ -22,6 +22,7 @@ extern int advice_add_embedded_repo;
extern int advice_ignored_hook;
extern int advice_waiting_for_editor;
extern int advice_graft_file_deprecated;
+extern int advice_checkout_ambiguous_remote_branch_name;
int git_default_advice_config(const char *var, const char *value);
__attribute__((format (printf, 1, 2)))
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8c93c55cbc..baa027455a 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -22,6 +22,7 @@
#include "resolve-undo.h"
#include "submodule-config.h"
#include "submodule.h"
+#include "advice.h"
static const char * const checkout_usage[] = {
N_("git checkout [<options>] <branch>"),
@@ -1267,6 +1268,18 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
UNLEAK(opts);
if (opts.patch_mode || opts.pathspec.nr) {
int ret = checkout_paths(&opts, new_branch_info.name);
+ if (ret && dwim_remotes_matched > 1 &&
+ advice_checkout_ambiguous_remote_branch_name)
+ advise(_("'%s' matched more than one remote tracking branch.\n"
+ "We found %d remotes with a reference that matched. So we fell back\n"
+ "on trying to resolve the argument as a path, but failed there too!\n"
+ "\n"
+ "If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
+ "you can do so by fully qualifying the name with the --track option:\n"
+ "\n"
+ " git checkout --track origin/<name>"),
+ argv[0],
+ dwim_remotes_matched);
return ret;
} else {
return checkout_branch(&opts, &new_branch_info);
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index ed32828105..fef263a858 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -76,6 +76,20 @@ test_expect_success 'checkout of branch from multiple remotes fails #1' '
test_branch master
'
+test_expect_success 'checkout of branch from multiple remotes fails with advice' '
+ git checkout -B master &&
+ test_might_fail git branch -D foo &&
+ test_must_fail git checkout foo 2>stderr &&
+ test_branch master &&
+ status_uno_is_clean &&
+ test_i18ngrep "^hint: " stderr &&
+ test_must_fail git -c advice.checkoutAmbiguousRemoteBranchName=false \
+ checkout foo 2>stderr &&
+ test_branch master &&
+ status_uno_is_clean &&
+ test_i18ngrep ! "^hint: " stderr
+'
+
test_expect_success 'checkout of branch from a single remote succeeds #1' '
git checkout -B master &&
test_might_fail git branch -D bar &&
--
2.17.0.290.gded63e768a
^ permalink raw reply related [flat|nested] 95+ messages in thread
* [PATCH v7 8/8] checkout & worktree: introduce checkout.defaultRemote
2018-06-02 11:50 ` [PATCH v6 " Ævar Arnfjörð Bjarmason
` (7 preceding siblings ...)
2018-06-05 14:40 ` [PATCH v7 7/8] checkout: add advice for ambiguous "checkout <branch>" Ævar Arnfjörð Bjarmason
@ 2018-06-05 14:40 ` Ævar Arnfjörð Bjarmason
8 siblings, 0 replies; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-06-05 14:40 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer,
Eric Sunshine, Ævar Arnfjörð Bjarmason
Introduce a checkout.defaultRemote setting which can be used to
designate a remote to prefer (via checkout.defaultRemote=origin) when
running e.g. "git checkout master" to mean origin/master, even though
there's other remotes that have the "master" branch.
I want this because it's very handy to use this workflow to checkout a
repository and create a topic branch, then get back to a "master" as
retrieved from upstream:
(
cd /tmp &&
rm -rf tbdiff &&
git clone git@github.com:trast/tbdiff.git &&
cd tbdiff &&
git branch -m topic &&
git checkout master
)
That will output:
Branch 'master' set up to track remote branch 'master' from 'origin'.
Switched to a new branch 'master'
But as soon as a new remote is added (e.g. just to inspect something
from someone else) the DWIMery goes away:
(
cd /tmp &&
rm -rf tbdiff &&
git clone git@github.com:trast/tbdiff.git &&
cd tbdiff &&
git branch -m topic &&
git remote add avar git@github.com:avar/tbdiff.git &&
git fetch avar &&
git checkout master
)
Will output (without the advice output added earlier in this series):
error: pathspec 'master' did not match any file(s) known to git.
The new checkout.defaultRemote config allows me to say that whenever
that ambiguity comes up I'd like to prefer "origin", and it'll still
work as though the only remote I had was "origin".
Also adjust the advice.checkoutAmbiguousRemoteBranchName message to
mention this new config setting to the user, the full output on my
git.git is now (the last paragraph is new):
$ ./git --exec-path=$PWD checkout master
error: pathspec 'master' did not match any file(s) known to git.
hint: 'master' matched more than one remote tracking branch.
hint: We found 26 remotes with a reference that matched. So we fell back
hint: on trying to resolve the argument as a path, but failed there too!
hint:
hint: If you meant to check out a remote tracking branch on, e.g. 'origin',
hint: you can do so by fully qualifying the name with the --track option:
hint:
hint: git checkout --track origin/<name>
hint:
hint: If you'd like to always have checkouts of an ambiguous <name> prefer
hint: one remote, e.g. the 'origin' remote, consider setting
hint: checkout.defaultRemote=origin in your config.
I considered splitting this into checkout.defaultRemote and
worktree.defaultRemote, but it's probably less confusing to break our
own rules that anything shared between config should live in core.*
than have two config settings, and I couldn't come up with a short
name under core.* that made sense (core.defaultRemoteForCheckout?).
See also 70c9ac2f19 ("DWIM "git checkout frotz" to "git checkout -b
frotz origin/frotz"", 2009-10-18) which introduced this DWIM feature
to begin with, and 4e85333197 ("worktree: make add <path> <branch>
dwim", 2017-11-26) which added it to git-worktree.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Documentation/config.txt | 21 ++++++++++++++++++++-
Documentation/git-checkout.txt | 9 +++++++++
Documentation/git-worktree.txt | 9 +++++++++
builtin/checkout.c | 12 +++++++++---
checkout.c | 26 ++++++++++++++++++++++++--
t/t2024-checkout-dwim.sh | 18 +++++++++++++++++-
t/t2025-worktree-add.sh | 21 +++++++++++++++++++++
7 files changed, 109 insertions(+), 7 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index dfc0413a84..aef2769211 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -350,7 +350,10 @@ advice.*::
remote tracking branch on more than one remote in
situations where an unambiguous argument would have
otherwise caused a remote-tracking branch to be
- checked out.
+ checked out. See the `checkout.defaultRemote`
+ configuration variable for how to set a given remote
+ to used by default in some situations where this
+ advice would be printed.
amWorkDir::
Advice that shows the location of the patch file when
linkgit:git-am[1] fails to apply it.
@@ -1105,6 +1108,22 @@ browser.<tool>.path::
browse HTML help (see `-w` option in linkgit:git-help[1]) or a
working repository in gitweb (see linkgit:git-instaweb[1]).
+checkout.defaultRemote::
+ When you run 'git checkout <something>' and only have one
+ remote, it may implicitly fall back on checking out and
+ tracking e.g. 'origin/<something>'. This stops working as soon
+ as you have more than one remote with a '<something>'
+ reference. This setting allows for setting the name of a
+ preferred remote that should always win when it comes to
+ disambiguation. The typical use-case is to set this to
+ `origin`.
++
+Currently this is used by linkgit:git-checkout[1] when 'git checkout
+<something>' will checkout the '<something>' branch on another remote,
+and by linkgit:git-worktree[1] when 'git worktree add' refers to a
+remote branch. This setting might be used for other checkout-like
+commands or functionality in the future.
+
clean.requireForce::
A boolean to make git-clean do nothing unless given -f,
-i or -n. Defaults to true.
diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index ca5fc9c798..9db02928c4 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -38,6 +38,15 @@ equivalent to
$ git checkout -b <branch> --track <remote>/<branch>
------------
+
+If the branch exists in multiple remotes and one of them is named by
+the `checkout.defaultRemote` configuration variable, we'll use that
+one for the purposes of disambiguation, even if the `<branch>` isn't
+unique across all remotes. Set it to
+e.g. `checkout.defaultRemote=origin` to always checkout remote
+branches from there if `<branch>` is ambiguous but exists on the
+'origin' remote. See also `checkout.defaultRemote` in
+linkgit:git-config[1].
++
You could omit <branch>, in which case the command degenerates to
"check out the current branch", which is a glorified no-op with
rather expensive side-effects to show only the tracking information,
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index afc6576a14..9c26be40f4 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -60,6 +60,15 @@ with a matching name, treat as equivalent to:
$ git worktree add --track -b <branch> <path> <remote>/<branch>
------------
+
+If the branch exists in multiple remotes and one of them is named by
+the `checkout.defaultRemote` configuration variable, we'll use that
+one for the purposes of disambiguation, even if the `<branch>` isn't
+unique across all remotes. Set it to
+e.g. `checkout.defaultRemote=origin` to always checkout remote
+branches from there if `<branch>` is ambiguous but exists on the
+'origin' remote. See also `checkout.defaultRemote` in
+linkgit:git-config[1].
++
If `<commit-ish>` is omitted and neither `-b` nor `-B` nor `--detach` used,
then, as a convenience, the new worktree is associated with a branch
(call it `<branch>`) named after `$(basename <path>)`. If `<branch>`
diff --git a/builtin/checkout.c b/builtin/checkout.c
index baa027455a..5b357e922a 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -912,8 +912,10 @@ static int parse_branchname_arg(int argc, const char **argv,
* (b) If <something> is _not_ a commit, either "--" is present
* or <something> is not a path, no -t or -b was given, and
* and there is a tracking branch whose name is <something>
- * in one and only one remote, then this is a short-hand to
- * fork local <something> from that remote-tracking branch.
+ * in one and only one remote (or if the branch exists on the
+ * remote named in checkout.defaultRemote), then this is a
+ * short-hand to fork local <something> from that
+ * remote-tracking branch.
*
* (c) Otherwise, if "--" is present, treat it like case (1).
*
@@ -1277,7 +1279,11 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
"If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
"you can do so by fully qualifying the name with the --track option:\n"
"\n"
- " git checkout --track origin/<name>"),
+ " git checkout --track origin/<name>\n"
+ "\n"
+ "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
+ "one remote, e.g. the 'origin' remote, consider setting\n"
+ "checkout.defaultRemote=origin in your config."),
argv[0],
dwim_remotes_matched);
return ret;
diff --git a/checkout.c b/checkout.c
index ee3a7e9c05..c72e9f9773 100644
--- a/checkout.c
+++ b/checkout.c
@@ -2,15 +2,19 @@
#include "remote.h"
#include "refspec.h"
#include "checkout.h"
+#include "config.h"
struct tracking_name_data {
/* const */ char *src_ref;
char *dst_ref;
struct object_id *dst_oid;
int num_matches;
+ const char *default_remote;
+ char *default_dst_ref;
+ struct object_id *default_dst_oid;
};
-#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 0 }
+#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 0, NULL, NULL, NULL }
static int check_tracking_name(struct remote *remote, void *cb_data)
{
@@ -24,6 +28,12 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
return 0;
}
cb->num_matches++;
+ if (cb->default_remote && !strcmp(remote->name, cb->default_remote)) {
+ struct object_id *dst = xmalloc(sizeof(*cb->default_dst_oid));
+ cb->default_dst_ref = xstrdup(query.dst);
+ oidcpy(dst, cb->dst_oid);
+ cb->default_dst_oid = dst;
+ }
if (cb->dst_ref) {
free(query.dst);
return 0;
@@ -36,14 +46,26 @@ const char *unique_tracking_name(const char *name, struct object_id *oid,
int *dwim_remotes_matched)
{
struct tracking_name_data cb_data = TRACKING_NAME_DATA_INIT;
+ const char *default_remote = NULL;
+ if (!git_config_get_string_const("checkout.defaultremote", &default_remote))
+ cb_data.default_remote = default_remote;
cb_data.src_ref = xstrfmt("refs/heads/%s", name);
cb_data.dst_oid = oid;
for_each_remote(check_tracking_name, &cb_data);
if (dwim_remotes_matched)
*dwim_remotes_matched = cb_data.num_matches;
free(cb_data.src_ref);
- if (cb_data.num_matches == 1)
+ free((char *)default_remote);
+ if (cb_data.num_matches == 1) {
+ free(cb_data.default_dst_ref);
+ free(cb_data.default_dst_oid);
return cb_data.dst_ref;
+ }
free(cb_data.dst_ref);
+ if (cb_data.default_dst_ref) {
+ oidcpy(oid, cb_data.default_dst_oid);
+ free(cb_data.default_dst_oid);
+ return cb_data.default_dst_ref;
+ }
return NULL;
}
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index fef263a858..9f17ac92f1 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -87,7 +87,23 @@ test_expect_success 'checkout of branch from multiple remotes fails with advice'
checkout foo 2>stderr &&
test_branch master &&
status_uno_is_clean &&
- test_i18ngrep ! "^hint: " stderr
+ test_i18ngrep ! "^hint: " stderr &&
+ # Make sure the likes of checkout -p do not print this hint
+ git checkout -p foo 2>stderr &&
+ test_i18ngrep ! "^hint: " stderr &&
+ status_uno_is_clean
+'
+
+test_expect_success 'checkout of branch from multiple remotes succeeds with checkout.defaultRemote #1' '
+ git checkout -B master &&
+ status_uno_is_clean &&
+ test_might_fail git branch -D foo &&
+
+ git -c checkout.defaultRemote=repo_a checkout foo &&
+ status_uno_is_clean &&
+ test_branch foo &&
+ test_cmp_rev remotes/repo_a/foo HEAD &&
+ test_branch_upstream foo repo_a foo
'
test_expect_success 'checkout of branch from a single remote succeeds #1' '
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index d2e49f7632..be6e093142 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -402,6 +402,27 @@ test_expect_success '"add" <path> <branch> dwims' '
)
'
+test_expect_success '"add" <path> <branch> dwims with checkout.defaultRemote' '
+ test_when_finished rm -rf repo_upstream repo_dwim foo &&
+ setup_remote_repo repo_upstream repo_dwim &&
+ git init repo_dwim &&
+ (
+ cd repo_dwim &&
+ git remote add repo_upstream2 ../repo_upstream &&
+ git fetch repo_upstream2 &&
+ test_must_fail git worktree add ../foo foo &&
+ git -c checkout.defaultRemote=repo_upstream worktree add ../foo foo &&
+ >status.expect &&
+ git status -uno --porcelain >status.actual &&
+ test_cmp status.expect status.actual
+ ) &&
+ (
+ cd foo &&
+ test_branch_upstream foo repo_upstream foo &&
+ test_cmp_rev refs/remotes/repo_upstream/foo refs/heads/foo
+ )
+'
+
test_expect_success 'git worktree add does not match remote' '
test_when_finished rm -rf repo_a repo_b foo &&
setup_remote_repo repo_a repo_b &&
--
2.17.0.290.gded63e768a
^ permalink raw reply related [flat|nested] 95+ messages in thread
* [PATCH v6 1/8] checkout tests: index should be clean after dwim checkout
2018-06-01 21:10 ` [PATCH v5 0/8] " Ævar Arnfjörð Bjarmason
2018-06-02 11:50 ` [PATCH v6 " Ævar Arnfjörð Bjarmason
@ 2018-06-02 11:50 ` Ævar Arnfjörð Bjarmason
2018-06-02 11:50 ` [PATCH v6 2/8] checkout.h: wrap the arguments to unique_tracking_name() Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
8 siblings, 0 replies; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-06-02 11:50 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer,
Eric Sunshine, Ævar Arnfjörð Bjarmason
Assert that whenever there's a DWIM checkout that the index should be
clean afterwards, in addition to the correct branch being checked-out.
The way the DWIM checkout code in checkout.[ch] works is by looping
over all remotes, and for each remote trying to find if a given
reference name only exists on that remote, or if it exists anywhere
else.
This is done by starting out with a `unique = 1` tracking variable in
a struct shared by the entire loop, which will get set to `0` if the
data reference is not unique.
Thus if we find a match we know the dst_oid member of
tracking_name_data must be correct, since it's associated with the
only reference on the only remote that could have matched our query.
But if there was ever a mismatch there for some reason we might end up
with the correct branch checked out, but at the wrong oid, which would
show whatever the difference between the two staged in the
index (checkout branch A, stage changes from the state of branch B).
So let's amend the tests (mostly added in) 399e4a1c56 ("t2024: Add
tests verifying current DWIM behavior of 'git checkout <branch>'",
2013-04-21) to always assert that "status" is clean after we run
"checkout", that's being done with "-uno" because there's going to be
some untracked files related to the test itself which we don't care
about.
In all these tests (DWIM or otherwise) we start with a clean index, so
these tests are asserting that that's still the case after the
"checkout", failed or otherwise.
Then if we ever run into this sort of regression, either in the
existing code or with a new feature, we'll know.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/t2024-checkout-dwim.sh | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index 3e5ac81bd2..ed32828105 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -23,6 +23,12 @@ test_branch_upstream () {
test_cmp expect.upstream actual.upstream
}
+status_uno_is_clean() {
+ >status.expect &&
+ git status -uno --porcelain >status.actual &&
+ test_cmp status.expect status.actual
+}
+
test_expect_success 'setup' '
test_commit my_master &&
git init repo_a &&
@@ -55,6 +61,7 @@ test_expect_success 'checkout of non-existing branch fails' '
test_might_fail git branch -D xyzzy &&
test_must_fail git checkout xyzzy &&
+ status_uno_is_clean &&
test_must_fail git rev-parse --verify refs/heads/xyzzy &&
test_branch master
'
@@ -64,6 +71,7 @@ test_expect_success 'checkout of branch from multiple remotes fails #1' '
test_might_fail git branch -D foo &&
test_must_fail git checkout foo &&
+ status_uno_is_clean &&
test_must_fail git rev-parse --verify refs/heads/foo &&
test_branch master
'
@@ -73,6 +81,7 @@ test_expect_success 'checkout of branch from a single remote succeeds #1' '
test_might_fail git branch -D bar &&
git checkout bar &&
+ status_uno_is_clean &&
test_branch bar &&
test_cmp_rev remotes/repo_a/bar HEAD &&
test_branch_upstream bar repo_a bar
@@ -83,6 +92,7 @@ test_expect_success 'checkout of branch from a single remote succeeds #2' '
test_might_fail git branch -D baz &&
git checkout baz &&
+ status_uno_is_clean &&
test_branch baz &&
test_cmp_rev remotes/other_b/baz HEAD &&
test_branch_upstream baz repo_b baz
@@ -90,6 +100,7 @@ test_expect_success 'checkout of branch from a single remote succeeds #2' '
test_expect_success '--no-guess suppresses branch auto-vivification' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D bar &&
test_must_fail git checkout --no-guess bar &&
@@ -99,6 +110,7 @@ test_expect_success '--no-guess suppresses branch auto-vivification' '
test_expect_success 'setup more remotes with unconventional refspecs' '
git checkout -B master &&
+ status_uno_is_clean &&
git init repo_c &&
(
cd repo_c &&
@@ -128,27 +140,33 @@ test_expect_success 'setup more remotes with unconventional refspecs' '
test_expect_success 'checkout of branch from multiple remotes fails #2' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D bar &&
test_must_fail git checkout bar &&
+ status_uno_is_clean &&
test_must_fail git rev-parse --verify refs/heads/bar &&
test_branch master
'
test_expect_success 'checkout of branch from multiple remotes fails #3' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D baz &&
test_must_fail git checkout baz &&
+ status_uno_is_clean &&
test_must_fail git rev-parse --verify refs/heads/baz &&
test_branch master
'
test_expect_success 'checkout of branch from a single remote succeeds #3' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D spam &&
git checkout spam &&
+ status_uno_is_clean &&
test_branch spam &&
test_cmp_rev refs/remotes/extra_dir/repo_c/extra_dir/spam HEAD &&
test_branch_upstream spam repo_c spam
@@ -156,9 +174,11 @@ test_expect_success 'checkout of branch from a single remote succeeds #3' '
test_expect_success 'checkout of branch from a single remote succeeds #4' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D eggs &&
git checkout eggs &&
+ status_uno_is_clean &&
test_branch eggs &&
test_cmp_rev refs/repo_d/eggs HEAD &&
test_branch_upstream eggs repo_d eggs
@@ -166,32 +186,38 @@ test_expect_success 'checkout of branch from a single remote succeeds #4' '
test_expect_success 'checkout of branch with a file having the same name fails' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D spam &&
>spam &&
test_must_fail git checkout spam &&
+ status_uno_is_clean &&
test_must_fail git rev-parse --verify refs/heads/spam &&
test_branch master
'
test_expect_success 'checkout of branch with a file in subdir having the same name fails' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D spam &&
>spam &&
mkdir sub &&
mv spam sub/spam &&
test_must_fail git -C sub checkout spam &&
+ status_uno_is_clean &&
test_must_fail git rev-parse --verify refs/heads/spam &&
test_branch master
'
test_expect_success 'checkout <branch> -- succeeds, even if a file with the same name exists' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D spam &&
>spam &&
git checkout spam -- &&
+ status_uno_is_clean &&
test_branch spam &&
test_cmp_rev refs/remotes/extra_dir/repo_c/extra_dir/spam HEAD &&
test_branch_upstream spam repo_c spam
@@ -200,6 +226,7 @@ test_expect_success 'checkout <branch> -- succeeds, even if a file with the same
test_expect_success 'loosely defined local base branch is reported correctly' '
git checkout master &&
+ status_uno_is_clean &&
git branch strict &&
git branch loose &&
git commit --allow-empty -m "a bit more" &&
@@ -210,7 +237,9 @@ test_expect_success 'loosely defined local base branch is reported correctly' '
test_config branch.loose.merge master &&
git checkout strict | sed -e "s/strict/BRANCHNAME/g" >expect &&
+ status_uno_is_clean &&
git checkout loose | sed -e "s/loose/BRANCHNAME/g" >actual &&
+ status_uno_is_clean &&
test_cmp expect actual
'
--
2.17.0.290.gded63e768a
^ permalink raw reply related [flat|nested] 95+ messages in thread
* [PATCH v6 2/8] checkout.h: wrap the arguments to unique_tracking_name()
2018-06-01 21:10 ` [PATCH v5 0/8] " Ævar Arnfjörð Bjarmason
2018-06-02 11:50 ` [PATCH v6 " Ævar Arnfjörð Bjarmason
2018-06-02 11:50 ` [PATCH v6 1/8] checkout tests: index should be clean after dwim checkout Ævar Arnfjörð Bjarmason
@ 2018-06-02 11:50 ` Ævar Arnfjörð Bjarmason
2018-06-02 11:50 ` [PATCH v6 3/8] checkout.c: introduce an *_INIT macro Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
8 siblings, 0 replies; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-06-02 11:50 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer,
Eric Sunshine, Ævar Arnfjörð Bjarmason
The line was too long already, and will be longer still when a later
change adds another argument.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
checkout.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/checkout.h b/checkout.h
index 9980711179..4cd4cd1c23 100644
--- a/checkout.h
+++ b/checkout.h
@@ -8,6 +8,7 @@
* tracking branch. Return the name of the remote if such a branch
* exists, NULL otherwise.
*/
-extern const char *unique_tracking_name(const char *name, struct object_id *oid);
+extern const char *unique_tracking_name(const char *name,
+ struct object_id *oid);
#endif /* CHECKOUT_H */
--
2.17.0.290.gded63e768a
^ permalink raw reply related [flat|nested] 95+ messages in thread
* [PATCH v6 3/8] checkout.c: introduce an *_INIT macro
2018-06-01 21:10 ` [PATCH v5 0/8] " Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2018-06-02 11:50 ` [PATCH v6 2/8] checkout.h: wrap the arguments to unique_tracking_name() Ævar Arnfjörð Bjarmason
@ 2018-06-02 11:50 ` Ævar Arnfjörð Bjarmason
2018-06-02 11:50 ` [PATCH v6 4/8] checkout.c]: change "unique" member to "num_matches" Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
8 siblings, 0 replies; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-06-02 11:50 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer,
Eric Sunshine, Ævar Arnfjörð Bjarmason
Add an *_INIT macro for the tracking_name_data similar to what exists
elsewhere in the codebase, e.g. OID_ARRAY_INIT in sha1-array.h. This
will make it more idiomatic in later changes to add more fields to the
struct & its initialization macro.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
checkout.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/checkout.c b/checkout.c
index bdefc888ba..80e430cda8 100644
--- a/checkout.c
+++ b/checkout.c
@@ -10,6 +10,8 @@ struct tracking_name_data {
int unique;
};
+#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 1 }
+
static int check_tracking_name(struct remote *remote, void *cb_data)
{
struct tracking_name_data *cb = cb_data;
@@ -32,7 +34,7 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
const char *unique_tracking_name(const char *name, struct object_id *oid)
{
- struct tracking_name_data cb_data = { NULL, NULL, NULL, 1 };
+ struct tracking_name_data cb_data = TRACKING_NAME_DATA_INIT;
cb_data.src_ref = xstrfmt("refs/heads/%s", name);
cb_data.dst_oid = oid;
for_each_remote(check_tracking_name, &cb_data);
--
2.17.0.290.gded63e768a
^ permalink raw reply related [flat|nested] 95+ messages in thread
* [PATCH v6 4/8] checkout.c]: change "unique" member to "num_matches"
2018-06-01 21:10 ` [PATCH v5 0/8] " Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2018-06-02 11:50 ` [PATCH v6 3/8] checkout.c: introduce an *_INIT macro Ævar Arnfjörð Bjarmason
@ 2018-06-02 11:50 ` Ævar Arnfjörð Bjarmason
2018-06-02 11:50 ` [PATCH v6 5/8] checkout: pass the "num_matches" up to callers Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
8 siblings, 0 replies; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-06-02 11:50 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer,
Eric Sunshine, Ævar Arnfjörð Bjarmason
Internally track how many matches we find in the check_tracking_name()
callback. Nothing uses this now, but it will be made use of in a later
change.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
checkout.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/checkout.c b/checkout.c
index 80e430cda8..7662a39a62 100644
--- a/checkout.c
+++ b/checkout.c
@@ -7,10 +7,10 @@ struct tracking_name_data {
/* const */ char *src_ref;
char *dst_ref;
struct object_id *dst_oid;
- int unique;
+ int num_matches;
};
-#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 1 }
+#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 0 }
static int check_tracking_name(struct remote *remote, void *cb_data)
{
@@ -23,9 +23,9 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
free(query.dst);
return 0;
}
+ cb->num_matches++;
if (cb->dst_ref) {
free(query.dst);
- cb->unique = 0;
return 0;
}
cb->dst_ref = query.dst;
@@ -39,7 +39,7 @@ const char *unique_tracking_name(const char *name, struct object_id *oid)
cb_data.dst_oid = oid;
for_each_remote(check_tracking_name, &cb_data);
free(cb_data.src_ref);
- if (cb_data.unique)
+ if (cb_data.num_matches == 1)
return cb_data.dst_ref;
free(cb_data.dst_ref);
return NULL;
--
2.17.0.290.gded63e768a
^ permalink raw reply related [flat|nested] 95+ messages in thread
* [PATCH v6 5/8] checkout: pass the "num_matches" up to callers
2018-06-01 21:10 ` [PATCH v5 0/8] " Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2018-06-02 11:50 ` [PATCH v6 4/8] checkout.c]: change "unique" member to "num_matches" Ævar Arnfjörð Bjarmason
@ 2018-06-02 11:50 ` Ævar Arnfjörð Bjarmason
2018-06-02 11:50 ` [PATCH v6 6/8] builtin/checkout.c: use "ret" variable for return Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
8 siblings, 0 replies; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-06-02 11:50 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer,
Eric Sunshine, Ævar Arnfjörð Bjarmason
Pass the previously added "num_matches" struct value up to the callers
of unique_tracking_name(). This will allow callers to optionally print
better error messages in a later change.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/checkout.c | 10 +++++++---
builtin/worktree.c | 4 ++--
checkout.c | 5 ++++-
checkout.h | 3 ++-
4 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2e1d2376d2..72457fb7d5 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -878,7 +878,8 @@ static int parse_branchname_arg(int argc, const char **argv,
int dwim_new_local_branch_ok,
struct branch_info *new_branch_info,
struct checkout_opts *opts,
- struct object_id *rev)
+ struct object_id *rev,
+ int *dwim_remotes_matched)
{
struct tree **source_tree = &opts->source_tree;
const char **new_branch = &opts->new_branch;
@@ -972,7 +973,8 @@ static int parse_branchname_arg(int argc, const char **argv,
recover_with_dwim = 0;
if (recover_with_dwim) {
- const char *remote = unique_tracking_name(arg, rev);
+ const char *remote = unique_tracking_name(arg, rev,
+ dwim_remotes_matched);
if (remote) {
*new_branch = arg;
arg = remote;
@@ -1109,6 +1111,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
struct branch_info new_branch_info;
char *conflict_style = NULL;
int dwim_new_local_branch = 1;
+ int dwim_remotes_matched = 0;
struct option options[] = {
OPT__QUIET(&opts.quiet, N_("suppress progress reporting")),
OPT_STRING('b', NULL, &opts.new_branch, N_("branch"),
@@ -1219,7 +1222,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
opts.track == BRANCH_TRACK_UNSPECIFIED &&
!opts.new_branch;
int n = parse_branchname_arg(argc, argv, dwim_ok,
- &new_branch_info, &opts, &rev);
+ &new_branch_info, &opts, &rev,
+ &dwim_remotes_matched);
argv += n;
argc -= n;
}
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 5c7d2bb180..a763dbdccb 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -412,7 +412,7 @@ static const char *dwim_branch(const char *path, const char **new_branch)
if (guess_remote) {
struct object_id oid;
const char *remote =
- unique_tracking_name(*new_branch, &oid);
+ unique_tracking_name(*new_branch, &oid, NULL);
return remote;
}
return NULL;
@@ -484,7 +484,7 @@ static int add(int ac, const char **av, const char *prefix)
commit = lookup_commit_reference_by_name(branch);
if (!commit) {
- remote = unique_tracking_name(branch, &oid);
+ remote = unique_tracking_name(branch, &oid, NULL);
if (remote) {
new_branch = branch;
branch = remote;
diff --git a/checkout.c b/checkout.c
index 7662a39a62..ee3a7e9c05 100644
--- a/checkout.c
+++ b/checkout.c
@@ -32,12 +32,15 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
return 0;
}
-const char *unique_tracking_name(const char *name, struct object_id *oid)
+const char *unique_tracking_name(const char *name, struct object_id *oid,
+ int *dwim_remotes_matched)
{
struct tracking_name_data cb_data = TRACKING_NAME_DATA_INIT;
cb_data.src_ref = xstrfmt("refs/heads/%s", name);
cb_data.dst_oid = oid;
for_each_remote(check_tracking_name, &cb_data);
+ if (dwim_remotes_matched)
+ *dwim_remotes_matched = cb_data.num_matches;
free(cb_data.src_ref);
if (cb_data.num_matches == 1)
return cb_data.dst_ref;
diff --git a/checkout.h b/checkout.h
index 4cd4cd1c23..6b2073310c 100644
--- a/checkout.h
+++ b/checkout.h
@@ -9,6 +9,7 @@
* exists, NULL otherwise.
*/
extern const char *unique_tracking_name(const char *name,
- struct object_id *oid);
+ struct object_id *oid,
+ int *dwim_remotes_matched);
#endif /* CHECKOUT_H */
--
2.17.0.290.gded63e768a
^ permalink raw reply related [flat|nested] 95+ messages in thread
* [PATCH v6 6/8] builtin/checkout.c: use "ret" variable for return
2018-06-01 21:10 ` [PATCH v5 0/8] " Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2018-06-02 11:50 ` [PATCH v6 5/8] checkout: pass the "num_matches" up to callers Ævar Arnfjörð Bjarmason
@ 2018-06-02 11:50 ` Ævar Arnfjörð Bjarmason
2018-06-02 11:50 ` [PATCH v6 7/8] checkout: add advice for ambiguous "checkout <branch>" Ævar Arnfjörð Bjarmason
2018-06-02 11:50 ` [PATCH v6 8/8] checkout & worktree: introduce checkout.defaultRemote Ævar Arnfjörð Bjarmason
8 siblings, 0 replies; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-06-02 11:50 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer,
Eric Sunshine, Ævar Arnfjörð Bjarmason
There is no point in doing this right now, but in later change the
"ret" variable will be inspected. This change makes that meaningful
change smaller.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/checkout.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 72457fb7d5..8c93c55cbc 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1265,8 +1265,10 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
}
UNLEAK(opts);
- if (opts.patch_mode || opts.pathspec.nr)
- return checkout_paths(&opts, new_branch_info.name);
- else
+ if (opts.patch_mode || opts.pathspec.nr) {
+ int ret = checkout_paths(&opts, new_branch_info.name);
+ return ret;
+ } else {
return checkout_branch(&opts, &new_branch_info);
+ }
}
--
2.17.0.290.gded63e768a
^ permalink raw reply related [flat|nested] 95+ messages in thread
* [PATCH v6 7/8] checkout: add advice for ambiguous "checkout <branch>"
2018-06-01 21:10 ` [PATCH v5 0/8] " Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2018-06-02 11:50 ` [PATCH v6 6/8] builtin/checkout.c: use "ret" variable for return Ævar Arnfjörð Bjarmason
@ 2018-06-02 11:50 ` Ævar Arnfjörð Bjarmason
2018-06-02 11:50 ` [PATCH v6 8/8] checkout & worktree: introduce checkout.defaultRemote Ævar Arnfjörð Bjarmason
8 siblings, 0 replies; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-06-02 11:50 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer,
Eric Sunshine, Ævar Arnfjörð Bjarmason
As the "checkout" documentation describes:
If <branch> is not found but there does exist a tracking branch in
exactly one remote (call it <remote>) with a matching name, treat
as equivalent to [...] <remote>/<branch.
This is a really useful feature. The problem is that when you add
another remote (e.g. a fork), git won't find a unique branch name
anymore, and will instead print this unhelpful message:
$ git checkout master
error: pathspec 'master' did not match any file(s) known to git
Now it will, on my git.git checkout, print:
$ ./git --exec-path=$PWD checkout master
error: pathspec 'master' did not match any file(s) known to git.
hint: 'master' matched more than one remote tracking branch.
hint: We found 26 remotes with a reference that matched. So we fell back
hint: on trying to resolve the argument as a path, but failed there too!
hint:
hint: If you meant to check out a remote tracking branch on, e.g. 'origin',
hint: you can do so by fully qualifying the name with the --track option:
hint:
hint: git checkout --track origin/<name>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Documentation/config.txt | 7 +++++++
advice.c | 2 ++
advice.h | 1 +
builtin/checkout.c | 13 +++++++++++++
t/t2024-checkout-dwim.sh | 14 ++++++++++++++
5 files changed, 37 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index ab641bf5a9..dfc0413a84 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -344,6 +344,13 @@ advice.*::
Advice shown when you used linkgit:git-checkout[1] to
move to the detach HEAD state, to instruct how to create
a local branch after the fact.
+ checkoutAmbiguousRemoteBranchName::
+ Advice shown when the argument to
+ linkgit:git-checkout[1] ambiguously resolves to a
+ remote tracking branch on more than one remote in
+ situations where an unambiguous argument would have
+ otherwise caused a remote-tracking branch to be
+ checked out.
amWorkDir::
Advice that shows the location of the patch file when
linkgit:git-am[1] fails to apply it.
diff --git a/advice.c b/advice.c
index 370a56d054..75e7dede90 100644
--- a/advice.c
+++ b/advice.c
@@ -21,6 +21,7 @@ int advice_add_embedded_repo = 1;
int advice_ignored_hook = 1;
int advice_waiting_for_editor = 1;
int advice_graft_file_deprecated = 1;
+int advice_checkout_ambiguous_remote_branch_name = 1;
static int advice_use_color = -1;
static char advice_colors[][COLOR_MAXLEN] = {
@@ -72,6 +73,7 @@ static struct {
{ "ignoredhook", &advice_ignored_hook },
{ "waitingforeditor", &advice_waiting_for_editor },
{ "graftfiledeprecated", &advice_graft_file_deprecated },
+ { "checkoutambiguousremotebranchname", &advice_checkout_ambiguous_remote_branch_name },
/* make this an alias for backward compatibility */
{ "pushnonfastforward", &advice_push_update_rejected }
diff --git a/advice.h b/advice.h
index 9f5064e82a..4d11d51d43 100644
--- a/advice.h
+++ b/advice.h
@@ -22,6 +22,7 @@ extern int advice_add_embedded_repo;
extern int advice_ignored_hook;
extern int advice_waiting_for_editor;
extern int advice_graft_file_deprecated;
+extern int advice_checkout_ambiguous_remote_branch_name;
int git_default_advice_config(const char *var, const char *value);
__attribute__((format (printf, 1, 2)))
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8c93c55cbc..baa027455a 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -22,6 +22,7 @@
#include "resolve-undo.h"
#include "submodule-config.h"
#include "submodule.h"
+#include "advice.h"
static const char * const checkout_usage[] = {
N_("git checkout [<options>] <branch>"),
@@ -1267,6 +1268,18 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
UNLEAK(opts);
if (opts.patch_mode || opts.pathspec.nr) {
int ret = checkout_paths(&opts, new_branch_info.name);
+ if (ret && dwim_remotes_matched > 1 &&
+ advice_checkout_ambiguous_remote_branch_name)
+ advise(_("'%s' matched more than one remote tracking branch.\n"
+ "We found %d remotes with a reference that matched. So we fell back\n"
+ "on trying to resolve the argument as a path, but failed there too!\n"
+ "\n"
+ "If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
+ "you can do so by fully qualifying the name with the --track option:\n"
+ "\n"
+ " git checkout --track origin/<name>"),
+ argv[0],
+ dwim_remotes_matched);
return ret;
} else {
return checkout_branch(&opts, &new_branch_info);
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index ed32828105..fef263a858 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -76,6 +76,20 @@ test_expect_success 'checkout of branch from multiple remotes fails #1' '
test_branch master
'
+test_expect_success 'checkout of branch from multiple remotes fails with advice' '
+ git checkout -B master &&
+ test_might_fail git branch -D foo &&
+ test_must_fail git checkout foo 2>stderr &&
+ test_branch master &&
+ status_uno_is_clean &&
+ test_i18ngrep "^hint: " stderr &&
+ test_must_fail git -c advice.checkoutAmbiguousRemoteBranchName=false \
+ checkout foo 2>stderr &&
+ test_branch master &&
+ status_uno_is_clean &&
+ test_i18ngrep ! "^hint: " stderr
+'
+
test_expect_success 'checkout of branch from a single remote succeeds #1' '
git checkout -B master &&
test_might_fail git branch -D bar &&
--
2.17.0.290.gded63e768a
^ permalink raw reply related [flat|nested] 95+ messages in thread
* [PATCH v6 8/8] checkout & worktree: introduce checkout.defaultRemote
2018-06-01 21:10 ` [PATCH v5 0/8] " Ævar Arnfjörð Bjarmason
` (7 preceding siblings ...)
2018-06-02 11:50 ` [PATCH v6 7/8] checkout: add advice for ambiguous "checkout <branch>" Ævar Arnfjörð Bjarmason
@ 2018-06-02 11:50 ` Ævar Arnfjörð Bjarmason
2018-06-03 7:58 ` Eric Sunshine
8 siblings, 1 reply; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-06-02 11:50 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer,
Eric Sunshine, Ævar Arnfjörð Bjarmason
Introduce a checkout.defaultRemote setting which can be used to
designate a remote to prefer (via checkout.defaultRemote=origin) when
running e.g. "git checkout master" to mean origin/master, even though
there's other remotes that have the "master" branch.
I want this because it's very handy to use this workflow to checkout a
repository and create a topic branch, then get back to a "master" as
retrieved from upstream:
(
cd /tmp &&
rm -rf tbdiff &&
git clone git@github.com:trast/tbdiff.git &&
cd tbdiff &&
git branch -m topic &&
git checkout master
)
That will output:
Branch 'master' set up to track remote branch 'master' from 'origin'.
Switched to a new branch 'master'
But as soon as a new remote is added (e.g. just to inspect something
from someone else) the DWIMery goes away:
(
cd /tmp &&
rm -rf tbdiff &&
git clone git@github.com:trast/tbdiff.git &&
cd tbdiff &&
git branch -m topic &&
git remote add avar git@github.com:avar/tbdiff.git &&
git fetch avar &&
git checkout master
)
Will output (without the advice output added earlier in this series):
error: pathspec 'master' did not match any file(s) known to git.
The new checkout.defaultRemote config allows me to say that whenever
that ambiguity comes up I'd like to prefer "origin", and it'll still
work as though the only remote I had was "origin".
Also adjust the advice.checkoutAmbiguousRemoteBranchName message to
mention this new config setting to the user, the full output on my
git.git is now (the last paragraph is new):
$ ./git --exec-path=$PWD checkout master
error: pathspec 'master' did not match any file(s) known to git.
hint: The argument 'master' matched more than one remote tracking branch.
hint: We found 26 remotes with a reference that matched. So we fell back
hint: on trying to resolve the argument as a path, but failed there too!
hint:
hint: If you meant to check out a remote tracking branch on e.g. 'origin'
hint: you can do so by fully-qualifying the name with the --track option:
hint:
hint: git checkout --track origin/<name>
hint:
hint: If you'd like to always have checkouts of an ambiguous <name> prefer
hint: one remote, e.g. the 'origin' remote, consider setting
hint: checkout.defaultRemote=origin in your config.
I considered splitting this into checkout.defaultRemote and
worktree.defaultRemote, but it's probably less confusing to break our
own rules that anything shared between config should live in core.*
than have two config settings, and I couldn't come up with a short
name under core.* that made sense (core.defaultRemoteForCheckout?).
See also 70c9ac2f19 ("DWIM "git checkout frotz" to "git checkout -b
frotz origin/frotz"", 2009-10-18) which introduced this DWIM feature
to begin with, and 4e85333197 ("worktree: make add <path> <branch>
dwim", 2017-11-26) which added it to git-worktree.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Documentation/config.txt | 21 ++++++++++++++++++++-
Documentation/git-checkout.txt | 9 +++++++++
Documentation/git-worktree.txt | 9 +++++++++
builtin/checkout.c | 12 +++++++++---
checkout.c | 26 ++++++++++++++++++++++++--
t/t2024-checkout-dwim.sh | 18 +++++++++++++++++-
t/t2025-worktree-add.sh | 21 +++++++++++++++++++++
7 files changed, 109 insertions(+), 7 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index dfc0413a84..aef2769211 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -350,7 +350,10 @@ advice.*::
remote tracking branch on more than one remote in
situations where an unambiguous argument would have
otherwise caused a remote-tracking branch to be
- checked out.
+ checked out. See the `checkout.defaultRemote`
+ configuration variable for how to set a given remote
+ to used by default in some situations where this
+ advice would be printed.
amWorkDir::
Advice that shows the location of the patch file when
linkgit:git-am[1] fails to apply it.
@@ -1105,6 +1108,22 @@ browser.<tool>.path::
browse HTML help (see `-w` option in linkgit:git-help[1]) or a
working repository in gitweb (see linkgit:git-instaweb[1]).
+checkout.defaultRemote::
+ When you run 'git checkout <something>' and only have one
+ remote, it may implicitly fall back on checking out and
+ tracking e.g. 'origin/<something>'. This stops working as soon
+ as you have more than one remote with a '<something>'
+ reference. This setting allows for setting the name of a
+ preferred remote that should always win when it comes to
+ disambiguation. The typical use-case is to set this to
+ `origin`.
++
+Currently this is used by linkgit:git-checkout[1] when 'git checkout
+<something>' will checkout the '<something>' branch on another remote,
+and by linkgit:git-worktree[1] when 'git worktree add' refers to a
+remote branch. This setting might be used for other checkout-like
+commands or functionality in the future.
+
clean.requireForce::
A boolean to make git-clean do nothing unless given -f,
-i or -n. Defaults to true.
diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index ca5fc9c798..9db02928c4 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -38,6 +38,15 @@ equivalent to
$ git checkout -b <branch> --track <remote>/<branch>
------------
+
+If the branch exists in multiple remotes and one of them is named by
+the `checkout.defaultRemote` configuration variable, we'll use that
+one for the purposes of disambiguation, even if the `<branch>` isn't
+unique across all remotes. Set it to
+e.g. `checkout.defaultRemote=origin` to always checkout remote
+branches from there if `<branch>` is ambiguous but exists on the
+'origin' remote. See also `checkout.defaultRemote` in
+linkgit:git-config[1].
++
You could omit <branch>, in which case the command degenerates to
"check out the current branch", which is a glorified no-op with
rather expensive side-effects to show only the tracking information,
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index afc6576a14..9c26be40f4 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -60,6 +60,15 @@ with a matching name, treat as equivalent to:
$ git worktree add --track -b <branch> <path> <remote>/<branch>
------------
+
+If the branch exists in multiple remotes and one of them is named by
+the `checkout.defaultRemote` configuration variable, we'll use that
+one for the purposes of disambiguation, even if the `<branch>` isn't
+unique across all remotes. Set it to
+e.g. `checkout.defaultRemote=origin` to always checkout remote
+branches from there if `<branch>` is ambiguous but exists on the
+'origin' remote. See also `checkout.defaultRemote` in
+linkgit:git-config[1].
++
If `<commit-ish>` is omitted and neither `-b` nor `-B` nor `--detach` used,
then, as a convenience, the new worktree is associated with a branch
(call it `<branch>`) named after `$(basename <path>)`. If `<branch>`
diff --git a/builtin/checkout.c b/builtin/checkout.c
index baa027455a..5b357e922a 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -912,8 +912,10 @@ static int parse_branchname_arg(int argc, const char **argv,
* (b) If <something> is _not_ a commit, either "--" is present
* or <something> is not a path, no -t or -b was given, and
* and there is a tracking branch whose name is <something>
- * in one and only one remote, then this is a short-hand to
- * fork local <something> from that remote-tracking branch.
+ * in one and only one remote (or if the branch exists on the
+ * remote named in checkout.defaultRemote), then this is a
+ * short-hand to fork local <something> from that
+ * remote-tracking branch.
*
* (c) Otherwise, if "--" is present, treat it like case (1).
*
@@ -1277,7 +1279,11 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
"If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
"you can do so by fully qualifying the name with the --track option:\n"
"\n"
- " git checkout --track origin/<name>"),
+ " git checkout --track origin/<name>\n"
+ "\n"
+ "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
+ "one remote, e.g. the 'origin' remote, consider setting\n"
+ "checkout.defaultRemote=origin in your config."),
argv[0],
dwim_remotes_matched);
return ret;
diff --git a/checkout.c b/checkout.c
index ee3a7e9c05..c72e9f9773 100644
--- a/checkout.c
+++ b/checkout.c
@@ -2,15 +2,19 @@
#include "remote.h"
#include "refspec.h"
#include "checkout.h"
+#include "config.h"
struct tracking_name_data {
/* const */ char *src_ref;
char *dst_ref;
struct object_id *dst_oid;
int num_matches;
+ const char *default_remote;
+ char *default_dst_ref;
+ struct object_id *default_dst_oid;
};
-#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 0 }
+#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 0, NULL, NULL, NULL }
static int check_tracking_name(struct remote *remote, void *cb_data)
{
@@ -24,6 +28,12 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
return 0;
}
cb->num_matches++;
+ if (cb->default_remote && !strcmp(remote->name, cb->default_remote)) {
+ struct object_id *dst = xmalloc(sizeof(*cb->default_dst_oid));
+ cb->default_dst_ref = xstrdup(query.dst);
+ oidcpy(dst, cb->dst_oid);
+ cb->default_dst_oid = dst;
+ }
if (cb->dst_ref) {
free(query.dst);
return 0;
@@ -36,14 +46,26 @@ const char *unique_tracking_name(const char *name, struct object_id *oid,
int *dwim_remotes_matched)
{
struct tracking_name_data cb_data = TRACKING_NAME_DATA_INIT;
+ const char *default_remote = NULL;
+ if (!git_config_get_string_const("checkout.defaultremote", &default_remote))
+ cb_data.default_remote = default_remote;
cb_data.src_ref = xstrfmt("refs/heads/%s", name);
cb_data.dst_oid = oid;
for_each_remote(check_tracking_name, &cb_data);
if (dwim_remotes_matched)
*dwim_remotes_matched = cb_data.num_matches;
free(cb_data.src_ref);
- if (cb_data.num_matches == 1)
+ free((char *)default_remote);
+ if (cb_data.num_matches == 1) {
+ free(cb_data.default_dst_ref);
+ free(cb_data.default_dst_oid);
return cb_data.dst_ref;
+ }
free(cb_data.dst_ref);
+ if (cb_data.default_dst_ref) {
+ oidcpy(oid, cb_data.default_dst_oid);
+ free(cb_data.default_dst_oid);
+ return cb_data.default_dst_ref;
+ }
return NULL;
}
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index fef263a858..1495c248a7 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -87,7 +87,23 @@ test_expect_success 'checkout of branch from multiple remotes fails with advice'
checkout foo 2>stderr &&
test_branch master &&
status_uno_is_clean &&
- test_i18ngrep ! "^hint: " stderr
+ test_i18ngrep ! "^hint: " stderr &&
+ # Make sure the likes of checkout -p don not print this hint
+ git checkout -p foo 2>stderr &&
+ test_i18ngrep ! "^hint: " stderr &&
+ status_uno_is_clean
+'
+
+test_expect_success 'checkout of branch from multiple remotes succeeds with checkout.defaultRemote #1' '
+ git checkout -B master &&
+ status_uno_is_clean &&
+ test_might_fail git branch -D foo &&
+
+ git -c checkout.defaultRemote=repo_a checkout foo &&
+ status_uno_is_clean &&
+ test_branch foo &&
+ test_cmp_rev remotes/repo_a/foo HEAD &&
+ test_branch_upstream foo repo_a foo
'
test_expect_success 'checkout of branch from a single remote succeeds #1' '
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index d2e49f7632..be6e093142 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -402,6 +402,27 @@ test_expect_success '"add" <path> <branch> dwims' '
)
'
+test_expect_success '"add" <path> <branch> dwims with checkout.defaultRemote' '
+ test_when_finished rm -rf repo_upstream repo_dwim foo &&
+ setup_remote_repo repo_upstream repo_dwim &&
+ git init repo_dwim &&
+ (
+ cd repo_dwim &&
+ git remote add repo_upstream2 ../repo_upstream &&
+ git fetch repo_upstream2 &&
+ test_must_fail git worktree add ../foo foo &&
+ git -c checkout.defaultRemote=repo_upstream worktree add ../foo foo &&
+ >status.expect &&
+ git status -uno --porcelain >status.actual &&
+ test_cmp status.expect status.actual
+ ) &&
+ (
+ cd foo &&
+ test_branch_upstream foo repo_upstream foo &&
+ test_cmp_rev refs/remotes/repo_upstream/foo refs/heads/foo
+ )
+'
+
test_expect_success 'git worktree add does not match remote' '
test_when_finished rm -rf repo_a repo_b foo &&
setup_remote_repo repo_a repo_b &&
--
2.17.0.290.gded63e768a
^ permalink raw reply related [flat|nested] 95+ messages in thread
* Re: [PATCH v6 8/8] checkout & worktree: introduce checkout.defaultRemote
2018-06-02 11:50 ` [PATCH v6 8/8] checkout & worktree: introduce checkout.defaultRemote Ævar Arnfjörð Bjarmason
@ 2018-06-03 7:58 ` Eric Sunshine
0 siblings, 0 replies; 95+ messages in thread
From: Eric Sunshine @ 2018-06-03 7:58 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git List, Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer
On Sat, Jun 2, 2018 at 7:50 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Introduce a checkout.defaultRemote setting which can be used to
> designate a remote to prefer (via checkout.defaultRemote=origin) when
> running e.g. "git checkout master" to mean origin/master, even though
> there's other remotes that have the "master" branch.
> [...]
> Also adjust the advice.checkoutAmbiguousRemoteBranchName message to
> mention this new config setting to the user, the full output on my
> git.git is now (the last paragraph is new):
>
> $ ./git --exec-path=$PWD checkout master
> error: pathspec 'master' did not match any file(s) known to git.
> hint: The argument 'master' matched more than one remote tracking branch.
In v6, the "The argument" prefix has been dropped from the hint, so
this commit message needs a tweak to match.
> hint: We found 26 remotes with a reference that matched. So we fell back
> hint: on trying to resolve the argument as a path, but failed there too!
> [...]
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
> @@ -87,7 +87,23 @@ test_expect_success 'checkout of branch from multiple remotes fails with advice'
> - test_i18ngrep ! "^hint: " stderr
> + test_i18ngrep ! "^hint: " stderr &&
> + # Make sure the likes of checkout -p don not print this hint
s/don/do/
> + git checkout -p foo 2>stderr &&
> + test_i18ngrep ! "^hint: " stderr &&
> + status_uno_is_clean
> +'
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH v5 1/8] checkout tests: index should be clean after dwim checkout
2018-05-31 19:52 ` [PATCH v4 0/9] ambiguous checkout UI & checkout.defaultRemote Ævar Arnfjörð Bjarmason
2018-06-01 21:10 ` [PATCH v5 0/8] " Ævar Arnfjörð Bjarmason
@ 2018-06-01 21:10 ` Ævar Arnfjörð Bjarmason
2018-06-01 21:10 ` [PATCH v5 2/8] checkout.h: wrap the arguments to unique_tracking_name() Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
8 siblings, 0 replies; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-06-01 21:10 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer,
Eric Sunshine, Ævar Arnfjörð Bjarmason
Assert that whenever there's a DWIM checkout that the index should be
clean afterwards, in addition to the correct branch being checked-out.
The way the DWIM checkout code in checkout.[ch] works is by looping
over all remotes, and for each remote trying to find if a given
reference name only exists on that remote, or if it exists anywhere
else.
This is done by starting out with a `unique = 1` tracking variable in
a struct shared by the entire loop, which will get set to `0` if the
data reference is not unique.
Thus if we find a match we know the dst_oid member of
tracking_name_data must be correct, since it's associated with the
only reference on the only remote that could have matched our query.
But if there was ever a mismatch there for some reason we might end up
with the correct branch checked out, but at the wrong oid, which would
show whatever the difference between the two staged in the
index (checkout branch A, stage changes from the state of branch B).
So let's amend the tests (mostly added in) 399e4a1c56 ("t2024: Add
tests verifying current DWIM behavior of 'git checkout <branch>'",
2013-04-21) to always assert that "status" is clean after we run
"checkout", that's being done with "-uno" because there's going to be
some untracked files related to the test itself which we don't care
about.
In all these tests (DWIM or otherwise) we start with a clean index, so
these tests are asserting that that's still the case after the
"checkout", failed or otherwise.
Then if we ever run into this sort of regression, either in the
existing code or with a new feature, we'll know.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/t2024-checkout-dwim.sh | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index 3e5ac81bd2..ed32828105 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -23,6 +23,12 @@ test_branch_upstream () {
test_cmp expect.upstream actual.upstream
}
+status_uno_is_clean() {
+ >status.expect &&
+ git status -uno --porcelain >status.actual &&
+ test_cmp status.expect status.actual
+}
+
test_expect_success 'setup' '
test_commit my_master &&
git init repo_a &&
@@ -55,6 +61,7 @@ test_expect_success 'checkout of non-existing branch fails' '
test_might_fail git branch -D xyzzy &&
test_must_fail git checkout xyzzy &&
+ status_uno_is_clean &&
test_must_fail git rev-parse --verify refs/heads/xyzzy &&
test_branch master
'
@@ -64,6 +71,7 @@ test_expect_success 'checkout of branch from multiple remotes fails #1' '
test_might_fail git branch -D foo &&
test_must_fail git checkout foo &&
+ status_uno_is_clean &&
test_must_fail git rev-parse --verify refs/heads/foo &&
test_branch master
'
@@ -73,6 +81,7 @@ test_expect_success 'checkout of branch from a single remote succeeds #1' '
test_might_fail git branch -D bar &&
git checkout bar &&
+ status_uno_is_clean &&
test_branch bar &&
test_cmp_rev remotes/repo_a/bar HEAD &&
test_branch_upstream bar repo_a bar
@@ -83,6 +92,7 @@ test_expect_success 'checkout of branch from a single remote succeeds #2' '
test_might_fail git branch -D baz &&
git checkout baz &&
+ status_uno_is_clean &&
test_branch baz &&
test_cmp_rev remotes/other_b/baz HEAD &&
test_branch_upstream baz repo_b baz
@@ -90,6 +100,7 @@ test_expect_success 'checkout of branch from a single remote succeeds #2' '
test_expect_success '--no-guess suppresses branch auto-vivification' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D bar &&
test_must_fail git checkout --no-guess bar &&
@@ -99,6 +110,7 @@ test_expect_success '--no-guess suppresses branch auto-vivification' '
test_expect_success 'setup more remotes with unconventional refspecs' '
git checkout -B master &&
+ status_uno_is_clean &&
git init repo_c &&
(
cd repo_c &&
@@ -128,27 +140,33 @@ test_expect_success 'setup more remotes with unconventional refspecs' '
test_expect_success 'checkout of branch from multiple remotes fails #2' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D bar &&
test_must_fail git checkout bar &&
+ status_uno_is_clean &&
test_must_fail git rev-parse --verify refs/heads/bar &&
test_branch master
'
test_expect_success 'checkout of branch from multiple remotes fails #3' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D baz &&
test_must_fail git checkout baz &&
+ status_uno_is_clean &&
test_must_fail git rev-parse --verify refs/heads/baz &&
test_branch master
'
test_expect_success 'checkout of branch from a single remote succeeds #3' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D spam &&
git checkout spam &&
+ status_uno_is_clean &&
test_branch spam &&
test_cmp_rev refs/remotes/extra_dir/repo_c/extra_dir/spam HEAD &&
test_branch_upstream spam repo_c spam
@@ -156,9 +174,11 @@ test_expect_success 'checkout of branch from a single remote succeeds #3' '
test_expect_success 'checkout of branch from a single remote succeeds #4' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D eggs &&
git checkout eggs &&
+ status_uno_is_clean &&
test_branch eggs &&
test_cmp_rev refs/repo_d/eggs HEAD &&
test_branch_upstream eggs repo_d eggs
@@ -166,32 +186,38 @@ test_expect_success 'checkout of branch from a single remote succeeds #4' '
test_expect_success 'checkout of branch with a file having the same name fails' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D spam &&
>spam &&
test_must_fail git checkout spam &&
+ status_uno_is_clean &&
test_must_fail git rev-parse --verify refs/heads/spam &&
test_branch master
'
test_expect_success 'checkout of branch with a file in subdir having the same name fails' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D spam &&
>spam &&
mkdir sub &&
mv spam sub/spam &&
test_must_fail git -C sub checkout spam &&
+ status_uno_is_clean &&
test_must_fail git rev-parse --verify refs/heads/spam &&
test_branch master
'
test_expect_success 'checkout <branch> -- succeeds, even if a file with the same name exists' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D spam &&
>spam &&
git checkout spam -- &&
+ status_uno_is_clean &&
test_branch spam &&
test_cmp_rev refs/remotes/extra_dir/repo_c/extra_dir/spam HEAD &&
test_branch_upstream spam repo_c spam
@@ -200,6 +226,7 @@ test_expect_success 'checkout <branch> -- succeeds, even if a file with the same
test_expect_success 'loosely defined local base branch is reported correctly' '
git checkout master &&
+ status_uno_is_clean &&
git branch strict &&
git branch loose &&
git commit --allow-empty -m "a bit more" &&
@@ -210,7 +237,9 @@ test_expect_success 'loosely defined local base branch is reported correctly' '
test_config branch.loose.merge master &&
git checkout strict | sed -e "s/strict/BRANCHNAME/g" >expect &&
+ status_uno_is_clean &&
git checkout loose | sed -e "s/loose/BRANCHNAME/g" >actual &&
+ status_uno_is_clean &&
test_cmp expect actual
'
--
2.17.0.290.gded63e768a
^ permalink raw reply related [flat|nested] 95+ messages in thread
* [PATCH v5 2/8] checkout.h: wrap the arguments to unique_tracking_name()
2018-05-31 19:52 ` [PATCH v4 0/9] ambiguous checkout UI & checkout.defaultRemote Ævar Arnfjörð Bjarmason
2018-06-01 21:10 ` [PATCH v5 0/8] " Ævar Arnfjörð Bjarmason
2018-06-01 21:10 ` [PATCH v5 1/8] checkout tests: index should be clean after dwim checkout Ævar Arnfjörð Bjarmason
@ 2018-06-01 21:10 ` Ævar Arnfjörð Bjarmason
2018-06-01 21:10 ` [PATCH v5 3/8] checkout.[ch]: introduce an *_INIT macro Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
8 siblings, 0 replies; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-06-01 21:10 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer,
Eric Sunshine, Ævar Arnfjörð Bjarmason
The line was too long already, and will be longer still when a later
change adds another argument.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
checkout.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/checkout.h b/checkout.h
index 9980711179..4cd4cd1c23 100644
--- a/checkout.h
+++ b/checkout.h
@@ -8,6 +8,7 @@
* tracking branch. Return the name of the remote if such a branch
* exists, NULL otherwise.
*/
-extern const char *unique_tracking_name(const char *name, struct object_id *oid);
+extern const char *unique_tracking_name(const char *name,
+ struct object_id *oid);
#endif /* CHECKOUT_H */
--
2.17.0.290.gded63e768a
^ permalink raw reply related [flat|nested] 95+ messages in thread
* [PATCH v5 3/8] checkout.[ch]: introduce an *_INIT macro
2018-05-31 19:52 ` [PATCH v4 0/9] ambiguous checkout UI & checkout.defaultRemote Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2018-06-01 21:10 ` [PATCH v5 2/8] checkout.h: wrap the arguments to unique_tracking_name() Ævar Arnfjörð Bjarmason
@ 2018-06-01 21:10 ` Ævar Arnfjörð Bjarmason
2018-06-01 22:40 ` Eric Sunshine
2018-06-01 21:10 ` [PATCH v5 4/8] checkout.[ch]: change "unique" member to "num_matches" Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
8 siblings, 1 reply; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-06-01 21:10 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer,
Eric Sunshine, Ævar Arnfjörð Bjarmason
Add an *_INIT macro for the tracking_name_data similar to what exists
elsewhere in the codebase, e.g. OID_ARRAY_INIT in sha1-array.h. This
will make it more idiomatic in later changes to add more fields to the
struct & its initialization macro.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
checkout.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/checkout.c b/checkout.c
index bdefc888ba..80e430cda8 100644
--- a/checkout.c
+++ b/checkout.c
@@ -10,6 +10,8 @@ struct tracking_name_data {
int unique;
};
+#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 1 }
+
static int check_tracking_name(struct remote *remote, void *cb_data)
{
struct tracking_name_data *cb = cb_data;
@@ -32,7 +34,7 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
const char *unique_tracking_name(const char *name, struct object_id *oid)
{
- struct tracking_name_data cb_data = { NULL, NULL, NULL, 1 };
+ struct tracking_name_data cb_data = TRACKING_NAME_DATA_INIT;
cb_data.src_ref = xstrfmt("refs/heads/%s", name);
cb_data.dst_oid = oid;
for_each_remote(check_tracking_name, &cb_data);
--
2.17.0.290.gded63e768a
^ permalink raw reply related [flat|nested] 95+ messages in thread
* Re: [PATCH v5 3/8] checkout.[ch]: introduce an *_INIT macro
2018-06-01 21:10 ` [PATCH v5 3/8] checkout.[ch]: introduce an *_INIT macro Ævar Arnfjörð Bjarmason
@ 2018-06-01 22:40 ` Eric Sunshine
0 siblings, 0 replies; 95+ messages in thread
From: Eric Sunshine @ 2018-06-01 22:40 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git List, Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer
On Fri, Jun 1, 2018 at 5:10 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> checkout.[ch]: introduce an *_INIT macro
checkout.c: introduce...
> Add an *_INIT macro for the tracking_name_data similar to what exists
> elsewhere in the codebase, e.g. OID_ARRAY_INIT in sha1-array.h. This
> will make it more idiomatic in later changes to add more fields to the
> struct & its initialization macro.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH v5 4/8] checkout.[ch]: change "unique" member to "num_matches"
2018-05-31 19:52 ` [PATCH v4 0/9] ambiguous checkout UI & checkout.defaultRemote Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2018-06-01 21:10 ` [PATCH v5 3/8] checkout.[ch]: introduce an *_INIT macro Ævar Arnfjörð Bjarmason
@ 2018-06-01 21:10 ` Ævar Arnfjörð Bjarmason
2018-06-01 22:41 ` Eric Sunshine
2018-06-01 21:10 ` [PATCH v5 5/8] checkout: pass the "num_matches" up to callers Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
8 siblings, 1 reply; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-06-01 21:10 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer,
Eric Sunshine, Ævar Arnfjörð Bjarmason
Internally track how many matches we find in the check_tracking_name()
callback. Nothing uses this now, but it will be made use of in a later
change.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
checkout.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/checkout.c b/checkout.c
index 80e430cda8..7662a39a62 100644
--- a/checkout.c
+++ b/checkout.c
@@ -7,10 +7,10 @@ struct tracking_name_data {
/* const */ char *src_ref;
char *dst_ref;
struct object_id *dst_oid;
- int unique;
+ int num_matches;
};
-#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 1 }
+#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 0 }
static int check_tracking_name(struct remote *remote, void *cb_data)
{
@@ -23,9 +23,9 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
free(query.dst);
return 0;
}
+ cb->num_matches++;
if (cb->dst_ref) {
free(query.dst);
- cb->unique = 0;
return 0;
}
cb->dst_ref = query.dst;
@@ -39,7 +39,7 @@ const char *unique_tracking_name(const char *name, struct object_id *oid)
cb_data.dst_oid = oid;
for_each_remote(check_tracking_name, &cb_data);
free(cb_data.src_ref);
- if (cb_data.unique)
+ if (cb_data.num_matches == 1)
return cb_data.dst_ref;
free(cb_data.dst_ref);
return NULL;
--
2.17.0.290.gded63e768a
^ permalink raw reply related [flat|nested] 95+ messages in thread
* Re: [PATCH v5 4/8] checkout.[ch]: change "unique" member to "num_matches"
2018-06-01 21:10 ` [PATCH v5 4/8] checkout.[ch]: change "unique" member to "num_matches" Ævar Arnfjörð Bjarmason
@ 2018-06-01 22:41 ` Eric Sunshine
0 siblings, 0 replies; 95+ messages in thread
From: Eric Sunshine @ 2018-06-01 22:41 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git List, Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer
On Fri, Jun 1, 2018 at 5:10 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> checkout.[ch]: change "unique" member to "num_matches"
checkout.c: change...
> Internally track how many matches we find in the check_tracking_name()
> callback. Nothing uses this now, but it will be made use of in a later
> change.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH v5 5/8] checkout: pass the "num_matches" up to callers
2018-05-31 19:52 ` [PATCH v4 0/9] ambiguous checkout UI & checkout.defaultRemote Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2018-06-01 21:10 ` [PATCH v5 4/8] checkout.[ch]: change "unique" member to "num_matches" Ævar Arnfjörð Bjarmason
@ 2018-06-01 21:10 ` Ævar Arnfjörð Bjarmason
2018-06-01 21:10 ` [PATCH v5 6/8] builtin/checkout.c: use "ret" variable for return Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
8 siblings, 0 replies; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-06-01 21:10 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer,
Eric Sunshine, Ævar Arnfjörð Bjarmason
Pass the previously added "num_matches" struct value up to the callers
of unique_tracking_name(). This will allow callers to optionally print
better error messages in a later change.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/checkout.c | 10 +++++++---
builtin/worktree.c | 4 ++--
checkout.c | 5 ++++-
checkout.h | 3 ++-
4 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2e1d2376d2..72457fb7d5 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -878,7 +878,8 @@ static int parse_branchname_arg(int argc, const char **argv,
int dwim_new_local_branch_ok,
struct branch_info *new_branch_info,
struct checkout_opts *opts,
- struct object_id *rev)
+ struct object_id *rev,
+ int *dwim_remotes_matched)
{
struct tree **source_tree = &opts->source_tree;
const char **new_branch = &opts->new_branch;
@@ -972,7 +973,8 @@ static int parse_branchname_arg(int argc, const char **argv,
recover_with_dwim = 0;
if (recover_with_dwim) {
- const char *remote = unique_tracking_name(arg, rev);
+ const char *remote = unique_tracking_name(arg, rev,
+ dwim_remotes_matched);
if (remote) {
*new_branch = arg;
arg = remote;
@@ -1109,6 +1111,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
struct branch_info new_branch_info;
char *conflict_style = NULL;
int dwim_new_local_branch = 1;
+ int dwim_remotes_matched = 0;
struct option options[] = {
OPT__QUIET(&opts.quiet, N_("suppress progress reporting")),
OPT_STRING('b', NULL, &opts.new_branch, N_("branch"),
@@ -1219,7 +1222,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
opts.track == BRANCH_TRACK_UNSPECIFIED &&
!opts.new_branch;
int n = parse_branchname_arg(argc, argv, dwim_ok,
- &new_branch_info, &opts, &rev);
+ &new_branch_info, &opts, &rev,
+ &dwim_remotes_matched);
argv += n;
argc -= n;
}
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 5c7d2bb180..a763dbdccb 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -412,7 +412,7 @@ static const char *dwim_branch(const char *path, const char **new_branch)
if (guess_remote) {
struct object_id oid;
const char *remote =
- unique_tracking_name(*new_branch, &oid);
+ unique_tracking_name(*new_branch, &oid, NULL);
return remote;
}
return NULL;
@@ -484,7 +484,7 @@ static int add(int ac, const char **av, const char *prefix)
commit = lookup_commit_reference_by_name(branch);
if (!commit) {
- remote = unique_tracking_name(branch, &oid);
+ remote = unique_tracking_name(branch, &oid, NULL);
if (remote) {
new_branch = branch;
branch = remote;
diff --git a/checkout.c b/checkout.c
index 7662a39a62..ee3a7e9c05 100644
--- a/checkout.c
+++ b/checkout.c
@@ -32,12 +32,15 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
return 0;
}
-const char *unique_tracking_name(const char *name, struct object_id *oid)
+const char *unique_tracking_name(const char *name, struct object_id *oid,
+ int *dwim_remotes_matched)
{
struct tracking_name_data cb_data = TRACKING_NAME_DATA_INIT;
cb_data.src_ref = xstrfmt("refs/heads/%s", name);
cb_data.dst_oid = oid;
for_each_remote(check_tracking_name, &cb_data);
+ if (dwim_remotes_matched)
+ *dwim_remotes_matched = cb_data.num_matches;
free(cb_data.src_ref);
if (cb_data.num_matches == 1)
return cb_data.dst_ref;
diff --git a/checkout.h b/checkout.h
index 4cd4cd1c23..6b2073310c 100644
--- a/checkout.h
+++ b/checkout.h
@@ -9,6 +9,7 @@
* exists, NULL otherwise.
*/
extern const char *unique_tracking_name(const char *name,
- struct object_id *oid);
+ struct object_id *oid,
+ int *dwim_remotes_matched);
#endif /* CHECKOUT_H */
--
2.17.0.290.gded63e768a
^ permalink raw reply related [flat|nested] 95+ messages in thread
* [PATCH v5 6/8] builtin/checkout.c: use "ret" variable for return
2018-05-31 19:52 ` [PATCH v4 0/9] ambiguous checkout UI & checkout.defaultRemote Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2018-06-01 21:10 ` [PATCH v5 5/8] checkout: pass the "num_matches" up to callers Ævar Arnfjörð Bjarmason
@ 2018-06-01 21:10 ` Ævar Arnfjörð Bjarmason
2018-06-01 21:10 ` [PATCH v5 7/8] checkout: add advice for ambiguous "checkout <branch>" Ævar Arnfjörð Bjarmason
2018-06-01 21:10 ` [PATCH v5 8/8] checkout & worktree: introduce checkout.defaultRemote Ævar Arnfjörð Bjarmason
8 siblings, 0 replies; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-06-01 21:10 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer,
Eric Sunshine, Ævar Arnfjörð Bjarmason
There is no point in doing this right now, but in later change the
"ret" variable will be inspected. This change makes that meaningful
change smaller.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/checkout.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 72457fb7d5..8c93c55cbc 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1265,8 +1265,10 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
}
UNLEAK(opts);
- if (opts.patch_mode || opts.pathspec.nr)
- return checkout_paths(&opts, new_branch_info.name);
- else
+ if (opts.patch_mode || opts.pathspec.nr) {
+ int ret = checkout_paths(&opts, new_branch_info.name);
+ return ret;
+ } else {
return checkout_branch(&opts, &new_branch_info);
+ }
}
--
2.17.0.290.gded63e768a
^ permalink raw reply related [flat|nested] 95+ messages in thread
* [PATCH v5 7/8] checkout: add advice for ambiguous "checkout <branch>"
2018-05-31 19:52 ` [PATCH v4 0/9] ambiguous checkout UI & checkout.defaultRemote Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2018-06-01 21:10 ` [PATCH v5 6/8] builtin/checkout.c: use "ret" variable for return Ævar Arnfjörð Bjarmason
@ 2018-06-01 21:10 ` Ævar Arnfjörð Bjarmason
2018-06-01 22:52 ` Eric Sunshine
2018-06-01 21:10 ` [PATCH v5 8/8] checkout & worktree: introduce checkout.defaultRemote Ævar Arnfjörð Bjarmason
8 siblings, 1 reply; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-06-01 21:10 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer,
Eric Sunshine, Ævar Arnfjörð Bjarmason
As the "checkout" documentation describes:
If <branch> is not found but there does exist a tracking branch in
exactly one remote (call it <remote>) with a matching name, treat
as equivalent to [...] <remote>/<branch.
This is a really useful feature. The problem is that when you and
another remote (e.g. a fork) git won't find a unique branch name
anymore, and will instead print this nondescript message:
$ git checkout master
error: pathspec 'master' did not match any file(s) known to git
Now it will, on my git.git checkout, print:
$ ./git --exec-path=$PWD checkout master
error: pathspec 'master' did not match any file(s) known to git.
hint: The argument 'master' matched more than one remote tracking branch.
hint: We found 26 remotes with a reference that matched. So we fell back
hint: on trying to resolve the argument as a path, but failed there too!
hint:
hint: If you meant to check out a remote tracking branch on e.g. 'origin'
hint: you can do so by fully-qualifying the name with the --track option:
hint:
hint: git checkout --track origin/<name>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Documentation/config.txt | 7 +++++++
advice.c | 2 ++
advice.h | 1 +
builtin/checkout.c | 13 +++++++++++++
t/t2024-checkout-dwim.sh | 14 ++++++++++++++
5 files changed, 37 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index ab641bf5a9..dfc0413a84 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -344,6 +344,13 @@ advice.*::
Advice shown when you used linkgit:git-checkout[1] to
move to the detach HEAD state, to instruct how to create
a local branch after the fact.
+ checkoutAmbiguousRemoteBranchName::
+ Advice shown when the argument to
+ linkgit:git-checkout[1] ambiguously resolves to a
+ remote tracking branch on more than one remote in
+ situations where an unambiguous argument would have
+ otherwise caused a remote-tracking branch to be
+ checked out.
amWorkDir::
Advice that shows the location of the patch file when
linkgit:git-am[1] fails to apply it.
diff --git a/advice.c b/advice.c
index 370a56d054..75e7dede90 100644
--- a/advice.c
+++ b/advice.c
@@ -21,6 +21,7 @@ int advice_add_embedded_repo = 1;
int advice_ignored_hook = 1;
int advice_waiting_for_editor = 1;
int advice_graft_file_deprecated = 1;
+int advice_checkout_ambiguous_remote_branch_name = 1;
static int advice_use_color = -1;
static char advice_colors[][COLOR_MAXLEN] = {
@@ -72,6 +73,7 @@ static struct {
{ "ignoredhook", &advice_ignored_hook },
{ "waitingforeditor", &advice_waiting_for_editor },
{ "graftfiledeprecated", &advice_graft_file_deprecated },
+ { "checkoutambiguousremotebranchname", &advice_checkout_ambiguous_remote_branch_name },
/* make this an alias for backward compatibility */
{ "pushnonfastforward", &advice_push_update_rejected }
diff --git a/advice.h b/advice.h
index 9f5064e82a..4d11d51d43 100644
--- a/advice.h
+++ b/advice.h
@@ -22,6 +22,7 @@ extern int advice_add_embedded_repo;
extern int advice_ignored_hook;
extern int advice_waiting_for_editor;
extern int advice_graft_file_deprecated;
+extern int advice_checkout_ambiguous_remote_branch_name;
int git_default_advice_config(const char *var, const char *value);
__attribute__((format (printf, 1, 2)))
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8c93c55cbc..4dfb8f1535 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -22,6 +22,7 @@
#include "resolve-undo.h"
#include "submodule-config.h"
#include "submodule.h"
+#include "advice.h"
static const char * const checkout_usage[] = {
N_("git checkout [<options>] <branch>"),
@@ -1267,6 +1268,18 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
UNLEAK(opts);
if (opts.patch_mode || opts.pathspec.nr) {
int ret = checkout_paths(&opts, new_branch_info.name);
+ if (ret && dwim_remotes_matched > 1 &&
+ advice_checkout_ambiguous_remote_branch_name)
+ advise(_("The argument '%s' matched more than one remote tracking branch.\n"
+ "We found %d remotes with a reference that matched. So we fell back\n"
+ "on trying to resolve the argument as a path, but failed there too!\n"
+ "\n"
+ "If you meant to check out a remote tracking branch on e.g. 'origin'\n"
+ "you can do so by fully-qualifying the name with the --track option:\n"
+ "\n"
+ " git checkout --track origin/<name>"),
+ argv[0],
+ dwim_remotes_matched);
return ret;
} else {
return checkout_branch(&opts, &new_branch_info);
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index ed32828105..fef263a858 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -76,6 +76,20 @@ test_expect_success 'checkout of branch from multiple remotes fails #1' '
test_branch master
'
+test_expect_success 'checkout of branch from multiple remotes fails with advice' '
+ git checkout -B master &&
+ test_might_fail git branch -D foo &&
+ test_must_fail git checkout foo 2>stderr &&
+ test_branch master &&
+ status_uno_is_clean &&
+ test_i18ngrep "^hint: " stderr &&
+ test_must_fail git -c advice.checkoutAmbiguousRemoteBranchName=false \
+ checkout foo 2>stderr &&
+ test_branch master &&
+ status_uno_is_clean &&
+ test_i18ngrep ! "^hint: " stderr
+'
+
test_expect_success 'checkout of branch from a single remote succeeds #1' '
git checkout -B master &&
test_might_fail git branch -D bar &&
--
2.17.0.290.gded63e768a
^ permalink raw reply related [flat|nested] 95+ messages in thread
* Re: [PATCH v5 7/8] checkout: add advice for ambiguous "checkout <branch>"
2018-06-01 21:10 ` [PATCH v5 7/8] checkout: add advice for ambiguous "checkout <branch>" Ævar Arnfjörð Bjarmason
@ 2018-06-01 22:52 ` Eric Sunshine
0 siblings, 0 replies; 95+ messages in thread
From: Eric Sunshine @ 2018-06-01 22:52 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git List, Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer
On Fri, Jun 1, 2018 at 5:10 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> As the "checkout" documentation describes:
>
> If <branch> is not found but there does exist a tracking branch in
> exactly one remote (call it <remote>) with a matching name, treat
> as equivalent to [...] <remote>/<branch.
>
> This is a really useful feature. The problem is that when you and
s/and/add/
> another remote (e.g. a fork) git won't find a unique branch name
Missing comma: s/)/),/
> anymore, and will instead print this nondescript message:
Perhaps s/nondescript/unhelpful/ ?
> $ git checkout master
> error: pathspec 'master' did not match any file(s) known to git
>
> Now it will, on my git.git checkout, print:
>
> $ ./git --exec-path=$PWD checkout master
> error: pathspec 'master' did not match any file(s) known to git.
> hint: The argument 'master' matched more than one remote tracking branch.
> hint: We found 26 remotes with a reference that matched. So we fell back
> hint: on trying to resolve the argument as a path, but failed there too!
> hint:
> hint: If you meant to check out a remote tracking branch on e.g. 'origin'
Missing commas: s/on e.g. 'origin'/on, e.g. 'origin',/
> hint: you can do so by fully-qualifying the name with the --track option:
s/fully-qualifying/fully qualifying/
> hint:
> hint: git checkout --track origin/<name>
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Aside from the s/and/add/ botch, all of the above are tiny nits which
don't actually impact the meaning of the commit message, thus not
really important.
> ---
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> @@ -1267,6 +1268,18 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
> + if (ret && dwim_remotes_matched > 1 &&
> + advice_checkout_ambiguous_remote_branch_name)
> + advise(_("The argument '%s' matched more than one remote tracking branch.\n"
You could drop "The argument" prefix, without hurting the meaning at
all, in order to gain a bit more horizontal space for the '%s'
interpolation. (Not worth a re-roll.)
> + "We found %d remotes with a reference that matched. So we fell back\n"
> + "on trying to resolve the argument as a path, but failed there too!\n"
> + "\n"
> + "If you meant to check out a remote tracking branch on e.g. 'origin'\n"
> + "you can do so by fully-qualifying the name with the --track option:\n"
> + "\n"
> + " git checkout --track origin/<name>"),
> + argv[0],
> + dwim_remotes_matched);
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH v5 8/8] checkout & worktree: introduce checkout.defaultRemote
2018-05-31 19:52 ` [PATCH v4 0/9] ambiguous checkout UI & checkout.defaultRemote Ævar Arnfjörð Bjarmason
` (7 preceding siblings ...)
2018-06-01 21:10 ` [PATCH v5 7/8] checkout: add advice for ambiguous "checkout <branch>" Ævar Arnfjörð Bjarmason
@ 2018-06-01 21:10 ` Ævar Arnfjörð Bjarmason
8 siblings, 0 replies; 95+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-06-01 21:10 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
Nguyễn Thái Ngọc Duy, Thomas Gummerer,
Eric Sunshine, Ævar Arnfjörð Bjarmason
Introduce a checkout.defaultRemote setting which can be used to
designate a remote to prefer (via checkout.defaultRemote=origin) when
running e.g. "git checkout master" to mean origin/master, even though
there's other remotes that have the "master" branch.
I want this because it's very handy to use this workflow to checkout a
repository and create a topic branch, then get back to a "master" as
retrieved from upstream:
(
cd /tmp &&
rm -rf tbdiff &&
git clone git@github.com:trast/tbdiff.git &&
cd tbdiff &&
git branch -m topic &&
git checkout master
)
That will output:
Branch 'master' set up to track remote branch 'master' from 'origin'.
Switched to a new branch 'master'
But as soon as a new remote is added (e.g. just to inspect something
from someone else) the DWIMery goes away:
(
cd /tmp &&
rm -rf tbdiff &&
git clone git@github.com:trast/tbdiff.git &&
cd tbdiff &&
git branch -m topic &&
git remote add avar git@github.com:avar/tbdiff.git &&
git fetch avar &&
git checkout master
)
Will output (without the advice output added earlier in this series):
error: pathspec 'master' did not match any file(s) known to git.
The new checkout.defaultRemote config allows me to say that whenever
that ambiguity comes up I'd like to prefer "origin", and it'll still
work as though the only remote I had was "origin".
Also adjust the advice.checkoutAmbiguousRemoteBranchName message to
mention this new config setting to the user, the full output on my
git.git is now (the last paragraph is new):
$ ./git --exec-path=$PWD checkout master
error: pathspec 'master' did not match any file(s) known to git.
hint: The argument 'master' matched more than one remote tracking branch.
hint: We found 26 remotes with a reference that matched. So we fell back
hint: on trying to resolve the argument as a path, but failed there too!
hint:
hint: If you meant to check out a remote tracking branch on e.g. 'origin'
hint: you can do so by fully-qualifying the name with the --track option:
hint:
hint: git checkout --track origin/<name>
hint:
hint: If you'd like to always have checkouts of an ambiguous <name> prefer
hint: one remote, e.g. the 'origin' remote, consider setting
hint: checkout.defaultRemote=origin in your config.
I considered splitting this into checkout.defaultRemote and
worktree.defaultRemote, but it's probably less confusing to break our
own rules that anything shared between config should live in core.*
than have two config settings, and I couldn't come up with a short
name under core.* that made sense (core.defaultRemoteForCheckout?).
See also 70c9ac2f19 ("DWIM "git checkout frotz" to "git checkout -b
frotz origin/frotz"", 2009-10-18) which introduced this DWIM feature
to begin with, and 4e85333197 ("worktree: make add <path> <branch>
dwim", 2017-11-26) which added it to git-worktree.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Documentation/config.txt | 21 ++++++++++++++++++++-
Documentation/git-checkout.txt | 9 +++++++++
Documentation/git-worktree.txt | 9 +++++++++
builtin/checkout.c | 12 +++++++++---
checkout.c | 26 ++++++++++++++++++++++++--
t/t2024-checkout-dwim.sh | 18 +++++++++++++++++-
t/t2025-worktree-add.sh | 21 +++++++++++++++++++++
7 files changed, 109 insertions(+), 7 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index dfc0413a84..aef2769211 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -350,7 +350,10 @@ advice.*::
remote tracking branch on more than one remote in
situations where an unambiguous argument would have
otherwise caused a remote-tracking branch to be
- checked out.
+ checked out. See the `checkout.defaultRemote`
+ configuration variable for how to set a given remote
+ to used by default in some situations where this
+ advice would be printed.
amWorkDir::
Advice that shows the location of the patch file when
linkgit:git-am[1] fails to apply it.
@@ -1105,6 +1108,22 @@ browser.<tool>.path::
browse HTML help (see `-w` option in linkgit:git-help[1]) or a
working repository in gitweb (see linkgit:git-instaweb[1]).
+checkout.defaultRemote::
+ When you run 'git checkout <something>' and only have one
+ remote, it may implicitly fall back on checking out and
+ tracking e.g. 'origin/<something>'. This stops working as soon
+ as you have more than one remote with a '<something>'
+ reference. This setting allows for setting the name of a
+ preferred remote that should always win when it comes to
+ disambiguation. The typical use-case is to set this to
+ `origin`.
++
+Currently this is used by linkgit:git-checkout[1] when 'git checkout
+<something>' will checkout the '<something>' branch on another remote,
+and by linkgit:git-worktree[1] when 'git worktree add' refers to a
+remote branch. This setting might be used for other checkout-like
+commands or functionality in the future.
+
clean.requireForce::
A boolean to make git-clean do nothing unless given -f,
-i or -n. Defaults to true.
diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index ca5fc9c798..9db02928c4 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -38,6 +38,15 @@ equivalent to
$ git checkout -b <branch> --track <remote>/<branch>
------------
+
+If the branch exists in multiple remotes and one of them is named by
+the `checkout.defaultRemote` configuration variable, we'll use that
+one for the purposes of disambiguation, even if the `<branch>` isn't
+unique across all remotes. Set it to
+e.g. `checkout.defaultRemote=origin` to always checkout remote
+branches from there if `<branch>` is ambiguous but exists on the
+'origin' remote. See also `checkout.defaultRemote` in
+linkgit:git-config[1].
++
You could omit <branch>, in which case the command degenerates to
"check out the current branch", which is a glorified no-op with
rather expensive side-effects to show only the tracking information,
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index afc6576a14..9c26be40f4 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -60,6 +60,15 @@ with a matching name, treat as equivalent to:
$ git worktree add --track -b <branch> <path> <remote>/<branch>
------------
+
+If the branch exists in multiple remotes and one of them is named by
+the `checkout.defaultRemote` configuration variable, we'll use that
+one for the purposes of disambiguation, even if the `<branch>` isn't
+unique across all remotes. Set it to
+e.g. `checkout.defaultRemote=origin` to always checkout remote
+branches from there if `<branch>` is ambiguous but exists on the
+'origin' remote. See also `checkout.defaultRemote` in
+linkgit:git-config[1].
++
If `<commit-ish>` is omitted and neither `-b` nor `-B` nor `--detach` used,
then, as a convenience, the new worktree is associated with a branch
(call it `<branch>`) named after `$(basename <path>)`. If `<branch>`
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 4dfb8f1535..b78481dead 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -912,8 +912,10 @@ static int parse_branchname_arg(int argc, const char **argv,
* (b) If <something> is _not_ a commit, either "--" is present
* or <something> is not a path, no -t or -b was given, and
* and there is a tracking branch whose name is <something>
- * in one and only one remote, then this is a short-hand to
- * fork local <something> from that remote-tracking branch.
+ * in one and only one remote (or if the branch exists on the
+ * remote named in checkout.defaultRemote), then this is a
+ * short-hand to fork local <something> from that
+ * remote-tracking branch.
*
* (c) Otherwise, if "--" is present, treat it like case (1).
*
@@ -1277,7 +1279,11 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
"If you meant to check out a remote tracking branch on e.g. 'origin'\n"
"you can do so by fully-qualifying the name with the --track option:\n"
"\n"
- " git checkout --track origin/<name>"),
+ " git checkout --track origin/<name>\n"
+ "\n"
+ "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
+ "one remote, e.g. the 'origin' remote, consider setting\n"
+ "checkout.defaultRemote=origin in your config."),
argv[0],
dwim_remotes_matched);
return ret;
diff --git a/checkout.c b/checkout.c
index ee3a7e9c05..c72e9f9773 100644
--- a/checkout.c
+++ b/checkout.c
@@ -2,15 +2,19 @@
#include "remote.h"
#include "refspec.h"
#include "checkout.h"
+#include "config.h"
struct tracking_name_data {
/* const */ char *src_ref;
char *dst_ref;
struct object_id *dst_oid;
int num_matches;
+ const char *default_remote;
+ char *default_dst_ref;
+ struct object_id *default_dst_oid;
};
-#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 0 }
+#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 0, NULL, NULL, NULL }
static int check_tracking_name(struct remote *remote, void *cb_data)
{
@@ -24,6 +28,12 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
return 0;
}
cb->num_matches++;
+ if (cb->default_remote && !strcmp(remote->name, cb->default_remote)) {
+ struct object_id *dst = xmalloc(sizeof(*cb->default_dst_oid));
+ cb->default_dst_ref = xstrdup(query.dst);
+ oidcpy(dst, cb->dst_oid);
+ cb->default_dst_oid = dst;
+ }
if (cb->dst_ref) {
free(query.dst);
return 0;
@@ -36,14 +46,26 @@ const char *unique_tracking_name(const char *name, struct object_id *oid,
int *dwim_remotes_matched)
{
struct tracking_name_data cb_data = TRACKING_NAME_DATA_INIT;
+ const char *default_remote = NULL;
+ if (!git_config_get_string_const("checkout.defaultremote", &default_remote))
+ cb_data.default_remote = default_remote;
cb_data.src_ref = xstrfmt("refs/heads/%s", name);
cb_data.dst_oid = oid;
for_each_remote(check_tracking_name, &cb_data);
if (dwim_remotes_matched)
*dwim_remotes_matched = cb_data.num_matches;
free(cb_data.src_ref);
- if (cb_data.num_matches == 1)
+ free((char *)default_remote);
+ if (cb_data.num_matches == 1) {
+ free(cb_data.default_dst_ref);
+ free(cb_data.default_dst_oid);
return cb_data.dst_ref;
+ }
free(cb_data.dst_ref);
+ if (cb_data.default_dst_ref) {
+ oidcpy(oid, cb_data.default_dst_oid);
+ free(cb_data.default_dst_oid);
+ return cb_data.default_dst_ref;
+ }
return NULL;
}
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index fef263a858..1495c248a7 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -87,7 +87,23 @@ test_expect_success 'checkout of branch from multiple remotes fails with advice'
checkout foo 2>stderr &&
test_branch master &&
status_uno_is_clean &&
- test_i18ngrep ! "^hint: " stderr
+ test_i18ngrep ! "^hint: " stderr &&
+ # Make sure the likes of checkout -p don not print this hint
+ git checkout -p foo 2>stderr &&
+ test_i18ngrep ! "^hint: " stderr &&
+ status_uno_is_clean
+'
+
+test_expect_success 'checkout of branch from multiple remotes succeeds with checkout.defaultRemote #1' '
+ git checkout -B master &&
+ status_uno_is_clean &&
+ test_might_fail git branch -D foo &&
+
+ git -c checkout.defaultRemote=repo_a checkout foo &&
+ status_uno_is_clean &&
+ test_branch foo &&
+ test_cmp_rev remotes/repo_a/foo HEAD &&
+ test_branch_upstream foo repo_a foo
'
test_expect_success 'checkout of branch from a single remote succeeds #1' '
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index d2e49f7632..be6e093142 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -402,6 +402,27 @@ test_expect_success '"add" <path> <branch> dwims' '
)
'
+test_expect_success '"add" <path> <branch> dwims with checkout.defaultRemote' '
+ test_when_finished rm -rf repo_upstream repo_dwim foo &&
+ setup_remote_repo repo_upstream repo_dwim &&
+ git init repo_dwim &&
+ (
+ cd repo_dwim &&
+ git remote add repo_upstream2 ../repo_upstream &&
+ git fetch repo_upstream2 &&
+ test_must_fail git worktree add ../foo foo &&
+ git -c checkout.defaultRemote=repo_upstream worktree add ../foo foo &&
+ >status.expect &&
+ git status -uno --porcelain >status.actual &&
+ test_cmp status.expect status.actual
+ ) &&
+ (
+ cd foo &&
+ test_branch_upstream foo repo_upstream foo &&
+ test_cmp_rev refs/remotes/repo_upstream/foo refs/heads/foo
+ )
+'
+
test_expect_success 'git worktree add does not match remote' '
test_when_finished rm -rf repo_a repo_b foo &&
setup_remote_repo repo_a repo_b &&
--
2.17.0.290.gded63e768a
^ permalink raw reply related [flat|nested] 95+ messages in thread