git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jacob Keller <jacob.keller@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Jacob Keller <jacob.e.keller@intel.com>,
	Git mailing list <git@vger.kernel.org>
Subject: Re: [PATCH] format-patch: cancel useAutoBase if base is invalid
Date: Thu, 17 Sep 2020 12:26:20 -0700	[thread overview]
Message-ID: <CA+P7+xrvbwVevFaCB=cm4mZB4=nFAL2rs4gFgYF85qSopq+dwg@mail.gmail.com> (raw)
In-Reply-To: <xmqqr1r1tfil.fsf@gitster.c.googlers.com>

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

  parent reply	other threads:[~2020-09-17 20:23 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [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 [this message]
2020-09-18  0:04     ` 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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to='CA+P7+xrvbwVevFaCB=cm4mZB4=nFAL2rs4gFgYF85qSopq+dwg@mail.gmail.com' \
    --to=jacob.keller@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jacob.e.keller@intel.com \
    /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.
Code repositories for project(s) associated with this public inbox

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

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