Created attachment 311638 [details] [review]
Refactor operation merging (v1)

Hi Curtis,

In fixing the above I ended up refactoring operation merging.  Includes
changing some very old code (existed at the start of Git history of
GParted) which open codes merging of resize/move and format of not yet
created partitions with the pending create option into normal merge
operation calls.

Here's patchset v1.

Thanks,
Mike

Created attachment 311654 [details] [review]
Refactor operation merging (v2)

Hi Curtis,

Testing cases:

Operation           Partition   Merge
-----------------   ---------   ------
resize/move         REAL        MERGE_LAST_WITH_PREV
resize/move         NEW         MERGE_LAST_WITH_ANY
delete              NEW         MERGE_ALL_ADJACENT
format              REAL        MERGE_LAST_WITH_PREV
format              NEW         MERGE_LAST_WITH_ANY
check               REAL [1]    MERGE_LAST_WITH_ANY
label file system   REAL [1]    MERGE_LAST_WITH_ANY
name partition      REAL [1]    MERGE_LAST_WITH_ANY
new UUID            REAL [1]    MERGE_LAST_WITH_ANY

[1] These operations can only be applied to real partition, where as the
    operations can be applied to new, pending create partitions too.


When re-doing my testing I found a bug.  MERGE_LAST_WITH_ANY was
actually implementing MERGE_LAST_WITH_FIRST because of a stray semicolon
after the if condition and before the intended conditional break, making
it always break out the loop after trying to merge with the first
operation only.

Fixed in this patchset v2.

Thanks,
Mike

Created attachment 311665 [details] [review]
Refactor operation merging (v3)

Hi Curtis,

Here's patchset v3.

Compared to patchset v2, it just adds a code comment summarising the
merging rules and slightly updates another comment.

Thanks,
Mike

Hi Curtis,

Please feel free to test this when ready, but don't commit upstream just
yet.  I may have a few more minor comment and commit messages changes to
include.

Thanks,
Mike

Created attachment 311763 [details] [review]
Refactor operation merging (v4)

Hi Curtis,

Here's patchset v4.

Changes compares to v3 are:
* Remove unnecessary use of this-> referring to member variables in the
  Operation*::merge_operations() methods.
* Minor comment and commit message update.

Must try to stop messing about with this patchset now.

Thanks,
Mike

Hi Mike,

Thanks for working to migrate the merge functionality to the operation
class.  I agree that this is the correct place for the merge logic.

While testing patch set v4 from comment #5 I encountered a problem
with resize/move when there were two or more create new partitions in
the queue.  The problem is that one of the new partitions is lost from
the graphical display and list display of partitions.  Note that the
creation of both partition does remain in the job queue.

Following are queue steps to demonstrate the problem.

1)  Create a new ext2 partition (leave unallocated space after)

2)  Create a new ext4 partition (leave unallocated space between this
    and the other new partition).

3)  Resize the right end point of the ext2 partition (first new
    partition).

    *** Problem occurs here. ***

    Notice that the ext4 partition (second new partition) disappears
    from the graphical display and list display of partitions.

I also tested using the above steps with GParted 0.23.0 and the
display problem does not occur.  However, I did notice that GParted
0.23.0 appears to reverse the order of partition creation when these
operations are merged.  This would change the device path names due to
the change in order.

Thanks,
Curtis

Created attachment 312166 [details]
GParted operation merging and viewing

Hi Curtis,

Execelent job testing, catching this special case.

Bisected it to this patch (not supprising really):
Replace open coded merge of resize/move into create operation (#755214)

See the attached document for a semi-visual picture and more detailed
description of what is happening.  (I'm seeing old school Tetris S and Z
blocks before my eyes!)  Merging of operations has similarities to
rebasing code it Git.  Git has a sequence of ordered changes to code.
GParted has a sequence of ordered operations changing the disk.
Sometimes when reordering and merging Git commits manual intervention is
required to fix the outcome because Git detects clashing changes.  This
is the case with GParted here.  Operations have been merged and
reordered and the before partitions (original_partition in the
operation) no longer match the current state of the visual disk so
apply_to_visual() doesn't apply the effect of the operation and the
partition isn't shown.

In GParted 0.23.0 and earlier when resizing an new partition the effect
is to merge forward.  The original create of the ext2 partition is
removed and replaced with a new create a bigger ext2 partition, this
time after creation of the ext4 partition.

My patch did the reverse change.  It merged the resize of the ext2
into the initial create operation.  This made the layout of the disk to
which the second create an ext4 partition different to what it expected
and so matching the original partition failed, hence not visually
applying the operation.

There are two choices to fix this:
1) Update how the original_partition is matched when visually applying
   the operations, possibly just for the operation create; or
2) Implement merge forward as the original code does, instead of using
   merge backwards.

