v5, 3 partspull: stop warning on every pull

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.

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(-)

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

Makes sense.

I agree with this point.  I've always found it suboptimal.

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.

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.

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.

On Fri, Dec 11, 2020 at 12:54 AM Junio C Hamano <gitster@pobox.com> wrote:

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).

...

Yeah, there's only 38 global variables. We wouldn't want another one which makes the code pretty straightforward.

Which is right before the fork between rebase and merge.

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.

Should actually be something like:

        if (rebase_unspecified && !can_ff)
                die("Not a fast-forward; must either merge or rebase");

But yeah.

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

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.

On Fri, Dec 11, 2020 at 6:00 PM Junio C Hamano <gitster@pobox.com> wrote:

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 <felipe.contreras@gmail.com> writes:

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.

Junio C Hamano wrote:

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.

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.

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 wrote:

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).

It's much cleaner this way.

Also, we would like to be able to make this check before the decision to rebase is made.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/pull.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

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

It is obvious that a focused single purpose helper with less indentation is easier to follow both at the calling site and the implementation itself.

"It's much cleaner" is not something you need to say.

If you were making it uglier, but other benefits outweigh the ugliness, that may deserve to be said, but that is not the case here.

... in a future step.

That is something we want to say upfront, not "Also".

The patch itself looks quite straightforward.

Thanks.

Hello,

Just to state that I'm not ignoring this feedback.

Junio C Hamano wrote:

Yes, but the reader hasn't reached that point of the story yet.

From the top of my head I can think of a few other reasons to refactor code: a) it's more logical, b) it's less code, c) it's helps further changes.

It's not necessary to say, but that's what I want to say; this reason, and no other reason, is the main reason for this patch's existence.

Right.

It's not necessarily the case (could be an indeterminate future), but it is the case in this patch series.

Not from my point of view. Even if it didn't help in future steps, cleaning up code is generally desirable.

Cheers.

There's no need to display the annoying warning on every pull... only the ones that are not fast-forward.

This requires the tests to pick another base, so the merge is not fast-forward. And in the cases where --ff-only is specified add test_must_fail (since now they are non-fast-forward).

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/pull.c               |  7 +++++--
 t/t7601-merge-pull-config.sh | 28 +++++++++++++++++-----------
 2 files changed, 22 insertions(+), 13 deletions(-)

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

Yes! And thanks to the previous two steps, the change to the code is quite obvious. I don't have to give any further comment on the part that changes code---it is well done, period.

I am not sure what this means.  

Existing tests pull histories that may or may not be descendants of the HEAD. If we do not change the pair of commits involved in the tests, i.e. if we do not apply many s/c0/c2/ replacement I see in the patch, some of these tests would change their behaviour with respect to their advice output (but not the outcome). The ones that pulled fast-forward would stop showing the warning, and that is a good effect of this change. We want to see that in the update to the tests, no?

The ones that pulled non-fast-forward history would still show the warning as they used to, and that is also what we want to see after this change. Which means we want the tests to keep doing what they do, while adjusting what we expect out of the tested commands to the world that is made better with the code change in this patch. I do not think we need to add new tests to demonstrate the new behaviour, as they (i.e. when the test pulls fast-forwardable history, with various combinations of options and configuration) seem to be pretty well covered already.

In other words, the changes the paragraph says that the commit made to the tests sound quite backwards.

The actual changes to some of the tests do look sensible, testing both sides of the coin. I've looked at the changes to the tests, but cannot convince me that we are not making irrelevant changes to the tests, or losing coverage needlessly because of s/c0/c2/ (i.e. turning tests that used to check fast-forward situations into tests that check non-ff situations).

This is not "keeping what the original test does and adjusting the expectation, to demonstrate how behaviour changed"; instead, we make sure that the message we originally gave and we intend to keep giving is shown in non-ff situation by choosing the current commit that won't allow a ff merge. This is OK if we did not lose test coverage---as long as we test that we no longer give the message in ff situation somewhere else, And that happens later, I think.

This is the new test to check the other side of the coin. It sees how the original test to merge c1 into c0 would behave with the new code. We make sure we do not give the advice because it is irrelevant in this situation.

So the above two are good, even though the way this patch updates tests is probably a bit more error prone than necessary.

Since we have checked how the new code behave for fast-forward with this new test, the remainder of the entire test script can be modified to test only non-ff situation without losing test coverage?

I am not sure if that is the case.

