git-format-patch useAutoBase (was Re: [PATCH v2] migration: Handle block ... )

[Juan Quintela <quintela@redhat.com>]

Eric Blake <eblake@redhat.com> wrote:
> On Thu, Apr 20, 2023 at 12:41:25PM +0200, Juan Quintela wrote:
>> Eric Blake <eblake@redhat.com> wrote:
>
> ...lots of lines...
>
>> > ---
>> >  migration/migration.c | 5 ++---
>> >  1 file changed, 2 insertions(+), 3 deletions(-)
>
> ...describing a tiny change ;)
>
>> >
>> > diff --git a/migration/migration.c b/migration/migration.c
>> > index bda47891933..cb0d42c0610 100644
>> > --- a/migration/migration.c
>> > +++ b/migration/migration.c
>> > @@ -3444,13 +3444,11 @@ static void migration_completion(MigrationState *s)
>> >                                              MIGRATION_STATUS_DEVICE);
>> >              }
>> >              if (ret >= 0) {
>> > +                s->block_inactive = inactivate;
>> >                  qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX);
>> >                  ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
>> >                                                           inactivate);
>> >              }
>> > -            if (inactivate && ret >= 0) {
>> > -                s->block_inactive = true;
>> > -            }
>> >          }
>> >          qemu_mutex_unlock_iothread();
>> 
>> And I still have to look at the file to understand this "simple" patch.
>> (simple in size, not in what it means).
>
> Indeed - hence the long commit message!
>
>> 
>> I will add this to my queue, but if you are in the "mood", I would like
>> to remove the declaration of inactivate and change this to something like:
>> 
>>              if (ret >= 0) {
>>                  /* Colo don't stop disks in normal operation */
>>                  s->block_inactive = !migrate_colo_enabled();
>>                  qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX);
>>                  ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
>>                                                           s->block_inactive);
>>              }
>> 
>> Or something around that lines?
>
> Yes, that looks like a trivial refactoring that preserves the same
> semantics.
>
>> 
>> > @@ -3522,6 +3520,7 @@ fail_invalidate:
>> >          bdrv_activate_all(&local_err);
>> >          if (local_err) {
>> >              error_report_err(local_err);
>> > +            s->block_inactive = true;
>> >          } else {
>> >              s->block_inactive = false;
>> >          }
>> > base-commit: 7dbd6f8a27e30fe14adb3d5869097cddf24038d6
>> 
>> Just wondering, what git magic creates this line?
>
> git send-email --base=COMMIT_ID
>
> or even 'git config format.useAutoBase whenAble' to try and automate
> the use of this.  (If my own git habits were cleaner, of always
> sticking patches in fresh branches, --base=auto is handy; but in
> practice, I tend to send one-off patches like this in the middle of
> 'git rebase' of a larger series, at which point I'm not on a clean
> branch where --base=auto works, so I end up having to manually specify
> one at the command line.  Either way, including the base-commit: info
> can be very informative for applying a patch at the branch point then
> rebasing it locally, when attempting to apply the patch sent through
> email hits merge conflicts when applying it directly to changes on
> master in the meantime; I believe 'git am -3' is even able to exploit
> the comment when present to make smarter decisions about which parent
> commit it tries for doing 3-way patch resolution)

Thanks a lot.

It does the right thing for "trivial" stuff, i.e. when I sent a single
patch or a series against qemu/master.

I am not completely sure that it does the right thing when I send a
series on top of my previous pull request.

097387873b (HEAD -> atomic_counters) migration: Make dirty_bytes_last_sync atomic
3f699a13b2 migration: Make dirty_pages_rate atomic
276a275895 (next) migration: Move qmp_migrate_set_parameters() to options.c
ab13b47801 migration: Move migrate_use_tls() to options.c
ecf5c18eac MAINTAINERS: Add Leonardo and Peter as reviewers
6e5dda696c migration: Disable postcopy + multifd migration
ac5f7bf8e2 (qemu/staging, qemu/master, qemu/HEAD) Merge tag 'migration-20230424-pull-request' of https://gitlab.com/juan.quintela/qemu into staging

where the branchs are:

qemu/master: obvious
next: whatever is going to be on next migration PULL request, I will
      rename this to migration-$date and send this series to the list
      1st. I.e. assume they are on list but still not on master.
HEAD/atomic_counters: This is the series that I am sending

I have done:


>>                  ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
>>                                                           s->block_inactive);
>>              }
>> 
>> Or something around that lines?
>
> Yes, that looks like a trivial refactoring that preserves the same
> semantics.
>
>> 
>> > @@ -3522,6 +3520,7 @@ fail_invalidate:
>> >          bdrv_activate_all(&local_err);
>> >          if (local_err) {
>> >              error_report_err(local_err);
>> > +            s->block_inactive = true;
>> >          } else {
>> >              s->block_inactive = false;
>> >          }
>> > base-commit: 7dbd6f8a27e30fe14adb3d5869097cddf24038d6
>> 
>> Just wondering, what git magic creates this line?
>
> git send-email --base=COMMIT_ID
>
> or even 'git config format.useAutoBase whenAble' to try and automate
> the use of this.  (If my own git habits were cleaner, of always
> sticking patches in fresh branches, --base=auto is handy; but in
> practice, I tend to send one-off patches like this in the middle of
> 'git rebase' of a larger series, at which point I'm not on a clean
> branch where --base=auto works, so I end up having to manually specify
> one at the command line.  Either way, including the base-commit: info
> can be very informative for applying a patch at the branch point then
> rebasing it locally, when attempting to apply the patch sent through
> email hits merge conflicts when applying it directly to changes on
> master in the meantime; I believe 'git am -3' is even able to exploit
> the comment when present to make smarter decisions about which parent
> commit it tries for doing 3-way patch resolution)

Thanks a lot.

It does the right thing for "trivial" stuff, i.e. when I sent a single
patch or a series against qemu/master.

I am not completely sure that it does the right thing when I send a
series on top of my previous pull request.

097387873b (HEAD -> atomic_counters) migration: Make dirty_bytes_last_sync atomic
3f699a13b2 migration: Make dirty_pages_rate atomic
276a275895 (next) migration: Move qmp_migrate_set_parameters() to options.c
ab13b47801 migration: Move migrate_use_tls() to options.c
ecf5c18eac MAINTAINERS: Add Leonardo and Peter as reviewers
6e5dda696c migration: Disable postcopy + multifd migration
ac5f7bf8e2 (qemu/staging, qemu/master, qemu/HEAD) Merge tag 'migration-20230424-pull-request' of https://gitlab.com/juan.quintela/qemu into staging

where the branchs are:

qemu/master: obvious
next: whatever is going to be on next migration PULL request, I will
      rename this to migration-$date and send this series to the list
      1st. I.e. assume they are on list but still not on master.
HEAD/atomic_counters: This is the series that I am sending


I have done first:

git branch --set-upstream-to=qemu/master

Because that is what the "real" master is, migration-$date is just an
intermediate "state".

git format-patch --cover-letter next -o /tmp/kk

In this case both useAutoBase=whenAble and useAutoBase=true do the same
"right" thing.

From 097387873b2ef1894d5713fdfda8a7b2492476e5 Mon Sep 17 00:00:00 2001
...

Juan Quintela (2):
  migration: Make dirty_pages_rate atomic
  migration: Make dirty_bytes_last_sync atomic

 migration/migration.c | 10 +++++++---
 migration/ram.c       |  8 +++++---
 migration/ram.h       |  4 ++--
 3 files changed, 14 insertions(+), 8 deletions(-)


base-commit: ac5f7bf8e208cd7893dbb1a9520559e569a4677c
prerequisite-patch-id: 08dd37c2ffd8463398e00cade80765b017200b68
prerequisite-patch-id: 3a1d25d5e4f1f615b6e2c6749dcf891959ca48b5
prerequisite-patch-id: 5607c75cc228370df8953987c390682de3093b65
prerequisite-patch-id: ccb4d94973bd111380e4b50f781eeb6cfa7ce5ff

In obvious cases I do the rebase on top of qemu/master, but that is when
there is no dependencies with the PULL request, and that is not the
"interesting" case.

Thanks again, Juan.