git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* Re: [PATCH] format-patch: cancel useAutoBase if base is invalid
       [not found] <20200916234916.422553-1-jacob.e.keller@intel.com>
@ 2020-09-17  0:26 ` Junio C Hamano
  2020-09-17  0:43   ` Junio C Hamano
  2020-09-17 19:26   ` Jacob Keller
  0 siblings, 2 replies; 4+ messages in thread
From: Junio C Hamano @ 2020-09-17  0:26 UTC (permalink / raw)
  To: Jacob Keller; +Cc: git, Jacob Keller

Jacob Keller <jacob.e.keller@intel.com> writes:

> Make get_base_commit detect when useAutoBase is set, and avoid failing
> if the base commit is picked up automatically. We still attempt to fail
> if --base=auto is explicitly requested on the command line.

Makes sense.  I also think we should fail in such a broken base is
chosen, when useAutoBase is set by configuration and is not
overriden from the command line with an explicit use of --no-base
option, because the end-user expects an appropriate base to be used
that is computed automatically, but we are failing to find such a
base---going ahead silently in such a case would be wrong.

> Unfortunately, at least with how --base is handled by the option parsing
> now, if useAutoBase is true *and* --base=auto is provided, we'll still
> cancel the base option instead of failing.

If we are doing the usual "prime with configuration variables and
then override from the commnad line option" thing, then you cannot
tell these two cases apart, but I happen to think that it is better
to fail (and not to ignore silently) consistently whether the
"compute an appropriate base automatically" came from config or
command line, so...

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] format-patch: cancel useAutoBase if base is invalid
  2020-09-17  0:26 ` [PATCH] format-patch: cancel useAutoBase if base is invalid Junio C Hamano
@ 2020-09-17  0:43   ` Junio C Hamano
  2020-09-17 19:26   ` Jacob Keller
  1 sibling, 0 replies; 4+ messages in thread
From: Junio C Hamano @ 2020-09-17  0:43 UTC (permalink / raw)
  To: Jacob Keller; +Cc: git, Jacob Keller

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

> If we are doing the usual "prime with configuration variables and
> then override from the commnad line option" thing, then you cannot
> tell these two cases apart, but I happen to think that it is better
> to fail (and not to ignore silently) consistently whether the
> "compute an appropriate base automatically" came from config or
> command line, so...

If we really need to tell these two apart, we'd invent a new
variable (we only need a single bit) to record "did we get this base
setting from the command line?" along with what we are already
doing.  You would need to use a option callback instead of a simple
OPT_STRING() to parse the "base" option, but I think it should be
straightforward.





^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] format-patch: cancel useAutoBase if base is invalid
  2020-09-17  0:26 ` [PATCH] format-patch: cancel useAutoBase if base is invalid Junio C Hamano
  2020-09-17  0:43   ` Junio C Hamano
@ 2020-09-17 19:26   ` Jacob Keller
  2020-09-18  0:04     ` Junio C Hamano
  1 sibling, 1 reply; 4+ messages in thread
From: Jacob Keller @ 2020-09-17 19:26 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jacob Keller, Git mailing list

On Wed, Sep 16, 2020 at 5:26 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
> > Make get_base_commit detect when useAutoBase is set, and avoid failing
> > if the base commit is picked up automatically. We still attempt to fail
> > if --base=auto is explicitly requested on the command line.
>
> Makes sense.  I also think we should fail in such a broken base is
> chosen, when useAutoBase is set by configuration and is not
> overriden from the command line with an explicit use of --no-base
> option, because the end-user expects an appropriate base to be used
> that is computed automatically, but we are failing to find such a
> base---going ahead silently in such a case would be wrong.
>

I am not sure if I follow here. The whole point of this patch is that

git config format.useAutoBase true
git format-patch -1 <old id>

causes failure that is very unexpected, especially if it's been a long
time since you set useAutoBase.

I do want git format-patch --base=auto <old id> to fail, certainly.

I wonder if there's a way we can tell when the format patch revisions
in question make no sense with the automatic base. So.. rather than
looking at "is this base valid" think of it as "the base for this
branch is XYZ, is that valid for the set of requested commits?"

> > Unfortunately, at least with how --base is handled by the option parsing
> > now, if useAutoBase is true *and* --base=auto is provided, we'll still
> > cancel the base option instead of failing.
>
> If we are doing the usual "prime with configuration variables and
> then override from the commnad line option" thing, then you cannot
> tell these two cases apart, but I happen to think that it is better
> to fail (and not to ignore silently) consistently whether the
> "compute an appropriate base automatically" came from config or
> command line, so...

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] format-patch: cancel useAutoBase if base is invalid
  2020-09-17 19:26   ` Jacob Keller
@ 2020-09-18  0:04     ` Junio C Hamano
  0 siblings, 0 replies; 4+ messages in thread
From: Junio C Hamano @ 2020-09-18  0:04 UTC (permalink / raw)
  To: Jacob Keller; +Cc: Jacob Keller, Git mailing list

Jacob Keller <jacob.keller@gmail.com> writes:

> On Wed, Sep 16, 2020 at 5:26 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Jacob Keller <jacob.e.keller@intel.com> writes:
>>
>> > Make get_base_commit detect when useAutoBase is set, and avoid failing
>> > if the base commit is picked up automatically. We still attempt to fail
>> > if --base=auto is explicitly requested on the command line.
>>
>> Makes sense.  I also think we should fail in such a broken base is
>> chosen, when useAutoBase is set by configuration and is not
>> overriden from the command line with an explicit use of --no-base
>> option, because the end-user expects an appropriate base to be used
>> that is computed automatically, but we are failing to find such a
>> base---going ahead silently in such a case would be wrong.
>>
>
> I am not sure if I follow here. The whole point of this patch is that
>
> git config format.useAutoBase true
> git format-patch -1 <old id>
>
> causes failure that is very unexpected, especially if it's been a long
> time since you set useAutoBase.
>
> I do want git format-patch --base=auto <old id> to fail, certainly.

I understand.  And further, I do not think it is a good idea to
silently ignore the configured format.useAutoBase when there is no
command line override.  IOW, we want both to fail, but with a better
message (e.g. "appropriate base not found").

> I wonder if there's a way we can tell when the format patch revisions
> in question make no sense with the automatic base.

Sorry, I don't quite know what you mean---the fact that you are
already getting a cryptic error message means the existing code
already knows, no?



^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-09-18  0:04 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20200916234916.422553-1-jacob.e.keller@intel.com>
2020-09-17  0:26 ` [PATCH] format-patch: cancel useAutoBase if base is invalid Junio C Hamano
2020-09-17  0:43   ` Junio C Hamano
2020-09-17 19:26   ` Jacob Keller
2020-09-18  0:04     ` Junio C Hamano

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git