Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Space cleanup, UI consistency #849

Closed
wants to merge 3 commits into from
Closed

Space cleanup, UI consistency #849

wants to merge 3 commits into from

Conversation

ehem
Copy link
Contributor

@ehem ehem commented Aug 29, 2019

Right now I've got two commits in this. The first is simply find * -type f -print0 | xargs -0 sed -i -es/\[\[:space:\]\]\\+\$// followed by checking to ensure nothing was corrupted by the changes (the test files include sample outputs which can have end of line spaces). The second is simply aiming for consistency in the commands inside fdisk, this distinctly improves user friendliness.

I'm also looking at an issue with the behavior of fdisk when dealing with hybrid MBRs. The current behavior is pretty broken. No MBR actually has to conform to some of the limitations used by fdisk. In most cases 1MB alignment is good, but not required by any other software. When making a hybrid you may want to deliberately violate normal practice...

The case I'm trying to do, I want one entry from the GPT to appear in the MBR, but everything else to be covered by protective entries. The protective entry covering the begining GPT and the early area needs to start at block 1, which the 'M' menu is troublesome about creating. If the entry being manifested in the MBR starts at block 2048, and the 0xEE entry covering everything after is created first, you get an error about no free space.

The third commit solves the issue, but I suspect it produces broken behavior for other situations (notably someone actually using the traditional MBR).

@ehem ehem changed the title Push Space cleanup, UI consistency Aug 29, 2019
@ehem
Copy link
Contributor Author

ehem commented Aug 29, 2019

This looks like someone forgot to update test output and the current official master is failing. My master branch has the same commits on top, but until the new test output is confirmed correct these rebased backwards ones are going to be on the GitHub pull request (feel free to grab from my master branch instead).

@ehem
Copy link
Contributor Author

ehem commented Aug 29, 2019

Okay, seems the tests are a bit more capable than I thought, so now I get to figure out how to run them locally on my machine and correct the test output...

@karelzak
Copy link
Collaborator

Ignore the tests on travis for now, we need to fix it -- the latest tests changes (unbuffered output) makes it useless on some systems. I'm going to work on it in next days.

@ehem
Copy link
Contributor Author

ehem commented Aug 29, 2019

The thing which had me puzzled was I was following the links expecting it to test at the tip of my branch (1128da7) whereas instead first a merge is being generated (56df006) and then Travis CI is testing that instead.

I was concerned I might have genuinely broken a test since really thorough testing could notice changing the 'M' to get back from the hybrid MBR menu to an 'r'. Ideally testing would be sensitive to things like this.

@ehem
Copy link
Contributor Author

ehem commented Aug 30, 2019

Having played with this a bit more, there needs to be a message when returning from this menu that changes have not been written to disk and you must leave the menu with "w" in order to write changes to the hybrid MBR to storage media. Without this someone can leave this menu and think exiting there by hitting "w" will write all changes (as occurs with the expert menu).

@ehem
Copy link
Contributor Author

ehem commented Aug 30, 2019

Grrf! Stupid cut&paste adding an extra newline, corrected.

Sorry detail-oriented people tend to wipe these out if they notice them.
Add in automated tools and lots of excess end-of-line spaces get wiped
out.
Returning from expert and BSD label editing modes both used 'r'.  The
protective/hybrid MBR menu was the only one using 'M'.  Consistency is
highly valuable in user-interface design.
The traditional MBR has pretty well NO limitations on slices.  They can
be a single misaligned sector if desired.  While this is undesireable
for most real world uses, for the few places they're still used extra
limitations cause breakage not safety.
@ehem
Copy link
Contributor Author

ehem commented Sep 4, 2019

I'm unsure how you prefer commits from pulled repositories. I've been rebasing forwards, but also refining a bit.

I suspect the logic in add_partition() could be further simplified, but I'm unsure of exactly how that is supposed to work and thus I worry I'll break things if I change too much.

@karelzak
Copy link
Collaborator

karelzak commented Sep 17, 2019

The patch "fdisk: Make 'r' consistent in returning from submenus" is backwardly incompatible, we cannot change the keys.

EDIT: but we can add an alias I guess. You're right it's not intuitive.

@karelzak
Copy link
Collaborator

Ah, I see why we have 'M' and 'r' as separate keys. In the 'M' (nested) you can still enter expert mode as for standard MBR and in expert mode you can still leave nested mode:

Command (m for help): M
Entering protective/hybrid MBR disklabel.

Command (m for help): x

Expert command (m for help): m

Help (expert commands):

  DOS (MBR)
   b   move beginning of data in a partition
   i   change the disk identifier
   M   return from protective/hybrid MBR to GPT           <----

  Geometry (for the current label)
   c   change number of cylinders
   h   change number of heads
   s   change number of sectors/track

  Generic
   p   print the partition table
   v   verify the partition table
   d   print the raw data of the first sector from the device
   D   print the raw data of the disklabel from the device
   f   fix partitions order
   m   print this menu

  Save & Exit
   q   quit without saving changes
   r   return to main menu                              <----

You're editing nested 'dos' partition table, primary partition table is 'gpt'.

Expert command (m for help): 

And now go back from nested expert mode to the primary partition table main menu:

Expert command (m for help): M
Leaving nested disklabel.

Command (m for help): 

@ehem
Copy link
Contributor Author

ehem commented Sep 17, 2019

The fact that you had to play with it for an hour to figure out why it was that way suggests few people are ever likely to take advantage of that. I hadn't the faintest idea one could transition from the hybrid expert menu to the GPT expert menu with a single command. I've been using fdisk for years and I never looked for a way to do that transition, I would have expected to do 'r', 'r', 'x'. I think you'll need to search really hard to find someone else who knows this is doable. At which point I'm forced to argue in favor of the intuitive option, instead of preserving a very recently added whacky option.

karelzak added a commit that referenced this pull request Sep 30, 2019
The current code uses 'M' to switch between MBR and GPT, but it's not
intuitive to "go back" by 'M'. It seems more user-friendly to use 'r'
as in another places (for example when go from expert menu or from BRD
menu).

The 'M' to return to GPT is still supported for backward compatibility.

Addresses: #849
Signed-off-by: Karel Zak <kzak@redhat.com>
karelzak pushed a commit that referenced this pull request Oct 1, 2019
Sorry detail-oriented people tend to wipe these out if they notice them.
Add in automated tools and lots of excess end-of-line spaces get wiped
out.

Addresses: #849
Signed-off-by: Karel Zak <kzak@redhat.com>
@karelzak
Copy link
Collaborator

karelzak commented Oct 1, 2019

I have merged the "fdisk: Correct handling of hybrid MBR" and "cleanup: Remove some spurious spaces" patches. I have also fixed issues related to the 'w'rite command for hybrid MBR.

Now all the issues should be fixed. Thanks!

@karelzak karelzak closed this Oct 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants