Conversation
kormide
force-pushed
the
from-to-file
branch
19 times, most recently
from
c3fd961 to
321acff
Compare
kormide
force-pushed
the
from-to-file
branch
4 times, most recently
from
a94662b to
4b2bf38
Compare
| return (from_file, to_file, file_path) | ||
|
|
||
| def _build_command(bin_dir, diff_bin, args, files, patch, type): | ||
| zero_timestamp_and_offset = "0000-00-00 00:00:00.000000000 +0000" |
There was a problem hiding this comment.
nit: some systems compute a time delta, and the year 0 causes them to overflow a datatype (say, seconds in the period). So I think it's better to use a default value like we do in tar.bzl which is effectively "start of UNIX epoch (1970) or maybe it's a more recent date than that.
There was a problem hiding this comment.
Good idea. I'll switch to using the unix epoch.
| def _build_command(bin_dir, diff_bin, args, files, patch, type): | ||
| zero_timestamp_and_offset = "0000-00-00 00:00:00.000000000 +0000" | ||
| if type == "unified": | ||
| match_timestamp_and_offset = "[0-9]{4}-[0-9]{2}-[0-9]{2}\\s+[0-9]{2}:[0-9]{2}:[0-9]{2}\\.[0-9]+\\s+[-+][0-9]{4}" |
There was a problem hiding this comment.
nit: offset actually means timezone right? it's not an offset within the file?
There was a problem hiding this comment.
That's correct. It's the timezone offset. +0000 is the offset for UTC.
| if [[ $? == '2' ]]; then | ||
| exit 2 | ||
| fi | ||
| echo "$DIFF" | sed -r 's#^((---|\\+\\+\\+)\\s+)({}/)?(\\S+)\\s+{}#\\1\\4 {}#' > {} |
There was a problem hiding this comment.
nit: can we get sed from somewhere like coreutils?
There was a problem hiding this comment.
sed isn't part of coreutils unfortunately so we can't use the bazel-lib toolchain. There is a source build on the BCR, but it doesn't work on windows. I'm hesitant to use it for Linux because I'm going to have to write windows PS or batch support eventually and I can't use that source build. Maybe I'll try fixing that upstream.
There was a problem hiding this comment.
I'd like to use a hermetic sed but I can live with not doing that at this moment.
| """.format( | ||
| diff_bin, | ||
| " ".join(args), | ||
| " ".join([file.path for file in files]), |
There was a problem hiding this comment.
if possible, for optimization I think it's better to accumulate these in an Args object so the file paths are lazily evaluated at execution time rather than during analysis
There was a problem hiding this comment.
Neat. I didn't know about Args.
There was a problem hiding this comment.
should this file be in .gitattributes as generated so the delta doesn't appear in the PR twice?
2 participants
Diff can create multifile 1-N patches with
--from-fileand--to-file.Due to a bug in
diffwhere--labelflags don't capture all of the file labels when using--to-fileand--from-file, I stopped setting overridden labels and instead I'm zeroing out the timestamps to make the patches deterministic.For other multifile patch cases, I'm going to make new rule later that takes in
difftargets asdeps. This is in line with keeping thediffrules as functional as the diff binary itself, which can't arbitrarily combine patches unless it's using one of the two options in this PR or--recursive.