[PATCH v7 0/5] making pull advice not to trigger when unneeded

[PATCH v7 0/5] making pull advice not to trigger when unneeded

[Junio C Hamano]

Just to show what was pushed out on 'seen' while I was cutting the
preview release...

This is based on Felipe's v6 (which was mislabled as v5 but sent on
a different day from the true v5), with two clean-up commits
inserted between Felipe's second step (now 2/5) and the third step
(now 5/5, with necessary adjustments).

Even with the "correct condition" clean-up, I am not quite happy
with the clarity of the logic around there.

I think !opt_ff does not exactly belong to the condition, if we
consider that the eventual endgame should be to stop non-ff
operation without merge or rebase by default with an error, and that
should happen in this block.  The error message there should say
"unable to fast-forward; must merge or rebase", which should equally
apply to those who gave "--ff-only" explicitly, even though they may
not need the advice message.

So with the help of can_ff bit introduced by 5/5, I _think_ this
part should become like

-	if (rebase_unspecified && !opt_ff && !can_ff) {
+	if (rebase_unspecified && !can_ff &&
+	    (!opt_ff || !strcmp("--ff-only", opt_ff))) {

before/when we make this codepath error out.  We won't hit the body
of this if statement when we can fast-forward.

To avoid the ugly-looking "strcmp()" in the above, we may need to
adjust "--ff" (fast-forward without creating an extra merge commit)
and "--no-ff" (create an extra merge commit when the history could
be fast-forwarded) to imply "merge", though.  It would automatically
make rebase_unspecified would become false.  With such a tweak, we
can then simplify it further to

-	if (rebase_unspecified && !can_ff &&
-	    (!opt_ff || !strcmp("--ff-only", opt_ff))) {
+	if (rebase_unspecified && !can_ff) {

But these are not part of this round.


Felipe Contreras (3):
  pull: refactor fast-forward check
  pull: give the advice for choosing rebase/merge much later
  pull: display default warning only when non-ff

Junio C Hamano (2):
  pull: get rid of unnecessary global variable
  pull: correct condition to trigger non-ff advice

 builtin/pull.c               | 70 ++++++++++++++++++++++--------------
 t/t7601-merge-pull-config.sh | 66 +++++++++++++++++++++++++++++++---
 2 files changed, 104 insertions(+), 32 deletions(-)

-- 
2.30.0-rc0-186-g20447144ec

[PATCH v7 1/5] pull: refactor fast-forward check

[Junio C Hamano]

From: Felipe Contreras <felipe.contreras@gmail.com>

We would like to be able to make this check before the decision to
rebase is made in a future step.  Besides, using a separate helper
makes the code easier to follow.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.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.30.0-rc0-186-g20447144ec

[PATCH v7 2/5] pull: give the advice for choosing rebase/merge much later

[Junio C Hamano]

From: Felipe Contreras <felipe.contreras@gmail.com>

Eventually we want to be omit the advice when we can fast-forward
in which case there is no reason to require the user to choose
between rebase or merge.

In order to do so, we need to delay giving the advice 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.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.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.30.0-rc0-186-g20447144ec

[PATCH v7 3/5] pull: get rid of unnecessary global variable

[Junio C Hamano]

It is easy enough to do, gives a more descriptive name to the
variable, and there is no reason to make the code deliberately worse
by ignoring improvement offered on the list.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/pull.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index ff8e3ce137..2976b8e5cb 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -27,8 +27,6 @@
 #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
@@ -326,7 +324,7 @@ static const char *config_get_ff(void)
  * looks for the value of "pull.rebase". If both configuration keys do not
  * exist, returns REBASE_FALSE.
  */
-static enum rebase_type config_get_rebase(void)
+static enum rebase_type config_get_rebase(int *rebase_unspecified)
 {
 	struct branch *curr_branch = branch_get("HEAD");
 	const char *value;
@@ -346,7 +344,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);
 
-	default_mode = 1;
+	*rebase_unspecified = 1;
 
 	return REBASE_FALSE;
 }
@@ -934,6 +932,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 rebase_unspecified = 0;
 
 	if (!getenv("GIT_REFLOG_ACTION"))
 		set_reflog_message(argc, argv);
@@ -955,7 +954,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		opt_ff = xstrdup_or_null(config_get_ff());
 
 	if (opt_rebase < 0)
-		opt_rebase = config_get_rebase();
+		opt_rebase = config_get_rebase(&rebase_unspecified);
 
 	if (read_cache_unmerged())
 		die_resolve_conflict("pull");
@@ -1029,7 +1028,7 @@ 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) {
+	if (rebase_unspecified && 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"
-- 
2.30.0-rc0-186-g20447144ec

[PATCH v7 4/5] pull: correct condition to trigger non-ff advice

[Junio C Hamano]

Refactor the advise() call that teaches users how they can choose
between merge and rebase into a helper function.  This revealed that
the caller's logic needs to be further clarified to allow future
actions (like "erroring out" instead of the current "go ahead and
merge anyway") that should happen whether the advice message is
squelched out.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/pull.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 2976b8e5cb..1b87ea95eb 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -925,6 +925,22 @@ static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_
 	return ret;
 }
 
+static void show_advice_pull_non_ff(void)
+{
+	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"));
+}
+
 int cmd_pull(int argc, const char **argv, const char *prefix)
 {
 	const char *repo, **refspecs;
@@ -1028,19 +1044,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 (rebase_unspecified && 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 (rebase_unspecified && !opt_ff) {
+		if (opt_verbosity >= 0)
+			show_advice_pull_non_ff();
 	}
 
 	if (opt_rebase) {
-- 
2.30.0-rc0-186-g20447144ec

[PATCH v7 5/5] pull: display default warning only when non-ff

[Junio C Hamano]

From: Felipe Contreras <felipe.contreras@gmail.com>

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>
Signed-off-by: Junio C Hamano <gitster@pobox.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 1b87ea95eb..e8927fc2ff 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -949,6 +949,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	struct object_id rebase_fork_point;
 	int autostash;
 	int rebase_unspecified = 0;
+	int can_ff;
 
 	if (!getenv("GIT_REFLOG_ACTION"))
 		set_reflog_message(argc, argv);
@@ -1044,7 +1045,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 (rebase_unspecified && !opt_ff) {
+	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
+
+	if (rebase_unspecified && !opt_ff && !can_ff) {
 		if (opt_verbosity >= 0)
 			show_advice_pull_non_ff();
 	}
@@ -1063,7 +1066,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.30.0-rc0-186-g20447144ec

RE: [PATCH v7 3/5] pull: get rid of unnecessary global variable

[Felipe Contreras]

Junio C Hamano wrote:
> It is easy enough to do,

Yes.

> gives a more descriptive name to the variable,

I disagree.

> and there is no reason to make the code deliberately worse by ignoring
> improvement offered on the list.

I doubt any person contributing to the mailing is making the code
deliberately worse.

And I certainly did not ignore any improvement on the list. I responded
to the suggestion, I just disagreed it's actually an improvement.

In my opinion differences of opinion must be tolerated.

And I don't think any of that belongs in the commit message. "Gives a
more descriptive name to the variable" (in your opinion) should be
enough.

Anyway, I like the fact that your opinion and my opinion are clearly
demarcated in two different commits.

Cheers.

-- 
Felipe Contreras

RE: [PATCH v7 4/5] pull: correct condition to trigger non-ff advice

[Felipe Contreras]

Junio C Hamano wrote:
> Refactor the advise() call that teaches users how they can choose
> between merge and rebase into a helper function.  This revealed that
> the caller's logic needs to be further clarified to allow future
> actions (like "erroring out" instead of the current "go ahead and
> merge anyway") that should happen whether the advice message is
> squelched out.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/pull.c | 32 +++++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 2976b8e5cb..1b87ea95eb 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -925,6 +925,22 @@ static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_
>  	return ret;
>  }
>  
> +static void show_advice_pull_non_ff(void)
> +{
> +	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"));
> +}


While this change is good, I see a lot of advice code setting up a
separate constant message, like:

  static const char message_advice_pull_non_ff[] =
          N_("...");

>  int cmd_pull(int argc, const char **argv, const char *prefix)
>  {
>  	const char *repo, **refspecs;
> @@ -1028,19 +1044,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 (rebase_unspecified && 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 (rebase_unspecified && !opt_ff) {
> +		if (opt_verbosity >= 0)
> +			show_advice_pull_non_ff();

Then, we could just do:

  if (opt_verbosity >= 0)
          advise(_(message_advice_pull_non_ff)).

Or even better:

  if (opt_verbosity >= 0)
          advise_if_enabled(ADVICE_PULL_NON_FF, _(message_advice_pull_non_ff));

I'm not familiar with the advise code, I don't know if there's any good
reason to setup these separate messages, so feel free to disregard this
suggestion.

Cheers.

-- 
Felipe Contreras

RE: [PATCH v7 5/5] pull: display default warning only when non-ff

[Felipe Contreras]

Junio C Hamano wrote:
> @@ -1044,7 +1045,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 (rebase_unspecified && !opt_ff) {
> +	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
> +
> +	if (rebase_unspecified && !opt_ff && !can_ff) {
>  		if (opt_verbosity >= 0)
>  			show_advice_pull_non_ff();
>  	}

I strongly predict the conditionals will end up looking similar to:

  if (!can_ff) {
          if (rebase_unspecified && !opt_ff && opt_verbosity >= 0)
                  show_advice_pull_non_ff();
  }

But OK.

-- 
Felipe Contreras

Re: [PATCH v7 3/5] pull: get rid of unnecessary global variable

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Junio C Hamano wrote:
>> It is easy enough to do,
>
> Yes.
>
>> gives a more descriptive name to the variable,
>
> I disagree.
>
>> and there is no reason to make the code deliberately worse by ignoring
>> improvement offered on the list.
>
> I doubt any person contributing to the mailing is making the code
> deliberately worse.

Oh, I doubt that you do so with what you send out.  I am saying that
you do so by not taking improvements.  It wastes reviewers' time,
raises noise ratio in the list traffic, and demotivates readers.

Re: [PATCH v7 4/5] pull: correct condition to trigger non-ff advice

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Then, we could just do:
>
>   if (opt_verbosity >= 0)
>           advise(_(message_advice_pull_non_ff)).
>
> Or even better:
>
>   if (opt_verbosity >= 0)
>           advise_if_enabled(ADVICE_PULL_NON_FF, _(message_advice_pull_non_ff));

I do not think we've decided what's the right way to squelch this
advice, so it is a bit premature to favor the latter over the
former.  Between a fixed message[] variable and a single-purpose
helper function I have no real preference.  With either, we can
reword the message easily without disrupting any improvements to the
codeflow.  At least the first sentence in the existing message needs
to be separated out of the advice and turned into a separate error
or a warning.

Re: [PATCH v7 5/5] pull: display default warning only when non-ff

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Junio C Hamano wrote:
>> @@ -1044,7 +1045,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 (rebase_unspecified && !opt_ff) {
>> +	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
>> +
>> +	if (rebase_unspecified && !opt_ff && !can_ff) {
>>  		if (opt_verbosity >= 0)
>>  			show_advice_pull_non_ff();
>>  	}
>
> I strongly predict the conditionals will end up looking similar to:
>
>   if (!can_ff) {
>           if (rebase_unspecified && !opt_ff && opt_verbosity >= 0)
>                   show_advice_pull_non_ff();
>   }
>
> But OK.

I may have failed to mention this in the cover letter, but I think
the placement of opt_ff in this part of the logic needs to be
rethought, so I strongly predict that this part will have to further
change ;-)

Re: [PATCH v7 3/5] pull: get rid of unnecessary global variable

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> > Junio C Hamano wrote:
> >> It is easy enough to do,
> >
> > Yes.
> >
> >> gives a more descriptive name to the variable,
> >
> > I disagree.
> >
> >> and there is no reason to make the code deliberately worse by ignoring
> >> improvement offered on the list.
> >
> > I doubt any person contributing to the mailing is making the code
> > deliberately worse.
> 
> Oh, I doubt that you do so with what you send out.  I am saying that
> you do so by not taking improvements.

I do take improvements, when I agree they are improvements. In fact I
did take virtually all of Elijah Newren's improvements.

> It wastes reviewers' time, raises noise ratio in the list traffic, and
> demotivates readers.

Are you saying I must always agree with you, or I waste your time?

In my view no one is infallible, just because X person says Y is an
improvement that doesn't necessarily mean it actually is.

I thought this was a collaborative process where you are supposed to
listen to my feedback to your suggestions too.

But I guess I shall take your "suggestions" as *orders*.

-- 
Felipe Contreras

Re: [PATCH v7 5/5] pull: display default warning only when non-ff

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> > Junio C Hamano wrote:

> > I strongly predict the conditionals will end up looking similar to:
> >
> >   if (!can_ff) {
> >           if (rebase_unspecified && !opt_ff && opt_verbosity >= 0)
> >                   show_advice_pull_non_ff();
> >   }
> >
> > But OK.
> 
> I may have failed to mention this in the cover letter, but I think
> the placement of opt_ff in this part of the logic needs to be
> rethought, so I strongly predict that this part will have to further
> change ;-)

And precisely because I predicted the same is that I deliberately chose
not to preemptively organize the conditionals in any particular way.

Cheers.

-- 
Felipe Contreras

RE: [PATCH v7 0/5] making pull advice not to trigger when unneeded

[Felipe Contreras]

Junio C Hamano wrote:
> Just to show what was pushed out on 'seen' while I was cutting the
> preview release...
> 
> This is based on Felipe's v6 (which was mislabled as v5 but sent on
> a different day from the true v5), with two clean-up commits
> inserted between Felipe's second step (now 2/5) and the third step
> (now 5/5, with necessary adjustments).
> 
> Even with the "correct condition" clean-up, I am not quite happy
> with the clarity of the logic around there.
> 
> I think !opt_ff does not exactly belong to the condition, if we
> consider that the eventual endgame should be to stop non-ff
> operation without merge or rebase by default with an error, and that
> should happen in this block.  The error message there should say
> "unable to fast-forward; must merge or rebase", which should equally
> apply to those who gave "--ff-only" explicitly, even though they may
> not need the advice message.

Agreed. Additionally it doesn't make sense that --ff skips the warning
entirely, even though the user has not specified a merge or a rebase (as
you yourself noted in a review of the test cases).

> To avoid the ugly-looking "strcmp()" in the above, we may need to
> adjust "--ff" (fast-forward without creating an extra merge commit)
> and "--no-ff" (create an extra merge commit when the history could
> be fast-forwarded) to imply "merge", though.  It would automatically
> make rebase_unspecified would become false.  With such a tweak, we
> can then simplify it further to
> 
> -	if (rebase_unspecified && !can_ff &&
> -	    (!opt_ff || !strcmp("--ff-only", opt_ff))) {
> +	if (rebase_unspecified && !can_ff) {

Currently this is a rebase:

  git pull --rebase --ff

With your proposed change it would be an implied merge. I think it makes
sense, but it's still a change in behavior.

This patch should do the trick:

--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -973,6 +973,11 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
        if (opt_rebase < 0)
                opt_rebase = config_get_rebase(&rebase_unspecified);
 
+       if (opt_ff && (!strcmp(opt_ff, "--ff") || !strcmp(opt_ff, "--no-ff"))) {
+               opt_rebase = 0;
+               rebase_unspecified = 0;
+       }
+
        if (read_cache_unmerged())
                die_resolve_conflict("pull");
 
Or do you mean --ff doesn't override --rebase? Therefore it's more of an
internal conceptual change.

-- 
Felipe Contreras

Re: [PATCH v7 4/5] pull: correct condition to trigger non-ff advice

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > Then, we could just do:
> >
> >   if (opt_verbosity >= 0)
> >           advise(_(message_advice_pull_non_ff)).
> >
> > Or even better:
> >
> >   if (opt_verbosity >= 0)
> >           advise_if_enabled(ADVICE_PULL_NON_FF, _(message_advice_pull_non_ff));
> 
> I do not think we've decided what's the right way to squelch this
> advice, so it is a bit premature to favor the latter over the
> former.

I agree. Just thinking about the future.

> Between a fixed message[] variable and a single-purpose
> helper function I have no real preference.

OK. I just mentioned for consistencty with other parts of git.

-- 
Felipe Contreras

Re: [PATCH v7 0/5] making pull advice not to trigger when unneeded

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

>> To avoid the ugly-looking "strcmp()" in the above, we may need to
>> adjust "--ff" (fast-forward without creating an extra merge commit)
>> and "--no-ff" (create an extra merge commit when the history could
>> be fast-forwarded) to imply "merge", though.
>> ...
>
> Or do you mean --ff doesn't override --rebase? Therefore it's more of an
> internal conceptual change.

I meant "--ff/--no-ff" without saying anything between merge and
rebase would make merge implied.  It was merely for the purpose of
getting rid of !opt_ff in the condition there and such an implied
merge would be one way to ensure that rebase_unspecified becomes
false.

"--no-ff --rebase" (in any order) would be a nonsense combination,
as it asks "please create an extra merge commit even when the
history fast-forwards, but by the way I do not want merge I want
rebase" [*1*].  It should error out when the history fast-forwards,
I think, and it probably should also error out when the history does
not fast-forward, instead of rebasing.

"--ff --rebase" (in any order), on the other hand, would be "if the
history fast-forwards, just do so without creating an extra merge
commit, by the way, I prefer rebase and not merge".  If the history
fast-forwards, it is OK to just fast-forward (using the merge
backend if needed), as rebasing against the history that is ahead of
us would mean all our unique development since we diverged from them,
which is nothing, will be replayed on top of their history.  If the
history does not fast-forward, then the precondition of "--ff" part
does not hold, so just rebasing as if "--ff" does not exist would
probably be OK, too.

In any case, I haven't thought the opt_ff part of the expression
through.  During the review of v5 3/3 (and doing v7 5/5), what I was
more interested in was to ensure the verbosity option, which would
control how verbose a message we should be giving, can stay
independent of other conditionals, which would control if we even
need to error out and/or give hints.

Thanks.


[Footnote]

*1* Even though the current code may simply ignore --no-ff after the
control is given to the rebase codepath,

Re: [PATCH v7 0/5] making pull advice not to trigger when unneeded

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> >> To avoid the ugly-looking "strcmp()" in the above, we may need to
> >> adjust "--ff" (fast-forward without creating an extra merge commit)
> >> and "--no-ff" (create an extra merge commit when the history could
> >> be fast-forwarded) to imply "merge", though.
> >> ...
> >
> > Or do you mean --ff doesn't override --rebase? Therefore it's more of an
> > internal conceptual change.
> 
> I meant "--ff/--no-ff" without saying anything between merge and
> rebase would make merge implied.  It was merely for the purpose of
> getting rid of !opt_ff in the condition there and such an implied
> merge would be one way to ensure that rebase_unspecified becomes
> false.

I see.

> "--no-ff --rebase" (in any order) would be a nonsense combination,
> as it asks "please create an extra merge commit even when the
> history fast-forwards, but by the way I do not want merge I want
> rebase" [*1*].  It should error out when the history fast-forwards,
> I think, and it probably should also error out when the history does
> not fast-forward, instead of rebasing.

But we shoud not imply what the user didn't say.

Yes, "--no-ff --rebase" is obviously nonsense, but that's a
simplification of setups the user may have, for example:

  git config pull.ff false
  git pull --rebase

Here, I think the user is saying: "please do a rebase, and ignore the
pull.ff configuration".

But the other way would be:

  git config pull.rebase true
  git pull --no-ff

Following the same logic, the user is saying: "please do a
non-fast-forward [merge], and ignore the pull.rebase configuration".

Either we imply the merge, or we don't.

I don't think it makes sense for the code to imply the user did say
--merge, and therefore don't show the advice (or in the future error
out), but then continue as if the user did say --rebase.

> In any case, I haven't thought the opt_ff part of the expression
> through.

It is tricky.

The reason I think I see it more clearly is that I have explored many
options, and at this point I have 27 branches with different approaches
of this topic. I see the dead-ends because I literally have a branch
with the test-case failure on it.

But as I said in the other thread [1], it all boils down to this:

 1. git -c pull.ff=only pull
 2. git -c pull.ff=only pull --merge

Even if you remove the opt_ff part of the expression, and the logic of
the advice/error is flawless, opt_ff is still going to come bite us in
the end, because it's meant to be passed to cmd_merge(), and it's still
going to fail, just at a different point, with a different error message.

The best way to get rid of opt_ff from the expression, is to actually
completely ignore it, and leave the current behavior as it is.

By adding a different configuration, the logic becomes really simple:

  if (!can_ff) {
	  if (!mode && opt_verbosity >= 0)
		  show_advice_pull_non_ff();
  }

And then opt_ff doesn't interact with --rebase at all, just like it is
the case right now.

Simple.

Cheers.

[1] https://lore.kernel.org/git/5fd8213598c33_d7c4820837@natae.notmuch

-- 
Felipe Contreras

Re: [PATCH v7 0/5] making pull advice not to trigger when unneeded

[Felipe Contreras]

Felipe Contreras wrote:
> Junio C Hamano wrote:

> > "--no-ff --rebase" (in any order) would be a nonsense combination,
> > as it asks "please create an extra merge commit even when the
> > history fast-forwards, but by the way I do not want merge I want
> > rebase" [*1*].  It should error out when the history fast-forwards,
> > I think, and it probably should also error out when the history does
> > not fast-forward, instead of rebasing.
> 
> But we shoud not imply what the user didn't say.
> 
> Yes, "--no-ff --rebase" is obviously nonsense, but that's a
> simplification of setups the user may have, for example:
> 
>   git config pull.ff false
>   git pull --rebase
> 
> Here, I think the user is saying: "please do a rebase, and ignore the
> pull.ff configuration".
> 
> But the other way would be:
> 
>   git config pull.rebase true
>   git pull --no-ff
> 
> Following the same logic, the user is saying: "please do a
> non-fast-forward [merge], and ignore the pull.rebase configuration".
> 
> Either we imply the merge, or we don't.
> 
> I don't think it makes sense for the code to imply the user did say
> --merge, and therefore don't show the advice (or in the future error
> out), but then continue as if the user did say --rebase.

I didn't see a response to this, but after thinking more about, I think
I have a clean solution: just remove all the opt_ff logic altogether.

It's clear --ff doesn't imply a merge, so we shouldn't act as if it was.

The warning should still be displayed.

-- 
Felipe Contreras

Re: [PATCH v7 0/5] making pull advice not to trigger when unneeded

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> It's clear --ff doesn't imply a merge, so we shouldn't act as if it was.

Do you specifically mean --ff, or do you talk collectively about
anything that goes in opt_ff in the C code?

The "--ff" option means "we are allowing fast-forward, so please do
not make new commit object unnecessarily, but it is just we are
allowing---we are not limiting ourselves to fast-forard; feel free
to create a merge commit if necessary".  So it does imply that the
user prefers to merge and does not want to rebase.

If you meant what opt_ff can relay, then there are "--no-ff" and
"--ff-only" to consider:

 - "--no-ff" says "we do not allow fast-forward; when the other side
   is pure descendant of ours, create a merge commit to make them
   the second parent, so that our side of the history stays to be
   the first-parent chain that merged them as a side topic."  It may
   not say what should happen when the history does not
   fast-forward, and it _is_ possible to argue, for the sake of
   argument, that it asks to rebase if not fast-forward (so that
   their history becomes the primary and we build on top of them)
   while asking to merge if fast-forward (so that our history stays
   the primary and we absorb their work as a side branch), but that
   is a behavior that does not make much sense.  It is much easier
   to reason about if we accept that the user who says "--no-ff"
   expects a merge to happen, not a rebase.

 - "--ff-only" says "when their history is pure descendant of ours,
   just fast-forward our branch to match their history, and
   otherwise fail."  This one does not have to imply either merge or
   rebase, as both would give us identical result (i.e. merge would
   fast-forward and rebase would replay *no* work of our own on top
   of theirs.  Either case, the result is that our branch tip now
   points at the tip of their history).

   The topic under discussion is based on the "we do not have to
   give advice between merge and rebase if the history
   fast-forwards", and anybody in support of the topic would be in
   agreement with this case.

In any case, I think what we have in 'seen' already is a good
stopping point for this cycle.  We are not erroring out any new case
and simply not showing an advice in a situation that it would not
apply---the question "does --ff imply merge?" does not have to be
answered in order to evaluate the 5-patch series we have.

Re: [PATCH v7 0/5] making pull advice not to trigger when unneeded

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > It's clear --ff doesn't imply a merge, so we shouldn't act as if it was.
> 
> Do you specifically mean --ff, or do you talk collectively about
> anything that goes in opt_ff in the C code?

I meant --ff, but the rationale can be extended to all of opt_ff.

> The "--ff" option means "we are allowing fast-forward, so please do
> not make new commit object unnecessarily, but it is just we are
> allowing---we are not limiting ourselves to fast-forard; feel free
> to create a merge commit if necessary".

Yes. *If* a rebase is not specified.

> So it does imply that the user prefers to merge and does not want to
> rebase.

We could imply that, but currently it doesn't.

Currently this does not do a merge:

  git config pull.rebase true
  git pull --ff

> If you meant what opt_ff can relay, then there are "--no-ff" and
> "--ff-only" to consider:
> 
>  - "--no-ff" says "we do not allow fast-forward; when the other side
>    is pure descendant of ours, create a merge commit to make them
>    the second parent, so that our side of the history stays to be
>    the first-parent chain that merged them as a side topic."  It may
>    not say what should happen when the history does not
>    fast-forward, and it _is_ possible to argue, for the sake of
>    argument, that it asks to rebase if not fast-forward (so that
>    their history becomes the primary and we build on top of them)
>    while asking to merge if fast-forward (so that our history stays
>    the primary and we absorb their work as a side branch), but that
>    is a behavior that does not make much sense.

I agree it doesn't make much sense; if the user wants a rebase in case
of non-fast-forward, --no-ff is the only way.

>    It is much easier to reason about if we accept that the user who
>    says "--no-ff" expects a merge to happen, not a rebase.

Yes, but currently that's not the case.

Currently this doesn't do a merge:

  git config pull.rebase true
  git pull --no-ff

We would need to change the semantics.

>  - "--ff-only" says "when their history is pure descendant of ours,
>    just fast-forward our branch to match their history, and
>    otherwise fail."  This one does not have to imply either merge or
>    rebase, as both would give us identical result (i.e. merge would
>    fast-forward and rebase would replay *no* work of our own on top
>    of theirs.  Either case, the result is that our branch tip now
>    points at the tip of their history).
> 
>    The topic under discussion is based on the "we do not have to
>    give advice between merge and rebase if the history
>    fast-forwards", and anybody in support of the topic would be in
>    agreement with this case.

Yes.

> In any case, I think what we have in 'seen' already is a good
> stopping point for this cycle.

It's not a bad stopping point.

But the next patches are needed too. Up to the first 6 patches should be
uncontroversial.

> We are not erroring out any new case and simply not showing an advice
> in a situation that it would not apply---the question "does --ff imply
> merge?" does not have to be answered in order to evaluate the 5-patch
> series we have.

Not my patches.

The patch you introduced regarding rebase_unspecified does depend on
what happens next. If we decide to change the semantics of --ff* and
imply a merge, then my patch to add REBASE_DEFAULT is needed, and as you
can see in another patch series [1], that basically has to revert your patch.

Cheers.

[1] https://lore.kernel.org/git/20201218211026.1937168-8-felipe.contreras@gmail.com/

-- 
Felipe Contreras