A week ago or so, MozReview started showing a "virtual file" containing only the commit message, as part of the changeset-to-be-reviewed. That virtual file is supposed to be removed when the commit actually lands. But in at least one case, it wasn't: https://hg.mozilla.org/integration/autoland/rev/b17b5fdb4ae7 That ^^ changeset added a file called commit-message-875d6 containing the commit message for that bug. I've since removed that file (after the changeset was merged to mozilla-central.) But we should definitely get to the bottom of this so we don't have more instances of bogus commit-message-XYZ files being checked into the tree by mistake...

The changeset in question was landed to autoland in bug 1345498 comment 17. jeremychen, do you know if there was anything odd about your workflow of getting this changeset into Autoland that might've triggered this? E.g. perhaps you pulled the changeset from autoland into your local tree (with the bonus commit-message-file-creation), and then you pushed that same changeset back to autoland (and then it treated the file-creation as a legitimate request from the patch author rather than an artifact of its own process)?

Yeah, I suspect something like my hypothetical scenario in comment 1 must've happened. Note that the initial revision on that bug only had a single "commit-message" file (this is correct & expected I think): https://reviewboard.mozilla.org/r/118206/diff/1/ ...but every subsequent revision had *two* "commit-message" files as part of the patch (one with the hex suffix that landed here): https://reviewboard.mozilla.org/r/118206/diff/2/ [...] https://reviewboard.mozilla.org/r/118206/diff/5/ So there must have been some extra pulling-from-MozReview-and-then-pushing-that-back-to-MozReview that happened in revision 2 I'm guessing.

So maybe there's some human error involved here (or some several-tools-interacting-poorly error), which means this isn't a MozReview bug after all. But it would be great if this is something that MozReview could automatically detect and warn about. I'm going to go out on a limb and say that no legitimate patch will ever *intentionally* create a file in the root directory called "commit-message-", so MozReview should perhaps treat that as a reserved name and complain/refuse-to-land commits that create *real* files with that naming scheme...

[ni=mcote for thoughts on the suggestion at the end of comment 3]

I suspect whether there is human error. As I observed in bug 1346443 comment 1, there are inconsistencies in diff vs. inter-diff. So although orig..2 shows both "commit-message" and "commit-message-800ba", 1..2 [1] only shows "commit-message", which indicates that the "commit-message-800ba" isn't a real file introduced in the patch. [1] https://reviewboard.mozilla.org/r/118206/diff/1-2/

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #6) > I suspect whether there is human error. > > As I observed in bug 1346443 comment 1, there are inconsistencies in diff > vs. inter-diff. So although orig..2 shows both "commit-message" and > "commit-message-800ba", 1..2 [1] only shows "commit-message" This is a bit clearer if you look at the raw diff (from "Download Diff") from an orig...1 vs orig...2. * orig...1 (i.e. patch revision 1): MozReview: https://reviewboard.mozilla.org/r/118206/diff/1/ (shows a file called "commit-message") Raw Diff: https://reviewboard.mozilla.org/r/118206/diff/1/raw/ (shows a file called "commit-message-800ba") * orig...2 (i.e. patch revision 2): MozReview: https://reviewboard.mozilla.org/r/118206/diff/2/ (shows 2 files, one called "commit-message" and one called "commit-message-800ba") Raw diff: https://reviewboard.mozilla.org/r/118206/diff/2/raw/ (shows 2 files both named "commit-message-800ba") My conclusions: - MozReview tries to gracefully hide the hex suffix on commit-message "virtual files", in its web UI -- though it only does this for the first such file. - jeremychen somehow ended up with a patch (revision 2) that added the *same* commit-message-800ba file twice. - It still seems to me like this likely came from importing a patch from mozreview [including the commit-message-800ba file], and then pushing that imported patch to mozreview [which prompts it to generate an additional "virtual" commit-message-800ba file for the new patch, and treat the pushed one as if it's a real file] (Awaiting clarification from jeremychen, to find out what his actual workflow was that triggered this odd state of affairs.)