We are merely allowing fast-forward merges without unnecessary merge commits, but we are faced to merge c1 into c2, which is not ff. The command goes ahead and merges anyway, but why shouldn't we be seeing the message? I am puzzled.

This used to test that fast-forwarding the HEAD from c0 to c2 would be done successfully without issuing the message. Shouldn't that still be true in the improved "do not complain on fast-forward" code?

We seem to be losing test coverage by checking how pull.ff=only prevents the command from working in a non-ff merge.

I'd stop my review on the tests here, but I generally think s/c0/c2/ done in this patch is a wrong thing to do. We are changing the condition under which the messages is given (we are narrowing it to avoid giving it when it is irrelevant), without changing the final outcome (even though we changed the condition to give the message, we didn't change what the final outcome of the pull command would be), so I'd strongly prefer testing the same set of scenarios and update the expectation to an improved reality.

On Fri, Dec 11, 2020 at 3:22 AM Junio C Hamano <gitster@pobox.com> wrote:

It means in order to test the previous warning--which appeared in fast-forward and non-fast-forward merges, and now appears only on non-fast-forward merges--we need non-fast-forward merge situations.

All the tests, actually.

Yes.

Currently no test is doing that; all are in a fast-forward situation.

I'm not sure how it is backwards. All the tests are fast-forward, we need them not fast-forward, so we pick another base... so the merges are not fast-forward.

No test is checking fast-forward situations, because the warning doesn't check fast-forward situations.

Any suggestions to make it less error prone are welcome.

It is the case. That's what the patch does.

Because that's what the current code does; it just checks that any opt_ff is set, it doesn't check for any specific values.

I tried to fix that since v1, v2, v3, and v4 of the series [1], which only received comments from Elijah Newren, but that's a separate patch, and as I mentioned in the cover letter of v5; I've dropped all those other patches.

No. It doesn't care if it's fast-forward or not; it's just checking that "pull.ff=only" is set.

Before the code just checks "!opt_ff", now it checks "!opt_ff && !can_ff".

So, in order to do the "pull.ff=only" check, we need a non-fast-forward merge.

No we don't. Remove the "test_config pull.ff only" and the test fails, as expected.

We are testing *exactly* the same test scenarios, we are just forcing can_ff to be false.

If I remove the precise thing each test-case is supposed to test:

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

Assuming only one kind of breakage and try to break exactly that thing does not prove much.

I'll keep this short as I am supposed to be off officially.

With pulling without choosing between rebase/merge, the old code had one behaviour wrt the message---it always advised, whether the pull was ff or not.

The new code has two behaviour wrt this aspect. It behaves differently when the pull is ff or non-ff. That would double the possibility that needs to be tested if we wanted to keep covering the original set of conditions *and* cover all new possibilities.

I am saying that you should keep the original ones, and add new ones to cover the new cases if that matters. Otherwise the conditions under which the original tests were checking would no longer be tested.

This used to make sure an attempt to rebase c1 onto c0, which can be fast-forwarded, would work fine, even though it used to give warning. We should keep testing the same condition. The expectation of seeing the warning is what must be changed, not the test condition (i.e. rebasing c1 onto c2 instead of c0)---you are no longer making sure that c1 can be rebased onto c0 cleanly.

On Fri, Dec 11, 2020 at 5:56 PM Junio C Hamano <gitster@pobox.com> wrote:

It proves we are testing exactly the thing the test is meant to test.

No test is ever perfect.

I appreciate that.

I disagree. The old condition is perfectly well tested with this test:

  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
  '

In other words; we have tests going horizontally, and tests going vertically. That should cover all the bases.

But fine, if you want a matrix of tests I can do that.

No it didn't. It may very well have done a merge, or done nothing at all.

The tests that actually check that "git pull --rebase" works both in the fast-forward and non-fast-forward are in t/t5520-pull.sh.

The goal of this particular test is to check that the warning is not there.

We are testing what was originally tested: that the warning is not presented.

We disagree on what the "test condition" is supposed to be on this test.

But fine, I'll create the matrix.

Cheers.

Junio C Hamano <gitster@pobox.com> writes:

Let's try to explain it in a different way.

The original author of this test cared that pulling c1 with --rebase into c0 succeeds, and that it does not give the error message. We have no right [*1*] to say that scenario (i.e. "pull --rebase" c1 into c0) no longer matters without a good justification. And it is not a good justification to say that the current code happens to behave identically whether running "pull --rebase" of c1 into c0 or c2 so it is sufficient to test the operation into c2. The test is *not* about how the current code happens to work. It is to make sure the scenarios test authors care about will keep behaving the same way.

