-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Split patches #37
Split patches #37
Conversation
98d97e6
to
c0033ed
Compare
c0033ed
to
5ab2540
Compare
// When splitting one large diff into a per-file diff, there are a few ways | ||
// you can go about it. Because different files can have the same name | ||
// (by being located in different directories), you need to avoid collisions. | ||
// Mirroring the directory structure seems undesirable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that it is undesirable. I think we should either mimic the actual directory structure or make the directory part of the file name (net/base/network_delegate.cc
-> net-base-network_delegate.cc
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the reason is that if I'm searching for a particular patch file, I want an easy way to identify it without looking at the actual contents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way it currently works is net/base/network_delegate.cc -> net.base.network_delegate.cc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(We can make it do dashes by changing desiredReplacementString
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, misread it. Yea, let's use -
instead
lib/updatePatches.js
Outdated
} | ||
|
||
if (replacingBackslashes) { | ||
revised = revised.replace(/\\/g, desiredReplacementString) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
git always uses forward slashes, right?
this looks good, but we should probably do an antimuon PR as well to actually split them |
0a1811c
to
29eae05
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
console.log('updatePatches wrote ' + (1 + i) + '/' + n + ': ' + filename) | ||
} | ||
|
||
// finish off by creating one big patch for deleted files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can probably leave this out. I can't think of any reason why we would need to delete a file vs patching a build script to leave it out
outside issue: brave/brave-browser-snap#36 outside pr: brave/brave-browser-snap#37 standardize to a/ b/ diff prefixes
and one big file for Deleted files for issue brave/brave-browser-snap#36 fixes from pr comments: use '-' as separator only need to replace '/' not '\\' standardize diff prefixes to a/ and b/
29eae05
to
fb79dd6
Compare
outside issue: brave/brave-browser-snap#36 outside pr: brave/brave-browser-snap#37 standardize to a/ b/ diff prefixes remove master_deleted_patch
Build and use dump_syms on Windows and Mac
brave/brave-browser-snap#36