commit-queue can get stuck with a local commit
Created attachment 177536 [details] Patch
Comment on attachment 177536 [details] Patch As discussed over IRC, I think this patch is okay, but "clean_working_directory" means something different than "reset to merge base" to me ... maybe we should rename this command/method to reset_to_master() or something?
Comment on attachment 177536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177536&action=review > Tools/Scripts/webkitpy/tool/steps/commit.py:-53 > - working_directory_message = "" if error.working_directory_is_clean else " and working copy changes" > - return ('There are %s local commits%s. Everything will be committed as a single commit. ' > + return ('There are %s local commits (and possibly changes in the working directory. ' > + 'Everything will be committed as a single commit. ' > 'To avoid this prompt, set "git config webkit-patch.commit-should-always-squash true".' % ( > - error.num_local_commits, working_directory_message)) I'm confused. We can still hit this if someone is committing locally. Why change this?
> I'm confused. We can still hit this if someone is committing locally. Why change this? Just because the semantics of working_directory_is_clean have changed slightly. It now tells you whether there are any changes relative to the merge base, so we don't know if there are uncommitted changed in the working copy.
> As discussed over IRC, I think this patch is okay, but "clean_working_directory" means something different than "reset to merge base" to me ... maybe we should rename this command/method to reset_to_master() or something? Interesting idea. The trickiness is that we need a name that makes sense for SVN too.
Comment on attachment 177536 [details] Patch Clearing flags on attachment: 177536 Committed r136583: <http://trac.webkit.org/changeset/136583>
All reviewed patches have been landed. Closing bug.
(In reply to comment #5) > > As discussed over IRC, I think this patch is okay, but "clean_working_directory" means something different than "reset to merge base" to me ... maybe we should rename this command/method to reset_to_master() or something? > > Interesting idea. The trickiness is that we need a name that makes sense for SVN too. Yeah, dunno ... If we can't come up with a better name we should at least add a comment or a docstring describing the intent of the method (to prepare the checkout so that we can update cleanly) ... maybe clean_for_update() or prepare_for_update() or something?
This makes webkit-patch land -g HEAD to fail complaining that working copy is not clean, even though it's clean. The problem is that working_directory_is_clean() not gets the diff between HEAD and remote_merge_base(), which is the commit you want to land.
(In reply to comment #4) > > I'm confused. We can still hit this if someone is committing locally. Why change this? > > Just because the semantics of working_directory_is_clean have changed slightly. It now tells you whether there are any changes relative to the merge base, so we don't know if there are uncommitted changed in the working copy. I see. It's unfortunate losing this extra information about why the commit failed. I've hit this before and it's saved me debugging time. Also, really we should probably rename working_directory_is_clean to checkout_is_clean or something like that and clean_working_directory to clean_checkout maybe? I agree w Dirk. The code is now confusing.
I filed bug 104198 about this issue. Hopefully I'll get a patch written tomorrow, but it might need to wait until Jan. Sorry for making this code more confusing. :(