(In reply to Daniel Holbert [:dholbert] from comment #7) > - It still seems to me like this likely came from importing a patch from > mozreview [including the commit-message-800ba file], and then pushing that > imported patch to mozreview [which prompts it to generate an additional > "virtual" commit-message-800ba file for the new patch, and treat the pushed > one as if it's a real file] OK, I just tried this locally with a dummy patch associated with this bug (using the "hg import" command that mozreview provides and then re-pushing), and it actually *didn't* trigger a double-file as I was suspecting it might. The "hg import" command in the MozReview UI is pointing at a version of the commit that does not have the "commit-message" file inside of it, it seems: https://reviewboard-hg.mozilla.org/gecko/rev/ba37a2701a763962b01f7f7fe178fca16ab6cecd So it's safe to import & re-push without risk of "creating" the commit-message file. So now I don't have a good mental model of what steps would've caused this problem...

I'm not sure if this is something related, but I use git workflow did used "git bz apply" to sync my patch between different PC/NBs. That's the reason why I got hit by a git-bz-moz bug (https://github.com/mozilla/git-bz-moz/issues/90). The modified authorship in the pushed patch is a proof: https://hg.mozilla.org/integration/autoland/rev/b17b5fdb4ae7 So, using "git bz apply" and "push mozreview" back and forth is my workflow, but I'm not sure if the workflow triggered this. Might worth some more experiments... Another thing I can try is to run "git reflog" to see if I did something strange on PC/NBs that I've worked on. But, I can only check one of them cause I'm not in the office.

Hmmmm... looks like I found the root cause.... Here's my STR: 1. git bz apply 1346321 This would apply the patch that Daniel pushed before 2. git-push-to-hg -t [path_to_hg_repo] Yeah, one of my working NB is still using the old git-to-hg workflow :( 3. run "hg status --change tip" in hg repo, and get following result: M README.txt A commit-message-a8d49 4. run "hg push review" in hg repo An extra commit-message file is created, see: https://reviewboard-hg.mozilla.org/gecko/rev/0e122d1e319f3fb2299186b709fbe4429e47efc5 Since "git bz apply" treat the commit-message as part of the patch, I believe the git-cinabar workflow would be affected by this as well.

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #12) > Hmmmm... looks like I found the root cause.... > > Here's my STR: > > 1. git bz apply 1346321 > This would apply the patch that Daniel pushed before [...] > Since "git bz apply" treat the commit-message as part of the patch, I > believe the git-cinabar workflow would be affected by this as well. So just to be clear -- "git bz apply" creates the "commit-message-***" file locally as part of the imported commit? If so, that seems like it clarifies the part that needs fixing. Jeremy, would you mind reporting this at https://github.com/mozilla/git-bz-moz ? (I'm CC'ing mccr8 who I think maintains [or just contributes a lot] to those tools, too).

(In reply to Daniel Holbert [:dholbert] from comment #13) > So just to be clear -- "git bz apply" creates the "commit-message-***" file > locally as part of the imported commit? If so, that seems like it clarifies > the part that needs fixing. > > Jeremy, would you mind reporting this at > https://github.com/mozilla/git-bz-moz ? (I'm CC'ing mccr8 who I think > maintains [or just contributes a lot] to those tools, too). Filed https://github.com/mozilla/git-bz-moz/issues/94. BTW, Boris might have an idea about the alternative of "git bz apply" for git-cinnabar users.

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #14) > (In reply to Daniel Holbert [:dholbert] from comment #13) > > So just to be clear -- "git bz apply" creates the "commit-message-***" file > > locally as part of the imported commit? If so, that seems like it clarifies > > the part that needs fixing. > > > > Jeremy, would you mind reporting this at > > https://github.com/mozilla/git-bz-moz ? (I'm CC'ing mccr8 who I think > > maintains [or just contributes a lot] to those tools, too). > > Filed https://github.com/mozilla/git-bz-moz/issues/94. > > BTW, Boris might have an idea about the alternative of "git bz apply" for > git-cinnabar users. I think we can use something like: `git mozreview fetch ba37a2701a763962b01f7f7fe178fca16ab6cecd`, which fetch the patches from Daniel's patches in this bug into FECTH_HEAD, so you can call `git merge FETCH_HEAD` to merge them into your branch. However, I haven't tested the virtual commit for this way.

One second thought, since the commit-message is just for review purpose, maybe we should remove "commit-message-***" from raw diff (the Download Diff button on MozReview) and make it be consistent with that imported by "hg import https://reviewboard-hg.mozilla.org/gecko/rev/xxxxx". It seems to me that including this kind of virtual/hidden files in the raw diff is not reasonable, they should be only used for MozReview UI, right? From this point of view, then this bug looks like a MozReview bug to me.

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #16) > It seems to me that including this kind of virtual/hidden files in the raw > diff is not reasonable, they should be only used for MozReview UI, right? > From this point of view, then this bug looks like a MozReview bug to me. +1 this file is not real, and should not be present in the 'download diff' response, which is what git-bz-moz uses to fetch the patch. https://github.com/mozilla/git-bz-moz/blob/master/git-bz#L772

This ipatch is fixing the issue. Solution in production might be more sophisticated.

Clearing my NI since it seems that the cause has been identified.

Comment on attachment 8847630 [details] mozreview: filter out commit message FileDiff from downloadable diff (bug 1346321) https://reviewboard.mozilla.org/r/120580/#review122566 ::: hgext/reviewboard/tests/test-commit-message-as-file.t (Diff revision 1) > - - diff --git a/commit-message-df673 b/commit-message-df673 > - - new file mode 100644 > - - '--- /dev/null' > - - +++ b/commit-message-df673 > - - '@@ -0,0 +1,3 @@' > - - +Bug 1 - Foo 1 > - - + > - - '+MozReview-Commit-ID: 124Bxg' This test is almost meaningless, since it is using the output of the diff you're changing to actually check the file creation.

Comment on attachment 8847630 [details] mozreview: filter out commit message FileDiff from downloadable diff (bug 1346321) https://reviewboard.mozilla.org/r/120580/#review122566 > This test is almost meaningless, since it is using the output of the diff you're changing to actually check the file creation. Confirmed - when there is no commit message in the patch, there is nothing to check. I'll change the message to check for the commit_message_filediff_ids or remove it.

Comment on attachment 8847629 [details] mozreview: HOTFIX allow to modify the DiffSet's files list for diff download. (bug 1346321) https://reviewboard.mozilla.org/r/120570/#review122790

Comment on attachment 8847630 [details] mozreview: filter out commit message FileDiff from downloadable diff (bug 1346321) https://reviewboard.mozilla.org/r/120580/#review122816 as a hotfix this lgtm.

Pushed by bjones@mozilla.com: https://hg.mozilla.org/webtools/reviewboard/rev/12009690bcc2 mozreview: HOTFIX allow to modify the DiffSet's files list for diff download. r=glob

Pushed by bjones@mozilla.com: https://hg.mozilla.org/hgcustom/version-control-tools/rev/58cf736cff7b mozreview: filter out commit message FileDiff from downloadable diff r=glob