Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Show diff with HEAD~ in Staged Changes list when amending - #259

Merged
maxbrunsfeld merged 10 commits into
masterfrom
mb-show-head-when-amending
Sep 28, 2016
Merged

maxbrunsfeld merged 10 commits into
masterfrom
mb-show-head-when-amending

Conversation

@maxbrunsfeld

@maxbrunsfeld maxbrunsfeld commented Sep 23, 2016 •

Copy link
Copy Markdown
Contributor

Closes #256

I put this together so we could discuss the merits of this UX. Personally, I think it's a lot more intuitive to show the commit that you're editing, but maybe that's because I used Gitx for a while.

GIF showing the new interaction below

/cc @kuychaco @BinaryMuse @simurai @nathansobo

This method does no caching for now.
@maxbrunsfeld

maxbrunsfeld commented Sep 23, 2016 •

Copy link
Copy Markdown
Contributor Author

new-amend

The lag after unchecking the box is me confirming the discard the current commit message dialog box. As a side note, I think we should probably remove that dialog box when unchecking 'amend', and just keep the message only if they've changed it since checking the box.

EDIT - I've now tweaked the dialog so that we only use it when there's actually a chance of losing information.

@maxbrunsfeld
maxbrunsfeld force-pushed the mb-show-head-when-amending branch from 88e8505 to 59d6c95 Compare September 23, 2016 18:41
@maxbrunsfeld
maxbrunsfeld force-pushed the mb-show-head-when-amending branch from 59d6c95 to ff21ed8 Compare September 23, 2016 18:54
@simurai

simurai commented Sep 24, 2016

Copy link
Copy Markdown
Contributor

I tried a couple other clients: Tower, GitKraken, SourceTree, Fork, GitUp. And they all behave like current master. So only GitX does it like this PR.

That said, I'm not totally against merging because it adds some extra flexibility that is not possible to do in another way. Once/if we add a commit history tough, I think it's more clear to separate amend and reset into two separate actions.

tl;dr: Merge now, revert later (once a reset option is available)

@nathansobo

Copy link
Copy Markdown
Contributor

Basically I won't be able to stop using GitX if we don't support this, which would be pretty sad. I don't see what we lose by enabling this use case.

@maxbrunsfeld
maxbrunsfeld force-pushed the mb-show-head-when-amending branch from 1960675 to 87c472f Compare September 24, 2016 05:00
@maxbrunsfeld

maxbrunsfeld commented Sep 24, 2016 •

Copy link
Copy Markdown
Contributor Author

I really think this UX is much better, not just because Gitx does it. Suppose you want to remove some changes from a commit that you just created, but you still want those changes in your working copy. Before, you'd have to manually undo those changes, save the file, stage the undone changes, and then redo the changes manually. Now you can simply edit the commit and remove the changes, without ever changing your working copy:

new-amend-ux-3

@maxbrunsfeld

maxbrunsfeld commented Sep 25, 2016 •

Copy link
Copy Markdown
Contributor Author

I also just think it makes more sense on a conceptual level. When a UI lets you edit some object, you should have a full view of the object, the same view that you had when you were first creating the object. With this behavior, when you amend a commit you have a full view of its contents just like you did when creating it.

If for whatever reason you want to see your staged changes since the last commit, you still can: simply uncheck the Amend box.

@maxbrunsfeld

maxbrunsfeld commented Sep 25, 2016 •

Copy link
Copy Markdown
Contributor Author

@simurai Regarding Reset, I'm wondering if we even need a UI for reset --soft if we allow other forms of history manipulation like commit squashing and reordering via drag-and-drop. Curious to hear your thoughts on the use cases for Reset. Now I'm really excited to talk about this stuff next week.

@simurai

simurai commented Sep 25, 2016 •

Copy link
Copy Markdown
Contributor

@maxbrunsfeld Just hit that green button already and stop suffering! 😄 Unless @kuychaco is really against it.

#259 (comment) are really good arguments for it.

