"GitHub Mac"-like behaviour for commit button #201

Closed
wants to merge 4 commits into
from

2 participants

@slobo

Added a "sync: toggle next to commit button that:

  • automatically stages ALL files (unmerged, merged, untracked) if none are staged
  • pull --rebase after commit
  • push after commit

This is as per wishlist #199

I'm really bundling potentially two separate features here, maybe it would be better to have a setting for the "idiot mode - just add everything to index if nothing selected" and have the sync toggle control only the push/pull aspect.

Also, there is potentially a lot of situations this doesn't cover, such as syncing an untracked branch, and also we assume git push to do a sensible thing rather than explicitly specifying the branch to push to.

And I'm thinking push/pull should display progress and not block UI, but couldn't figure out how to automate the RemoteActionDialogs.

(I'm not a python guy so I'm sure the code makes you cringe, please criticize and help me get better)

@davvid davvid commented on an outdated diff Sep 29, 2013
cola/widgets/commitmsg.py
@@ -228,6 +238,13 @@ def update_actions(self):
self.commit_button.setEnabled(commit_enabled)
self.commit_action.setEnabled(commit_enabled)
+ def update_commit_button_label(self, sync_button_is_checked):
+ if sync_button_is_checked:
+ self.commit_button.setText(N_('Commit + Sync@@verb'))
@davvid
git-cola member
davvid added a line comment Sep 29, 2013

You don't need @@verb` here -- that is only needed to differentiate single-word nouns from the same single-word verb (e.g. noun "commit" and verb "commit").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davvid davvid commented on the diff Sep 29, 2013
cola/widgets/commitmsg.py
- 'You must stage at least 1 file before you can commit.')
- if self.model.modified:
- informative_text = N_('Would you like to stage and '
- 'commit all modified files?')
- if not confirm(N_('Stage and commit?'),
- error_msg,
- informative_text,
- N_('Stage and Commit'),
- default=False,
- icon=save_icon()):
- return
+ if self.sync_toggle_button.isChecked():
+ # If sync is enabled, stage all changes to repo
+ cmds.do(cmds.StageModified)
+ cmds.do(cmds.StageUnmerged)
+ cmds.do(cmds.StageUntracked)
@davvid
git-cola member
davvid added a line comment Sep 29, 2013

I don't think it should stage untracked files. It's maybe useful for the first day of the project when all files are new, but it's not as helpful later, nor for folks who have untracked files in their worktree for whatever reason. This should be made conditional and guarded behind a cola.syncuntracked configuration option, which can default to false. See cola.displayuntracked and the commits that introduced it for an example of how to add it to the preferences dialog.

@davvid
git-cola member
davvid added a line comment Oct 1, 2013

Please rebase against master. I thought about this more. I changed the default in the prompt when nothing is staged so that it defaults to "Stage + Commit" instead of "Cancel".

I think this eliminates the need for the "Sync" mode to worry about staging since things will already get staged. You can type the commit message, hit "Ctrl+Enter" to commit, and then the prompt comes up. If you hit "Enter" again it'll go ahead and stage everything before committing. I think that behavior should stay as-is.

That nullifies what I said about cola.syncuntracked.. in that I really don't think we need it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davvid davvid commented on the diff Sep 29, 2013
cola/widgets/commitmsg.py
@@ -387,6 +410,31 @@ def commit(self):
N_('"git commit" returned exit code %s') %
(status,),
output)
+ return
+
+ if self.sync_toggle_button.isChecked():
+ # Proceed to pull/rebase and push
+ status, output = self.model.git.pull('--rebase',
+ with_stderr=True,
+ with_status=True)
@davvid
git-cola member
davvid added a line comment Sep 29, 2013

You can say rebase=True instead of --rebase, which frees this bit from needing to worry about getting the order of the positional arguments right. It's good that we catch the exit status from git pull --rebase; folks that use the topic-branch workflow will probably hit that case. This can be problematic when merging maintenance/topic branches, though. We don't preserve merges, so if you're on master and you've merged an old maint topic, the very next thing we do is pull --rebase which throws away the merge.

