Bug 38852 – webkit-patch land --squash commits too much if branch is not up to date

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Ojan Vafai]

Specifically, if trunk has been updated (e.g. git svn fetch), but the branch hasn't been rebases/merged to trunk, then squash will commit the whole diff from trunk. We should do our best to detect this case and bail. I think a simple check that the top commit on trunk is present in the branch is sufficient.

This is almost definitely the cause of https://bugs.webkit.org/show_bug.cgi?id=38798.

[Eric Seidel (no email)]

Nah, we should easily be able to detect what common point the current branch has with trunk.  I dont' remember the git-fu, but I know it's possible.

Then squash diffs between that point and head.

[Evan Martin]

# first shared commit between two branches
git merge-base trunk mybranch

# list any revs that are on trunk that aren't on mybranch 
git rev-list trunk ^mybranch


I think Ojan's suggestion is less likely to break: if you attempt to automatically merge with trunk before squashing you'll have to deal with the potential for merge conflicts.  I think your best bet is something like:

if `git rev-list --limit 1 trunk ^mybranch`; then
  echo "trunk has commits not in this branch; please merge or rebase it and try again"
  exit 1
fi

[Ojan Vafai]

Created attachment 56128 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 56128 [details]
Patch

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:578
 +          if squash is None:
Why not "if not squash"?

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:579
 +              config_squash = Git.read_git_config('webkit-patch.squash')
Seems like we should have a helper method for this...  One which might even cache the result.

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:580
 +              if (config_squash and config_squash is not ""):
Yes, the helper method would return True/False.

Oh, I guess this sorta is the helper method.  But maybe it would be useful to have a helper which just handled reading from the config.

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:585
 +                  if num_local_commits > 1 or num_local_commits > 0 and not self.working_directory_is_clean():
parens here would help me a lot. :)

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:586
 +                      working_directory_message = "" if self.working_directory_is_clean() else " and working copy changes"
I would have made generating the message its own helper function.

[Ojan Vafai]

*** Bug 38274 has been marked as a duplicate of this bug. ***

[Ojan Vafai]

Created attachment 56294 [details]
Patch

[Ojan Vafai]

(In reply to comment #4)
> (From update of attachment 56128 [details])
> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:578
>  +          if squash is None:
> Why not "if not squash"?

None, True and False are different valid values. This will go away when we get rid of --squash and replace it with --git-commit=*

> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:579
>  +              config_squash = Git.read_git_config('webkit-patch.squash')
> Seems like we should have a helper method for this...  One which might even cache the result.
> 
> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:580
>  +              if (config_squash and config_squash is not ""):
> Yes, the helper method would return True/False.

I'm not changing this logic, just indenting the code. Again, this will change when we get rid of --squash.

> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:585
>  +                  if num_local_commits > 1 or num_local_commits > 0 and not self.working_directory_is_clean():
> parens here would help me a lot. :)

Done.

> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:586
>  +                      working_directory_message = "" if self.working_directory_is_clean() else " and working copy changes"
> I would have made generating the message its own helper function.

Done.

[Eric Seidel (no email)]

Comment on attachment 56294 [details]
Patch

I would have put these 3 lines in their own function:
 596             config_squash = Git.read_git_config('webkit-patch.squash')
 597             if (config_squash and config_squash is not ""):
 598                 squash = config_squash.lower() == "true"

Then the other code doesn't have to know about the empty value meaning false or true or whatever.


 603                     raise ScriptError(message=self._get_squash_error_message(num_local_commits))

ScriptErorr has historically meant "there was an error running the script".  Used by run_command.  I think you want some other type of exception here.  Maybe not?  I know our infrstrcture is set up to handle ScriptERrors well, maybe tthat's what your'e going for here?

I still think distingushing false from None doesn't help us here:
 595         if squash is None:

Unless "NOne" is supposed to mean true?

Is this really the way you want to do this?
     def _svn_branch_has_extra_commits(self):
 612         # When head contains all the commits on the svn branch, show-branch --topics will have exactly 4 lines.
 613         # One line for head, one for trunk, one for the "--" seprator and one for the branch point on trunk.
 614         trunk_commits = run_command(['git', 'show-branch', '--topics', 'head', self.svn_branch_name()])
 615         return len(re.findall('\n', trunk_commits)) != 4

Seems like a strange hack.  Maybe I'm mis-remembering evan's recomendation... if he had one?

This is better than what we have, but It feels like a hack.  Please consider the above comments, especially the one about the "4 line" hack.  Are we testing the 4-line hack?

[Ojan Vafai]

(In reply to comment #8)
>  603                     raise ScriptError(message=self._get_squash_error_message(num_local_commits))
> 
> ScriptErorr has historically meant "there was an error running the script".  Used by run_command.  I think you want some other type of exception here.  Maybe not?  I know our infrstrcture is set up to handle ScriptERrors well, maybe tthat's what your'e going for here?

Yeah, using another error type gives an ugly stacktrace. Using ScriptError gives a nice, readable error message.

> I still think distingushing false from None doesn't help us here:

The point is that if you explicitly pass --squash or --no-squash  on the commandline it should override any other settings. So it shouldn't check the git config or anything. If you don't explicitly set it, then we need to do the other checks. 

It's gross, I know. But it will go away shortly when we replace --squash with --git-commit=* as per the webkit-patch discussion.

> Is this really the way you want to do this?
>      def _svn_branch_has_extra_commits(self):
>  612         # When head contains all the commits on the svn branch, show-branch --topics will have exactly 4 lines.
>  613         # One line for head, one for trunk, one for the "--" seprator and one for the branch point on trunk.
>  614         trunk_commits = run_command(['git', 'show-branch', '--topics', 'head', self.svn_branch_name()])
>  615         return len(re.findall('\n', trunk_commits)) != 4
> 
> Seems like a strange hack.  Maybe I'm mis-remembering evan's recomendation... if he had one?
> 
> This is better than what we have, but It feels like a hack.  Please consider the above comments, especially the one about the "4 line" hack.  Are we testing the 4-line hack?

Yeah, I was just being dumb here. Changing it to use rev-list as evan suggested.

[Ojan Vafai]

Committed r59979: <http://trac.webkit.org/changeset/59979>