| Message ID | 20210705123209.1808663-2-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git update: fix broken git pull | expand |
On Mon, Jul 05 2021, Felipe Contreras wrote: > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > --- > builtin/merge.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/builtin/merge.c b/builtin/merge.c > index a8a843b1f5..05e631229d 100644 > --- a/builtin/merge.c > +++ b/builtin/merge.c > @@ -1620,7 +1620,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) > } > > if (fast_forward == FF_ONLY) > - die(_("Not possible to fast-forward, aborting.")); > + die(_("unable to fast-forward")); I read the existing message a bit more like "this makes no sense anymore" (correct) and the latter more like "we encountered an error". Perhaps something like: "Can't merge X with Y, in --ff-only mode, would need to create a merge commit", tip_name
Ævar Arnfjörð Bjarmason wrote: > > On Mon, Jul 05 2021, Felipe Contreras wrote: > > > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > > --- > > builtin/merge.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/builtin/merge.c b/builtin/merge.c > > index a8a843b1f5..05e631229d 100644 > > --- a/builtin/merge.c > > +++ b/builtin/merge.c > > @@ -1620,7 +1620,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) > > } > > > > if (fast_forward == FF_ONLY) > > - die(_("Not possible to fast-forward, aborting.")); > > + die(_("unable to fast-forward")); > > I read the existing message a bit more like "this makes no sense > anymore" (correct) and the latter more like "we encountered an > error". I mean, this is the documentation of --ff-only: With `--ff-only`, resolve the merge as a fast-forward when possible. When not possible, refuse to merge and exit with a non-zero status. So if you do `git merge --ff-only` you are telling git: "I want you to exit with an error when the fast-forward is not possible". If you do: % git merge --ff-only fatal: Not possible to fast-forward, aborting. That "aborting" part is redundant; we know `git merge` should abort if the fast-forward is not possible, we explicitely told git to do that. Moreover the "fatal: " prefix also indicates that git aborted. Then you have "Not possible to fast-forward", which if memory serves well should be in lowercase (altghough can't find that in the guidelines). "unable to fast-forward" is simply a more succinct way of saying "not possible to fast-forward". Sure, we are not explaining why, although I can't think of any other reason why we could not fast-forward. > Perhaps something like: > > "Can't merge X with Y, in --ff-only mode, would need to create a merge commit", tip_name I don't think die() messages should be that big, how about? branches diverged, can't fast-forward
On July 6, 2021 4:40 PM, Felipe Contreras wrote: >Subject: Re: [RFC PATCH 01/35] merge: improve fatal fast-forward message > >Ævar Arnfjörð Bjarmason wrote: >> >> On Mon, Jul 05 2021, Felipe Contreras wrote: >> >> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> >> > --- >> > builtin/merge.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/builtin/merge.c b/builtin/merge.c index >> > a8a843b1f5..05e631229d 100644 >> > --- a/builtin/merge.c >> > +++ b/builtin/merge.c >> > @@ -1620,7 +1620,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) >> > } >> > >> > if (fast_forward == FF_ONLY) >> > - die(_("Not possible to fast-forward, aborting.")); >> > + die(_("unable to fast-forward")); >> >> I read the existing message a bit more like "this makes no sense >> anymore" (correct) and the latter more like "we encountered an error". > >I mean, this is the documentation of --ff-only: > > With `--ff-only`, resolve the merge as a fast-forward when possible. > When not possible, refuse to merge and exit with a non-zero status. > >So if you do `git merge --ff-only` you are telling git: "I want you to exit with an error when the fast-forward is not possible". > >If you do: > > % git merge --ff-only > fatal: Not possible to fast-forward, aborting. > >That "aborting" part is redundant; we know `git merge` should abort if the fast-forward is not possible, we explicitely told git to do that. `git merge` is a special operation where errors (conflicts, for one) may leave the repository in a merge pending state where you subsequently may have to use `git merge --abort` to reset the situation or `git add` to continue. The `aborting` output makes it clear that you do not have to do the `--abort` and *cannot* do the `add` because there was an implicit `--abort` done resulting from the failure. This is important information for the user.
"Randall S. Becker" <rsbecker@nexbridge.com> writes: >>If you do: >> >> % git merge --ff-only >> fatal: Not possible to fast-forward, aborting. >> >>That "aborting" part is redundant; we know `git merge` should abort > if the fast-forward is not possible, we explicitely told git to do > that. > > `git merge` is a special operation where errors (conflicts, for one) > may leave the repository in a merge pending state where you > subsequently may have to use `git merge --abort` to reset the > situation or `git add` to continue. The `aborting` output makes it > clear that you do not have to do the `--abort` and *cannot* do the > `add` because there was an implicit `--abort` done resulting from the > failure. This is important information for the user. If so, adding ", aborting" to the end is misleading. In this particular failure mode, the command pretends that the merge did not even start.
Randall S. Becker wrote: > On July 6, 2021 4:40 PM, Felipe Contreras wrote: > >Subject: Re: [RFC PATCH 01/35] merge: improve fatal fast-forward message > > > >Ævar Arnfjörð Bjarmason wrote: > >> > >> On Mon, Jul 05 2021, Felipe Contreras wrote: > >> > >> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > >> > --- > >> > builtin/merge.c | 2 +- > >> > 1 file changed, 1 insertion(+), 1 deletion(-) > >> > > >> > diff --git a/builtin/merge.c b/builtin/merge.c index > >> > a8a843b1f5..05e631229d 100644 > >> > --- a/builtin/merge.c > >> > +++ b/builtin/merge.c > >> > @@ -1620,7 +1620,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) > >> > } > >> > > >> > if (fast_forward == FF_ONLY) > >> > - die(_("Not possible to fast-forward, aborting.")); > >> > + die(_("unable to fast-forward")); > >> > >> I read the existing message a bit more like "this makes no sense > >> anymore" (correct) and the latter more like "we encountered an error". > > > >I mean, this is the documentation of --ff-only: > > > > With `--ff-only`, resolve the merge as a fast-forward when possible. > > When not possible, refuse to merge and exit with a non-zero status. > > > >So if you do `git merge --ff-only` you are telling git: "I want you to exit with an error when the fast-forward is not possible". > > > >If you do: > > > > % git merge --ff-only > > fatal: Not possible to fast-forward, aborting. > > > >That "aborting" part is redundant; we know `git merge` should abort if the fast-forward is not possible, we explicitely told git to do that. > > `git merge` is a special operation where errors (conflicts, for one) > may leave the repository in a merge pending state where you > subsequently may have to use `git merge --abort` to reset the > situation or `git add` to continue. The `aborting` output makes it > clear that you do not have to do the `--abort` and *cannot* do the > `add` because there was an implicit `--abort` done resulting from the > failure. But this is not a `git merge`, this is a `git merge --ff-only`; they are different operations. There *never* is a need for `--abort` with `git merge --ff-only`. Anyway, the error message is meant for `git fast-forward` which definitely doesn't need any `--abort`. Initially I created a new variable to have a different error message for `git merge --ff-only` and `git fast-forward`, precisely to avoid changing the current error message of `git merge --ff-only` and thus avoid any inertial comments like this one. But then I thought there was no need to complicate the series when both can be improved at once. Apparently that's not the case. I guess I'll add it back. Cheers.
Junio C Hamano wrote: > "Randall S. Becker" <rsbecker@nexbridge.com> writes: > > >>If you do: > >> > >> % git merge --ff-only > >> fatal: Not possible to fast-forward, aborting. > >> > >>That "aborting" part is redundant; we know `git merge` should abort > > if the fast-forward is not possible, we explicitely told git to do > > that. > > > > `git merge` is a special operation where errors (conflicts, for one) > > may leave the repository in a merge pending state where you > > subsequently may have to use `git merge --abort` to reset the > > situation or `git add` to continue. The `aborting` output makes it > > clear that you do not have to do the `--abort` and *cannot* do the > > `add` because there was an implicit `--abort` done resulting from the > > failure. This is important information for the user. > > If so, adding ", aborting" to the end is misleading. In this > particular failure mode, the command pretends that the merge did not > even start. That's true. Whatever is the case for that "aborting" to be there I don't think it's adding any value.
On July 6, 2021 5:12 PM, Felipe Contreras wrote: >Randall S. Becker wrote: >> On July 6, 2021 4:40 PM, Felipe Contreras wrote: >> >Subject: Re: [RFC PATCH 01/35] merge: improve fatal fast-forward >> >message >> > >> >Ævar Arnfjörð Bjarmason wrote: >> >> >> >> On Mon, Jul 05 2021, Felipe Contreras wrote: >> >> >> >> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> >> >> > --- >> >> > builtin/merge.c | 2 +- >> >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> >> > >> >> > diff --git a/builtin/merge.c b/builtin/merge.c index >> >> > a8a843b1f5..05e631229d 100644 >> >> > --- a/builtin/merge.c >> >> > +++ b/builtin/merge.c >> >> > @@ -1620,7 +1620,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) >> >> > } >> >> > >> >> > if (fast_forward == FF_ONLY) >> >> > - die(_("Not possible to fast-forward, aborting.")); >> >> > + die(_("unable to fast-forward")); >> >> >> >> I read the existing message a bit more like "this makes no sense >> >> anymore" (correct) and the latter more like "we encountered an error". >> > >> >I mean, this is the documentation of --ff-only: >> > >> > With `--ff-only`, resolve the merge as a fast-forward when possible. >> > When not possible, refuse to merge and exit with a non-zero status. >> > >> >So if you do `git merge --ff-only` you are telling git: "I want you to exit with an error when the fast-forward is not possible". >> > >> >If you do: >> > >> > % git merge --ff-only >> > fatal: Not possible to fast-forward, aborting. >> > >> >That "aborting" part is redundant; we know `git merge` should abort if the fast-forward is not possible, we explicitely told git to do that. >> >> `git merge` is a special operation where errors (conflicts, for one) >> may leave the repository in a merge pending state where you >> subsequently may have to use `git merge --abort` to reset the >> situation or `git add` to continue. The `aborting` output makes it >> clear that you do not have to do the `--abort` and *cannot* do the >> `add` because there was an implicit `--abort` done resulting from the >> failure. > >But this is not a `git merge`, this is a `git merge --ff-only`; they are different operations. There *never* is a need for `--abort` with `git >merge --ff-only`. Well, you know that and I know that, but having to explain this to every new git user who will operationally use git merge --ff-only within hours or days of their first clone is a different matter. >Anyway, the error message is meant for `git fast-forward` which definitely doesn't need any `--abort`. > >Initially I created a new variable to have a different error message for `git merge --ff-only` and `git fast-forward`, precisely to avoid >changing the current error message of `git merge --ff-only` and thus avoid any inertial comments like this one. But then I thought there >was no need to complicate the series when both can be improved at once. >Apparently that's not the case. > >I guess I'll add it back. Thanks. I understand the purpose here. -Randall
On July 6, 2021 4:56 PM, Junio C Hamano wrote: >"Randall S. Becker" <rsbecker@nexbridge.com> writes: > >>>If you do: >>> >>> % git merge --ff-only >>> fatal: Not possible to fast-forward, aborting. >>> >>>That "aborting" part is redundant; we know `git merge` should abort >> if the fast-forward is not possible, we explicitely told git to do >> that. >> >> `git merge` is a special operation where errors (conflicts, for one) >> may leave the repository in a merge pending state where you >> subsequently may have to use `git merge --abort` to reset the >> situation or `git add` to continue. The `aborting` output makes it >> clear that you do not have to do the `--abort` and *cannot* do the >> `add` because there was an implicit `--abort` done resulting from the >> failure. This is important information for the user. > >If so, adding ", aborting" to the end is misleading. In this particular failure mode, the command pretends that the merge did not even >start. You know that, and I know that. I contend that git users do not generally want to care about failure modes. While a flow description might be instructive, I doubt many git users would look at it. We can only hope that front ends know how to make this clean for them. -Randall
"Randall S. Becker" <rsbecker@nexbridge.com> writes: > On July 6, 2021 4:56 PM, Junio C Hamano wrote: >>"Randall S. Becker" <rsbecker@nexbridge.com> writes: >> >>>>If you do: >>>> >>>> % git merge --ff-only >>>> fatal: Not possible to fast-forward, aborting. >>>> >>>>That "aborting" part is redundant; we know `git merge` should abort >>> if the fast-forward is not possible, we explicitely told git to do >>> that. >>> >>> `git merge` is a special operation where errors (conflicts, for one) >>> may leave the repository in a merge pending state where you >>> subsequently may have to use `git merge --abort` to reset the >>> situation or `git add` to continue. The `aborting` output makes it >>> clear that you do not have to do the `--abort` and *cannot* do the >>> `add` because there was an implicit `--abort` done resulting from the >>> failure. This is important information for the user. >> >>If so, adding ", aborting" to the end is misleading. In this particular failure mode, the command pretends that the merge did not even >>start. > > You know that, and I know that. I contend that git users do not > generally want to care about failure modes. While a flow > description might be instructive, I doubt many git users would > look at it. We can only hope that front ends know how to make this > clean for them. They may not care about "failure modes" but they would want to know that in this situation, unlike other times, they do not have to say "git merge --abort" (or "git reset --hard"). Saying ", aborting" does not help as much as saying say ", not starting a merge", for example.
Randall S. Becker wrote: > On July 6, 2021 5:12 PM, Felipe Contreras wrote: > >But this is not a `git merge`, this is a `git merge --ff-only`; they > >are different operations. There *never* is a need for `--abort` with > >`git merge --ff-only`. > > Well, you know that and I know that, but having to explain this to > every new git user who will operationally use git merge --ff-only > within hours or days of their first clone is a different matter. New users don't do `git merge --ff-only`, they very likely don't do `git merge` either, nor do they do `git pull --ff-only`. This is obvious from the countless threads where users do `git pull` without knowing that they will create a true merge by mistake. Even less likely is that they will know about `git merge --abort`, and even less that they will know both `git merge --ff-only` and `git merge --abort`. Even even less likely that they will mistakenly expect `git merge --ff-only` to be left inside a temporary stated to be resolved by `git merge --abort`. But as Junio said, *if* such a hypothetical user exists, we don't want to keep misleading them into thinking that a true merge was somehow aborted, because it wasn't. It's much better to simply let go the current error message. Cheers.
On July 6, 2021 5:54 PM Junio C Hamano wrote: >"Randall S. Becker" <rsbecker@nexbridge.com> writes: > >> On July 6, 2021 4:56 PM, Junio C Hamano wrote: >>>"Randall S. Becker" <rsbecker@nexbridge.com> writes: >>> >>>>>If you do: >>>>> >>>>> % git merge --ff-only >>>>> fatal: Not possible to fast-forward, aborting. >>>>> >>>>>That "aborting" part is redundant; we know `git merge` should abort >>>> if the fast-forward is not possible, we explicitely told git to do >>>> that. >>>> >>>> `git merge` is a special operation where errors (conflicts, for one) >>>> may leave the repository in a merge pending state where you >>>> subsequently may have to use `git merge --abort` to reset the >>>> situation or `git add` to continue. The `aborting` output makes it >>>> clear that you do not have to do the `--abort` and *cannot* do the >>>> `add` because there was an implicit `--abort` done resulting from >>>> the failure. This is important information for the user. >>> >>>If so, adding ", aborting" to the end is misleading. In this >>>particular failure mode, the command pretends that the merge did not even start. >> >> You know that, and I know that. I contend that git users do not >> generally want to care about failure modes. While a flow description >> might be instructive, I doubt many git users would look at it. We can >> only hope that front ends know how to make this clean for them. > >They may not care about "failure modes" but they would want to know that in this situation, unlike other times, they do not have to say >"git merge --abort" (or "git reset --hard"). Saying ", aborting" >does not help as much as saying say ", not starting a merge", for example. I like that a lot, actually "not starting a merge" is much more helpful.
diff --git a/builtin/merge.c b/builtin/merge.c index a8a843b1f5..05e631229d 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1620,7 +1620,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) } if (fast_forward == FF_ONLY) - die(_("Not possible to fast-forward, aborting.")); + die(_("unable to fast-forward")); if (autostash) create_autostash(the_repository,
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- builtin/merge.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)