Regarding Reset, I'm wondering if we even need a UI for reset --soft if we allow other forms of history manipulation like commit squashing and reordering via drag-and-drop.

Ya, if commit squashing allows to rename the commit, it's almost the same. Well, with reset, you could "squash" a couple of your last commits and then split them up differently into multiple commits again. I think as long as it's just behind a context menu, it wouldn't hurt having the option.

@kuychaco

Copy link
Copy Markdown
Contributor

@maxbrunsfeld Just hit that green button already and stop suffering! 😄 Unless @kuychaco is really against it.

heh I'm actually totally in favor of this feature, but just a slightly tweaked version of it =P

@maxbrunsfeld makes very good points and I'm totally sold on the idea behind this workflow. In fact, I do this sort of thing all the time, but my version consists of using reset on the command line.

The thing that I find a bit off about the current implementation is the fact that the Staged Changes list does not show files that have changed relative to HEAD as it normally would, and the fact that we've introduced complexity into our code in order to do this. We are conditionally showing the files that have changed relative to HEAD~ (its parent), and we now deviate from what Git would tell us if we typed git status on the command line. Which isn't thaaat big of a deal, and I can imagine there will come a time when we're discussing some killer new feature that requires some hand-waving and it might be worth getting a bit out of sync with Git. But I think for this particular feature there's a better way to give users the same general UX, but in a slightly different way that allows us to accurately reflect the state of Git, and without complicating our code to handle this conditional case and potentially creating hiding places for bugs.

I'll make a recommendation for tweaking this particular PR for now, and then discuss why I think we should eventually just do away with this whole "amend" checkbox entirely once we've built out a more comprehensive UI.

Let's take a step back and think about what we're actually trying to do here. The user has made a commit and then realized that they want to make a change to that commit's content. So basically they want to go back in time to right before they hit the commit button. Well, the way to get back to this state on the command line is to type git reset --soft HEAD~. So...

For now

To achieve the workflow desired with this PR, I suggest we make it such that checking the amend box does a git reset --soft HEAD~. This changes HEAD to point to the parent of the commit we want to amend, but doesn't change the index or working directory, making the changes introduced in the previous commit appear as staged changes. We store off the sha of what HEAD used to be and if the user wants to back out of this and un-checks the amend box, then we do a soft reset back to that sha. And again, the index and working directory remain untouched since we're just moving HEAD. We can still populate the commit box with the previous commit's message, and we end up with the same exact UX as is currently implemented. We achieve the same goal, but with less complicated code and we never get out of sync with Git. But eventually, I would like to get to a place where the amend checkbox is entirely unnecessary...

The future

Originally, the amend checkbox was created to mirror the git commit --amend command. But eventually such a thing should not be needed in Atom. Once we've built out @simurai's awesome commit history UI, if the user wants to edit the contents of any given commit, all they'll need to do is right-click on it and select "edit". And this could be the case for any commit, even if it's not the most recent one. There's no need to treat the last commit as special and give it it's own checkbox.

Here is where we can do better than Git's command line porcelain and create our own intuitive graphical equivalent. The user will be able to easily interact with their commit graph to edit, squash, delete, and re-arrange commits.

The vision

@nathansobo said it best in his Graphical Porcelain narrative:

Once we've given users a way to visualize the commit graph, the next step is to make it easy for them to reach out and manipulate it. This is where full control of the text editor starts to show its advantages...So by thinking of Atom as graphical Git porcelain, we unlock the true power of interacting with a Git-aware environment. It's not about editing code, then telling Git about our edits. It's about editing Git's data structures directly.

And when this ✨ dream ✨ becomes the reality for Atom, I say we 🔥 that ✅.

/cc @iolsen for thoughts on timing and milestone placement

@maxbrunsfeld
maxbrunsfeld merged commit 2e14bd8 into master Sep 28, 2016
@maxbrunsfeld
maxbrunsfeld deleted the mb-show-head-when-amending branch September 28, 2016 19:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants