Git Mailing List Archive mirror
 help / color / mirror / Atom feed
From: Phillip Wood <phillip.wood123@gmail.com>
To: Junio C Hamano <gitster@pobox.com>,
	Phillip Wood <phillip.wood123@gmail.com>
Cc: Alex Henrie <alexhenrie24@gmail.com>,
	git@vger.kernel.org, tao@klerks.biz, newren@gmail.com,
	Johannes.Schindelin@gmx.de, sorganov@gmail.com,
	chooglen@google.com, calvinwan@google.com,
	jonathantanmy@google.com, felipe.contreras@gmail.com
Subject: Re: [PATCH v8 3/3] rebase: add a config option for --rebase-merges
Date: Fri, 24 Mar 2023 14:52:44 +0000	[thread overview]
Message-ID: <830fe4e6-b2a3-d8ad-a415-1d385e759961@dunelm.org.uk> (raw)
In-Reply-To: <xmqqttyb8fxw.fsf@gitster.g>

Hi Junio

On 23/03/2023 18:45, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>>> @@ -1514,13 +1542,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>>    				break;
>>>      		if (i >= 0 || options.type == REBASE_APPLY) {
>>> -			if (is_merge(&options))
>>> -				die(_("apply options and merge options "
>>> -					  "cannot be used together"));
>>
>> This isn't a new change but having thought about it I'm not sure
>> moving this code is a good idea. If the user runs
>>
>> 	git -c rebase.updateRefs=true rebase --whitespace=fix --exec "make test"
>>
>> then instead of getting an message saying that they cannot use apply
>> and merge options together they'll get a message suggesting they pass
>> --no-update-refs which wont fix the problem for them.
> 
> Hmph.  Instead of dying here, ...
> 
>>> +			if (options.autosquash == -1 && options.config_autosquash == 1)
>>>    				die(_("apply options are incompatible with rebase.autoSquash.  Consider adding --no-autosquash"));
>>> +			else if (options.rebase_merges == -1 && options.config_rebase_merges == 1)
>>> +				die(_("apply options are incompatible with rebase.rebaseMerges.  Consider adding --no-rebase-merges"));
>>>    			else if (options.update_refs == -1 && options.config_update_refs == 1)
>>>    				die(_("apply options are incompatible with rebase.updateRefs.  Consider adding --no-update-refs"));
> 
> ... we get caught here, and the next one is not triggered.
> 
>>> +			else if (is_merge(&options))
>>> +				die(_("apply options and merge options "
>>> +					  "cannot be used together"));
>>>    			else
>>>    				options.type = REBASE_APPLY;
> 
> What's the reason why "cannot be used together" is moved to the last
> in the chain?
> 
> The first two new conditions in this chain try to catch an
> invocation with some apply-specific command line option
> (e.g. "--whitespace=fix") when used with configuration variables
> specific to the merge-backend (e.g. "rebase.merges") and suggest
> overriding the configuration from the command line, and I suspect
> that the motivation behind this change is that their error messages
> are more specific than the generic "apply and merge do not mix".
> 
> But are these two new errors and hints universally a good suggestion
> to make?  I actually think a command line option forcing us to use
> the apply backend should simply silently ignore (aka "override")
> configuration variables that take effect only when we are using the
> merge-backend.  "git rebase --whitespace=fix --rebase-merges",
> giving both from the command line, is making conflicting and
> unsatisfyable request, and it is worth giving an error message.
> 
> But if you configure rebase.autosquash only because you want to it
> to be in effect for your invocations of "git rebase -i", you
> shouldn't have to override it from the command line when you say
> "git rebase" or "git rebase --whitespace=fix", should you?

I think there are two conflicting viewpoints here which depend on what 
one thinks the user wants when they set these configuration variables. 
As I understand it Elijah's thinking was that if the user set 
rebase.autosquash they'd be surprised when their fixup commits were not 
squashed by

	git rebase --whitespace=fix

and that's why his patch series changed things to error out.

The other point of view is that the user expects that these 
configuration variables to apply only when they are using the "merge" 
backend and so we should not error out.

Personally I lean a little more towards the latter but I don't feel that 
strongly about it and can see an argument for having either behavior. I 
do think we should leave the ordering alone though so the user gets a 
useful error message in the case of "git rebase --exec 'make test' 
--whitespace=fix"

Best Wishes

Phillip

> Thanks.

  reply	other threads:[~2023-03-24 14:52 UTC|newest]

Thread overview: 96+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-02-23  5:34 [PATCH v4 1/3] rebase: add documentation and test for --no-rebase-merges Alex Henrie
2023-02-23  5:34 ` [PATCH v4 2/3] rebase: stop accepting --rebase-merges="" Alex Henrie
2023-02-24 13:54   ` Johannes Schindelin
2023-02-24 17:20     ` Junio C Hamano
2023-02-24 17:50       ` Alex Henrie
2023-02-24 18:08         ` Junio C Hamano
2023-02-24 18:23           ` Alex Henrie
2023-02-24 18:40             ` Junio C Hamano
2023-02-24 18:55               ` Alex Henrie
2023-02-24 19:13                 ` Junio C Hamano
2023-02-24 19:24                   ` Alex Henrie
2023-02-24 19:24                   ` Phillip Wood
2023-02-24 19:56                     ` Alex Henrie
2023-02-23  5:34 ` [PATCH v4 3/3] rebase: add a config option for --rebase-merges Alex Henrie
2023-02-24 13:53   ` Johannes Schindelin
2023-02-24 17:49     ` Alex Henrie
2023-02-24 14:55   ` Phillip Wood
2023-02-24 17:51     ` Alex Henrie
2023-02-23 17:28 ` [PATCH v4 1/3] rebase: add documentation and test for --no-rebase-merges Junio C Hamano
2023-02-24 13:57   ` Johannes Schindelin
2023-02-24 19:16     ` Junio C Hamano
2023-02-25 18:09     ` Alex Henrie
2023-02-25 18:03 ` [PATCH v5 0/3] rebase: add a config option for --rebase-merges Alex Henrie
2023-02-25 18:03   ` [PATCH v5 1/3] rebase: add documentation and test for --no-rebase-merges Alex Henrie
2023-03-01 23:23     ` Glen Choo
2023-02-25 18:03   ` [PATCH v5 2/3] rebase: deprecate --rebase-merges="" Alex Henrie
2023-03-01 23:46     ` Glen Choo
2023-03-02 10:07     ` Phillip Wood
2023-03-02 18:02     ` Calvin Wan
2023-02-25 18:03   ` [PATCH v5 3/3] rebase: add a config option for --rebase-merges Alex Henrie
2023-03-01 23:43     ` Glen Choo
2023-03-02  9:37     ` Phillip Wood
2023-03-04 23:24       ` Alex Henrie
2023-03-07 16:23         ` Phillip Wood
2023-03-12 20:57           ` Alex Henrie
2023-03-13 14:20             ` Phillip Wood
2023-03-13 16:12               ` Felipe Contreras
2023-03-13 19:46             ` Junio C Hamano
2023-03-24 14:47             ` About replaying "evil" merges... " Johannes Schindelin
2023-03-02 18:02     ` Calvin Wan
2023-03-04 23:24       ` Alex Henrie
2023-03-01 23:14   ` [PATCH v5 0/3] " Glen Choo
2023-03-02  5:02     ` Alex Henrie
2023-03-02  5:09       ` Alex Henrie
2023-03-05  5:07   ` [PATCH v6 0/3] rebase: document, clean up, and introduce " Alex Henrie
2023-03-05  5:07     ` [PATCH v6 1/3] rebase: add documentation and test for --no-rebase-merges Alex Henrie
2023-03-08 22:25       ` Sergey Organov
2023-03-05  5:07     ` [PATCH v6 2/3] rebase: deprecate --rebase-merges="" Alex Henrie
2023-03-07 14:59       ` Phillip Wood
2023-03-05  5:07     ` [PATCH v6 3/3] rebase: add a config option for --rebase-merges Alex Henrie
2023-03-07 14:56       ` Phillip Wood
2023-03-07 18:32         ` Junio C Hamano
2023-03-12 21:01           ` Alex Henrie
2023-03-08  0:09         ` Glen Choo
2023-03-08  0:02       ` Glen Choo
2023-03-12 21:03         ` Alex Henrie
2023-03-15  2:52         ` Alex Henrie
2023-03-16 17:32           ` Glen Choo
2023-03-16 18:11             ` Felipe Contreras
2023-03-16 22:46               ` Glen Choo
2023-03-16 23:48                 ` Felipe Contreras
2023-03-16 20:27             ` Alex Henrie
2023-03-16 22:39               ` Glen Choo
2023-03-18  5:59                 ` Alex Henrie
2023-03-24 15:05               ` Johannes Schindelin
2023-03-25 16:59               ` Sergey Organov
2023-03-05 12:22     ` [PATCH v6 0/3] rebase: document, clean up, and introduce " Sergey Organov
2023-03-05 21:33       ` Alex Henrie
2023-03-05 22:54         ` Sergey Organov
2023-03-06  0:02           ` Alex Henrie
2023-03-06 13:23             ` Sergey Organov
2023-03-06 19:08             ` Junio C Hamano
2023-03-06 17:19           ` Junio C Hamano
2023-03-06 16:24     ` Phillip Wood
2023-03-06 17:36       ` Alex Henrie
2023-03-07 15:07         ` Phillip Wood
2023-03-08  0:13     ` Glen Choo
2023-03-12 21:04     ` [PATCH v7 " Alex Henrie
2023-03-12 21:04       ` [PATCH v7 1/3] rebase: add documentation and test for --no-rebase-merges Alex Henrie
2023-03-12 21:04       ` [PATCH v7 2/3] rebase: deprecate --rebase-merges="" Alex Henrie
2023-03-12 21:04       ` [PATCH v7 3/3] rebase: add a config option for --rebase-merges Alex Henrie
2023-03-20  5:59       ` [PATCH v8 0/3] rebase: document, clean up, and introduce " Alex Henrie
2023-03-20  5:59         ` [PATCH v8 1/3] rebase: add documentation and test for --no-rebase-merges Alex Henrie
2023-03-20  5:59         ` [PATCH v8 2/3] rebase: deprecate --rebase-merges="" Alex Henrie
2023-03-20  5:59         ` [PATCH v8 3/3] rebase: add a config option for --rebase-merges Alex Henrie
2023-03-22 16:54           ` Phillip Wood
2023-03-23 18:45             ` Junio C Hamano
2023-03-24 14:52               ` Phillip Wood [this message]
2023-03-25  5:23               ` Alex Henrie
2023-03-25  5:21             ` Alex Henrie
2023-03-26  3:06         ` [PATCH v9 0/3] rebase: document, clean up, and introduce " Alex Henrie
2023-03-26  3:06           ` [PATCH v9 1/3] rebase: add documentation and test for --no-rebase-merges Alex Henrie
2023-03-26  3:06           ` [PATCH v9 2/3] rebase: deprecate --rebase-merges="" Alex Henrie
2023-03-26  3:06           ` [PATCH v9 3/3] rebase: add a config option for --rebase-merges Alex Henrie
2023-03-26 15:12           ` [PATCH v9 0/3] rebase: document, clean up, and introduce " Phillip Wood
2023-03-27 16:33             ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=830fe4e6-b2a3-d8ad-a415-1d385e759961@dunelm.org.uk \
    --to=phillip.wood123@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=alexhenrie24@gmail.com \
    --cc=calvinwan@google.com \
    --cc=chooglen@google.com \
    --cc=felipe.contreras@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jonathantanmy@google.com \
    --cc=newren@gmail.com \
    --cc=phillip.wood@dunelm.org.uk \
    --cc=sorganov@gmail.com \
    --cc=tao@klerks.biz \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).