-
-
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
RFC: DO NOT MERGE: patching: rewrite: try to stabilize patch index
stanzas as well as From lines
#6455
Conversation
For awareness, people I know have an interest in this: @SteeManMI (suffered from rewriting) @amazingfate (introduced fixes 12-char index) @ColorfulRhino (suffered), possibly more? Check the diff on the patch in this PR -- yes, all our patch indexes would change, but it would be "hopefully" for the last time. 🤔 Either way please don't merge -- if discussions prove positive I'll send the Python change separately and then we can do a batch of rewrites to equalize all. |
Interesting script! From https://git-scm.com/docs/git-diff#generate_patch_text_with_p :
Which means the index hashes are hashes from the actual file names, as in not the file name of the patch, but the file name of the file which is being patched. They should stay constant unless the file name changes or a new Git version adds more stuff to the hash as seen here: -index 0000000000..4899e23fa3
+index 000000000000..4899e23fa3aa |
I can't comment on the implementation here, but I will comment on the need for addressing this issue. As the code currently stands, these patch rewrites generate a lot of noise in the diffs that I feel potentially obscures any real changes. When scrolling through all of the changes, it is easy to pass over something small during a review. Also, even if this causes a one-time big hit on every patch, it will be worth it in the long run by having more readable/reviewable diffs. Now I leave it to the rest of you to figure out the best implementation. |
I agree. |
Thanks for the feedback.
true, but a separate problem. Also, I'd like to allow even the simplest of patches to come in -- lower barrier to entry -- and then be rewritten, eg in a dependabot-style, instead of blocking. |
ce437c4
to
c2742ec
Compare
For now I honestly still don't see how "removing" the file hashes would benefit us 😅 For example diff --git a/sound/soc/codecs/rk3308_codec.h b/sound/soc/codecs/rk3308_codec.h
index 6cfa69157785..93e089dae081 100644 The has
Yes, that's what I meant :) I'm not sure if Actions can modify a commit in a PR (e.g. modify a commit with a patch into the correct Armbian format) or if there needs to be a second commit afterwards (creating noise, but making it more understandable) |
No -- it comes from the git revisions's SHA1 in git from where we export the patches. Thus they change all the time, creating (for us) "noise". That "noise" would be very useful for a kernel developer (working on the mailing list) for example. |
It depends if the sender of the PR checked the "allow edits by maintainer & secrets" checkbox. |
Are you sure? I just created 3 test files git diff D:\Documents\test1.txt D:\Documents\test2.txt
diff --git "a/D:\\Documents\\test1.txt" "b/D:\\Documents\\test2.txt"
index 43dd47e..4c59102 100644
--- "a/D:\\Documents\\test1.txt"
+++ "b/D:\\Documents\\test2.txt"
@@ -1 +1 @@
-one
\ No newline at end of file
+two_and_three
\ No newline at end of file git diff D:\Documents\test1.txt D:\Documents\test3.txt
diff --git "a/D:\\Documents\\test1.txt" "b/D:\\Documents\\test3.txt"
index 43dd47e..4c59102 100644
--- "a/D:\\Documents\\test1.txt"
+++ "b/D:\\Documents\\test3.txt"
@@ -1 +1 @@
-one
\ No newline at end of file
+two_and_three
\ No newline at end of file Change the content of git diff D:\Documents\test1.txt D:\Documents\test2.txt
diff --git "a/D:\\Documents\\test1.txt" "b/D:\\Documents\\test2.txt"
index 43dd47e..6ab791f 100644
--- "a/D:\\Documents\\test1.txt"
+++ "b/D:\\Documents\\test2.txt"
@@ -1 +1 @@
-one
\ No newline at end of file
+only_two
\ No newline at end of file git diff D:\Documents\test1.txt D:\Documents\test3.txt
diff --git "a/D:\\Documents\\test1.txt" "b/D:\\Documents\\test3.txt"
index 43dd47e..4c59102 100644
--- "a/D:\\Documents\\test1.txt"
+++ "b/D:\\Documents\\test3.txt"
@@ -1 +1 @@
-one
\ No newline at end of file
+two_and_three
\ No newline at end of file By looking at the two numbers after I found the implementation here: https://github.com/git/git/blob/c2cbfbd2e28cbe27c194d62183b42f27a6a5bb87/diff.c#L4504C3-L4506C44 strbuf_addf(msg, "%s%sindex %s..%s", line_prefix, set,
diff_abbrev_oid(&one->oid, abbrev),
diff_abbrev_oid(&two->oid, abbrev));
[...]
static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)
{
if (startup_info->have_repository)
return repo_find_unique_abbrev(the_repository, oid, abbrev);
else {
char *hex = oid_to_hex(oid);
if (abbrev < 0)
abbrev = FALLBACK_DEFAULT_ABBREV;
if (abbrev > the_hash_algo->hexsz)
BUG("oid abbreviation out of range: %d", abbrev);
if (abbrev)
hex[abbrev] = '\0';
return hex;
}
} |
I guess you're not hitting this case. |
c2742ec
to
929bf04
Compare
Sorry to pester, @amazingfate -- what do you think about this? (Either way I need to test/fix file deletions done in patches, but I won't waste time if there's no general agreement on the "idea".) |
Hmm I think if it does create noise, this PR is good. I just haven't seen the |
I saw it changing now. But it looks like the index only changes when the base file changes as well, when the hunks also change. If that's the case, then I don't think this is "bad" noise. It would be bad if the hunks don't change, but the index does. Edit: I found one occasion where index got changed even though hunks were not changed. |
929bf04
to
a2795f7
Compare
…stanzas as well as From lines - `git format-patch --zero-commit` doesn't affect `index xxx...yyy` lines, only `From: ` - so use the _classy_ "use a regex with a callback" solution as git format-patch doesn't offer one - this will make _all_ patches change when rewritten, but hopefully _for the last time_ ! - not sure what other impacts this might have though - `git am` might not work any more? - we need to preserve `index 000000000000..xxx` as zeros, which indicate new file creation, thus: - new file creations are rewritten as `index 000000000000..111111111111` - non-creations are rewritten as `index 111111111111..222222222222`
a2795f7
to
3aa4733
Compare
This turned out untrue. File deletions don't have an index stanza at all. Thus this seems to actually complete. |
…rom lines - `git format-patch --zero-commit` doesn't affect `index xxx...yyy` lines, only `From: ` - so use the _classy_ "use a regex with a callback" solution as git format-patch doesn't offer one - this will make _all_ patches change when rewritten, but hopefully _for the last time_ ! - we need to preserve `index 000000000000..xxx` as zeros, which indicate new file creation, thus: - new file creations are rewritten as `index 000000000000..111111111111` - non-creations are rewritten as `index 111111111111..222222222222` - this is the final version of armbian#6455
…rom lines - `git format-patch --zero-commit` doesn't affect `index xxx...yyy` lines, only `From: ` - so use the _classy_ "use a regex with a callback" solution as git format-patch doesn't offer one - this will make _all_ patches change when rewritten, but hopefully _for the last time_ ! - we need to preserve `index 000000000000..xxx` as zeros, which indicate new file creation, thus: - new file creations are rewritten as `index 000000000000..111111111111` - non-creations are rewritten as `index 111111111111..222222222222` - this is the final version of armbian#6455
Sent the can-merge version of this in #6623 |
…rom lines - `git format-patch --zero-commit` doesn't affect `index xxx...yyy` lines, only `From: ` - so use the _classy_ "use a regex with a callback" solution as git format-patch doesn't offer one - this will make _all_ patches change when rewritten, but hopefully _for the last time_ ! - we need to preserve `index 000000000000..xxx` as zeros, which indicate new file creation, thus: - new file creations are rewritten as `index 000000000000..111111111111` - non-creations are rewritten as `index 111111111111..222222222222` - this is the final version of armbian#6455
…rom lines - `git format-patch --zero-commit` doesn't affect `index xxx...yyy` lines, only `From: ` - so use the _classy_ "use a regex with a callback" solution as git format-patch doesn't offer one - this will make _all_ patches change when rewritten, but hopefully _for the last time_ ! - we need to preserve `index 000000000000..xxx` as zeros, which indicate new file creation, thus: - new file creations are rewritten as `index 000000000000..111111111111` - non-creations are rewritten as `index 111111111111..222222222222` - this is the final version of armbian#6455
…rom lines - `git format-patch --zero-commit` doesn't affect `index xxx...yyy` lines, only `From: ` - so use the _classy_ "use a regex with a callback" solution as git format-patch doesn't offer one - this will make _all_ patches change when rewritten, but hopefully _for the last time_ ! - we need to preserve `index 000000000000..xxx` as zeros, which indicate new file creation, thus: - new file creations are rewritten as `index 000000000000..111111111111` - non-creations are rewritten as `index 111111111111..222222222222` - this is the final version of armbian#6455
…rom lines - `git format-patch --zero-commit` doesn't affect `index xxx...yyy` lines, only `From: ` - so use the _classy_ "use a regex with a callback" solution as git format-patch doesn't offer one - this will make _all_ patches change when rewritten, but hopefully _for the last time_ ! - we need to preserve `index 000000000000..xxx` as zeros, which indicate new file creation, thus: - new file creations are rewritten as `index 000000000000..111111111111` - non-creations are rewritten as `index 111111111111..222222222222` - this is the final version of armbian#6455
…rom lines - `git format-patch --zero-commit` doesn't affect `index xxx...yyy` lines, only `From: ` - so use the _classy_ "use a regex with a callback" solution as git format-patch doesn't offer one - this will make _all_ patches change when rewritten, but hopefully _for the last time_ ! - we need to preserve `index 000000000000..xxx` as zeros, which indicate new file creation, thus: - new file creations are rewritten as `index 000000000000..111111111111` - non-creations are rewritten as `index 111111111111..222222222222` - this is the final version of armbian#6455
…rom lines - `git format-patch --zero-commit` doesn't affect `index xxx...yyy` lines, only `From: ` - so use the _classy_ "use a regex with a callback" solution as git format-patch doesn't offer one - this will make _all_ patches change when rewritten, but hopefully _for the last time_ ! - we need to preserve `index 000000000000..xxx` as zeros, which indicate new file creation, thus: - new file creations are rewritten as `index 000000000000..111111111111` - non-creations are rewritten as `index 111111111111..222222222222` - this is the final version of armbian#6455
…rom lines - `git format-patch --zero-commit` doesn't affect `index xxx...yyy` lines, only `From: ` - so use the _classy_ "use a regex with a callback" solution as git format-patch doesn't offer one - this will make _all_ patches change when rewritten, but hopefully _for the last time_ ! - we need to preserve `index 000000000000..xxx` as zeros, which indicate new file creation, thus: - new file creations are rewritten as `index 000000000000..111111111111` - non-creations are rewritten as `index 111111111111..222222222222` - this is the final version of armbian#6455
…rom lines - `git format-patch --zero-commit` doesn't affect `index xxx...yyy` lines, only `From: ` - so use the _classy_ "use a regex with a callback" solution as git format-patch doesn't offer one - this will make _all_ patches change when rewritten, but hopefully _for the last time_ ! - we need to preserve `index 000000000000..xxx` as zeros, which indicate new file creation, thus: - new file creations are rewritten as `index 000000000000..111111111111` - non-creations are rewritten as `index 111111111111..222222222222` - this is the final version of armbian#6455
…rom lines - `git format-patch --zero-commit` doesn't affect `index xxx...yyy` lines, only `From: ` - so use the _classy_ "use a regex with a callback" solution as git format-patch doesn't offer one - this will make _all_ patches change when rewritten, but hopefully _for the last time_ ! - we need to preserve `index 000000000000..xxx` as zeros, which indicate new file creation, thus: - new file creations are rewritten as `index 000000000000..111111111111` - non-creations are rewritten as `index 111111111111..222222222222` - this is the final version of armbian#6455
…rom lines - `git format-patch --zero-commit` doesn't affect `index xxx...yyy` lines, only `From: ` - so use the _classy_ "use a regex with a callback" solution as git format-patch doesn't offer one - this will make _all_ patches change when rewritten, but hopefully _for the last time_ ! - we need to preserve `index 000000000000..xxx` as zeros, which indicate new file creation, thus: - new file creations are rewritten as `index 000000000000..111111111111` - non-creations are rewritten as `index 111111111111..222222222222` - this is the final version of armbian#6455
…rom lines - `git format-patch --zero-commit` doesn't affect `index xxx...yyy` lines, only `From: ` - so use the _classy_ "use a regex with a callback" solution as git format-patch doesn't offer one - this will make _all_ patches change when rewritten, but hopefully _for the last time_ ! - we need to preserve `index 000000000000..xxx` as zeros, which indicate new file creation, thus: - new file creations are rewritten as `index 000000000000..111111111111` - non-creations are rewritten as `index 111111111111..222222222222` - this is the final version of armbian#6455
…rom lines - `git format-patch --zero-commit` doesn't affect `index xxx...yyy` lines, only `From: ` - so use the _classy_ "use a regex with a callback" solution as git format-patch doesn't offer one - this will make _all_ patches change when rewritten, but hopefully _for the last time_ ! - we need to preserve `index 000000000000..xxx` as zeros, which indicate new file creation, thus: - new file creations are rewritten as `index 000000000000..111111111111` - non-creations are rewritten as `index 111111111111..222222222222` - this is the final version of armbian#6455
…rom lines - `git format-patch --zero-commit` doesn't affect `index xxx...yyy` lines, only `From: ` - so use the _classy_ "use a regex with a callback" solution as git format-patch doesn't offer one - this will make _all_ patches change when rewritten, but hopefully _for the last time_ ! - we need to preserve `index 000000000000..xxx` as zeros, which indicate new file creation, thus: - new file creations are rewritten as `index 000000000000..111111111111` - non-creations are rewritten as `index 111111111111..222222222222` - this is the final version of armbian#6455
…rom lines - `git format-patch --zero-commit` doesn't affect `index xxx...yyy` lines, only `From: ` - so use the _classy_ "use a regex with a callback" solution as git format-patch doesn't offer one - this will make _all_ patches change when rewritten, but hopefully _for the last time_ ! - we need to preserve `index 000000000000..xxx` as zeros, which indicate new file creation, thus: - new file creations are rewritten as `index 000000000000..111111111111` - non-creations are rewritten as `index 111111111111..222222222222` - this is the final version of armbian#6455
…rom lines - `git format-patch --zero-commit` doesn't affect `index xxx...yyy` lines, only `From: ` - so use the _classy_ "use a regex with a callback" solution as git format-patch doesn't offer one - this will make _all_ patches change when rewritten, but hopefully _for the last time_ ! - we need to preserve `index 000000000000..xxx` as zeros, which indicate new file creation, thus: - new file creations are rewritten as `index 000000000000..111111111111` - non-creations are rewritten as `index 111111111111..222222222222` - this is the final version of armbian#6455
…rom lines - `git format-patch --zero-commit` doesn't affect `index xxx...yyy` lines, only `From: ` - so use the _classy_ "use a regex with a callback" solution as git format-patch doesn't offer one - this will make _all_ patches change when rewritten, but hopefully _for the last time_ ! - we need to preserve `index 000000000000..xxx` as zeros, which indicate new file creation, thus: - new file creations are rewritten as `index 000000000000..111111111111` - non-creations are rewritten as `index 111111111111..222222222222` - this is the final version of armbian#6455
…rom lines - `git format-patch --zero-commit` doesn't affect `index xxx...yyy` lines, only `From: ` - so use the _classy_ "use a regex with a callback" solution as git format-patch doesn't offer one - this will make _all_ patches change when rewritten, but hopefully _for the last time_ ! - we need to preserve `index 000000000000..xxx` as zeros, which indicate new file creation, thus: - new file creations are rewritten as `index 000000000000..111111111111` - non-creations are rewritten as `index 111111111111..222222222222` - this is the final version of armbian#6455
…rom lines - `git format-patch --zero-commit` doesn't affect `index xxx...yyy` lines, only `From: ` - so use the _classy_ "use a regex with a callback" solution as git format-patch doesn't offer one - this will make _all_ patches change when rewritten, but hopefully _for the last time_ ! - we need to preserve `index 000000000000..xxx` as zeros, which indicate new file creation, thus: - new file creations are rewritten as `index 000000000000..111111111111` - non-creations are rewritten as `index 111111111111..222222222222` - this is the final version of #6455
…rom lines - `git format-patch --zero-commit` doesn't affect `index xxx...yyy` lines, only `From: ` - so use the _classy_ "use a regex with a callback" solution as git format-patch doesn't offer one - this will make _all_ patches change when rewritten, but hopefully _for the last time_ ! - we need to preserve `index 000000000000..xxx` as zeros, which indicate new file creation, thus: - new file creations are rewritten as `index 000000000000..111111111111` - non-creations are rewritten as `index 111111111111..222222222222` - this is the final version of #6455
RFC: DO NOT MERGE: patching: rewrite: try to stabilize patch
index
stanzas as well as From linesindex
stanzas as well as From linesgit format-patch --zero-commit
doesn't affectindex xxx...yyy
lines, onlyFrom:
git am
might not work any more?index 000000000000..xxx
as zeros, which indicate new file creation, thus:index 000000000000..111111111111
index 111111111111..222222222222