* [PATCH v5 0/3] pull: stop warning on every pull
@ 2020-12-12 16:52 Felipe Contreras
2020-12-12 16:52 ` [PATCH v5 1/3] pull: refactor fast-forward check Felipe Contreras
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Felipe Contreras @ 2020-12-12 16:52 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Junio C Hamano, Jeff King, Vít Ondruch,
Felipe Contreras
The future of having a sensible "git pull" default is uncertain (it's
very likely that nothing will change), in the meantime there's no reason
to keep annoying our users unnecessarily.
This patch is doing one thing, and one thing only: stops warning the
users on every single pull (warns only if they are not-fast-forward).
Changes since v4:
1. Improved some commit messages
2. In patch 3 current the tests are kept as is, per Junio's request
(IMO they are no-ops)
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 6b4adab8b1..52e8ccc933 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -28,66 +28,116 @@ test_expect_success 'setup' '
'
test_expect_success 'pull.rebase not set' '
- git reset --hard c2 &&
- git -c color.advice=always pull . c1 2>err &&
- test_decode_color <err >decoded &&
- test_i18ngrep "<YELLOW>hint: " decoded &&
- test_i18ngrep "Pulling without specifying how to reconcile" decoded
+ git reset --hard c0 &&
+ git pull . c1 2>err &&
+ test_i18ngrep ! "Pulling without specifying how to reconcile" err
+'
+test_expect_success 'pull.rebase not set and pull.ff=true' '
+ git reset --hard c0 &&
+ test_config pull.ff true &&
+ git pull . c1 2>err &&
+ test_i18ngrep ! "Pulling without specifying how to reconcile" err
'
-test_expect_success 'pull.rebase not set (fast-forward)' '
+test_expect_success 'pull.rebase not set and pull.ff=false' '
git reset --hard c0 &&
+ test_config pull.ff false &&
git pull . c1 2>err &&
test_i18ngrep ! "Pulling without specifying how to reconcile" err
'
-test_expect_success 'pull.rebase not set and pull.ff=true' '
+test_expect_success 'pull.rebase not set and pull.ff=only' '
+ git reset --hard c0 &&
+ test_config pull.ff only &&
+ git pull . c1 2>err &&
+ test_i18ngrep ! "Pulling without specifying how to reconcile" err
+'
+
+test_expect_success 'pull.rebase not set and --rebase given' '
+ git reset --hard c0 &&
+ git pull --rebase . c1 2>err &&
+ test_i18ngrep ! "Pulling without specifying how to reconcile" err
+'
+
+test_expect_success 'pull.rebase not set and --no-rebase given' '
+ git reset --hard c0 &&
+ git pull --no-rebase . c1 2>err &&
+ test_i18ngrep ! "Pulling without specifying how to reconcile" err
+'
+
+test_expect_success 'pull.rebase not set and --ff given' '
+ git reset --hard c0 &&
+ git pull --ff . c1 2>err &&
+ test_i18ngrep ! "Pulling without specifying how to reconcile" err
+'
+
+test_expect_success 'pull.rebase not set and --no-ff given' '
+ git reset --hard c0 &&
+ git pull --no-ff . c1 2>err &&
+ test_i18ngrep ! "Pulling without specifying how to reconcile" err
+'
+
+test_expect_success 'pull.rebase not set and --ff-only given' '
+ git reset --hard c0 &&
+ git pull --ff-only . c1 2>err &&
+ test_i18ngrep ! "Pulling without specifying how to reconcile" err
+'
+
+test_expect_success 'pull.rebase not set (not-fast-forward)' '
+ git reset --hard c2 &&
+ git -c color.advice=always pull . c1 2>err &&
+ test_decode_color <err >decoded &&
+ test_i18ngrep "<YELLOW>hint: " decoded &&
+ test_i18ngrep "Pulling without specifying how to reconcile" decoded
+'
+
+test_expect_success 'pull.rebase not set and pull.ff=true (not-fast-forward)' '
git reset --hard c2 &&
test_config pull.ff true &&
git pull . c1 2>err &&
test_i18ngrep ! "Pulling without specifying how to reconcile" err
'
-test_expect_success 'pull.rebase not set and pull.ff=false' '
+test_expect_success 'pull.rebase not set and pull.ff=false (not-fast-forward)' '
git reset --hard c2 &&
test_config pull.ff false &&
git pull . c1 2>err &&
test_i18ngrep ! "Pulling without specifying how to reconcile" err
'
-test_expect_success 'pull.rebase not set and pull.ff=only' '
+test_expect_success 'pull.rebase not set and pull.ff=only (not-fast-forward)' '
git reset --hard c2 &&
test_config pull.ff only &&
test_must_fail git pull . c1 2>err &&
test_i18ngrep ! "Pulling without specifying how to reconcile" err
'
-test_expect_success 'pull.rebase not set and --rebase given' '
+test_expect_success 'pull.rebase not set and --rebase given (not-fast-forward)' '
git reset --hard c2 &&
git pull --rebase . c1 2>err &&
test_i18ngrep ! "Pulling without specifying how to reconcile" err
'
-test_expect_success 'pull.rebase not set and --no-rebase given' '
+test_expect_success 'pull.rebase not set and --no-rebase given (not-fast-forward)' '
git reset --hard c2 &&
git pull --no-rebase . c1 2>err &&
test_i18ngrep ! "Pulling without specifying how to reconcile" err
'
-test_expect_success 'pull.rebase not set and --ff given' '
+test_expect_success 'pull.rebase not set and --ff given (not-fast-forward)' '
git reset --hard c2 &&
git pull --ff . c1 2>err &&
test_i18ngrep ! "Pulling without specifying how to reconcile" err
'
-test_expect_success 'pull.rebase not set and --no-ff given' '
+test_expect_success 'pull.rebase not set and --no-ff given (not-fast-forward)' '
git reset --hard c2 &&
git pull --no-ff . c1 2>err &&
test_i18ngrep ! "Pulling without specifying how to reconcile" err
'
-test_expect_success 'pull.rebase not set and --ff-only given' '
+test_expect_success 'pull.rebase not set and --ff-only given (not-fast-forward)' '
git reset --hard c2 &&
test_must_fail git pull --ff-only . c1 2>err &&
test_i18ngrep ! "Pulling without specifying how to reconcile" err
Felipe Contreras (3):
pull: refactor fast-forward check
pull: move default warning
pull: display default warning only when non-ff
builtin/pull.c | 61 +++++++++++++++++++--------------
t/t7601-merge-pull-config.sh | 66 +++++++++++++++++++++++++++++++++---
2 files changed, 97 insertions(+), 30 deletions(-)
--
2.29.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v5 1/3] pull: refactor fast-forward check
2020-12-12 16:52 [PATCH v5 0/3] pull: stop warning on every pull Felipe Contreras
@ 2020-12-12 16:52 ` Felipe Contreras
2020-12-12 16:52 ` [PATCH v5 2/3] pull: move default warning Felipe Contreras
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Felipe Contreras @ 2020-12-12 16:52 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Junio C Hamano, Jeff King, Vít Ondruch,
Felipe Contreras
It makes the code much cleaner.
Plus, we would like to be able to make this check before the decision to
rebase is made in a future step.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
builtin/pull.c | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/builtin/pull.c b/builtin/pull.c
index aa56ebcdd0..03e6d53243 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -924,6 +924,20 @@ static int run_rebase(const struct object_id *newbase,
return ret;
}
+static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_head)
+{
+ int ret;
+ struct commit_list *list = NULL;
+ struct commit *merge_head, *head;
+
+ head = lookup_commit_reference(the_repository, orig_head);
+ commit_list_insert(head, &list);
+ merge_head = lookup_commit_reference(the_repository, orig_merge_head);
+ ret = repo_is_descendant_of(the_repository, merge_head, list);
+ free_commit_list(list);
+ return ret;
+}
+
int cmd_pull(int argc, const char **argv, const char *prefix)
{
const char *repo, **refspecs;
@@ -1040,22 +1054,12 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
submodule_touches_in_range(the_repository, &upstream, &curr_head))
die(_("cannot rebase with locally recorded submodule modifications"));
if (!autostash) {
- struct commit_list *list = NULL;
- struct commit *merge_head, *head;
-
- head = lookup_commit_reference(the_repository,
- &orig_head);
- commit_list_insert(head, &list);
- merge_head = lookup_commit_reference(the_repository,
- &merge_heads.oid[0]);
- if (repo_is_descendant_of(the_repository,
- merge_head, list)) {
+ if (get_can_ff(&orig_head, &merge_heads.oid[0])) {
/* we can fast-forward this without invoking rebase */
opt_ff = "--ff-only";
ran_ff = 1;
ret = run_merge();
}
- free_commit_list(list);
}
if (!ran_ff)
ret = run_rebase(&newbase, &upstream);
--
2.29.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v5 2/3] pull: move default warning
2020-12-12 16:52 [PATCH v5 0/3] pull: stop warning on every pull Felipe Contreras
2020-12-12 16:52 ` [PATCH v5 1/3] pull: refactor fast-forward check Felipe Contreras
@ 2020-12-12 16:52 ` Felipe Contreras
2020-12-12 16:52 ` [PATCH v5 3/3] pull: display default warning only when non-ff Felipe Contreras
2020-12-12 16:56 ` [PATCH v5 0/3] pull: stop warning on every pull Felipe Contreras
3 siblings, 0 replies; 13+ messages in thread
From: Felipe Contreras @ 2020-12-12 16:52 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Junio C Hamano, Jeff King, Vít Ondruch,
Felipe Contreras
Eventually we want to be able to display the warning only when
fast-forward merges are not possible.
In order to do so, we need to move the default warning up to the point
where we can check if we can fast-forward or not.
Additionally, config_get_rebase() was probably never its true home.
This requires a temporary variable to check if we are in the
"default mode" (no --rebase or --no-rebase specified). But this variable
can be removed later on with further code reorganization.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
builtin/pull.c | 32 ++++++++++++++++++--------------
1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/builtin/pull.c b/builtin/pull.c
index 03e6d53243..ff8e3ce137 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -27,6 +27,8 @@
#include "commit-reach.h"
#include "sequencer.h"
+static int default_mode;
+
/**
* Parses the value of --rebase. If value is a false value, returns
* REBASE_FALSE. If value is a true value, returns REBASE_TRUE. If value is
@@ -344,20 +346,7 @@ static enum rebase_type config_get_rebase(void)
if (!git_config_get_value("pull.rebase", &value))
return parse_config_rebase("pull.rebase", value, 1);
- if (opt_verbosity >= 0 && !opt_ff) {
- advise(_("Pulling without specifying how to reconcile divergent branches is\n"
- "discouraged. You can squelch this message by running one of the following\n"
- "commands sometime before your next pull:\n"
- "\n"
- " git config pull.rebase false # merge (the default strategy)\n"
- " git config pull.rebase true # rebase\n"
- " git config pull.ff only # fast-forward only\n"
- "\n"
- "You can replace \"git config\" with \"git config --global\" to set a default\n"
- "preference for all repositories. You can also pass --rebase, --no-rebase,\n"
- "or --ff-only on the command line to override the configured default per\n"
- "invocation.\n"));
- }
+ default_mode = 1;
return REBASE_FALSE;
}
@@ -1040,6 +1029,21 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
if (opt_rebase && merge_heads.nr > 1)
die(_("Cannot rebase onto multiple branches."));
+ if (default_mode && opt_verbosity >= 0 && !opt_ff) {
+ advise(_("Pulling without specifying how to reconcile divergent branches is\n"
+ "discouraged. You can squelch this message by running one of the following\n"
+ "commands sometime before your next pull:\n"
+ "\n"
+ " git config pull.rebase false # merge (the default strategy)\n"
+ " git config pull.rebase true # rebase\n"
+ " git config pull.ff only # fast-forward only\n"
+ "\n"
+ "You can replace \"git config\" with \"git config --global\" to set a default\n"
+ "preference for all repositories. You can also pass --rebase, --no-rebase,\n"
+ "or --ff-only on the command line to override the configured default per\n"
+ "invocation.\n"));
+ }
+
if (opt_rebase) {
int ret = 0;
int ran_ff = 0;
--
2.29.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v5 3/3] pull: display default warning only when non-ff
2020-12-12 16:52 [PATCH v5 0/3] pull: stop warning on every pull Felipe Contreras
2020-12-12 16:52 ` [PATCH v5 1/3] pull: refactor fast-forward check Felipe Contreras
2020-12-12 16:52 ` [PATCH v5 2/3] pull: move default warning Felipe Contreras
@ 2020-12-12 16:52 ` Felipe Contreras
2020-12-12 16:56 ` [PATCH v5 0/3] pull: stop warning on every pull Felipe Contreras
3 siblings, 0 replies; 13+ messages in thread
From: Felipe Contreras @ 2020-12-12 16:52 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Junio C Hamano, Jeff King, Vít Ondruch,
Felipe Contreras
There's no need to display the annoying warning on every pull... only
the ones that are not fast-forward.
The current warning tests still pass, but not because of the arguments
or the configuration, but because they are all fast-forward.
We need to test non-fast-forward situations now.
Suggestions-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
builtin/pull.c | 7 ++--
t/t7601-merge-pull-config.sh | 66 +++++++++++++++++++++++++++++++++---
2 files changed, 66 insertions(+), 7 deletions(-)
diff --git a/builtin/pull.c b/builtin/pull.c
index ff8e3ce137..9a7caf3a3e 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -934,6 +934,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
struct object_id orig_head, curr_head;
struct object_id rebase_fork_point;
int autostash;
+ int can_ff;
if (!getenv("GIT_REFLOG_ACTION"))
set_reflog_message(argc, argv);
@@ -1029,7 +1030,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
if (opt_rebase && merge_heads.nr > 1)
die(_("Cannot rebase onto multiple branches."));
- if (default_mode && opt_verbosity >= 0 && !opt_ff) {
+ can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
+
+ if (default_mode && !can_ff && opt_verbosity >= 0 && !opt_ff) {
advise(_("Pulling without specifying how to reconcile divergent branches is\n"
"discouraged. You can squelch this message by running one of the following\n"
"commands sometime before your next pull:\n"
@@ -1058,7 +1061,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
submodule_touches_in_range(the_repository, &upstream, &curr_head))
die(_("cannot rebase with locally recorded submodule modifications"));
if (!autostash) {
- if (get_can_ff(&orig_head, &merge_heads.oid[0])) {
+ if (can_ff) {
/* we can fast-forward this without invoking rebase */
opt_ff = "--ff-only";
ran_ff = 1;
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 6774e9d86f..52e8ccc933 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -29,11 +29,8 @@ test_expect_success 'setup' '
test_expect_success 'pull.rebase not set' '
git reset --hard c0 &&
- git -c color.advice=always pull . c1 2>err &&
- test_decode_color <err >decoded &&
- test_i18ngrep "<YELLOW>hint: " decoded &&
- test_i18ngrep "Pulling without specifying how to reconcile" decoded
-
+ git pull . c1 2>err &&
+ test_i18ngrep ! "Pulling without specifying how to reconcile" err
'
test_expect_success 'pull.rebase not set and pull.ff=true' '
@@ -87,6 +84,65 @@ test_expect_success 'pull.rebase not set and --ff-only given' '
test_i18ngrep ! "Pulling without specifying how to reconcile" err
'
+test_expect_success 'pull.rebase not set (not-fast-forward)' '
+ git reset --hard c2 &&
+ git -c color.advice=always pull . c1 2>err &&
+ test_decode_color <err >decoded &&
+ test_i18ngrep "<YELLOW>hint: " decoded &&
+ test_i18ngrep "Pulling without specifying how to reconcile" decoded
+'
+
+test_expect_success 'pull.rebase not set and pull.ff=true (not-fast-forward)' '
+ git reset --hard c2 &&
+ test_config pull.ff true &&
+ git pull . c1 2>err &&
+ test_i18ngrep ! "Pulling without specifying how to reconcile" err
+'
+
+test_expect_success 'pull.rebase not set and pull.ff=false (not-fast-forward)' '
+ git reset --hard c2 &&
+ test_config pull.ff false &&
+ git pull . c1 2>err &&
+ test_i18ngrep ! "Pulling without specifying how to reconcile" err
+'
+
+test_expect_success 'pull.rebase not set and pull.ff=only (not-fast-forward)' '
+ git reset --hard c2 &&
+ test_config pull.ff only &&
+ test_must_fail git pull . c1 2>err &&
+ test_i18ngrep ! "Pulling without specifying how to reconcile" err
+'
+
+test_expect_success 'pull.rebase not set and --rebase given (not-fast-forward)' '
+ git reset --hard c2 &&
+ git pull --rebase . c1 2>err &&
+ test_i18ngrep ! "Pulling without specifying how to reconcile" err
+'
+
+test_expect_success 'pull.rebase not set and --no-rebase given (not-fast-forward)' '
+ git reset --hard c2 &&
+ git pull --no-rebase . c1 2>err &&
+ test_i18ngrep ! "Pulling without specifying how to reconcile" err
+'
+
+test_expect_success 'pull.rebase not set and --ff given (not-fast-forward)' '
+ git reset --hard c2 &&
+ git pull --ff . c1 2>err &&
+ test_i18ngrep ! "Pulling without specifying how to reconcile" err
+'
+
+test_expect_success 'pull.rebase not set and --no-ff given (not-fast-forward)' '
+ git reset --hard c2 &&
+ git pull --no-ff . c1 2>err &&
+ test_i18ngrep ! "Pulling without specifying how to reconcile" err
+'
+
+test_expect_success 'pull.rebase not set and --ff-only given (not-fast-forward)' '
+ git reset --hard c2 &&
+ test_must_fail git pull --ff-only . c1 2>err &&
+ test_i18ngrep ! "Pulling without specifying how to reconcile" err
+'
+
test_expect_success 'merge c1 with c2' '
git reset --hard c1 &&
test -f c0.c &&
--
2.29.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v5 0/3] pull: stop warning on every pull
2020-12-12 16:52 [PATCH v5 0/3] pull: stop warning on every pull Felipe Contreras
` (2 preceding siblings ...)
2020-12-12 16:52 ` [PATCH v5 3/3] pull: display default warning only when non-ff Felipe Contreras
@ 2020-12-12 16:56 ` Felipe Contreras
3 siblings, 0 replies; 13+ messages in thread
From: Felipe Contreras @ 2020-12-12 16:56 UTC (permalink / raw)
To: Git; +Cc: Elijah Newren, Junio C Hamano, Jeff King, Vít Ondruch
On Sat, Dec 12, 2020 at 10:52 AM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> The future of having a sensible "git pull" default is uncertain (it's
> very likely that nothing will change), in the meantime there's no reason
> to keep annoying our users unnecessarily.
>
> This patch is doing one thing, and one thing only: stops warning the
> users on every single pull (warns only if they are not-fast-forward).
>
> Changes since v4:
Hmm, it's not 4 -> 5, It's 5 -> 6.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v5 0/3] pull: stop warning on every pull
@ 2020-12-10 10:05 Felipe Contreras
2020-12-10 10:05 ` [PATCH v5 2/3] pull: move default warning Felipe Contreras
0 siblings, 1 reply; 13+ messages in thread
From: Felipe Contreras @ 2020-12-10 10:05 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Junio C Hamano, Jeff King, Vít Ondruch,
Felipe Contreras
The discussion about making fast-forward-only pulls the default is stuck on mud, and there's no
agreement about what we should even be warning our users about.
Even my straightforward patches about improving documentation, and the consistency of the UI with
--merge and other obvious fixes lost traction.
This is not the totality of part I, it's doing one thing, and one thing only: stops warning the
users on every single pull (warns only if they are not-fast-forward).
Everything else is dropped.
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 21b50aff77..5c3fb67c01 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -38,20 +38,6 @@ as set by linkgit:git-branch[1] `--track`.
Assume the following history exists and the current branch is
"`master`":
-------------
- A---B---C master on origin
- /
- D---E master
-------------
-
-Then `git pull` will merge in a fast-forward way up to the new master.
-
-------------
- D---E---A---B---C master, origin/master
-------------
-
-However, a non-fast-forward case looks very different.
-
------------
A---B---C master on origin
/
@@ -60,9 +46,6 @@ However, a non-fast-forward case looks very different.
origin/master in your repository
------------
-By default `git pull` will warn about these situations, however, most likely
-you would want to force a merge, which you can do with `git pull --merge`.
-
Then "`git pull`" will fetch and replay the changes from the remote
`master` branch since it diverged from the local `master` (i.e., `E`)
until its current commit (`C`) on top of `master` and record the
@@ -148,11 +131,8 @@ It rewrites history, which does not bode well when you
published that history already. Do *not* use this option
unless you have read linkgit:git-rebase[1] carefully.
--m::
---merge::
- Force a merge.
-+
-Previously this was --no-rebase, but that usage has been deprecated.
+--no-rebase::
+ Override earlier --rebase.
Options related to fetching
~~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/builtin/pull.c b/builtin/pull.c
index 118fbdeb62..9a7caf3a3e 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -27,6 +27,8 @@
#include "commit-reach.h"
#include "sequencer.h"
+static int default_mode;
+
/**
* Parses the value of --rebase. If value is a false value, returns
* REBASE_FALSE. If value is a true value, returns REBASE_TRUE. If value is
@@ -74,7 +76,7 @@ static char *opt_progress;
static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
/* Options passed to git-merge or git-rebase */
-static enum rebase_type opt_rebase;
+static enum rebase_type opt_rebase = -1;
static char *opt_diffstat;
static char *opt_log;
static char *opt_signoff;
@@ -126,11 +128,9 @@ static struct option pull_options[] = {
/* Options passed to git-merge or git-rebase */
OPT_GROUP(N_("Options related to merging")),
OPT_CALLBACK_F('r', "rebase", &opt_rebase,
- "(false|true|merges|preserve|interactive)",
- N_("incorporate changes by rebasing rather than merging"),
- PARSE_OPT_OPTARG, parse_opt_rebase),
- OPT_SET_INT('m', "merge", &opt_rebase,
- N_("incorporate changes by merging"), REBASE_FALSE),
+ "(false|true|merges|preserve|interactive)",
+ N_("incorporate changes by rebasing rather than merging"),
+ PARSE_OPT_OPTARG, parse_opt_rebase),
OPT_PASSTHRU('n', NULL, &opt_diffstat, NULL,
N_("do not show a diffstat at the end of the merge"),
PARSE_OPT_NOARG | PARSE_OPT_NONEG),
@@ -346,7 +346,9 @@ static enum rebase_type config_get_rebase(void)
if (!git_config_get_value("pull.rebase", &value))
return parse_config_rebase("pull.rebase", value, 1);
- return REBASE_DEFAULT;
+ default_mode = 1;
+
+ return REBASE_FALSE;
}
/**
@@ -441,7 +443,7 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs
const char *remote = curr_branch ? curr_branch->remote_name : NULL;
if (*refspecs) {
- if (opt_rebase >= REBASE_TRUE)
+ if (opt_rebase)
fprintf_ln(stderr, _("There is no candidate for rebasing against among the refs that you just fetched."));
else
fprintf_ln(stderr, _("There are no candidates for merging among the refs that you just fetched."));
@@ -454,7 +456,7 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs
repo);
} else if (!curr_branch) {
fprintf_ln(stderr, _("You are not currently on a branch."));
- if (opt_rebase >= REBASE_TRUE)
+ if (opt_rebase)
fprintf_ln(stderr, _("Please specify which branch you want to rebase against."));
else
fprintf_ln(stderr, _("Please specify which branch you want to merge with."));
@@ -469,7 +471,7 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs
remote_name = _("<remote>");
fprintf_ln(stderr, _("There is no tracking information for the current branch."));
- if (opt_rebase >= REBASE_TRUE)
+ if (opt_rebase)
fprintf_ln(stderr, _("Please specify which branch you want to rebase against."));
else
fprintf_ln(stderr, _("Please specify which branch you want to merge with."));
@@ -931,11 +933,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
struct oid_array merge_heads = OID_ARRAY_INIT;
struct object_id orig_head, curr_head;
struct object_id rebase_fork_point;
+ int autostash;
int can_ff;
- opt_ff = xstrdup_or_null(config_get_ff());
- opt_rebase = config_get_rebase();
-
if (!getenv("GIT_REFLOG_ACTION"))
set_reflog_message(argc, argv);
@@ -952,6 +952,12 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
parse_repo_refspecs(argc, argv, &repo, &refspecs);
+ if (!opt_ff)
+ opt_ff = xstrdup_or_null(config_get_ff());
+
+ if (opt_rebase < 0)
+ opt_rebase = config_get_rebase();
+
if (read_cache_unmerged())
die_resolve_conflict("pull");
@@ -961,8 +967,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
if (get_oid("HEAD", &orig_head))
oidclr(&orig_head);
- if (opt_rebase >= REBASE_TRUE) {
- int autostash = config_autostash;
+ autostash = config_autostash;
+ if (opt_rebase) {
if (opt_autostash != -1)
autostash = opt_autostash;
@@ -1021,28 +1027,29 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
die(_("Cannot merge multiple branches into empty head."));
return pull_into_void(merge_heads.oid, &curr_head);
}
- if (opt_rebase >= REBASE_TRUE && merge_heads.nr > 1)
+ if (opt_rebase && merge_heads.nr > 1)
die(_("Cannot rebase onto multiple branches."));
can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
- if (!opt_rebase && !can_ff && opt_verbosity >= 0 && (!opt_ff || !strcmp(opt_ff, "--ff"))) {
- advise(_("Pulling without specifying how to reconcile divergent branches is discouraged;\n"
- "you need to specify if you want a merge, or a rebase.\n"
- "You can squelch this message by running one of the following commands:\n"
- "\n"
- " git config pull.rebase false # merge (the default strategy)\n"
- " git config pull.rebase true # rebase\n"
- " git config pull.ff only # fast-forward only\n"
- "\n"
- "You can replace \"git config\" with \"git config --global\" to set a default\n"
- "preference for all repositories.\n"
- "If unsure, run \"git pull --merge\".\n"
- "Read \"git pull --help\" for more information."));
+ if (default_mode && !can_ff && opt_verbosity >= 0 && !opt_ff) {
+ advise(_("Pulling without specifying how to reconcile divergent branches is\n"
+ "discouraged. You can squelch this message by running one of the following\n"
+ "commands sometime before your next pull:\n"
+ "\n"
+ " git config pull.rebase false # merge (the default strategy)\n"
+ " git config pull.rebase true # rebase\n"
+ " git config pull.ff only # fast-forward only\n"
+ "\n"
+ "You can replace \"git config\" with \"git config --global\" to set a default\n"
+ "preference for all repositories. You can also pass --rebase, --no-rebase,\n"
+ "or --ff-only on the command line to override the configured default per\n"
+ "invocation.\n"));
}
- if (opt_rebase >= REBASE_TRUE) {
+ if (opt_rebase) {
int ret = 0;
+ int ran_ff = 0;
struct object_id newbase;
struct object_id upstream;
@@ -1053,15 +1060,16 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&
submodule_touches_in_range(the_repository, &upstream, &curr_head))
die(_("cannot rebase with locally recorded submodule modifications"));
-
- if (can_ff) {
- /* we can fast-forward this without invoking rebase */
- free(opt_ff);
- opt_ff = xstrdup_or_null("--ff-only");
- ret = run_merge();
- } else {
- ret = run_rebase(&newbase, &upstream);
+ if (!autostash) {
+ if (can_ff) {
+ /* we can fast-forward this without invoking rebase */
+ opt_ff = "--ff-only";
+ ran_ff = 1;
+ ret = run_merge();
+ }
}
+ if (!ran_ff)
+ ret = run_rebase(&newbase, &upstream);
if (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||
recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))
diff --git a/rebase.h b/rebase.h
index 34d4acfd74..cc723d4748 100644
--- a/rebase.h
+++ b/rebase.h
@@ -3,8 +3,7 @@
enum rebase_type {
REBASE_INVALID = -1,
- REBASE_DEFAULT = 0,
- REBASE_FALSE,
+ REBASE_FALSE = 0,
REBASE_TRUE,
REBASE_PRESERVE,
REBASE_MERGES,
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 1a4fe2583a..db1a381cd9 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -11,10 +11,10 @@ test_expect_success 'setup' '
git commit -m one)
'
-test_expect_success 'git pull -q' '
+test_expect_success 'git pull -q --no-rebase' '
mkdir clonedq &&
(cd clonedq && git init &&
- git pull -q "../parent" >out 2>err &&
+ git pull -q --no-rebase "../parent" >out 2>err &&
test_must_be_empty err &&
test_must_be_empty out)
'
@@ -30,10 +30,10 @@ test_expect_success 'git pull -q --rebase' '
test_must_be_empty out)
'
-test_expect_success 'git pull' '
+test_expect_success 'git pull --no-rebase' '
mkdir cloned &&
(cd cloned && git init &&
- git pull "../parent" >out 2>err &&
+ git pull --no-rebase "../parent" >out 2>err &&
test -s err &&
test_must_be_empty out)
'
@@ -46,10 +46,10 @@ test_expect_success 'git pull --rebase' '
test_must_be_empty out)
'
-test_expect_success 'git pull -v' '
+test_expect_success 'git pull -v --no-rebase' '
mkdir clonedv &&
(cd clonedv && git init &&
- git pull -v "../parent" >out 2>err &&
+ git pull -v --no-rebase "../parent" >out 2>err &&
test -s err &&
test_must_be_empty out)
'
@@ -62,25 +62,25 @@ test_expect_success 'git pull -v --rebase' '
test_must_be_empty out)
'
-test_expect_success 'git pull -v -q' '
+test_expect_success 'git pull -v -q --no-rebase' '
mkdir clonedvq &&
(cd clonedvq && git init &&
- git pull -v -q "../parent" >out 2>err &&
+ git pull -v -q --no-rebase "../parent" >out 2>err &&
test_must_be_empty out &&
test_must_be_empty err)
'
-test_expect_success 'git pull -q -v' '
+test_expect_success 'git pull -q -v --no-rebase' '
mkdir clonedqv &&
(cd clonedqv && git init &&
- git pull -q -v "../parent" >out 2>err &&
+ git pull -q -v --no-rebase "../parent" >out 2>err &&
test_must_be_empty out &&
test -s err)
'
test_expect_success 'git pull --cleanup errors early on invalid argument' '
mkdir clonedcleanup &&
(cd clonedcleanup && git init &&
- test_must_fail git pull --cleanup invalid "../parent" >out 2>err &&
+ test_must_fail git pull --no-rebase --cleanup invalid "../parent" >out 2>err &&
test_must_be_empty out &&
test -s err)
'
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 8a6aae564a..6b4adab8b1 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -33,6 +33,7 @@ test_expect_success 'pull.rebase not set' '
test_decode_color <err >decoded &&
test_i18ngrep "<YELLOW>hint: " decoded &&
test_i18ngrep "Pulling without specifying how to reconcile" decoded
+
'
test_expect_success 'pull.rebase not set (fast-forward)' '
@@ -45,7 +46,7 @@ test_expect_success 'pull.rebase not set and pull.ff=true' '
git reset --hard c2 &&
test_config pull.ff true &&
git pull . c1 2>err &&
- test_i18ngrep "Pulling without specifying how to reconcile" err
+ test_i18ngrep ! "Pulling without specifying how to reconcile" err
'
test_expect_success 'pull.rebase not set and pull.ff=false' '
@@ -68,16 +69,16 @@ test_expect_success 'pull.rebase not set and --rebase given' '
test_i18ngrep ! "Pulling without specifying how to reconcile" err
'
-test_expect_success 'pull.rebase not set and --merge given' '
+test_expect_success 'pull.rebase not set and --no-rebase given' '
git reset --hard c2 &&
- git pull --merge . c1 2>err &&
+ git pull --no-rebase . c1 2>err &&
test_i18ngrep ! "Pulling without specifying how to reconcile" err
'
test_expect_success 'pull.rebase not set and --ff given' '
git reset --hard c2 &&
git pull --ff . c1 2>err &&
- test_i18ngrep "Pulling without specifying how to reconcile" err
+ test_i18ngrep ! "Pulling without specifying how to reconcile" err
'
test_expect_success 'pull.rebase not set and --no-ff given' '
Felipe Contreras (3):
pull: refactor fast-forward check
pull: move default warning
pull: display default warning only when non-ff
builtin/pull.c | 61 +++++++++++++++++++++---------------
t/t7601-merge-pull-config.sh | 28 ++++++++++-------
2 files changed, 53 insertions(+), 36 deletions(-)
--
2.29.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v5 2/3] pull: move default warning
2020-12-10 10:05 Felipe Contreras
@ 2020-12-10 10:05 ` Felipe Contreras
2020-12-11 6:54 ` Junio C Hamano
0 siblings, 1 reply; 13+ messages in thread
From: Felipe Contreras @ 2020-12-10 10:05 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Junio C Hamano, Jeff King, Vít Ondruch,
Felipe Contreras
Eventually we want to be able to display the warning only when
fast-forward merges are not possible.
In order to do so we need to move the default warning up to the point
where we can check if we can fast-forward or not.
Additionally, config_get_rebase() was probably never its true home.
This requires a temporary variable to check if we are in the
"default mode" (no --rebase or --no-rebase specified).
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
builtin/pull.c | 32 ++++++++++++++++++--------------
1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/builtin/pull.c b/builtin/pull.c
index 03e6d53243..ff8e3ce137 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -27,6 +27,8 @@
#include "commit-reach.h"
#include "sequencer.h"
+static int default_mode;
+
/**
* Parses the value of --rebase. If value is a false value, returns
* REBASE_FALSE. If value is a true value, returns REBASE_TRUE. If value is
@@ -344,20 +346,7 @@ static enum rebase_type config_get_rebase(void)
if (!git_config_get_value("pull.rebase", &value))
return parse_config_rebase("pull.rebase", value, 1);
- if (opt_verbosity >= 0 && !opt_ff) {
- advise(_("Pulling without specifying how to reconcile divergent branches is\n"
- "discouraged. You can squelch this message by running one of the following\n"
- "commands sometime before your next pull:\n"
- "\n"
- " git config pull.rebase false # merge (the default strategy)\n"
- " git config pull.rebase true # rebase\n"
- " git config pull.ff only # fast-forward only\n"
- "\n"
- "You can replace \"git config\" with \"git config --global\" to set a default\n"
- "preference for all repositories. You can also pass --rebase, --no-rebase,\n"
- "or --ff-only on the command line to override the configured default per\n"
- "invocation.\n"));
- }
+ default_mode = 1;
return REBASE_FALSE;
}
@@ -1040,6 +1029,21 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
if (opt_rebase && merge_heads.nr > 1)
die(_("Cannot rebase onto multiple branches."));
+ if (default_mode && opt_verbosity >= 0 && !opt_ff) {
+ advise(_("Pulling without specifying how to reconcile divergent branches is\n"
+ "discouraged. You can squelch this message by running one of the following\n"
+ "commands sometime before your next pull:\n"
+ "\n"
+ " git config pull.rebase false # merge (the default strategy)\n"
+ " git config pull.rebase true # rebase\n"
+ " git config pull.ff only # fast-forward only\n"
+ "\n"
+ "You can replace \"git config\" with \"git config --global\" to set a default\n"
+ "preference for all repositories. You can also pass --rebase, --no-rebase,\n"
+ "or --ff-only on the command line to override the configured default per\n"
+ "invocation.\n"));
+ }
+
if (opt_rebase) {
int ret = 0;
int ran_ff = 0;
--
2.29.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v5 2/3] pull: move default warning
2020-12-10 10:05 ` [PATCH v5 2/3] pull: move default warning Felipe Contreras
@ 2020-12-11 6:54 ` Junio C Hamano
2020-12-11 7:55 ` Felipe Contreras
0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2020-12-11 6:54 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Elijah Newren, Jeff King, Vít Ondruch
Felipe Contreras <felipe.contreras@gmail.com> writes:
> Eventually we want to be able to display the warning only when
> fast-forward merges are not possible.
>
> In order to do so we need to move the default warning up to the point
> where we can check if we can fast-forward or not.
Makes sense.
> Additionally, config_get_rebase() was probably never its true home.
I agree with this point. I've always found it suboptimal.
> This requires a temporary variable to check if we are in the
> "default mode" (no --rebase or --no-rebase specified).
Two points:
- "mode" is so overused a word; a more focused word is preferrable.
- by introducing a local variable in cmd_pull() and passing a
pointer to it to config_get_rebase(), we can easily avoid having
to rely on an extra global variable.
I'd suggest addressing the above along the following lines.
-static enum rebase_type config_get_rebase(void)
+static enum rebase_type config_get_rebase(int *rebase_unspecified)
{
+ *rebase_unspecified = 0;
... various "return" of configured values ...
+ *rebase_unspecified = 1;
return REBASE_FALSE;
}
Then the caller would declare
int rebase_unspecified = 0;
and call
if (opt_rebase < 0)
opt_rebase = config_get_rebase(&rebase_unspecified);
to possibly cause it to set to true, and use that instead of the
global variable to decide if we want to give the help text. When
the helper is not called due to opt_rebase already being set, it is
not using configured value but using the choice from the command
line, so rebase_unspecified is still false after this point.
> @@ -1040,6 +1029,21 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
> if (opt_rebase && merge_heads.nr > 1)
> die(_("Cannot rebase onto multiple branches."));
And this is the point where we finish various error checks and
starts to run either rebase or merge. It is as late as we could
delay the "non-ff and you are not configured" message. In other
words, the place chosen in cmd_pull() to move this code to is
optimal.
> + if (default_mode && opt_verbosity >= 0 && !opt_ff) {
> + advise(_("Pulling without specifying how to reconcile divergent branches is\n"
> + "discouraged. You can squelch this message by running one of the following\n"
> + "commands sometime before your next pull:\n"
> + "\n"
> + " git config pull.rebase false # merge (the default strategy)\n"
> + " git config pull.rebase true # rebase\n"
> + " git config pull.ff only # fast-forward only\n"
> + "\n"
> + "You can replace \"git config\" with \"git config --global\" to set a default\n"
> + "preference for all repositories. You can also pass --rebase, --no-rebase,\n"
> + "or --ff-only on the command line to override the configured default per\n"
> + "invocation.\n"));
> + }
Either as a part of this step, as a part of the next step, or a
separate follow-up patch, we should
- create a single-purpose helper function that only calls advise()
with the message and returns; name it show_advice_pull_non_ff().
- correct the if() statement above, so that regardless of verbosity
level, we can do _something_ common when the history does not
fast-forward. I.e.
if (rebase_unspecified && !opt_ff) {
if (opt_verbosity >= 0)
show_advice_pull_non_ff();
}
These would allow us to further turn the logic to
if (rebase_unspecified && !opt_ff) {
if (opt_verbosity >= 0 && advice_pull_non_ff)
show_advice_pull_non_ff();
die("not a fast-forward; must merge or rebase");
}
later in the far future, and we do not want that die() to be
affected by verbosity settings.
I'll queue such a fix-up patch on top of the series before pushing
the integration results out on 'seen'.
Thanks.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 2/3] pull: move default warning
2020-12-11 6:54 ` Junio C Hamano
@ 2020-12-11 7:55 ` Felipe Contreras
2020-12-12 0:00 ` Junio C Hamano
2020-12-12 16:42 ` Felipe Contreras
0 siblings, 2 replies; 13+ messages in thread
From: Felipe Contreras @ 2020-12-11 7:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git, Elijah Newren, Jeff King, Vít Ondruch
On Fri, Dec 11, 2020 at 12:54 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > Eventually we want to be able to display the warning only when
> > fast-forward merges are not possible.
> >
> > In order to do so we need to move the default warning up to the point
> > where we can check if we can fast-forward or not.
>
> Makes sense.
>
> > Additionally, config_get_rebase() was probably never its true home.
>
> I agree with this point. I've always found it suboptimal.
>
> > This requires a temporary variable to check if we are in the
> > "default mode" (no --rebase or --no-rebase specified).
>
> Two points:
>
> - "mode" is so overused a word; a more focused word is preferrable.
There's literally only one instance in the file, and it's a call to an
external function.
Plus, it refers to "git pull" without any --merge or --rebase (that's
the default mode in my book).
> - by introducing a local variable in cmd_pull() and passing a
> pointer to it to config_get_rebase(), we can easily avoid having
> to rely on an extra global variable.
>
> I'd suggest addressing the above along the following lines.
...
> to possibly cause it to set to true, and use that instead of the
> global variable to decide if we want to give the help text.
Yeah, there's only 38 global variables. We wouldn't want another one
which makes the code pretty straightforward.
> > @@ -1040,6 +1029,21 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
> > if (opt_rebase && merge_heads.nr > 1)
> > die(_("Cannot rebase onto multiple branches."));
>
> And this is the point where we finish various error checks and
> starts to run either rebase or merge. It is as late as we could
> delay the "non-ff and you are not configured" message. In other
> words, the place chosen in cmd_pull() to move this code to is
> optimal.
Which is right before the fork between rebase and merge.
> > + if (default_mode && opt_verbosity >= 0 && !opt_ff) {
> > + advise(_("Pulling without specifying how to reconcile divergent branches is\n"
> > + "discouraged. You can squelch this message by running one of the following\n"
> > + "commands sometime before your next pull:\n"
> > + "\n"
> > + " git config pull.rebase false # merge (the default strategy)\n"
> > + " git config pull.rebase true # rebase\n"
> > + " git config pull.ff only # fast-forward only\n"
> > + "\n"
> > + "You can replace \"git config\" with \"git config --global\" to set a default\n"
> > + "preference for all repositories. You can also pass --rebase, --no-rebase,\n"
> > + "or --ff-only on the command line to override the configured default per\n"
> > + "invocation.\n"));
> > + }
>
> Either as a part of this step, as a part of the next step, or a
> separate follow-up patch, we should
>
> - create a single-purpose helper function that only calls advise()
> with the message and returns; name it show_advice_pull_non_ff().
If we are going for low-hanging fruit there were many in v4 of this
series, which actually improve behavior, not just code organization,
but OK.
> - correct the if() statement above, so that regardless of verbosity
> level, we can do _something_ common when the history does not
> fast-forward. I.e.
>
> if (rebase_unspecified && !opt_ff) {
> if (opt_verbosity >= 0)
> show_advice_pull_non_ff();
> }
>
> These would allow us to further turn the logic to
>
> if (rebase_unspecified && !opt_ff) {
> if (opt_verbosity >= 0 && advice_pull_non_ff)
> show_advice_pull_non_ff();
> die("not a fast-forward; must merge or rebase");
> }
Should actually be something like:
if (rebase_unspecified && !can_ff)
die("Not a fast-forward; must either merge or rebase");
But yeah.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 2/3] pull: move default warning
2020-12-11 7:55 ` Felipe Contreras
@ 2020-12-12 0:00 ` Junio C Hamano
2020-12-12 1:05 ` Felipe Contreras
2020-12-12 16:42 ` Felipe Contreras
1 sibling, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2020-12-12 0:00 UTC (permalink / raw)
To: Felipe Contreras; +Cc: Git, Elijah Newren, Jeff King, Vít Ondruch
Felipe Contreras <felipe.contreras@gmail.com> writes:
>> - correct the if() statement above, so that regardless of verbosity
>> level, we can do _something_ common when the history does not
>> fast-forward. I.e.
>>
>> if (rebase_unspecified && !opt_ff) {
>> if (opt_verbosity >= 0)
>> show_advice_pull_non_ff();
>> }
>>
>> These would allow us to further turn the logic to
>>
>> if (rebase_unspecified && !opt_ff) {
>> if (opt_verbosity >= 0 && advice_pull_non_ff)
>> show_advice_pull_non_ff();
>> die("not a fast-forward; must merge or rebase");
>> }
>
> Should actually be something like:
>
> if (rebase_unspecified && !can_ff)
> die("Not a fast-forward; must either merge or rebase");
The illustration I gave in the message you are responding to was
made in the context of patch 2/3; with patch 3/3 where can_ff
exists, it would not become like what you gave above. It should
instead become
if (rebase_unspecified && !opt_ff && !can_ff) {
if (opt_verbosity >= 0 && advice_pull_non_ff)
show_advice_pull_non_ff();
die("not a fast-forward; must merge or rebase");
}
i.e. when we can fast-forward, we do not trigger the "you must
specify rebase/merge" message, and we do not trigger the "not a
fast-forward" error.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 2/3] pull: move default warning
2020-12-12 0:00 ` Junio C Hamano
@ 2020-12-12 1:05 ` Felipe Contreras
2020-12-13 20:58 ` Junio C Hamano
0 siblings, 1 reply; 13+ messages in thread
From: Felipe Contreras @ 2020-12-12 1:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git, Elijah Newren, Jeff King, Vít Ondruch
On Fri, Dec 11, 2020 at 6:00 PM Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> > Should actually be something like:
> >
> > if (rebase_unspecified && !can_ff)
> > die("Not a fast-forward; must either merge or rebase");
>
> The illustration I gave in the message you are responding to was
> made in the context of patch 2/3; with patch 3/3 where can_ff
> exists, it would not become like what you gave above. It should
> instead become
>
> if (rebase_unspecified && !opt_ff && !can_ff) {
> if (opt_verbosity >= 0 && advice_pull_non_ff)
> show_advice_pull_non_ff();
> die("not a fast-forward; must merge or rebase");
> }
>
> i.e. when we can fast-forward, we do not trigger the "you must
> specify rebase/merge" message, and we do not trigger the "not a
> fast-forward" error.
It's not the !can_ff part I'm trying to highlight, it's the lack of
advice *after* we have decided to flip the switch.
As I said in another thread: I don't think we have any long
condescending error in any other command.
Cheers.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 2/3] pull: move default warning
2020-12-12 1:05 ` Felipe Contreras
@ 2020-12-13 20:58 ` Junio C Hamano
2020-12-14 11:02 ` Felipe Contreras
0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2020-12-13 20:58 UTC (permalink / raw)
To: Felipe Contreras; +Cc: Git, Elijah Newren, Jeff King, Vít Ondruch
Felipe Contreras <felipe.contreras@gmail.com> writes:
> On Fri, Dec 11, 2020 at 6:00 PM Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> > Should actually be something like:
>> >
>> > if (rebase_unspecified && !can_ff)
>> > die("Not a fast-forward; must either merge or rebase");
>>
>> The illustration I gave in the message you are responding to was
>> made in the context of patch 2/3; with patch 3/3 where can_ff
>> exists, it would not become like what you gave above. It should
>> instead become
>>
>> if (rebase_unspecified && !opt_ff && !can_ff) {
>> if (opt_verbosity >= 0 && advice_pull_non_ff)
>> show_advice_pull_non_ff();
>> die("not a fast-forward; must merge or rebase");
>> }
>>
>> i.e. when we can fast-forward, we do not trigger the "you must
>> specify rebase/merge" message, and we do not trigger the "not a
>> fast-forward" error.
>
> It's not the !can_ff part I'm trying to highlight, it's the lack of
> advice *after* we have decided to flip the switch.
>
> As I said in another thread: I don't think we have any long
> condescending error in any other command.
Only the "not a fast-forward and you must choose between merge and
rebase" part (i.e. what I listed as the first part of three-part
message) is the error.
The rest, the message that teaches how to choose between merge and
rebase from command line and configuration (or how to choose
permanently not to make the choice between the two), is not an
error---it's called advice.
Even if we were to introduce the third choice (i.e. permanently not
to choose between rebase or merge and instead just error out without
getting advice), the advice must stay for those who didn't make any
choice among the three (i.e. merge, rebase or ff-only).
We have many of them. An easy example to spot is a similar sized
onefor "git checkout HEAD^0". Neither that one or the one under
discussion is particularly condescending.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 2/3] pull: move default warning
2020-12-13 20:58 ` Junio C Hamano
@ 2020-12-14 11:02 ` Felipe Contreras
0 siblings, 0 replies; 13+ messages in thread
From: Felipe Contreras @ 2020-12-14 11:02 UTC (permalink / raw)
To: Junio C Hamano, Felipe Contreras
Cc: Git, Elijah Newren, Jeff King, Vít Ondruch
Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > On Fri, Dec 11, 2020 at 6:00 PM Junio C Hamano <gitster@pobox.com> wrote:
> >> Felipe Contreras <felipe.contreras@gmail.com> writes:
> >
> >> > Should actually be something like:
> >> >
> >> > if (rebase_unspecified && !can_ff)
> >> > die("Not a fast-forward; must either merge or rebase");
> >>
> >> The illustration I gave in the message you are responding to was
> >> made in the context of patch 2/3; with patch 3/3 where can_ff
> >> exists, it would not become like what you gave above. It should
> >> instead become
> >>
> >> if (rebase_unspecified && !opt_ff && !can_ff) {
> >> if (opt_verbosity >= 0 && advice_pull_non_ff)
> >> show_advice_pull_non_ff();
> >> die("not a fast-forward; must merge or rebase");
> >> }
> >>
> >> i.e. when we can fast-forward, we do not trigger the "you must
> >> specify rebase/merge" message, and we do not trigger the "not a
> >> fast-forward" error.
> >
> > It's not the !can_ff part I'm trying to highlight, it's the lack of
> > advice *after* we have decided to flip the switch.
> >
> > As I said in another thread: I don't think we have any long
> > condescending error in any other command.
>
> Only the "not a fast-forward and you must choose between merge and
> rebase" part (i.e. what I listed as the first part of three-part
> message) is the error.
>
> The rest, the message that teaches how to choose between merge and
> rebase from command line and configuration (or how to choose
> permanently not to make the choice between the two), is not an
> error---it's called advice.
Right. After re-reading the 2013 discussion, and the GitHub trainers'
input that Jeff King shared with us [1], it might make sense to
eventually keep this advice (permanently), since many users might not
know what a rebase is, and would not want to know at this point of their
learning curve.
That being said, the advice still needs to be improved, and I have
already yet another version taking this into consideration ready to be
sent. Just waiting for feedback on the previous can_ff series.
> Even if we were to introduce the third choice (i.e. permanently not
> to choose between rebase or merge and instead just error out without
> getting advice), the advice must stay for those who didn't make any
> choice among the three (i.e. merge, rebase or ff-only).
With the introduction of such mode, and the permanent advice, we would
have:
if (!can_ff) {
if (!mode && opt_verbosity >= 0 && advice_pull_non_ff)
show_advice_pull_non_ff();
if (mode == PULL_MODE_FF_ONLY)
die(_("Not a fast-forward, either merge or rebase.\n"));
}
If in the future we decide to make it the default, we would have:
if (!can_ff) {
if (!mode && opt_verbosity >= 0 && advice_pull_non_ff)
show_advice_pull_non_ff();
if (!mode || mode == PULL_MODE_FF_ONLY)
die(_("Not a fast-forward, either merge or rebase.\n"));
}
The transition is very straightforward.
> We have many of them. An easy example to spot is a similar sized
> onefor "git checkout HEAD^0". Neither that one or the one under
> discussion is particularly condescending.
I am not familiar with these "advices", since I basically ignore them,
and they are not presented to me in any different color, or special
presentation. I generally ignore textwalls in my command line.
But fine, if these walls of text already exist, perhaps it makes sense
to have yet another permanent one (just with better text, and better
options).
With an improved advice--which includes an option that is good enough we
*can* make the default (pull.mode=ff-only)--perhaps that would be
enough, and in a couple of years we can make the assessment if it's
enough or not.
But clearly *today* we don't have such mode, which people have found
missing for more than a decadte.
Cheers.
[1] https://lore.kernel.org/git/20130909201751.GA14437@sigill.intra.peff.net/
--
Felipe Contreras
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 2/3] pull: move default warning
2020-12-11 7:55 ` Felipe Contreras
2020-12-12 0:00 ` Junio C Hamano
@ 2020-12-12 16:42 ` Felipe Contreras
1 sibling, 0 replies; 13+ messages in thread
From: Felipe Contreras @ 2020-12-12 16:42 UTC (permalink / raw)
To: Felipe Contreras, Junio C Hamano
Cc: Git, Elijah Newren, Jeff King, Vít Ondruch
Felipe Contreras wrote:
> On Fri, Dec 11, 2020 at 12:54 AM Junio C Hamano <gitster@pobox.com> wrote:
> > - by introducing a local variable in cmd_pull() and passing a
> > pointer to it to config_get_rebase(), we can easily avoid having
> > to rely on an extra global variable.
> >
> > I'd suggest addressing the above along the following lines.
>
> ...
>
> > to possibly cause it to set to true, and use that instead of the
> > global variable to decide if we want to give the help text.
>
> Yeah, there's only 38 global variables. We wouldn't want another one
> which makes the code pretty straightforward.
Another thing that I mentioned in another version of the patch series
but not on this one (since the relevant patch was removed) is that this
variable is only needed temporarily; it can be removed by further code
reorganization.
So, I don't think it makes sense to change more code than necessary for
the sake of this variable (that might very well disappear).
--
Felipe Contreras
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-12-14 11:16 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-12 16:52 [PATCH v5 0/3] pull: stop warning on every pull Felipe Contreras
2020-12-12 16:52 ` [PATCH v5 1/3] pull: refactor fast-forward check Felipe Contreras
2020-12-12 16:52 ` [PATCH v5 2/3] pull: move default warning Felipe Contreras
2020-12-12 16:52 ` [PATCH v5 3/3] pull: display default warning only when non-ff Felipe Contreras
2020-12-12 16:56 ` [PATCH v5 0/3] pull: stop warning on every pull Felipe Contreras
-- strict thread matches above, loose matches on Subject: below --
2020-12-10 10:05 Felipe Contreras
2020-12-10 10:05 ` [PATCH v5 2/3] pull: move default warning Felipe Contreras
2020-12-11 6:54 ` Junio C Hamano
2020-12-11 7:55 ` Felipe Contreras
2020-12-12 0:00 ` Junio C Hamano
2020-12-12 1:05 ` Felipe Contreras
2020-12-13 20:58 ` Junio C Hamano
2020-12-14 11:02 ` Felipe Contreras
2020-12-12 16:42 ` Felipe Contreras
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).