Re: [PATCH 2/2] am: Allow specifying a base to check applying series

From: Konstantin Ryabitsev <konstantin@linuxfoundation.org>
To: Rob Herring <robh@kernel.org>
Cc: tools@linux.kernel.org
Subject: Re: [PATCH 2/2] am: Allow specifying a base to check applying series
Date: Tue, 22 Jun 2021 10:23:24 -0400	[thread overview]
Message-ID: <20210622142324.kotqghzxxro736fl@nitro.local> (raw)
In-Reply-To: <CAL_JsqKntMMa7PjkxJQjK+YTykEtLbpMc=Hj+YhHCbMGguQ+EA@mail.gmail.com>

On Mon, Jun 21, 2021 at 05:07:34PM -0600, Rob Herring wrote:
> > I just pushed a reimplementation of --guess-base into master -- can you see if
> > it does closer to something that you want?
> 
> Yeah, it works. It's a bit confusing that the result is 'Base: current
> tree' when it's not HEAD.

I've added some refinements that will hopefully improve both the matching
algorithm and the output when using -b.

> The more I think about it, the more I think '-b' needs to be a list of
> refs or should be a separate sub-command and the user can iterate thru
> branches themselves. It would work as-is, but it's kind of pointless
> to repeatedly go thru all the 'b4 am' steps each time. A separate
> sub-command would keep with the unix way of commands that do 1 thing.

By default, we'll use --all when matching blob indexes, but if -b is
specified, we'll pass it as-is to --branches, which is actually a pattern (see
"man git-log").

> I don't know that the guessing algorithm is any better now. When I ran
> it with my current tree being something a series doesn't apply to, it
> gave me some pretty arbitrary ref: v5.13-rc1-69-gcfe34bb7a770 (which I
> think happens to be the base I just did a rebase on)

I believe I've improved it in the latest commit.

NB: I think it's important to recognize that "all blob indexes matched" is not
a guarantee that the base we find is actually accurate or useful, at least not
for the purposes of CI -- a patch may be dependent on another set of changes
that modify totally different blobs than in the patch we're looking at.

E.g.: imagine that you are looking at a patch that changes files lib/foo.c and
lib/bar.c, but these changes are dependent on lib/baz.c. If you apply a patch
that modifies lib/baz.c, then the indexes for foo.c and bar.c would still
match the current tree, but the correct base-commit would actually be *before*
the baz.c modification.

We already try to handle this situation by not considering changes applied
after the patch was submitted (we force --until to be the date of the patch),
but providing an actual base-commit info in the patch/series is always going
to be more precise and reliable.

-K