I don't think this should use git pull. Instead, git fetch $remote, and git rebase --preserve-merges $tracking_branch is probably the way to go. If $remote is not configured, nor $tracking_branch, then bail. I think that'll make the Sync mode useful for integrators that merge proper topic branches as well since we'll be preserving whatever merge the user intentionally made. Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davvid davvid commented on the diff Sep 29, 2013
cola/widgets/commitmsg.py
+ return
+
+ if self.sync_toggle_button.isChecked():
+ # Proceed to pull/rebase and push
+ status, output = self.model.git.pull('--rebase',
+ with_stderr=True,
+ with_status=True)
+ if status != 0:
+ Interaction.critical(N_('Pull --rebase failed'),
+ N_('"git pull --rebase" returned exit code %s') %
+ (status,),
+ output)
+ return
+ Interaction.log(output)
+ status, output = self.model.git.push(with_stderr=True,
+ with_status=True)
@davvid
git-cola member
davvid added a line comment Sep 29, 2013

git push $remote $local_branch:$remote_branch. $remote comes from branch.<name>.remote (e.g. git config branch.master.remote) and $remote_branch comes from branch.<name>.merge, with refs/heads/ chopped off.

If $local_branch == $remote_branch then use the simpler syntax without : between local and remote, e.g. git push origin master. The reason for doing this instead of blindly saying git push is that we do not know the value of push.default. git push can behave differently unless you're explicit.

It's might be worth moving all this logic into a class the cmds module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@slobo

Thanks for all the comments davvid, I'll look them over and make the appropriate changes

Slobodan Miskovic Remove unneeded @verb 5c79538
@slobo

What if we add 3 checkboxes to the "No staged files" dialog box:

Would you like to stage and commit all:
  [X] Modified files
  [ ] Unmerged files
  [ ] Untracked files

Then the choice can be persisted in the conf file.

@davvid
git-cola member

Having seen people commit files with merge markers, I'm a little cautious about making that too easy to mistakenly do. I would rather have the user verify each unmerged file individually to ensure that the files have indeed been merged. It's very easy to make a mistake. I don't think it's really that much to ask to have to manually stage these in the UI; there are plenty of UI interactions and keyboard shortcuts defined to make staging very easy (e.g. the vim-like hotkeys for navigation in the status view, ctrl+s to stage, multi-select, etc).

I have a similar issue with making the sync-mode touch untracked files. It blurs the line between untracked files and tracked files. Nor do I think that displaying a big "choose between these things" dialog helps either; we're just presenting the user with choices they shouldn't need to make.

I guess I don't understand why there's such a desire for the untracked files thing. Is it a "not understanding Git" thing, and something that new-to-git (or revision control) users would moreso desire, or is it a real need? There are plenty of ways for the user to convey that a file should be added to Git, and once added it stays there. There's also a config option to not show untracked files at all -- cola.displayuntracked. We should be careful to not stage stuff the user does not see in the UI.

Maybe we can get an initial version that simply adds the remote part of the "Sync" (fetch, rebase, push)? We could always change our minds about the unmerged and untracked details later, perhaps by making it configurable via cola.sync.unmerged and cola.sync.untracked if we really need it.

If so, it would be good to do anything related to the commit behavior as a separate follow-up commit, after the fetch/rebase/push "sync" changes. The cola.sync.* knobs should be settings in the Preferences dialog; if we provide knives for dropping on feet then I want the user to have to manually enable the sharper blades ;-) I hope that makes sense.

@slobo

Thanks, makes perfect sense. I should have given some background as to my motivation.

The workflow I'm trying to create is what GitHub client for Mac enables. After modifying code and switching to the GItHub Mac client, it is ready for me to quickly see what changed in the working set, and defaults to committing everything (tracked and untracked alike). This allows me to just type in the message and click "Commit and Sync" and change will be published on remote repo. If I see a file I don't want to commit, I can uncheck it, or add to .gitignore first and then commit everything leftover quickly.

This is great when working in a team, it allows for quick collaboration.

image

Of course, this is not ideal for everyone. Your suggestion of hiding such functionality in the config screen sounds good (fwiw, I was contemplating the same thing before I got the bright idea to clutter the everyday interface with it :).

I'll proceed like you suggested, being mindful of other config / display options, such as whether untracked files are displayed.

@davvid
git-cola member

Closing per #199

@davvid davvid closed this Nov 26, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment