{"thread":{"id":"30538","subject":"git format-patch doesn't exclude merged hunks","startedAt":"2012-05-16T15:42:27Z","lastAt":"2012-05-16T22:42:14Z","messageCount":6,"participants":["Pádraig Brady","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"191594","messageId":"4FB3CAE3.6040608@draigBrady.com","threadId":"30538","inReplyTo":null,"subject":"git format-patch doesn't exclude merged hunks","fromName":"Pádraig Brady","fromEmail":"p@draigbrady.com","sentAt":"2012-05-16T15:42:27Z","receivedAt":"2012-05-16T15:42:27Z","isPatch":false,"sender":{"key":"p@draigbrady.com","avatar":"https://gravatar.com/avatar/6d16c619bfc08087da3aa2baf6e69e438044a3dd657f0a31637d8de065ef5b27?d=mp&s=160"},"body":"So I was using `git format-patch` to generate patches for\nconsumption by patch (as part of an RPM build),\nand one of the patches was failing as part of it was\nalready applied.\n\nIt seems like format-patch should exclude bits it's\nmerge previously, at least as an option.\n\nFor reference the two commits in question are:\nhttps://github.com/openstack/nova/commit/7028d66\nhttps://github.com/openstack/nova/commit/26dc6b7\nNotice how both make the same change to Authors.\n\ncheers,\nPádraig.\n"},{"id":"191599","messageId":"7vhavgc660.fsf@alter.siamese.dyndns.org","threadId":"30538","inReplyTo":"4FB3CAE3.6040608@draigBrady.com","subject":"Re: git format-patch doesn't exclude merged hunks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-16T18:49:43Z","receivedAt":"2012-05-16T18:49:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pádraig Brady <P@draigBrady.com> writes:\n\n> For reference the two commits in question are:\n> https://github.com/openstack/nova/commit/7028d66\n> https://github.com/openstack/nova/commit/26dc6b7\n> Notice how both make the same change to Authors.\n\nIf you compare the changes these two commits introduce, you will also\nnotice that the \"Authors\" file is the _only_ common part of them.\n\n\"format-patch\" (more precicely, the \"git cherry\" machinery that identifies\nthe same patch) does not _selectively_ drop only a part of a patch while\nkeeping the other parts.  It is not per \"hunk\", it is not even per \"file\".\n\nThis is very much on purpose, and I think it is a good design decision.\n\nIn this particular case, the behaviour does look suboptimal, but if you\nthink about it harder, you will realize that the perception comes largely\nbecause in this particular commit, the change to the \"Authors\" file is the\nleast interesting part of the change.\n\nImagine a case where you were replaying a commit that changes a file\nsignificantly and also changes another file in a trivial way, and where it\nwere the significant change that has already been applied to the receiving\ncodebase, not the insignificant change to \"Authors\" file.\n\nNow imagine that format-patch dropped the part that brings in the\nsignificant change as duplicate, and replayed only the insignificant part.\nMost likely, the log message of the original commit explains what issue\nthat significant change tried to solve, and how the implementation in the\npatch was determined to be an acceptable approach to solve it, and that is\nwhat you will be recording for the replayed commit that only introduces\nthe remaining insignificant change.\n\nI am not fundamentally opposed to the idea of (optionally) detecting and\nselectively dropping parts of a patch to an entire file or even hunks that\nhave already applied, but it needs to have a way remind the user somewhere\nin the workflow that it did so and the log message may no longer describe\nwhat the change does.  Most likely it would have to be done when producing\nformat-patch output, but an approach to make it a responsibility to notice\nand fix the resulting log message to the person who applies the output, I\nwould imagine.\n"},{"id":"191602","messageId":"4FB3FA59.1010707@draigBrady.com","threadId":"30538","inReplyTo":"7vhavgc660.fsf@alter.siamese.dyndns.org","subject":"Re: git format-patch doesn't exclude merged hunks","fromName":"Pádraig Brady","fromEmail":"p@draigbrady.com","sentAt":"2012-05-16T19:04:57Z","receivedAt":"2012-05-16T19:04:57Z","isPatch":false,"sender":{"key":"p@draigbrady.com","avatar":"https://gravatar.com/avatar/6d16c619bfc08087da3aa2baf6e69e438044a3dd657f0a31637d8de065ef5b27?d=mp&s=160"},"body":"On 05/16/2012 07:49 PM, Junio C Hamano wrote:\n> Pádraig Brady <P@draigBrady.com> writes:\n> \n>> For reference the two commits in question are:\n>> https://github.com/openstack/nova/commit/7028d66\n>> https://github.com/openstack/nova/commit/26dc6b7\n>> Notice how both make the same change to Authors.\n> \n> If you compare the changes these two commits introduce, you will also\n> notice that the \"Authors\" file is the _only_ common part of them.\n> \n> \"format-patch\" (more precicely, the \"git cherry\" machinery that identifies\n> the same patch) does not _selectively_ drop only a part of a patch while\n> keeping the other parts.  It is not per \"hunk\", it is not even per \"file\".\n> \n> This is very much on purpose, and I think it is a good design decision.\n> \n> In this particular case, the behaviour does look suboptimal, but if you\n> think about it harder, you will realize that the perception comes largely\n> because in this particular commit, the change to the \"Authors\" file is the\n> least interesting part of the change.\n> \n> Imagine a case where you were replaying a commit that changes a file\n> significantly and also changes another file in a trivial way, and where it\n> were the significant change that has already been applied to the receiving\n> codebase, not the insignificant change to \"Authors\" file.\n> \n> Now imagine that format-patch dropped the part that brings in the\n> significant change as duplicate, and replayed only the insignificant part.\n> Most likely, the log message of the original commit explains what issue\n> that significant change tried to solve, and how the implementation in the\n> patch was determined to be an acceptable approach to solve it, and that is\n> what you will be recording for the replayed commit that only introduces\n> the remaining insignificant change.\n> \n> I am not fundamentally opposed to the idea of (optionally) detecting and\n> selectively dropping parts of a patch to an entire file or even hunks that\n> have already applied, but it needs to have a way remind the user somewhere\n> in the workflow that it did so and the log message may no longer describe\n> what the change does.  Most likely it would have to be done when producing\n> format-patch output, but an approach to make it a responsibility to notice\n> and fix the resulting log message to the person who applies the output, I\n> would imagine.\n\nYep agreed, it would have to be optional.\nMaybe --ignore-duplicate-changes ?\n\nAppending a marker to the commit message of the adjusted patch would make sense,\nsimilar to how a 'Conflicts:' list is auto generated for commit messages.\n\ncheers,\nPádraig.\n"},{"id":"191603","messageId":"7v8vgsc544.fsf@alter.siamese.dyndns.org","threadId":"30538","inReplyTo":"4FB3FA59.1010707@draigBrady.com","subject":"Re: git format-patch doesn't exclude merged hunks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-16T19:12:27Z","receivedAt":"2012-05-16T19:12:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pádraig Brady <P@draigBrady.com> writes:\n\n> On 05/16/2012 07:49 PM, Junio C Hamano wrote:\n> \n>> I am not fundamentally opposed to the idea of (optionally) detecting and\n>> selectively dropping parts of a patch to an entire file or even hunks that\n>> have already applied, but it needs to have a way remind the user somewhere\n>> in the workflow that it did so and the log message may no longer describe\n>> what the change does.  Most likely it would have to be done when producing\n>> format-patch output, but an approach to make it a responsibility to notice\n>> and fix the resulting log message to the person who applies the output, I\n>> would imagine.\n>\n> Yep agreed, it would have to be optional.\n> Maybe --ignore-duplicate-changes ?\n>\n> Appending a marker to the commit message of the adjusted patch would make sense,\n> similar to how a 'Conflicts:' list is auto generated for commit messages.\n\nThese existing \"conflicts:\" are offered when recording manual resolutions\nof a conflicting merge, and the user is actively thrown into an editor\nwhen running \"git commit\" to record the result.\n\nA patch that is reduced in a way you propose will apply to the receiving\ntree cleanly without stopping, and does not offer an editor session to\nadjust the log before making a commit.  \"The user has a chance to notice\nand correct\" is not sufficient---nobody will spend extra effort to notice\nlet alone correct.  The reminder has to be a lot stronger than that, I\nthink, to cause the patch application to \"fail\" and require the user to\nactively look at the situation.\n"},{"id":"191606","messageId":"4FB40A7E.80705@draigBrady.com","threadId":"30538","inReplyTo":"7v8vgsc544.fsf@alter.siamese.dyndns.org","subject":"Re: git format-patch doesn't exclude merged hunks","fromName":"Pádraig Brady","fromEmail":"p@draigbrady.com","sentAt":"2012-05-16T20:13:50Z","receivedAt":"2012-05-16T20:13:50Z","isPatch":false,"sender":{"key":"p@draigbrady.com","avatar":"https://gravatar.com/avatar/6d16c619bfc08087da3aa2baf6e69e438044a3dd657f0a31637d8de065ef5b27?d=mp&s=160"},"body":"On 05/16/2012 08:12 PM, Junio C Hamano wrote:\n> Pádraig Brady <P@draigBrady.com> writes:\n> \n>> On 05/16/2012 07:49 PM, Junio C Hamano wrote:\n>>\n>>> I am not fundamentally opposed to the idea of (optionally) detecting and\n>>> selectively dropping parts of a patch to an entire file or even hunks that\n>>> have already applied, but it needs to have a way remind the user somewhere\n>>> in the workflow that it did so and the log message may no longer describe\n>>> what the change does.  Most likely it would have to be done when producing\n>>> format-patch output, but an approach to make it a responsibility to notice\n>>> and fix the resulting log message to the person who applies the output, I\n>>> would imagine.\n>>\n>> Yep agreed, it would have to be optional.\n>> Maybe --ignore-duplicate-changes ?\n>>\n>> Appending a marker to the commit message of the adjusted patch would make sense,\n>> similar to how a 'Conflicts:' list is auto generated for commit messages.\n> \n> These existing \"conflicts:\" are offered when recording manual resolutions\n> of a conflicting merge, and the user is actively thrown into an editor\n> when running \"git commit\" to record the result.\n> \n> A patch that is reduced in a way you propose will apply to the receiving\n> tree cleanly without stopping, and does not offer an editor session to\n> adjust the log before making a commit.  \"The user has a chance to notice\n> and correct\" is not sufficient---nobody will spend extra effort to notice\n> let alone correct.  The reminder has to be a lot stronger than that, I\n> think, to cause the patch application to \"fail\" and require the user to\n> actively look at the situation.\n\nYes it would make sense for `git am` to balk at\nsuch reduced patches, while allowing standard\npatch utilities to process the patches as normal.\n\ncheers,\nPádraig.\n"},{"id":"191612","messageId":"7v4nrfd9yx.fsf@alter.siamese.dyndns.org","threadId":"30538","inReplyTo":"4FB40A7E.80705@draigBrady.com","subject":"Re: git format-patch doesn't exclude merged hunks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-16T22:42:14Z","receivedAt":"2012-05-16T22:42:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pádraig Brady <P@draigBrady.com> writes:\n\n> On 05/16/2012 08:12 PM, Junio C Hamano wrote:\n>> A patch that is reduced in a way you propose will apply to the receiving\n>> tree cleanly without stopping, and does not offer an editor session to\n>> adjust the log before making a commit.  \"The user has a chance to notice\n>> and correct\" is not sufficient---nobody will spend extra effort to notice\n>> let alone correct.  The reminder has to be a lot stronger than that, I\n>> think, to cause the patch application to \"fail\" and require the user to\n>> actively look at the situation.\n>\n> Yes it would make sense for `git am` to balk at such reduced patches,\n> while allowing standard patch utilities to process the patches as\n> normal.\n\nThat certainly is one way to implement it, but \"am\" may not necessarily be\nthe best place to do so, depending on how you are using the output from\nformat-patch.  It does not matter if you are using \"format-patch\" piped to\n\"am -3\" as a more efficient way to cherry-pick or rebase commits, but if\nyou are sending the result out to somebody else, you would instead want to\nsanitize the mess on your end, wouldn't you?\n\nThat would mean that \"format-patch\" needs to do more than just \"mark a\npart of its output being suspicious\".  This is especially true as some\npeople blindly send out format-patch output using the interface to \"git\nsend-email\" without first verifying if the patches they are sending out is\nwhat they want to send out.\n"}]}