I'm going to do merge forwards.  It's how the old open coded merge
worked and I then don't need to risk breaking other visual apply
operations by changing original_partition matching.  Implementation
details still to be worked out.

There aren't any other problem cases with operation merging.  Problems
only occurs when the operation changes partition boundaries and which
can be merged over intermediate operations.  From the testing table in
comment #2 above  and basically the same information in the operation
merging rules comment being added to the code, only resize/move of a new
partition effects partition boundaries and merges over intermediate
operations.  No other merging rules match these criteria so can't be
affected.

Documenting this for the record.

Thanks,
Mike

Hi Curtis,


I've been thinking some more about this, triggered by you saying this
in comment #6:
> ...  However, I did notice that GParted 0.23.0 appears to reverse the
> order of partition creation when these operations are merged.  This
> would change the device path names due to the change in order.

So we want GParted to create the partitions in the order they were
created in the UI so that they get the same partition number ordering on
disk as shown in the UI (in this case of resizing an earlier create).


Reasoning:

Remember that every operation populates partition_original with the
before partition state and partition_new with the resultant state,
except for check and delete operations which only populate
partition_original with the before state.

The crux of the failure that you identified is that when GParted is
visually re-applying the pending operations to the disk graphic, it
is searching through the displayed partition vector to find the one
which matches partition_original (before state) so that this operation
can be visually applied.

The question is when visually re-applying operations is should it
identify the partition in the display vector by matching the
partition_original or partition_new, and in what cases does it matter?

* It only matters in operations where the boundaries of the partition
  are changed, because Operation::find_index_original() only matches on
  partition boundaries defined by sector_start and sector_end.

* OPERATION_CHECK, OPERATION_DELETE:
  For check and delete operations must match using partition_original
  as partition_new is not populated.

* OPERATION_CREATE:
  For the create operation, the problem case, matching using the
  unallocated partition_original in the operation doesn't work.  This is
  because the previous create operation has been merged creating a
  larger partition and leaving a smaller unallocated partition after.
  This smaller unallocated partition is not big enough to match with the
  partition_original in this create operation, even though it is big
  enough to hold the partition_new.  (Guaranteed by the the resize/move
  dialog not allowing the resize operation on the first partition to
  overlap with the already pending second create partition operation).

  Solution:
  For the create operation, match using partition_new.

* OPERATION_RESIZE_MOVE
  For a resize/move operation, if the match was done using
  partition_new, the after state, it would fail to match in cases when
  either the start or end moved outside the before partition.  This
  would prevent the operation re-applying to the visual.  Hence it must
  match using the before state, partition_original.

* OPERATION_FORMAT, OPERATION_COPY, OPERATION_LABEL_FILESYSTEM,
  OPERATION_CHANGE_UUID, OPERATION_NAME_PARTITION
  As mentioned above, for all the remaining cases it doesn't matter
  whether matching is done using the boundaries of the before state,
  partition_original, or after state, partition_new, because they are
  the same.


Summary:

So the create operation is special because we don't care whether the
unallocated before state, partition_original, recorded in the operation
matches, we only care that the newly created after partition fits, when visually re-applying the operation.

All the other operations modify existing partitions and matching should
be performed using partition_original, before state.

With this insight worked out I'll code it this way instead of what I
said in comment #7 above.


Thanks,
Mike

Hi Mike,

The case of creating more than one new partition and resizing an earlier partition is certainly a fringe case and not likely to occur very often.

I think that the ideal solution would preserve the order of partition creation.  However, if it is exceedingly difficult to code the solution as suggested in comment #8 then I would be okay if the GParted 0.23.0 behaviour was replicated.

When the patch is ready I will start testing anew.

Curtis

Created attachment 312237 [details] [review]
Refactor operation merging (v5)

Hi Curtis,

In the end fully understanding what the code was doing was the hard
part.  The above comment #7 and comment #8 are the result of that
understanding.  Changes to the code aren't very big or complicated in
the end.

So here's patchset v5 which fixes the issue with pending create
partition disappearing from the disk graphic.  It keeps the preferred
merge backwards so the create operations maintain their order.  It also
fixes the equivalent case when using copy/paste to create a partition,
and does a couple of associated tidyups.

Patchset v5 updates this commit message:

Replace open coded merge of format into create operation (#755214)

And adds these new 6 commits:

Fix visually re-applying create operation in create-create-grow-first sequence (#755214)
Share duplicate code substituting partitions in multiple operations (#755214)
Move code visually re-applying create operation into parent class (#755214)
Fix visually re-applying copy new operation in create-copy-grow-fist sequence (#755214)
Remove unused index parameter from Add_Operation() (#755214)
Replace Operation class members index and index_extended with local variables (#755214)

Thanks,
Mike

Hi Mike,

Thanks to your work the merge operations code continues to improve and
become easier to maintain.  :-)

All of my tests with the test cases mentioned in comment #2 have been
successful.  As such I am prepared to commit patch set v5.

Shall I commit patch set v5?

Or do you have more enhancements to include?


NOTE:  In patch set v5 I changed one small typo in the title of the
       11th patch:
          create-copy-grow-fist
       is now
          create-copy-grow-first


As a future enhancement (in a new bug report), we might consider
enabling merging of

  label file system, and
  name partition

on NEW partitions.  These are currently restricted to REAL partitions
in the UI, but I think it makes sense that we could change these label
and name values in a NEW partition prior to actually applying the
operation.

Curtis

Hi Curtis,


This is all the operation merging enhancements I have at this time.
Please push away.


Possibly future merging enhancements:

Note that the Create New Partition dialog already has these fields
(partition name and file system label) and the operation in the list
can be removed with the undo button and re-created with the desired
values.  However allowing these operations to be created on new
partitions and merged seems reasonable.  It's just that the user can
workaround it.

Closely related, it would be nice to allow name partition, label file
system and new UUID on a partition being created by copy/paste.  Note
this can already be done when copy/pasting into an existing partition,
just not when creating a new one.  These operations can't be merged
though as applying a copy won't know to change the UUID/label/name as
part of the same operation.

Anyway I'll leave these for a possible future update.


Mike

As agreed, patch set v5 from comment #10 has been committed to the git repository master branch for inclusion in the next release of GParted.

The relevant git commits can be viewed at the following links:

Better comment what each Merge_Operations() call is achieving (#755214)
https://git.gnome.org/browse/gparted/commit/?id=29a1f206885227bf8b4cc15b38668fb97e70de41

Encapsulate operation merging inside the Operation* classes (#755214)
https://git.gnome.org/browse/gparted/commit/?id=7f4ffd28d539a772258fe31cd5027dcd28706f20

Rename function to merge_two_operations() and update validation (#755214)
https://git.gnome.org/browse/gparted/commit/?id=d93d8abcc4943d0677ac11197336d35dc4b2dee9

Refactor merging rules into new merge_operations() (#755214)
https://git.gnome.org/browse/gparted/commit/?id=cf5d8d928c7fd8c1e2aad6e70dbb6de8c42f4a93

Replace open coded merge of format into create operation (#755214)
https://git.gnome.org/browse/gparted/commit/?id=3d21f0f192bd563b376473ba20a988aa36a70e32

Replace open coded merge of resize/move into create operation (#755214)
https://git.gnome.org/browse/gparted/commit/?id=5793c5ac03bea5b4818309e5755803b00feeb016

Add comment summarising current operation merging rules (#755214)
https://git.gnome.org/browse/gparted/commit/?id=27e5bbeeceff47539cbad1a3d228a7a0187c95cb

Fix visually re-applying create operation in create-create-grow-first sequence (#755214)
https://git.gnome.org/browse/gparted/commit/?id=9b497aae144ae5bf429de3e3d7d50bc41be0f3e0

Share duplicate code substituting partitions in multiple operations (#755214)
https://git.gnome.org/browse/gparted/commit/?id=27cbe36d0f4c33ada81527d7ec97cc92bf315475

Move code visually re-applying create operation into parent class (#755214)
https://git.gnome.org/browse/gparted/commit/?id=dc6ffc6a872c2aae80b14cae19ecc5d3513bda35

Fix visually re-applying copy new operation in create-copy-grow-first sequence (#755214)
https://git.gnome.org/browse/gparted/commit/?id=0e8f24b92b0a6ef497ed5db51a5d64a6acfdcccb

Remove unused index parameter from Add_Operation() (#755214)
https://git.gnome.org/browse/gparted/commit/?id=a1ab21285bb9ebe6080f4e99ac212ff72a829ec2

Replace Operation class members index and index_extended with local variables (#755214)
https://git.gnome.org/browse/gparted/commit/?id=cbfb7e51f50b36a7b190e8ade94ef0114f1fb3a8

Created attachment 312895 [details] [review]
Fix 'index' may be used uninitialised warning (v1)

Hi Curtis,

I have just noticed that the last commit "Replace Operation class
members index and index_extended with local variables (#755214)" missed
another case of the compiler warning that 'index' may be used
uninitialised.  This patch fixes that.

Thanks,
Mike

Hi Mike,

Using Fedora 22 and kubuntu 12.04 I confirmed that the fix patch v1 addressed the issue of the compiler warning message.

Fix patch v1 from comment #14 has been committed to the git repository.

The relevant git commit can be viewed at the following link:

Prevent 'index' may be used uninitialised warning in OperationDelete (#755214)
https://git.gnome.org/browse/gparted/commit/?id=81b673ff5d5eb0b1f98405a4e14a81a42873fece

Curtis

This enhancement was included in the GParted 0.24.0 release on October 27, 2015.