{"thread":{"id":"56730","subject":"[PATCH v3] Documentation: specify base point when generating MyFirstContribution patchset","startedAt":"2021-10-18T12:41:22Z","lastAt":"2021-10-19T04:37:03Z","messageCount":5,"participants":["Bagas Sanjaya","Junio C Hamano","Glen Choo","Eric Sunshine"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"438982","messageId":"20211018124106.542050-1-bagasdotme@gmail.com","threadId":"56730","inReplyTo":null,"subject":"[PATCH v3] Documentation: specify base point when generating MyFirstContribution patchset","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-10-18T12:41:06Z","receivedAt":"2021-10-18T12:41:22Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"Specifying base point (commit hash) can help reviewers and testers\ninterested on the patchset. Mention how to record it with `--base`\noption to `format-patch`.\n\nSigned-off-by: Bagas Sanjaya \n---\n Changes since v3 [1]:\n - rewording (suggested by Glen)\n\n I don't apply Junio's suggestion that use `--base=auto`, because in\n most cases invocations of the option requires full hash of base object\n and AFAIK people just do `git checkout -b` without specifying the\n tracking option (`-t`).\n\n [1]: https://lore.kernel.org/git/xmqqo87q6whk.fsf@gitster.g/T/#t\n\n Documentation/MyFirstContribution.txt | 19 ++++++++++++++++---\n 1 file changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/MyFirstContribution.txt b/Documentation/MyFirstContribution.txt\nindex b20bc8e914..1c4cd092ee 100644\n--- a/Documentation/MyFirstContribution.txt\n+++ b/Documentation/MyFirstContribution.txt\n@@ -902,10 +902,19 @@ is out of scope for the context of this tutorial.\n === Preparing Initial Patchset\n \n Sending emails with Git is a two-part process; before you can prepare the emails\n-themselves, you'll need to prepare the patches. Luckily, this is pretty simple:\n+themselves, you'll need to prepare the patches. Luckily, this is pretty simple.\n+First, we need to get hash of the commit the patchset is based on. We call\n+this commit the ``:\n \n ----\n-$ git format-patch --cover-letter -o psuh/ master..psuh\n+$ git show -s --format=\"%H\" master\n+----\n+\n+Now generate the patchset, passing the hash of `` to the `--base`\n+parameter:\n+\n+----\n+$ git format-patch --cover-letter --base= -o psuh/ master..psuh\n ----\n \n The `--cover-letter` parameter tells `format-patch` to create a cover letter\n@@ -916,6 +925,10 @@ The `-o psuh/` parameter tells `format-patch` to place the patch files into a\n directory. This is useful because `git send-email` can take a directory and\n send out all the patches from there.\n \n+The `--base=` parameter tells `format-patch` to embed base commit\n+hash to the cover letter. Reviewers and testers interested in the patchset\n+can create branch based on the specifed base commit in order to apply it.\n+\n `master..psuh` tells `format-patch` to generate patches for the difference\n between `master` and `psuh`. It will make one patch file per commit. After you\n run, you can go have a look at each of the patches with your favorite text\n@@ -1046,7 +1059,7 @@ reviewer comments. Once the patch series is ready for submission, generate your\n patches again, but with some new flags:\n \n ----\n-$ git format-patch -v2 --cover-letter -o psuh/ --range-diff master..psuh-v1 master..\n+$ git format-patch -v2 --cover-letter -o psuh/ --base= --range-diff master..psuh-v1 master..\n ----\n \n The `--range-diff master..psuh-v1` parameter tells `format-patch` to include a\n\nbase-commit: f443b226ca681d87a3a31e245a70e6bc2769123c\n-- \nAn old man doll... just what I always wanted! - Clara\n\n"},{"id":"438989","messageId":"xmqqzgr6w97p.fsf@gitster.g","threadId":"56730","inReplyTo":"20211018124106.542050-1-bagasdotme@gmail.com","subject":"Re: [PATCH v3] Documentation: specify base point when generating MyFirstContribution patchset","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-18T17:09:46Z","receivedAt":"2021-10-18T17:09:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bagas Sanjaya writes:\n\n> Specifying base point (commit hash) can help reviewers and testers\n> interested on the patchset. Mention how to record it with `--base`\n> option to `format-patch`.\n>\n> Signed-off-by: Bagas Sanjaya \n> ---\n> Changes since v3 [1]:\n> - rewording (suggested by Glen)\n>\n> I don't apply Junio's suggestion that use `--base=auto`, because in\n> most cases invocations of the option requires full hash of base object\n> and AFAIK people just do `git checkout -b` without specifying the\n> tracking option (`-t`).\n\nThat's actually quite sad, if it were true.\n\nOur home, when we added \"end-user/newbie friendly features\" to\n\"checkout\" (credit goes to Dscho, IIRC), was actually most new\npeople would not even have to say \"-b\" in the simplest tasks\n(instead they rely on \"git checkout foo\" gets turned into \"git\ncheckout -t -b foo origin/foo\" by DWIMmery) and the fact that\nbranch.autoSetupMerge defaulting to 'true' so that they get the @{u}\nand --base=auto support without even saying \"-t\" (as long as they do\nnot explicitly decline with \"--no-track\" from the command line, that\nis).\n\n> -$ git format-patch --cover-letter -o psuh/ master..psuh\n> +$ git show -s --format=\"%H\" master\n\nI said this is not good in my previous response already and told you\nto teach merge-base, if we were not to use the --base=auto, didn't I?\n\nThe range given to format-patch, i.e. master..psuh, would be the\ncorrect range of commits to format, as long as you didn't rewind the\nlocal master branch.\n\nBut that does not mean master would always be the right base, does\nit? What if you had a work totally unrelated to the contents of\nthis tutorial on the 'master' front? The person on the receiving\nend may not even know what object it refers to until you pushed your\n'master' branch out.\n"},{"id":"438993","messageId":"kl6lzgr6z1a1.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"56730","inReplyTo":"xmqqzgr6w97p.fsf@gitster.g","subject":"Re: [PATCH v3] Documentation: specify base point when generating MyFirstContribution patchset","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-10-18T17:32:54Z","receivedAt":"2021-10-18T17:32:58Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"\nThe fact that we are having this right/wrong base discussion makes me\nthink that including the base is out of scope for first time\ncontributors. \n\n> But that does not mean master would always be the right base, does\n> it? What if you had a work totally unrelated to the contents of\n> this tutorial on the 'master' front? The person on the receiving\n> end may not even know what object it refers to until you pushed your\n> 'master' branch out.\n\nThis is the crux of the problem, which is that the contributor's base is\nactually their private 'master' branch, but their patches go to the\nupstream 'master' branch. Does it even matter that the patches were\noriginally authored on a private 'master'?\n\nIf it matters, the 'master' should be part of the patchset or it can\nbe described using the conventions of the list (\"this series is based\noff a merge of ab/foo-bar and cd/baz\").\n\nIf it does not matter, then providing the base is not helpful.\n\nI suspect that we are documenting --base is that we do not have *any*\ndocumentation of this in our mailing list workflow docs, and\nMyFirstContribution is becoming a catch-all for all of our workflow\nregardless of whether it is truly for first-timers or not. My own docs\nchanges [1] are arguably guilty of doing the same thing.\n\nAs you mentioned in the v2 thread:\n\n Actually it is up to contributors whether they want to include `--base` \n or not.\n\nThis is a level of discretion that we shouldn't be leaving to first\ntimers (as a very recent first timer, I would not appreciate this\nambiguity). This document should be recommending good, easy-to-follow\ndefaults for first timers, thus I think that discretionary things belong\nelsewhere.\n\nWe might consider adding _yet another_ document designed for\nlevelling up contributors past their first contribution, something along\nthe lines of \"Patch submission tips and tricks\". This could hold\ninformation that we want contributors to know about, but are not\nnecessary for a first-timer e.g. --range-diff and --base.\n\n[1] https://lore.kernel.org/git/20210922202218.7986-1-chooglen@google.com/\n"},{"id":"439002","messageId":"xmqqwnmaumcz.fsf_-_@gitster.g","threadId":"56730","inReplyTo":"kl6lzgr6z1a1.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re* [PATCH v3] Documentation: specify base point when generating MyFirstContribution patchset","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-18T20:08:44Z","receivedAt":"2021-10-18T20:08:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n>> But that does not mean master would always be the right base, does\n>> it? What if you had a work totally unrelated to the contents of\n>> this tutorial on the 'master' front? The person on the receiving\n>> end may not even know what object it refers to until you pushed your\n>> 'master' branch out.\n>\n> This is the crux of the problem, which is that the contributor's base is\n> actually their private 'master' branch, but their patches go to the\n> upstream 'master' branch. Does it even matter that the patches were\n> originally authored on a private 'master'?\n\nNo, and that is why I suggested to update the example to take\nadvantage of the fact that we forked out of origin/master, not\nprivate master, in my review of the second round.\n\nAnd telling what the value of origin/master is, or letting\n--base=auto to compute the merge base between origin/master and the\nbranch you are sending out (as origin/master may have advanced since\nyou forked), which is even better, helps the recipient quite a lot.\n\nBeing on the receiving end all the time, I should know ;-).\n\n> As you mentioned in the v2 thread:\n>\n> Actually it is up to contributors whether they want to include `--base` \n> or not.\n>\n> This is a level of discretion that we shouldn't be leaving to first\n> timers (as a very recent first timer, I would not appreciate this\n> ambiguity).\n\nExactly. The document is about contributing to _this_ project, and\nwe can and should be more explicit to say what we prefer to see.\n\nGoing back and forth is often a good way to figure out what kinks to\niron out and make progress, and in this particular case, it seems it\nhave already consumed enough bandwidth to make as much progress as we\ncan make in this approach, so let's start concluding by suggesting a\nmuch simpler rewrite.\n\nHow about doing the following and call it done?\n\n----- >8 --------- >8 --------- >8 --------- >8 ----\nSubject: [PATCH] MyFirstContribution: teach to use \"format-patch --base=auto\"\n\nLet's encourage first-time contributors to tell us what commit they\nbased their work with the format-patch invocation. As the example\nalready forks from origin/master and by branch.autosetupmerge by\ndefault records the upstream when the psuh branch was created, we\ncan use --base=auto for this. Also, mention the readers that the\nrange of commits can simply be given with `@{u}` if they are on the\n`psuh` branch already.\n\nAs we are getting one more option on the command line, and spending\none paragraph each to explain them, let's reformat that part of the\ndescription as a bulletted list.\n\nSigned-off-by: Junio C Hamano \n---\n\n * Fell out as an alternative approach during a review by Bagas\n Sanjaya https://lore.kernel.org/git/xmqqo87q6whk.fsf@gitster.g/\n\n Documentation/MyFirstContribution.txt | 30 +++++++++++++++++++++---------\n 1 file changed, 21 insertions(+), 9 deletions(-)\n\ndiff --git c/Documentation/MyFirstContribution.txt w/Documentation/MyFirstContribution.txt\nindex b20bc8e914..e505a7ec87 100644\n--- c/Documentation/MyFirstContribution.txt\n+++ w/Documentation/MyFirstContribution.txt\n@@ -905,19 +905,31 @@ Sending emails with Git is a two-part process; before you can prepare the emails\n themselves, you'll need to prepare the patches. Luckily, this is pretty simple:\n \n ----\n-$ git format-patch --cover-letter -o psuh/ master..psuh\n+$ git format-patch --cover-letter -o psuh/ --base=auto psuh@{u}..psuh\n ----\n \n-The `--cover-letter` parameter tells `format-patch` to create a cover letter\n-template for you. You will need to fill in the template before you're ready\n-to send - but for now, the template will be next to your other patches.\n+ . The `--cover-letter` option tells `format-patch` to create a\n+ cover letter template for you. You will need to fill in the\n+ template before you're ready to send - but for now, the template\n+ will be next to your other patches.\n \n-The `-o psuh/` parameter tells `format-patch` to place the patch files into a\n-directory. This is useful because `git send-email` can take a directory and\n-send out all the patches from there.\n+ . The `-o psuh/` option tells `format-patch` to place the patch\n+ files into a directory. This is useful because `git send-email`\n+ can take a directory and send out all the patches from there.\n \n-`master..psuh` tells `format-patch` to generate patches for the difference\n-between `master` and `psuh`. It will make one patch file per commit. After you\n+ . The `--base=auto` option tells the command to record the \"base\n+ commit\", on which the recipient is expected to apply the patch\n+ series.\n+\n+ . The `psuh@{u}..psuh` option tells `format-patch` to generate\n+ patches for the commits you created on the `psuh` branch since it\n+ forked from its upstream (which is `origin/master` if you\n+ followed the example in the \"Set up your workspace\" section). If\n+ you are already on the `psuh` branch, you can just say `@{u}`,\n+ which means \"commits on the current branch since it forked from\n+ its upstream\", which is the same thing.\n+\n+The command will make one patch file per commit. After you\n run, you can go have a look at each of the patches with your favorite text\n editor and make sure everything looks alright; however, it's not recommended to\n make code fixups via the patch file. It's a better idea to make the change the\n"},{"id":"439015","messageId":"CAPig+cQU_TF-MbOv-PaE-0L5dEa_rgDXptOLAu+JOAn=RuJCqg@mail.gmail.com","threadId":"56730","inReplyTo":"xmqqwnmaumcz.fsf_-_@gitster.g","subject":"Re: Re* [PATCH v3] Documentation: specify base point when generating MyFirstContribution patchset","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-10-19T04:36:50Z","receivedAt":"2021-10-19T04:37:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Oct 18, 2021 at 4:09 PM Junio C Hamano wrote:\n> Subject: [PATCH] MyFirstContribution: teach to use \"format-patch --base=auto\"\n>\n> Let's encourage first-time contributors to tell us what commit they\n> based their work with the format-patch invocation. As the example\n> already forks from origin/master and by branch.autosetupmerge by\n\nI think you meant: s/and by/and/\n\n> default records the upstream when the psuh branch was created, we\n> can use --base=auto for this. Also, mention the readers that the\n\ns/the readers/to readers/\n\n> range of commits can simply be given with `@{u}` if they are on the\n> `psuh` branch already.\n>\n> As we are getting one more option on the command line, and spending\n> one paragraph each to explain them, let's reformat that part of the\n> description as a bulletted list.\n>\n> Signed-off-by: Junio C Hamano \n"}]}