| Message ID | eb5871db95b12500cc0a6b8b0e3a82ed9e8fcfbd.1645526016.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Update the die() preserve-merges messages to help some users | expand |
On Tue, Feb 22 2022, Philip Oakley via GitGitGadget wrote: > From: Philip Oakley <philipoakley@iee.email> > > The `--preserve-merges` option was removed by v2.35.0. However > users may not be aware that it is also a Pull option, and it is > still offered by major IDE vendors such as Visual Studio. > > Extend the `--preserve-merges` die message to direct users to > this option and it's locations. > > Signed-off-by: Philip Oakley <philipoakley@iee.email> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > builtin/rebase.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/builtin/rebase.c b/builtin/rebase.c > index 07221d0ae41..97f704bb297 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -1205,7 +1205,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > builtin_rebase_usage, 0); > > if (preserve_merges_selected) > - die(_("--preserve-merges was replaced by --rebase-merges")); > + die(_("--preserve-merges was replaced by --rebase-merges\n" > + "Also, check your `pull` configuration settings\n" > + "`git config --show-scope --show-origin --get-regexp 'pull.*'`\n" > + "which may also invoke this option.")); I may be missing some subtlety, but how is the user ever going to need to check their config? After 52f1e82178e (pull: remove support for `--rebase=preserve`, 2021-09-07) we: $ git -c pull.rebase=preserve pull error: rebase.c:29: preserve: 'preserve' superseded by 'merges' fatal: builtin/pull.c:45: Invalid value for pull.rebase: preserve I.e. we'd error before this, and the "preserve_merges_selected" variable being checked here is not affected by config, i.e. we only ever got to this "via config" route if "pull" et al was invoking us. But now that command dies. If there is still a codepath where we call "rebase --preserve-merges" on the basis of config that I've missed, shouldn't this die() be happening there?
On 22/02/2022 15:34, Ævar Arnfjörð Bjarmason wrote: > On Tue, Feb 22 2022, Philip Oakley via GitGitGadget wrote: > >> From: Philip Oakley <philipoakley@iee.email> >> >> The `--preserve-merges` option was removed by v2.35.0. However >> users may not be aware that it is also a Pull option, and it is >> still offered by major IDE vendors such as Visual Studio. >> >> Extend the `--preserve-merges` die message to direct users to >> this option and it's locations. >> >> Signed-off-by: Philip Oakley <philipoakley@iee.email> >> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> >> --- >> builtin/rebase.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/builtin/rebase.c b/builtin/rebase.c >> index 07221d0ae41..97f704bb297 100644 >> --- a/builtin/rebase.c >> +++ b/builtin/rebase.c >> @@ -1205,7 +1205,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) >> builtin_rebase_usage, 0); >> >> if (preserve_merges_selected) >> - die(_("--preserve-merges was replaced by --rebase-merges")); >> + die(_("--preserve-merges was replaced by --rebase-merges\n" >> + "Also, check your `pull` configuration settings\n" >> + "`git config --show-scope --show-origin --get-regexp 'pull.*'`\n" >> + "which may also invoke this option.")); > I may be missing some subtlety, but how is the user ever going to need > to check their config? > > After 52f1e82178e (pull: remove support for `--rebase=preserve`, > 2021-09-07) we: > > $ git -c pull.rebase=preserve pull > error: rebase.c:29: preserve: 'preserve' superseded by 'merges' > fatal: builtin/pull.c:45: Invalid value for pull.rebase: preserve > > I.e. we'd error before this, and the "preserve_merges_selected" variable > being checked here is not affected by config, i.e. we only ever got to > this "via config" route if "pull" et al was invoking us. > > But now that command dies. > > If there is still a codepath where we call "rebase --preserve-merges" on > the basis of config that I've missed, shouldn't this die() be happening > there? Hi Ævar, The preserve merges is a pull config item, still supported by Visual Studio, so could easily be set for many users. When they click on the "Pull" menu and get the old die() fatal: message they won't know why this happened or how to resolve it. Hence the extra info. If the user was actually running a rebase command then, yes, they would/should be reasonably OK with the original error message telling them what was wrong with their command line. But a `git pull`? especially as the manual says nothing about preserve merges any more. If we are to remove a capability the `give up and die` messages should be helpful in the edge cases. The issue (needing to check the pull, as well as the rebase configs) has already shown on the git-users list. It took a while to workout how it all happened. Not sure if I've covered your concerns properly, but this is one of the places that can and warn the user. Philip
Hi Ævar, On 22/02/2022 15:56, Philip Oakley wrote: > On 22/02/2022 15:34, Ævar Arnfjörð Bjarmason wrote: >> On Tue, Feb 22 2022, Philip Oakley via GitGitGadget wrote: >> >>> From: Philip Oakley <philipoakley@iee.email> >>> >>> The `--preserve-merges` option was removed by v2.35.0. However >>> users may not be aware that it is also a Pull option, and it is >>> still offered by major IDE vendors such as Visual Studio. >>> >>> Extend the `--preserve-merges` die message to direct users to >>> this option and it's locations. >>> >>> Signed-off-by: Philip Oakley <philipoakley@iee.email> >>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> >>> --- >>> builtin/rebase.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/builtin/rebase.c b/builtin/rebase.c >>> index 07221d0ae41..97f704bb297 100644 >>> --- a/builtin/rebase.c >>> +++ b/builtin/rebase.c >>> @@ -1205,7 +1205,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) >>> builtin_rebase_usage, 0); >>> >>> if (preserve_merges_selected) >>> - die(_("--preserve-merges was replaced by --rebase-merges")); >>> + die(_("--preserve-merges was replaced by --rebase-merges\n" >>> + "Also, check your `pull` configuration settings\n" >>> + "`git config --show-scope --show-origin --get-regexp 'pull.*'`\n" >>> + "which may also invoke this option.")); >> I may be missing some subtlety, but how is the user ever going to need >> to check their config? >> >> After 52f1e82178e (pull: remove support for `--rebase=preserve`, >> 2021-09-07) we: >> >> $ git -c pull.rebase=preserve pull >> error: rebase.c:29: preserve: 'preserve' superseded by 'merges' >> fatal: builtin/pull.c:45: Invalid value for pull.rebase: preserve >> >> I.e. we'd error before this, and the "preserve_merges_selected" variable >> being checked here is not affected by config, i.e. we only ever got to >> this "via config" route if "pull" et al was invoking us. >> >> But now that command dies. >> >> If there is still a codepath where we call "rebase --preserve-merges" on >> the basis of config that I've missed, shouldn't this die() be happening >> there? > Hi Ævar, > > The preserve merges is a pull config item, still supported by Visual > Studio, so could easily be set for many users. When they click on the > "Pull" menu and get the old die() fatal: message they won't know why > this happened or how to resolve it. Hence the extra info. > > If the user was actually running a rebase command then, yes, they > would/should be reasonably OK with the original error message telling > them what was wrong with their command line. But a `git pull`? > especially as the manual says nothing about preserve merges any more. > > If we are to remove a capability the `give up and die` messages should > be helpful in the edge cases. The issue (needing to check the pull, as > well as the rebase configs) has already shown on the git-users list. It > took a while to workout how it all happened. > > Not sure if I've covered your concerns properly, but this is one of the > places that can and warn the user. > > I don't think that came out well. For this message, it's about reminding users that they may *also* be using the `preserve` option within their pull options, and not just within their rebase command's options. A case of "don't use it here, and don't use it there either, please check". Philip
On Tue, Feb 22 2022, Philip Oakley wrote: > Hi Ævar, > > On 22/02/2022 15:56, Philip Oakley wrote: >> On 22/02/2022 15:34, Ævar Arnfjörð Bjarmason wrote: >>> On Tue, Feb 22 2022, Philip Oakley via GitGitGadget wrote: >>> >>>> From: Philip Oakley <philipoakley@iee.email> >>>> >>>> The `--preserve-merges` option was removed by v2.35.0. However >>>> users may not be aware that it is also a Pull option, and it is >>>> still offered by major IDE vendors such as Visual Studio. >>>> >>>> Extend the `--preserve-merges` die message to direct users to >>>> this option and it's locations. >>>> >>>> Signed-off-by: Philip Oakley <philipoakley@iee.email> >>>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> >>>> --- >>>> builtin/rebase.c | 5 ++++- >>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/builtin/rebase.c b/builtin/rebase.c >>>> index 07221d0ae41..97f704bb297 100644 >>>> --- a/builtin/rebase.c >>>> +++ b/builtin/rebase.c >>>> @@ -1205,7 +1205,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) >>>> builtin_rebase_usage, 0); >>>> >>>> if (preserve_merges_selected) >>>> - die(_("--preserve-merges was replaced by --rebase-merges")); >>>> + die(_("--preserve-merges was replaced by --rebase-merges\n" >>>> + "Also, check your `pull` configuration settings\n" >>>> + "`git config --show-scope --show-origin --get-regexp 'pull.*'`\n" >>>> + "which may also invoke this option.")); >>> I may be missing some subtlety, but how is the user ever going to need >>> to check their config? >>> >>> After 52f1e82178e (pull: remove support for `--rebase=preserve`, >>> 2021-09-07) we: >>> >>> $ git -c pull.rebase=preserve pull >>> error: rebase.c:29: preserve: 'preserve' superseded by 'merges' >>> fatal: builtin/pull.c:45: Invalid value for pull.rebase: preserve >>> >>> I.e. we'd error before this, and the "preserve_merges_selected" variable >>> being checked here is not affected by config, i.e. we only ever got to >>> this "via config" route if "pull" et al was invoking us. >>> >>> But now that command dies. >>> >>> If there is still a codepath where we call "rebase --preserve-merges" on >>> the basis of config that I've missed, shouldn't this die() be happening >>> there? >> Hi Ævar, >> >> The preserve merges is a pull config item, still supported by Visual >> Studio, so could easily be set for many users. When they click on the >> "Pull" menu and get the old die() fatal: message they won't know why >> this happened or how to resolve it. Hence the extra info. >> >> If the user was actually running a rebase command then, yes, they >> would/should be reasonably OK with the original error message telling >> them what was wrong with their command line. But a `git pull`? >> especially as the manual says nothing about preserve merges any more. >> >> If we are to remove a capability the `give up and die` messages should >> be helpful in the edge cases. The issue (needing to check the pull, as >> well as the rebase configs) has already shown on the git-users list. It >> took a while to workout how it all happened. >> >> Not sure if I've covered your concerns properly, but this is one of the >> places that can and warn the user. >> >> > I don't think that came out well. For this message, it's about reminding > users that they may *also* be using the `preserve` option within their > pull options, and not just within their rebase command's options. > > A case of "don't use it here, and don't use it there either, please check". I see. What I was confused with (and still find confusing) is that the error here says "[and the pull.rebase=preserve config] may also invoke this option". But it doesn't do that at all, it *used to*, but now "git pull" will promptly fail on it as well. So this is really just trying to tell us something like this, right? Hey, you invoked --preserve-merges, bad mojo! Also: if you invoke 'git pull' and used the related config you may or may not have used, that'll die too. So maybe check that out? :) Anyway, to the user the "invoke this option" isn't strictly accurate, but they probably don't care/know the difference, so I don't mind per-se, I was just wondering if I was missing something. I do wonder if this part is needed at all. I.e. at this point this will only benefit users who are *manually* invoking "git rebase --preserve-merges", everyone else (including that "VS" example you mentioned) will go through "git pull" and see the other error. IOW maybe just saying "this option is an error now" when we intercept the option is enough, and ditto for "this config is an error now" when we intercept the config, and not assume that a user using the option must also be using the config is simpler/sufficient. Just my 0.02. For my part whahtever you decide to do here is fine by me, I was just wondering if I'd misundestood the whole flow around this legacy option and how we invoke the small parts of die() boilerplate left of it.
Sorry for the delay. I've got some on-going family matters to attend to which have limited my availablilty. On 23/02/2022 10:27, Ævar Arnfjörð Bjarmason wrote: > On Tue, Feb 22 2022, Philip Oakley wrote: > >> Hi Ævar, >> >> On 22/02/2022 15:56, Philip Oakley wrote: >>> On 22/02/2022 15:34, Ævar Arnfjörð Bjarmason wrote: >>>> On Tue, Feb 22 2022, Philip Oakley via GitGitGadget wrote: >>>> >>>>> From: Philip Oakley <philipoakley@iee.email> >>>>> >>>>> The `--preserve-merges` option was removed by v2.35.0. However >>>>> users may not be aware that it is also a Pull option, and it is >>>>> still offered by major IDE vendors such as Visual Studio. >>>>> >>>>> Extend the `--preserve-merges` die message to direct users to >>>>> this option and it's locations. >>>>> >>>>> Signed-off-by: Philip Oakley <philipoakley@iee.email> >>>>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> >>>>> --- >>>>> builtin/rebase.c | 5 ++++- >>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/builtin/rebase.c b/builtin/rebase.c >>>>> index 07221d0ae41..97f704bb297 100644 >>>>> --- a/builtin/rebase.c >>>>> +++ b/builtin/rebase.c >>>>> @@ -1205,7 +1205,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) >>>>> builtin_rebase_usage, 0); >>>>> >>>>> if (preserve_merges_selected) >>>>> - die(_("--preserve-merges was replaced by --rebase-merges")); >>>>> + die(_("--preserve-merges was replaced by --rebase-merges\n" >>>>> + "Also, check your `pull` configuration settings\n" >>>>> + "`git config --show-scope --show-origin --get-regexp 'pull.*'`\n" >>>>> + "which may also invoke this option.")); >>>> I may be missing some subtlety, but how is the user ever going to need >>>> to check their config? >>>> >>>> After 52f1e82178e (pull: remove support for `--rebase=preserve`, >>>> 2021-09-07) we: >>>> >>>> $ git -c pull.rebase=preserve pull >>>> error: rebase.c:29: preserve: 'preserve' superseded by 'merges' >>>> fatal: builtin/pull.c:45: Invalid value for pull.rebase: preserve >>>> >>>> I.e. we'd error before this, and the "preserve_merges_selected" variable >>>> being checked here is not affected by config, i.e. we only ever got to >>>> this "via config" route if "pull" et al was invoking us. >>>> >>>> But now that command dies. >>>> >>>> If there is still a codepath where we call "rebase --preserve-merges" on >>>> the basis of config that I've missed, shouldn't this die() be happening >>>> there? >>> Hi Ævar, >>> >>> The preserve merges is a pull config item, still supported by Visual >>> Studio, so could easily be set for many users. When they click on the >>> "Pull" menu and get the old die() fatal: message they won't know why >>> this happened or how to resolve it. Hence the extra info. >>> >>> If the user was actually running a rebase command then, yes, they >>> would/should be reasonably OK with the original error message telling >>> them what was wrong with their command line. But a `git pull`? >>> especially as the manual says nothing about preserve merges any more. >>> >>> If we are to remove a capability the `give up and die` messages should >>> be helpful in the edge cases. The issue (needing to check the pull, as >>> well as the rebase configs) has already shown on the git-users list. It >>> took a while to workout how it all happened. >>> >>> Not sure if I've covered your concerns properly, but this is one of the >>> places that can and warn the user. >>> >>> >> I don't think that came out well. For this message, it's about reminding >> users that they may *also* be using the `preserve` option within their >> pull options, and not just within their rebase command's options. >> >> A case of "don't use it here, and don't use it there either, please check". > I see. What I was confused with (and still find confusing) is that the > error here says "[and the pull.rebase=preserve config] may also invoke > this option". > > But it doesn't do that at all, it *used to*, but now "git pull" will > promptly fail on it as well. Ok, That's probably my local idiom that confusing the now of the error and the past when the config would have been set up. I'll see if I can clarify that better. > So this is really just trying to tell us something like this, right? > > Hey, you invoked --preserve-merges, bad mojo! Also: if you invoke > 'git pull' and used the related config you may or may not have used, > that'll die too. So maybe check that out? > > :) > > Anyway, to the user the "invoke this option" isn't strictly accurate, > but they probably don't care/know the difference, so I don't mind > per-se, I was just wondering if I was missing something. > > I do wonder if this part is needed at all. I.e. at this point this will > only benefit users who are *manually* invoking "git rebase > --preserve-merges", everyone else (including that "VS" example you > mentioned) will go through "git pull" and see the other error. > > IOW maybe just saying "this option is an error now" when we intercept > the option is enough, and ditto for "this config is an error now" when > we intercept the config, and not assume that a user using the option > must also be using the config is simpler/sufficient. Error reporting is a tricky subject, especially when we add the human error and misunderstanding aspects into the mix. > > Just my 0.02. For my part whahtever you decide to do here is fine by me, > I was just wondering if I'd misundestood the whole flow around this > legacy option and how we invoke the small parts of die() boilerplate > left of it. Yep, it is tricky talking about disappeared options, the error messages, and the multiple ways that show themselves. In the originating edge case it wasn't helped by a Git advice message that contained the failing command line (`git rebase --continue`) suggestion. In general I was trying to cover all the possible sources in all the places the 'error' may occur. I'll have another look at the ways these edge cases could appear, and try an improve the commit message explanations where the diff doesn't show sufficient context. It'll be at least next week. Philip
Hi Philip, On Fri, 4 Mar 2022, Philip Oakley wrote: > I'll have another look at the ways these edge cases could appear, and > try an improve the commit message explanations where the diff doesn't > show sufficient context. It'll be at least next week. Can I punt this patch series back to you? (I sent it upstream on your behalf because I had assumed that you'd want me to, sorry for misunderstanding your intentions.) Ciao, Dscho
Hi Dscho, On 07/03/2022 16:43, Johannes Schindelin wrote: > Hi Philip, > > On Fri, 4 Mar 2022, Philip Oakley wrote: > >> I'll have another look at the ways these edge cases could appear, and >> try an improve the commit message explanations where the diff doesn't >> show sufficient context. It'll be at least next week. > Can I punt this patch series back to you? (I sent it upstream on your > behalf because I had assumed that you'd want me to, sorry for > misunderstanding your intentions.) > I'm happy to continue to work on this series, and am grateful for your support in pushing it through GGG. How is it best to transfer the 'ownership' at GGG? I've still got some family issues so it'll be later in the week, or even next week before I can update the series. Thanks. Philip
Hi Philip, On Mon, 7 Mar 2022, Philip Oakley wrote: > On 07/03/2022 16:43, Johannes Schindelin wrote: > > Hi Philip, > > > > On Fri, 4 Mar 2022, Philip Oakley wrote: > > > >> I'll have another look at the ways these edge cases could appear, and > >> try an improve the commit message explanations where the diff doesn't > >> show sufficient context. It'll be at least next week. > > Can I punt this patch series back to you? (I sent it upstream on your > > behalf because I had assumed that you'd want me to, sorry for > > misunderstanding your intentions.) > > > I'm happy to continue to work on this series, and am grateful for your > support in pushing it through GGG. How is it best to transfer the > 'ownership' at GGG? Sadly, I don't know of any way how I could transfer ownership to you, but maybe you can just open a new one and reference the first thread in the cover letter? > I've still got some family issues so it'll be later in the week, or even > next week before I can update the series. Sorry to hear that you have issues. These are tough times, and I feel for you. Ciao, Dscho
diff --git a/builtin/rebase.c b/builtin/rebase.c index 07221d0ae41..97f704bb297 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1205,7 +1205,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) builtin_rebase_usage, 0); if (preserve_merges_selected) - die(_("--preserve-merges was replaced by --rebase-merges")); + die(_("--preserve-merges was replaced by --rebase-merges\n" + "Also, check your `pull` configuration settings\n" + "`git config --show-scope --show-origin --get-regexp 'pull.*'`\n" + "which may also invoke this option.")); if (action != ACTION_NONE && total_argc != 2) { usage_with_options(builtin_rebase_usage,