Bug 104032 – commit-queue can get stuck with a local commit

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

commit-queue can get stuck with a local commit

[Adam Barth]

Created attachment 177536 [details]
Patch

[Dirk Pranke]

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?

[Ojan Vafai]

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?

[Adam Barth]

> 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.

[Adam Barth]

> 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.

[Adam Barth]

Comment on attachment 177536 [details]
Patch

Clearing flags on attachment: 177536

Committed r136583: <http://trac.webkit.org/changeset/136583>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.

[Dirk Pranke]

(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?

[Carlos Garcia Campos]

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.

[Ojan Vafai]

(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.

[Adam Barth]

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.  :(