Some tests may be expecting that pulling c1 into c0 would issue the message, and that the command succeeds, and with the patch 3/3 the outcome may become different, i.e. the command succeeds without annoying message. That would break the expectation of the original test authors, and it is a good thing. By recording a change to the expectation, we can document how the new behaviour works better under the same scenario.

[Footnote]

*1* That does not mean we must not care about other scenarios that are different from what have been tested with existing tests. If there is new behaviour introduced by patch 3/3, it is prudent to protect it from future breakage by adding a test that pulls c1 into c2, if that case is not already tested with existing tests.

I suspect we already make sure a non-ff merge gives the annoying message while going ahead, so there may be no new additional test required, though.

Junio C Hamano wrote:

I prefer not to attempt to read minds (plus, the author might have not cared that the pulling succeeds), and anyway; that's not what matters.

What matters is the current situation, and the desired situation.

But nobody is saying that. What I said is that in *this* particular test-case that's not the objective of the test. And if you consider hypothetical secondary objectives of the test, those are being tested elsewhere.

Again, I don't particularly care to mindread what the test authors might have cared about.

It's clear from the tests themselves what they are trying to do: check if the warning exists, or doesn't.

No. All the tests (sans 1) check that the warning is *not* present.

If you do a fast-forward pull, the warning will not be present (regardless of options), and the test would pass, but for the wrong reasons.

No, the expectation has not changed one iota; it's exactly same.

It's the reason for the same output that changed.

If I take the current tests, and I remove the thing that makes them special (arguments and/or configuration), essentially making them all "git pull":

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

You do not have to read mind. What is written in the test is clear enough: run "git pull --rebase . c1" into c0 and see what happens.

Junio C Hamano wrote:

That is precisely my point: it is written in the test, no mindreading neccessary.

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

The above perception of yours is mostly due to misunderstanding, I would have to say. We are in agreement on what we should be warning about at least, assuming that you are expressing what you want clearly in the latest round of responses and I understood them correctly [*1*].

I do not know if others on the list agree, though.

I do agree that there is no agreement on the behaviour in the endgame. In principle, I am in favor of disabling the more dangerous half of the "git pull" command for those who haven't configured anything. But I can understand those who do not want that behaviour, as the fallout would be quite big.

It may be obvious to you, but may not be to others on the list who spoke in the thread and who didn't speak but read the discussion.

I did see potential goodness in the documentation update and that was why I offered polishment on top of your patches in a v3 round, but seeing the suggestions dismissed without convincing arguments before v4 was sent out would have discouraged even the most patient reviewers among us. If you meant by "lost traction" the lack of comments on v4, that was my reason for not commenting.

In any case, these three patches in this round looked quite sensible to me, except for the tests in 3/3, and minor details of 2/3, both of which I gave a more detailed review and suggestion.

Thanks.

[Footnote]

*1* The only difference between us is whether it is sensible to allow explicitly ask to see the same behaviour as an unconfigured user except for the help text---I do not think it is, and I do want to avoid introducing pull.mode, but I've shown a way or two to get the behaviour without adding pull.mode in the mix.

On Fri, Dec 11, 2020 at 5:22 AM Junio C Hamano <gitster@pobox.com> wrote:

I'm not trying to be difficult here, but at every round where you have stated what it is that I want, it's not actually what I want, and the last round is no exception, in my option.

Let's assume that I'm not explaining clearly what I want.

In the last round you said you wanted an error, not a warning. That's not what I want; I'm proposing a warning.

But that's not what I was referring to here.

This is what I was referring to. Initially there seemed to be some interest, and suddenly that interest disappeared.

See? I disagree.

I think the endgame is clear. How we get there is where there's no agreement.

And who is that? Did anyone in the list express that they did not want that behavior?

I did not dismiss your suggestions, I replied to your suggestions [1]. You did not reply back.

Moreover, in patch 2 I saw you had some confusion [2], in which you said you didn't see any value in updating the message without changing the condition that triggers, to which I replied [3]: "Maybe it will be clearer when I send all the patches."

That's why I sent v4; not because I thought the review of v3 was done, but because we were stuck not seeing the evolution of the warning.

