{"thread":{"id":"28231","subject":"[PATCH v2 0/2] Fix reference name format check and normalization","startedAt":"2011-08-27T04:12:42Z","lastAt":"2011-08-29T22:22:55Z","messageCount":14,"participants":["Michael Haggerty","Junio C Hamano","Jeff King","Johannes Sixt"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"174365","messageId":"1314418364-2532-1-git-send-email-mhagger@alum.mit.edu","threadId":"28231","inReplyTo":null,"subject":"[PATCH v2 0/2] Fix reference name format check and normalization","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-08-27T04:12:42Z","receivedAt":"2011-08-27T04:12:42Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This patch series consists of \"Do not allow refnames to start with a\nslash\" as previously submitted (and improved by Junio) and a second\npatch that forbids DEL characters in reference names and adds tests of\ncontrol characters in reference names.  It should be applied to maint.\n\nMichael Haggerty (2):\n  check-ref-format --print: Normalize refnames that start with slashes\n  Forbid DEL characters in reference names\n\n builtin/check-ref-format.c  |    6 +++---\n refs.c                      |    2 +-\n t/t1402-check-ref-format.sh |    9 +++++++++\n 3 files changed, 13 insertions(+), 4 deletions(-)\n\n-- \n1.7.6.8.gd2879\n"},{"id":"174366","messageId":"1314418364-2532-2-git-send-email-mhagger@alum.mit.edu","threadId":"28231","inReplyTo":"1314418364-2532-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 1/2] check-ref-format --print: Normalize refnames that start with slashes","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-08-27T04:12:43Z","receivedAt":"2011-08-27T04:12:43Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"And add tests that such refnames are accepted and normalized\ncorrectly.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n\nThis patch includes the simplification suggested by Junio on the\nmailing list.\n\n builtin/check-ref-format.c  |    6 +++---\n t/t1402-check-ref-format.sh |    6 ++++++\n 2 files changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c\nindex ae3f281..0723cf2 100644\n--- a/builtin/check-ref-format.c\n+++ b/builtin/check-ref-format.c\n@@ -12,8 +12,8 @@ static const char builtin_check_ref_format_usage[] =\n \"   or: git check-ref-format --branch <branchname-shorthand>\";\n \n /*\n- * Replace each run of adjacent slashes in src with a single slash,\n- * and write the result to dst.\n+ * Remove leading slashes and replace each run of adjacent slashes in\n+ * src with a single slash, and write the result to dst.\n  *\n  * This function is similar to normalize_path_copy(), but stripped down\n  * to meet check_ref_format's simpler needs.\n@@ -21,7 +21,7 @@ static const char builtin_check_ref_format_usage[] =\n static void collapse_slashes(char *dst, const char *src)\n {\n \tchar ch;\n-\tchar prev = '\\0';\n+\tchar prev = '/';\n \n \twhile ((ch = *src++) != '\\0') {\n \t\tif (prev == '/' && ch == prev)\ndiff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh\nindex 1b0f82f..7563043 100755\n--- a/t/t1402-check-ref-format.sh\n+++ b/t/t1402-check-ref-format.sh\n@@ -18,6 +18,9 @@ invalid_ref 'foo'\n valid_ref 'foo/bar/baz'\n valid_ref 'refs///heads/foo'\n invalid_ref 'heads/foo/'\n+valid_ref '/heads/foo'\n+valid_ref '///heads/foo'\n+invalid_ref '/foo'\n invalid_ref './foo'\n invalid_ref '.refs/foo'\n invalid_ref 'heads/foo..bar'\n@@ -70,7 +73,10 @@ invalid_ref_normalized() {\n \n valid_ref_normalized 'heads/foo' 'heads/foo'\n valid_ref_normalized 'refs///heads/foo' 'refs/heads/foo'\n+valid_ref_normalized '/heads/foo' 'heads/foo'\n+valid_ref_normalized '///heads/foo' 'heads/foo'\n invalid_ref_normalized 'foo'\n+invalid_ref_normalized '/foo'\n invalid_ref_normalized 'heads/foo/../bar'\n invalid_ref_normalized 'heads/./foo'\n invalid_ref_normalized 'heads\\foo'\n-- \n1.7.6.8.gd2879\n"},{"id":"174367","messageId":"1314418364-2532-3-git-send-email-mhagger@alum.mit.edu","threadId":"28231","inReplyTo":"1314418364-2532-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 2/2] Forbid DEL characters in reference names","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-08-27T04:12:44Z","receivedAt":"2011-08-27T04:12:44Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"DEL is an ASCII control character and therefore should not be\npermitted in reference names.  Add tests for this and other unusual\ncharacters.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n\nPlease check that the uses of \"printf\" in the test script are portable\nand quoted correctly.\n\n refs.c                      |    2 +-\n t/t1402-check-ref-format.sh |    3 +++\n 2 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 3a8789d..0fa8dcf 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -837,7 +837,7 @@ int for_each_rawref(each_ref_fn fn, void *cb_data)\n \n static inline int bad_ref_char(int ch)\n {\n-\tif (((unsigned) ch) <= ' ' ||\n+\tif (((unsigned) ch) <= ' ' || ch == 0x7f ||\n \t    ch == '~' || ch == '^' || ch == ':' || ch == '\\\\')\n \t\treturn 1;\n \t/* 2.13 Pattern Matching Notation */\ndiff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh\nindex 7563043..ed4275a 100755\n--- a/t/t1402-check-ref-format.sh\n+++ b/t/t1402-check-ref-format.sh\n@@ -30,6 +30,9 @@ invalid_ref 'heads/foo.lock'\n valid_ref 'heads/foo@bar'\n invalid_ref 'heads/v@{ation'\n invalid_ref 'heads/foo\\bar'\n+invalid_ref \"$(printf 'heads/foo\\t')\"\n+invalid_ref \"$(printf 'heads/foo\\177')\"\n+valid_ref \"$(printf 'heads/fu\\303\\237')\"\n \n test_expect_success \"check-ref-format --branch @{-1}\" '\n \tT=$(git write-tree) &&\n-- \n1.7.6.8.gd2879\n"},{"id":"174368","messageId":"4E58710B.60507@alum.mit.edu","threadId":"28231","inReplyTo":"1314418364-2532-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 1/2] check-ref-format --print: Normalize refnames that start with slashes","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-08-27T04:22:35Z","receivedAt":"2011-08-27T04:22:35Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/27/2011 06:12 AM, Michael Haggerty wrote:\n> And add tests that such refnames are accepted and normalized\n> correctly.\n\n...oops, I just noticed that you have committed a this same patch to pu\nalready, but with a better log message.  Please retain that version.\n\nPatch 2/2, however, is new.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"174385","messageId":"7vty92adv0.fsf@alter.siamese.dyndns.org","threadId":"28231","inReplyTo":"4E58710B.60507@alum.mit.edu","subject":"Re: [PATCH v2 1/2] check-ref-format --print: Normalize refnames that start with slashes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-27T18:30:26Z","receivedAt":"2011-08-27T18:30:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> On 08/27/2011 06:12 AM, Michael Haggerty wrote:\n>> And add tests that such refnames are accepted and normalized\n>> correctly.\n>\n> ...oops, I just noticed that you have committed a this same patch to pu\n> already, but with a better log message.  Please retain that version.\n\nThanks. Very much appreciated.\n\nIt sometimes gets frustrating to see a re-rolled submission that ignores\nthe fix-ups to messages and patches I make locally before queued to 'pu'.\n\nIt is easy for me to say that they should fetch 'pu' to see what is queued\nbefore resubmitting, but I've been wondering if there is a better way to\ncommunicate back such differences, so that submitters can easily sanity\ncheck to see if my fix-ups are sensible, and to ensure that the re-rolled\npatches do not discard them by mistake before submitting.\n\nI could post what are queued in new topics back to the list as part of\nack, but that would make the list too noisy to read.\n"},{"id":"174386","messageId":"7vobzaaduy.fsf@alter.siamese.dyndns.org","threadId":"28231","inReplyTo":"1314418364-2532-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 2/2] Forbid DEL characters in reference names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-27T18:40:04Z","receivedAt":"2011-08-27T18:40:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> Please check that the uses of \"printf\" in the test script are portable\n> and quoted correctly.\n> ...\n> +invalid_ref \"$(printf 'heads/foo\\t')\"\n> +invalid_ref \"$(printf 'heads/foo\\177')\"\n> +valid_ref \"$(printf 'heads/fu\\303\\237')\"\n\nI think these should be fine.\n"},{"id":"174425","messageId":"4E5A63C2.5010207@alum.mit.edu","threadId":"28231","inReplyTo":"7vty92adv0.fsf@alter.siamese.dyndns.org","subject":"git project patch workflow","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-08-28T15:50:26Z","receivedAt":"2011-08-28T15:50:26Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/27/2011 08:30 PM, Junio C Hamano wrote:\n> It sometimes gets frustrating to see a re-rolled submission that ignores\n> the fix-ups to messages and patches I make locally before queued to 'pu'.\n> \n> It is easy for me to say that they should fetch 'pu' to see what is queued\n> before resubmitting [...]\n\nI usually try to do this, but it is awkward (or maybe I am doing it\nwrong).  If I run git fetch and notice that one of the upstream branches\nwas updated, then I still have to do a semi-manual step to see whether\nany of the new commits are from a patch series that I submitted,\npossibly which version of the patch series they are from, and whether\nthe patch or commit message was edited by you.  Sometimes it is not even\nclear which branch you will choose to apply a patch series to (maint vs\nmaster).  Contrast that, for example, with the ease of determining in\ngithub whether a patch series has made it upstream.\n\nIn this particular case your email mentioned your code improvement, and\nI obviously checked that your change made sense and squashed it onto my\nown patch.  But your email didn't mention your changes to the commit\nmessage, so I did not think to look for any.  Given that we, the\nunwashed masses, have to transact all of our git patches through the\nmailing list, I naively assumed that you would also mention your changes\non the mailing list.\n\nI am amazed at the volume, quality, promptness, and patience of your\nfeedback (thanks!) and we should all bend over backwards to make your\nlife easier even at the expense of some extra work for ourselves.  Junio\ndoesn't scale :-)  So my suggestions below should only be considered if\nthey do not increase your bureaucratic work at the expense of your\ntechnical work.\n\n> [...] but I've been wondering if there is a better way to\n> communicate back such differences, so that submitters can easily sanity\n> check to see if my fix-ups are sensible, and to ensure that the re-rolled\n> patches do not discard them by mistake before submitting.\n> \n> I could post what are queued in new topics back to the list as part of\n> ack, but that would make the list too noisy to read.\n\nI personally would find it helpful if you would mention any changes that\nyou made in an acknowledgment email.  Ideally, the email would also\nmention the branch name that you have chosen for the feature branch, to\nmake it easier to grep for the commit in your repo where it was merged\ninto pu and in your \"What's cooking\" messages.  In your case it would be\nenough to *mention* that you made a change because we can see the change\nitself in your repo.  I wouldn't worry too much about mailing list\ntraffic, because the acknowledgement would presumably be in the same\nthread as the original patch series and thus easily ignorable by people\nwho are not interested in the subject.\n\n\nTaking a step back, the real irony is that the git project hardly use\nthe distributed features of git *for the development of git itself*.  I\nwas flabbergasted when I first realized that fact.  Why is that?  It\nseems that the kind of fork/pull request/merge workflow that github\nmakes so easy would be more convenient (and if not, the dogfooding would\nforce us to *make* it more convenient).  I personally found it vastly\neasier to contribute my first patch to a github-based project than to git.\n\nWhat would it take to make a git-centered workflow meet all of the\nrequirements that are satisfied by the mailing-list centered workflow\nthat is used currently?\n\nISTM that if there were a pull request procedure that automatically sent\nPATCH emails to the mailing list for review (including the\nnon-log-message comments), then we would already be 95% of the way\nthere.  The goal would be to make git the definitive store of\ninformation, and the patch emails a side-product.  That's the real\nreason I started the thread \"'git format-patch' should get more\ninformation out of git\": if all necessary information were in git then\nthere would be no need for all changes to be serialized through the\nmailing list.\n\n1. Author creates v1 of patch series\n2. Author pushes changes to any publicly-visible git repo, including\nnon-log-message comments as notes\n3. Author creates v1 tag and submits pull request, which results in\nemails being sent to the mailing list as now (but with links to the git\nrepo and tag containing the patches)\n4. Reviewers submit feedback via the mailing list.  They can either work\ndirectly with the patch emails or can fetch the patches directly from\nthe author.  The feedback is handled informally as now, but might\ninclude links to changed versions in their own public repo).\n5. Author responds to feedback via the mailing list as now.  Goto 1 if\nneeded, creating tags for v2, v3 etc. of the patch series.\n6. When satisfied, maintainer pulls branch to shared repo.  *The\nauthor's commits retain their SHA1s.*\n\nPie in the sky?\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"174474","messageId":"20110829185011.GC756@sigill.intra.peff.net","threadId":"28231","inReplyTo":"7vty92adv0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 1/2] check-ref-format --print: Normalize refnames that start with slashes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-29T18:50:12Z","receivedAt":"2011-08-29T18:50:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 27, 2011 at 11:30:26AM -0700, Junio C Hamano wrote:\n\n> It sometimes gets frustrating to see a re-rolled submission that ignores\n> the fix-ups to messages and patches I make locally before queued to 'pu'.\n> \n> It is easy for me to say that they should fetch 'pu' to see what is queued\n> before resubmitting, but I've been wondering if there is a better way to\n> communicate back such differences, so that submitters can easily sanity\n> check to see if my fix-ups are sensible, and to ensure that the re-rolled\n> patches do not discard them by mistake before submitting.\n\nFWIW, I never do this, nor did I realize I was expected to. It takes\neffort on the part of the submitter, which means they're probably not\ninclined to do so unless there are changes that they know are pending.\nI.e., if I see changes as replies on the list, I grab them and put them\ninto my re-roll. But I am not in the habit of poking through pu, trying\nto match up equivalent patches, and then comparing them against my\nversions, just on the off chance that some change has been made that\nwasn't mentioned on the list. I always assumed that you did one of:\n\n  1. Comment on the patch via email, just as any other reviewer, so it\n     can go into the re-roll.\n\n  2. Fix-up the patch or commit message during \"am\", with the assumption\n     that it is ready to be merged at least to \"next\", at which point\n     re-rolls are no longer OK, anyway.\n\nI mentioned \"it takes effort\" above. I don't mean \"submitters shouldn't\nbe expected to put in extra effort\". But we should make sure the effort\nis well-spent, which means:\n\n  1. Giving them some indication that you tweaked things during\n     application. It doesn't have to be an inclusive list. Even saying\n     \"Thanks, applied with some spelling fixes\" instead of your usual\n     \"Thanks\" is enough (actually, I think you frequently do so\n     already).\n\n  2. Having better tool support for picking out the topics. Right now\n     we don't know the name of the topic branch you choose without\n     hunting for it in pu (or seeing it later in a What's Cooking\n     message). And even if we do, picking the tip commit out of pu\n     requires a bit of scripting. Have you considered publishing the\n     tips of topic branches you apply? Probably it makes sense to keep\n     them out of refs/heads/ in git.git, but even having them available\n     in refs/topics/ would allow interested parties to fetch them.\n\n  3. Having better tool support for comparing two sets of commits. The\n     ideal interface (to me) for this workflow would be something like:\n\n         $ git compare-series my-topic origin/my-topic origin\n         Patch 1: first patch subject...OK\n         Patch 2: second patch subject...\n\n         diff --git a/hello.c b/hello.c\n         index cef8b34..4f08083 100644\n         --- a/hello.c\n         +++ b/hello.c\n         @@ -1,6 +1,6 @@\n          #include <stdio.h>\n          int main(void)\n          {\n         -  printf(\"hello wrold\\n!\");\n         +  printf(\"hello world\\n!\");\n            return 0;\n          }\n         Accept change from upstream [y,n,q]?\n\n         Patch 3: third patch subject...\n\n         diff --git a/COMMIT_MSG b/COMMIT_MSG\n         index 54c8fa2..fd7b9be 100644\n         --- a/COMMIT_MSG\n         +++ b/COMMIT_MSG\n         @@ -1,3 +1,3 @@\n          third patch subject\n\n         -This patch has a commit message with a tpyo in it.\n         +This patch has a commit message with a typo in it.\n\n         Accept commit message update from upstream [y,n,q]?\n\n     where the implementation would be something like:\n\n       a. Get two series of commits as $3..$1 and $3..$2.\n\n       b. Try to match commits from series one to series two, ending up\n          with some ordered list of pairs like the one below (entries on\n          the left would be commit sha1s from series 1; entries on the\n          right would be commit sha1s from series 2).\n\n            (P1, P1)  (an unmodified version of a patch)\n            (P2, P2') (a modified version of a patch)\n            (P3, )    (dropped P3 in series 2)\n            (, P4)    (added P4 in series 2)\n\n          The matching would probably involve some text similarity\n          analysis of the commit messages (or possibly the patch\n          itself, though that can get confused if an early patch is\n          tweaked with a change that cascades through the series).\n\n       c. Do a sort of interactive rebase over this list. For unmodified\n          pairs, take the commit from either. For modified pairs,\n          checkout the commit from series 1, then \"checkout -p\" the\n          commit from series 2 on top of it. The resulting commit is\n          then applied to the intermediate rebase result. For modified\n          commit messages, do a \"git add -p\" in a one-off sub-repository\n          with only COMMIT_MSG in it, and then use the result as the\n          final commit message. For dropped patches, show the patch from\n          series 1 and say \"Drop this patch?\" For added patches, do the\n          same (but with \"Add this patch?\").\n\n          I'm not sure how reordering of patches would be handled, if at\n          all. Maybe just as a deletion and an addition.\n\nAnyway, that's just an idea I had while writing this message. I wouldn't\nbe surprised if there are a ton of awful corner cases I didn't think\nabout, or that somebody has a much better way of accomplishing the same\nthing.\n\nI often end up with something close to this by rebasing my topic\nbranches on top of master. Once your version hits master, then I see\nyour changes as conflicts. It's a bit annoying, though, because the\nconflicts are frequently annoying to resolve. E.g., you fix a typo in\nthe line, and then every patch in the series after that which modified\nthe same line (or even a nearby one) ends up conflicting.\n\n-Peff\n"},{"id":"174476","messageId":"20110829190118.GE756@sigill.intra.peff.net","threadId":"28231","inReplyTo":"20110829185011.GC756@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/2] check-ref-format --print: Normalize refnames that start with slashes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-29T19:01:18Z","receivedAt":"2011-08-29T19:01:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 29, 2011 at 02:50:12PM -0400, Jeff King wrote:\n\n>   3. Having better tool support for comparing two sets of commits. The\n>      ideal interface (to me) for this workflow would be something like:\n\nBTW, this discussion is obviously about comparing what was applied\nupstream with what was submitted, but I think such a tool could have\nother purposes, too. It should be able to provide a much nicer interdiff\nbetween two versions of the same series (e.g., for reviewers looking at\na re-roll). You could even use it as maintainer to apply a re-roll,\ngiving you a chance to review the differences as you apply.\n\n-Peff\n"},{"id":"174481","messageId":"4E5BEF47.5020608@kdbg.org","threadId":"28231","inReplyTo":"20110829185011.GC756@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/2] check-ref-format --print: Normalize refnames that start with slashes","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-08-29T19:57:59Z","receivedAt":"2011-08-29T19:57:59Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 29.08.2011 20:50, schrieb Jeff King:\n>      Have you considered publishing the\n>      tips of topic branches you apply?\n\nTry 'git remote add github git://github.com/gitster/git' and drop your\njaws on the wealth of branches you get on every 'git fetch github'.\n\n-- Hannes\n"},{"id":"174482","messageId":"20110829200204.GH756@sigill.intra.peff.net","threadId":"28231","inReplyTo":"4E5BEF47.5020608@kdbg.org","subject":"Re: [PATCH v2 1/2] check-ref-format --print: Normalize refnames that start with slashes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-29T20:02:04Z","receivedAt":"2011-08-29T20:02:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 29, 2011 at 09:57:59PM +0200, Johannes Sixt wrote:\n\n> Am 29.08.2011 20:50, schrieb Jeff King:\n> >      Have you considered publishing the\n> >      tips of topic branches you apply?\n> \n> Try 'git remote add github git://github.com/gitster/git' and drop your\n> jaws on the wealth of branches you get on every 'git fetch github'.\n\nHmph. I had no idea that existed. Searching the list, it was mentioned\nin a What's Cooking in May 2010.\n\n-Peff\n"},{"id":"174496","messageId":"7vhb50543g.fsf@alter.siamese.dyndns.org","threadId":"28231","inReplyTo":"20110829190118.GE756@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/2] check-ref-format --print: Normalize refnames that start with slashes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-29T20:56:35Z","receivedAt":"2011-08-29T20:56:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Aug 29, 2011 at 02:50:12PM -0400, Jeff King wrote:\n>\n>>   3. Having better tool support for comparing two sets of commits. The\n>>      ideal interface (to me) for this workflow would be something like:\n>\n> BTW, this discussion is obviously about comparing what was applied\n> upstream with what was submitted, but I think such a tool could have\n> other purposes, too. It should be able to provide a much nicer interdiff\n> between two versions of the same series (e.g., for reviewers looking at\n> a re-roll).\n\nI agree with it in principle, but I am not going to write one any time\nsoon. I think the most difficult case is when the submitter based a fix on\n'next' or 'pu' and I fixed it up so that it applies to 'maint'. It is not\ntrivial to express conflict resolution in such a case, as what your tool\nhas to do its work are (1) the original patch that may apply to\n\"somewhere\" (the tool could ask \"am -3\" to help it out), and (2) the patch\nthat is queued somewhere in 'pu'.\n"},{"id":"174502","messageId":"20110829212746.GA14299@sigill.intra.peff.net","threadId":"28231","inReplyTo":"7vhb50543g.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 1/2] check-ref-format --print: Normalize refnames that start with slashes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-29T21:27:46Z","receivedAt":"2011-08-29T21:27:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 29, 2011 at 01:56:35PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > On Mon, Aug 29, 2011 at 02:50:12PM -0400, Jeff King wrote:\n> >\n> >>   3. Having better tool support for comparing two sets of commits. The\n> >>      ideal interface (to me) for this workflow would be something like:\n> >\n> > BTW, this discussion is obviously about comparing what was applied\n> > upstream with what was submitted, but I think such a tool could have\n> > other purposes, too. It should be able to provide a much nicer interdiff\n> > between two versions of the same series (e.g., for reviewers looking at\n> > a re-roll).\n> \n> I agree with it in principle, but I am not going to write one any time\n> soon.\n\nHeh. I didn't expect you to. :) I may make an attempt at something, but\nI'd be happy if somebody else has ideas, too.\n\n> I think the most difficult case is when the submitter based a fix on\n> 'next' or 'pu' and I fixed it up so that it applies to 'maint'. It is not\n> trivial to express conflict resolution in such a case, as what your tool\n> has to do its work are (1) the original patch that may apply to\n> \"somewhere\" (the tool could ask \"am -3\" to help it out), and (2) the patch\n> that is queued somewhere in 'pu'.\n\nThe nastiest part of such a thing is not necessarily comparing the\npatches afterwards, but the actual conflicts you have to resolve to\nconvince \"am -3\" to apply the patch on \"master\" in the first place.\n\nConsider this toy example, which accidentally bases a topic off of\n\"next\" instead of \"master\":\n\n  git init repo &&\n  cd repo &&\n  perl -le 'print \"line $_\" for (1..20)' >file &&\n  git add file &&\n  git commit -m base &&\n  git checkout -b next &&\n  perl -pi -e 's/10/$& unrelated topic on next/' file &&\n  git commit -a -m 'unrelated topic' &&\n  git checkout -b our-topic next && # oops!\n  perl -pi -e 's/11/$& our topic/' file &&\n  git commit -a -m 'our topic' &&\n  git format-patch --stdout -1 >patch &&\n  git checkout master\n\nObviously \"git am\" will fail to apply the patch. You can do it with \"am\n-3\", but get a conflict like:\n\n  ++<<<<<<< HEAD\n   +line 10\n   +line 11\n  ++=======\n  + line 10 unrelated topic on next\n  + line 11 our topic\n  ++>>>>>>> our topic\n\nwhich is obviously easy to resolve in this toy example, but harder in\nthe real world (I assume in really hard cases, you just complain to the\nsubmitter to rebase it properly).\n\nBut assuming you resolve that properly to:\n\n  line 10\n  line 11 our topic\n\nwhat does the original submitter see? If they try to rebase their topic\nonto master, they'll get this pretty sane conflict:\n\n  ++<<<<<<< HEAD\n   +line 10\n  ++=======\n  + line 10 unrelated topic on next\n  ++>>>>>>> our topic\n\nI think the tool behavior I outlined in the previous message would\nyield:\n\n  diff --git a/file b/file\n  --- a/file\n  +++ b/file\n   line 8\n   line 9\n  -line 10 unrelated topic on next\n  +line 10\n   line 11 our topic\n   line 12\n  Accept this change [y,n,q]?\n\nThat is, it would look at the final versions of what each patch\nproduces, and try to apply the differences from one to the other, and\nthen use the result to apply on top of our rebase-in-progress.\n\nSo really, I think the ugliness of what the submitter would see is no\nuglier than what you had to do during \"git am\". If you make trivial\nchanges, they'll see trivial changes. If you had to pick apart large\nchunks of code into their constituent topics, then that's what the\nsubmitter will see when reviewing the differences.\n\nBut note that I didn't actually run the above \"this is what my tool\nshould do\" through an actual script. I haven't written anything yet, so\nI am just guessing, and there may be more complex cases where it breaks\ndown.\n\n-Peff\n"},{"id":"174514","messageId":"7vwrdv503k.fsf@alter.siamese.dyndns.org","threadId":"28231","inReplyTo":"20110829185011.GC756@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/2] check-ref-format --print: Normalize refnames that start with slashes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-29T22:22:55Z","receivedAt":"2011-08-29T22:22:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> ... I always assumed that you did one of:\n>\n>   1. Comment on the patch via email, just as any other reviewer, so it\n>      can go into the re-roll.\n>\n>   2. Fix-up the patch or commit message during \"am\", with the assumption\n>      that it is ready to be merged at least to \"next\", at which point\n>      re-rolls are no longer OK, anyway.\n\nNot really.\n\nI usually amend patches queued to 'pu' (or queue fixup! separately) when I\nam reasonably confident that my suggestion had enough merits.  Also I roll\nin suggestions that are obviously (to me) good from the list, which may or\nmay not be the same to the list concensus. Maybe I should try to be less\naggressive.\n\n> I mentioned \"it takes effort\" above. I don't mean \"submitters shouldn't\n> be expected to put in extra effort\". But we should make sure the effort\n> is well-spent, which means:\n>\n>   1. Giving them some indication that you tweaked things during\n>      application. It doesn't have to be an inclusive list. Even saying\n>      \"Thanks, applied with some spelling fixes\" instead of your usual\n>      \"Thanks\" is enough (actually, I think you frequently do so\n>      already).\n\nJust started consciously doing so yesterday, after starthing this offtopic\ntherad ;-)\n"}]}