Page MenuHomePhabricator

flags: introduce explicit testing for merging change to exec flag
ClosedPublic

Authored by marmoute on Sat, May 16, 4:11 PM.

Details

Summary

It turns out that we do not seems to test the simple case for merging exec flag
changes. More advanced case are test (merging exec flag without a common
ancestors, merging with a symlink, etc…) but not the basic.

We are about introduce various fixes to merging flag change across renames,
having the most basic case tested first seems useful.

note: We are only testing "adding" an exec flag here, not removing it. We
introduce basic test on stable and will consolidate them on default.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

marmoute created this revision.Sat, May 16, 4:11 PM
durin42 accepted this revision as: durin42.Mon, May 18, 12:59 PM
durin42 added a subscriber: durin42.

I'm okay with the series, but I'm a little hesitant to put something this big on stable. Can you sway me one way or the other?

marmoute added a comment.EditedFri, May 22, 4:59 AM

I'm okay with the series, but I'm a little hesitant to put something this big on stable. Can you sway me one way or the other?

Most of the "size" is about adding new tests which is less dangerous than code.

Patch-1: test only
Patch-2: test only
Patch-3: small test change, easy to double check (we if flag changed, this is not the same content)
Patch-4: test only
Patch-5: a score of new line, but the fundamental logic is quite simple: we do a three way merge on the flags.
Patch-6: test only
Patch-7: The most "slippery" one because we change existing code. However, the previous code was obviously wrong and no test barked when I changed it
Patch-8: test only, could easilly go on default later
Patch-9: test only, could easilly go on default later

Everything up to Patch-3 is straightforward and worries free. Up to Patch-5 seems simple and fine. Up to Patch 7 is fixing an anoying bug that I would rather see fixed on stable.

Does that help ?

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.