toggle trailing whitespace in client.py update_nss/nspr must be based on old state

RESOLVED FIXED in mozilla25

Status

Firefox Build System
General
RESOLVED FIXED
5 years ago
5 months ago

People

(Reporter: kaie, Assigned: kaie)

Tracking

24 Branch
mozilla25

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
The change we had implemented in bug 782784 is insufficient.

The issue is that currently, the script toggles the trailing blank line AFTER we update.

The following can happen:
- checked in to mozilla: has blank line
- updating from nss: doesn't have a blank line
- toggle script: switches to add a blank line
- result: dummy file as checked in previously, no change

We must remember the state prior to updating from HG.
Then, after the update, we must check the new state.
Only if the state is identical, then we should toggle the trailing blank line.
(Assignee)

Comment 1

5 years ago
Created attachment 761369 [details] [diff] [review]
Patch v1

Below is an example output of executing the script with the attached patch.

$ python client.py update_nss NSS_3_15_1_BETA1
reverting to HG version of  security/nss/coreconf/coreconf.dep  to get its blank line state
Executing command: ['hg', 'revert', 'security/nss/coreconf/coreconf.dep']
no changes needed to security/nss/coreconf/coreconf.dep
old state of  security/nss/coreconf/coreconf.dep  is:  no blank line
Executing command: ['hg', 'clone', '-u', 'NSS_3_15_1_BETA1', 'https://hg.mozilla.org/projects/nss', './security/nss']
requesting all changes
adding changesets
adding manifests
adding file changes
added 10811 changesets with 34489 changes to 6292 files (+391 heads)
updating to branch default
2500 files updated, 0 files merged, 0 files removed, 0 files unresolved
new state of  security/nss/coreconf/coreconf.dep  is:  has blank line
Attachment #761369 - Flags: review?(wtc)
(Assignee)

Comment 2

5 years ago
In the previous comment, no change was necessary after the update.
Here is another example, where the script actually performs the final step of adding a blank line:

$ python client.py update_nspr NSPR_4_9_4_RTM
reverting to HG version of  nsprpub/config/prdepend.h  to get its blank line state
Executing command: ['hg', 'revert', 'nsprpub/config/prdepend.h']
old state of  nsprpub/config/prdepend.h  is:  no blank line
Executing command: ['hg', 'clone', '-u', 'NSPR_4_9_4_RTM', 'https://hg.mozilla.org/projects/nspr', './nsprpub']
requesting all changes
adding changesets
adding manifests
adding file changes
added 4470 changesets with 14263 changes to 972 files (+395 heads)
updating to branch default
646 files updated, 0 files merged, 0 files removed, 0 files unresolved
new state of  nsprpub/config/prdepend.h  is:  no blank line
adding blank line to:  nsprpub/config/prdepend.h
(Assignee)

Comment 3

5 years ago
Created attachment 761373 [details] [diff] [review]
Patch v2
Attachment #761369 - Attachment is obsolete: true
Attachment #761369 - Flags: review?(wtc)
Attachment #761373 - Flags: review?(wtc)

Comment 4

5 years ago
Comment on attachment 761373 [details] [diff] [review]
Patch v2

Review of attachment 761373 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. Thanks.

::: client.py
@@ +114,5 @@
> +    do_hg_replace(destination, hgpath, tag, HG_EXCLUSIONS, options.hg)
> +    new_state = get_trailing_blank_line_state(depfile)
> +    print "new state of ", depfile, " is: ", new_state
> +    if old_state == new_state:
> +        print "adding blank line to: ", depfile

The comment should say "toggling blank line in: " because we may delete the
blank line if the state is "has blank line".
Attachment #761373 - Flags: review?(wtc) → review+

Comment 5

5 years ago
Comment on attachment 761373 [details] [diff] [review]
Patch v2

Review of attachment 761373 [details] [diff] [review]:
-----------------------------------------------------------------

::: client.py
@@ +106,5 @@
> +  else:
> +      return "no blank line"
> +
> +def update_nspr_or_nss(tag, depfile, destination, hgpath):
> +    print "reverting to HG version of ", depfile, " to get its blank line state"

It seems that Python adds a space between the print arguments separated
by commas. You can see the extra spaces in the sample outputs you provided.

So I think this line should be:
    print "reverting to HG version of", depfile, "to get its blank line state"
and similarly for the other print statements in this function.

I also noticed that some functions in this file seem to indent by 2 (and then
4) but this function indents by 4.

Comment 6

5 years ago
Comment on attachment 761373 [details] [diff] [review]
Patch v2

Review of attachment 761373 [details] [diff] [review]:
-----------------------------------------------------------------

::: client.py
@@ +113,5 @@
> +    print "old state of ", depfile, " is: ", old_state
> +    do_hg_replace(destination, hgpath, tag, HG_EXCLUSIONS, options.hg)
> +    new_state = get_trailing_blank_line_state(depfile)
> +    print "new state of ", depfile, " is: ", new_state
> +    if old_state == new_state:

I have another solution that doesn't require old_state and new_state.

After do_hg_replace(), we do
  hg diff depfile

If the hg diff command has no output, then we toggle the trailing blank line.
FWIW it would be more Pythonic to write this using string interpolation, like:
print "reverting to HG version of %s to get its blank line state" % depfile
(Assignee)

Comment 8

5 years ago
Created attachment 766632 [details] [diff] [review]
patch v3

This addresses comments 4, 5, 7.

I don't want to use the proposal from comment 6, because the existing solution seems to work, and I don't see a need to spend time to re-test the alternatively proposed solution.
Attachment #761373 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/0b06da71de7e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25

Updated

5 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.