In v4 I went through every step of the evolution [4], and I went back to what I said in v3:

  At this point we can update the warning to mention that we are inside
  a non-fast-forward case. But it's not necessary.

So I did not dismiss the suggestion, I replied to it, and put a pin on it.

You can certainly bring the same suggestion in v4, but I seem to have convinced Elijah Newren that "fast-forward" can be used as an adverb perfectly well, and it in fact is, in many places in the documentation both internal, and external.

Great.

That should improve the situation of most users. And also has the added benefit that it's 3 less patches I have to carry around on every round.

Cheers.

[1] https://lore.kernel.org/git/CAMP44s1ZDXzGfEqpTeiG=aGAYK40ebnBLQKAbA7KGtcePGARfw@mail.gmail.com/ [2] https://lore.kernel.org/git/xmqq4kkx9vzx.fsf@gitster.c.googlers.com/ [3] https://lore.kernel.org/git/CAMP44s1aYqzCVvELH8zULaTkOdgLSSAQ0LE8WfgQKLPfU2MHfg@mail.gmail.com/ [4] https://lore.kernel.org/git/CAMP44s2hUCd9qc83LReGyjy8N+u++eK6VjwGhDhrX0f0SbKmig@mail.gmail.com

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

Perhaps most of them are happy enough with the current behaviour.

Perhaps nobody cares strongly enough to say what they want to see, as they fear that by speaking up they would be drawn into a discussion that is needlessly hot and unpleasant.

What you want as the endgame may be clear to you.

But I do not think there is clear concensus among people on the list.

I thought that you at least saw Dscho's reaction to the breakage caused by "future" patch in response to one of the recent What's cooking reports. Doesn't that count "anyone on the list express"?

I am starting to feel myself that "don't do anything if this is not a fast-forward" may be something that would have been great if we had it from day one but is no longer a feasible default with existing users, to be quite honest, so you can count my 20% as another example.

Junio C Hamano wrote:

Or perhaps they are happhy enough the current behavior *for them*, but not the other users.

Or perhaps they got busy.

Or perhaps they got sick.

I don't see how listing hypotheticals is fruitful.

I do. It has been clear for many years.

Yes, I saw it, but it was a complaint about the breakage of test due to the merge of a patch that wasn't meant to be merged today.

No. These are two different statements.

 1. This will break things for *me*
 2. This is a bad thing for *everyone*

These two statements have a different object. Also, the first doesn't necessarily require rationale, we can take his word for it. But the second one does.

It's not enough to say "I think this patch is bad for everyone"; you have to explain *why*. I'd be happy to discuss with him his rationale, which he hasn't given.

Moreover, isn't Schindelin advocating for a change that will cause breakage (renaming the master branch).

As opposed to virtually everyone in this mailing list that has given an opinion about the topic in the last 10 years (including you I count 3 out of literally dozens).

I will do some mail archeology and present the results.

Anyway, this is not relevant today, because you were the one that proposed to go straight to an error.

What I propose for today is to introduce the option "pull.mode=fast-forward", and improve the warning. Not an error.

That doesn't break the behavior for anyone, including Schindelin.

Cheers.

On Sat, Dec 12, 2020 at 10:36 AM Felipe Contreras <felipe.contreras@gmail.com> wrote:

A quick update after reading a great deal of mails.

The only person I counted in 2013 that was against changing the default is Matthieu Moy, and reading back what he said, even him was in favor of adding a configuration for the new mode [1]. What he was against was making it the default, and adding a warning stating it would be the default.

Fortunately my patches introduce the mode [2] without making it necessarily the default (we don't even have to change the warning and say it will be changed in the future [3]).

So *nobody* was against the introduction of such mode.

Also, after reading the input from GitHub trainers, which Jeff provided [4], perhaps leaving the default as "merge" makes sense. Our curse of knowledge may bias us into thinking the user knows what a rebase is. Maybe a permanent advice warning is the way to go, I'm still mulling it over.

This is still true. We don't lose anything by introducing the mode.

We can make the decision to flip the default later (or not).

Cheers.

[1] https://lore.kernel.org/git/vpqbo41lo2v.fsf@anie.imag.fr/ [2] https://lore.kernel.org/git/20201208002648.1370414-18-felipe.contreras@gmail.com/ [3] https://lore.kernel.org/git/20201208002648.1370414-19-felipe.contreras@gmail.com/ [4] https://lore.kernel.org/git/20130909201751.GA14437@sigill.intra.peff.net/