Re: [PATCH 00/16] Consolidate reachability logic
Hi Stolee, On Wed, 18 Jul 2018, Derrick Stolee wrote: > On 7/18/2018 1:01 PM, Junio C Hamano wrote: > > No, fixing a tool that throws such a harder-to-read patch series in > > reader's mailbox is *not* something I'd spend my primary focus on, > > especially when many contributors are perfectly capable of sending > > reasonably formatted series without using such a tool under > > development. > > > > That won't stop those who want to improve the tool. But I'd wish > > those who want to make Git better spend their time on making Git, > > over making GitGitGadget, better. > > I appreciate the feedback in how this series caused reviewer pain. Hopefully > this date issue is now resolved. Any further feedback is welcome. > > I'm choosing to use and contribute to GitGitGadget not because I'm incapable > of sending series myself, but because I _have_ had difficulty. Using the email > submissions creates a friction that I'm willing to overcome, but we are > probably missing out on contributors who are not willing to push through that > friction. Perhaps having another way for new contributors to feel welcome is > an indirect way to make Git better. While I am a seasoned Git contributor, it is *still* too painful to contribute patches *even for me*. So hopefully you and I will get this easier contribution process to the point where other oldtimers do not want to take it. At least we now have something that does not share the downsides with SubmitGit, and is extensible enough that we can teach it new tricks. With a little luck, Junio will fix amlog so that it is not utter garbage for anybody but himself, and then GitGitGadget can give contributors useful feedback about the state of their patch series, including automated notifications when their patches have been mentioned in the What's Cooking mail (which no irregular contributor reads, as far as I know). Ciao, Dscho
Re: [PATCH 00/16] Consolidate reachability logic
Hi Peff, On Wed, 18 Jul 2018, Jeff King wrote: > On Wed, Jul 18, 2018 at 02:23:11PM +0200, Johannes Schindelin wrote: > > > > Yeah, they're out of order in mutt's threaded display. And the > > > back-dating means there's a much higher chance of them getting blocked > > > as spam (e.g., some of the dates are from weeks ago). > > > > > > git-send-email uses the current time minus an offset, and then > > > monotonically increases for each patch: > > > > > > $time = time - scalar $#files; > > > ... > > > my $date = format_2822_time($time++); > > > > > > which seems to work pretty well in practice. It does mean the original > > > dates are lost. The committer date is not interesting at all (there will > > > be a new committer via "git am" anyway). The original author date is > > > potentially of interest, but could be included as an in-body header. > > > AFAIK send-email doesn't have such an option, though, and people are > > > fine with date-of-sending becoming the new author date. > > > > > > +cc Johannes as the GitGitGadget author > > > > Thanks for dumping even more work on my shoulders. > > Wow. Here's my perspective on what I wrote. > > Somebody pointed out an issue in the tool. I tried to add an additional > data point (how other clients react, and that I've seen spam-related > problems). And I tried to point to an existing solution in another tool, > in case that was helpful. I thought cc-ing you would be a favor, since > you obviously have an interest in the tool, and it is easy to miss > discussions buried deep in a thread. > > So no, I didn't write the patch for you. But I tried to contribute > positively to the process. And I got yelled at for it. That makes me a > lot less inclined to try to help in the future. > > > Next time, I will ask you to jump in, instead of putting the onus on me. > > > > I mean, seriously, what is this? "You can use *any* mail program to work > > with the Git mailing list, *any* mailer. As long as it is mutt. And as > > long as you spend hours and hours on tooling that oh BTW nobody else can > > use." > > The irony here is that I actually _did_ look at the GitGitGadget > repository, and thought about making a patch to be helpful. But as it is > written in a language I'm not all that familiar with, using tools that I > don't normally use, I didn't want to spend hours and hours in order to > make what was probably going to be a one-line patch in software that I > don't use myself. I understand that. The web is not based on shell scripting, so there is no good way to implement a bot on GitHub using Bash scripts. Ciao, Dscho
Re: [PATCH 00/16] Consolidate reachability logic
Hi Junio, On Wed, 18 Jul 2018, Junio C Hamano wrote: > That won't stop those who want to improve the tool. But I'd wish > those who want to make Git better spend their time on making Git, > over making GitGitGadget, better. And I'd wish that you would not make this task harder by refusing to fix your process to update refs/notes/amlog. Thanks, Dscho
Re: [PATCH 00/16] Consolidate reachability logic
On Wed, Jul 18, 2018 at 02:23:11PM +0200, Johannes Schindelin wrote: > > Yeah, they're out of order in mutt's threaded display. And the > > back-dating means there's a much higher chance of them getting blocked > > as spam (e.g., some of the dates are from weeks ago). > > > > git-send-email uses the current time minus an offset, and then > > monotonically increases for each patch: > > > > $time = time - scalar $#files; > > ... > > my $date = format_2822_time($time++); > > > > which seems to work pretty well in practice. It does mean the original > > dates are lost. The committer date is not interesting at all (there will > > be a new committer via "git am" anyway). The original author date is > > potentially of interest, but could be included as an in-body header. > > AFAIK send-email doesn't have such an option, though, and people are > > fine with date-of-sending becoming the new author date. > > > > +cc Johannes as the GitGitGadget author > > Thanks for dumping even more work on my shoulders. Wow. Here's my perspective on what I wrote. Somebody pointed out an issue in the tool. I tried to add an additional data point (how other clients react, and that I've seen spam-related problems). And I tried to point to an existing solution in another tool, in case that was helpful. I thought cc-ing you would be a favor, since you obviously have an interest in the tool, and it is easy to miss discussions buried deep in a thread. So no, I didn't write the patch for you. But I tried to contribute positively to the process. And I got yelled at for it. That makes me a lot less inclined to try to help in the future. > Next time, I will ask you to jump in, instead of putting the onus on me. > > I mean, seriously, what is this? "You can use *any* mail program to work > with the Git mailing list, *any* mailer. As long as it is mutt. And as > long as you spend hours and hours on tooling that oh BTW nobody else can > use." The irony here is that I actually _did_ look at the GitGitGadget repository, and thought about making a patch to be helpful. But as it is written in a language I'm not all that familiar with, using tools that I don't normally use, I didn't want to spend hours and hours in order to make what was probably going to be a one-line patch in software that I don't use myself. -Peff
Re: [PATCH 00/16] Consolidate reachability logic
On 7/18/2018 1:01 PM, Junio C Hamano wrote: No, fixing a tool that throws such a harder-to-read patch series in reader's mailbox is *not* something I'd spend my primary focus on, especially when many contributors are perfectly capable of sending reasonably formatted series without using such a tool under development. That won't stop those who want to improve the tool. But I'd wish those who want to make Git better spend their time on making Git, over making GitGitGadget, better. I appreciate the feedback in how this series caused reviewer pain. Hopefully this date issue is now resolved. Any further feedback is welcome. I'm choosing to use and contribute to GitGitGadget not because I'm incapable of sending series myself, but because I _have_ had difficulty. Using the email submissions creates a friction that I'm willing to overcome, but we are probably missing out on contributors who are not willing to push through that friction. Perhaps having another way for new contributors to feel welcome is an indirect way to make Git better. Thanks, -Stolee
Re: [PATCH 00/16] Consolidate reachability logic
Duy Nguyen writes: >> In other words: if you see something that you don't like about >> GitGitGadget, get your butts off the ground and contribute a fix. > > Thank you for the frank words. I will choose to not review any mails > coming from GitGitGadget. I wouldn't say I will choose not to, but certainly I noticed that I'd backburner reading a series that are way out of order in my mailbox, no matter who authored them or how they were sent out, as they consume way more concentration-point out of my mind than they are often worth X-<. While there are easier-to-read and more nicely organized patch series, I'd deal with them first, consciously or not. No, fixing a tool that throws such a harder-to-read patch series in reader's mailbox is *not* something I'd spend my primary focus on, especially when many contributors are perfectly capable of sending reasonably formatted series without using such a tool under development. That won't stop those who want to improve the tool. But I'd wish those who want to make Git better spend their time on making Git, over making GitGitGadget, better.
Re: [PATCH 00/16] Consolidate reachability logic
On Wed, Jul 18, 2018 at 2:30 PM Johannes Schindelin wrote: > > Hi, > > On Mon, 16 Jul 2018, Derrick Stolee wrote: > > > On 7/16/2018 2:44 PM, Eric Sunshine wrote: > > > On Mon, Jul 16, 2018 at 1:27 PM Stefan Beller wrote: > > > > Another pain point of the Gadget is that CC's in the cover letter > > > > do not work as I would imagine. The line > > > > > > > > CC: sbel...@google.com > > > > > > > > did not put that email into the cc field. > > > gitgitgadget recognizes case-sensitive "Cc:" only[1]. > > > > > > [1]: > > > https://github.com/gitgitgadget/gitgitgadget/blob/c4805370f59532aa438283431b8ea7d4484c530f/lib/patch-series.ts#L188 > > > > Thanks for everyone's patience while we improve gitgitgadget (and - in this > > case - I learn how to use it). > > And let's please stop pretending that this GitGitGadget project is > somebody else's problem. > > It is our best shot at addressing the *constant* pain point that is the code > contribution process of Git. > > In other words: if you see something that you don't like about > GitGitGadget, get your butts off the ground and contribute a fix. Thank you for the frank words. I will choose to not review any mails coming from GitGitGadget. -- Duy
Re: [PATCH 00/16] Consolidate reachability logic
Hi Eric & Peff, On Mon, 16 Jul 2018, Eric Sunshine wrote: > On Mon, Jul 16, 2018 at 2:56 PM Jeff King wrote: > > On Mon, Jul 16, 2018 at 02:40:21PM -0400, Eric Sunshine wrote: > > > On Mon, Jul 16, 2018 at 12:18 PM Jeff King wrote: > > > > git-send-email uses the current time minus an offset, and then > > > > monotonically increases for each patch: > > > > > > Junio pointed this out to gitgitgadget developers in [1], which led to > > > an issue being opened[2]. That issue was merged today. > > > > > > [1]: > > > https://public-inbox.org/git/xmqq7em7gg3j@gitster-ct.c.googlers.com/ > > > [2]: https://github.com/gitgitgadget/gitgitgadget/pull/15 > > > > I was going to say "oh good, fixed", but it looks like it just merged > > adding that line to the TODO list. :) > > Erm, right. I actually knew a couple days ago that that issue was just > a change to the TODO list but forgot that important tidbit when I > wrote the above "was merged today". Anyhow, at least it's on the > radar. It is always nice to get such active contributions. Seriously again, do feel free to jump in and contribute improvements to GitGitGadget. We have a very time-consuming (read: time wasting) code contribution process, and it is an untenable situation, and GitGitGadget was designed to be able to address this huge problem. But I can't do it alone. And neither should you pretend that this is my problem alone. This problem is as much your problem as it is mine. (Whether you realize it or not.) Ciao, Dscho
Re: [PATCH 00/16] Consolidate reachability logic
Hi, On Mon, 16 Jul 2018, Derrick Stolee wrote: > On 7/16/2018 2:44 PM, Eric Sunshine wrote: > > On Mon, Jul 16, 2018 at 1:27 PM Stefan Beller wrote: > > > Another pain point of the Gadget is that CC's in the cover letter > > > do not work as I would imagine. The line > > > > > > CC: sbel...@google.com > > > > > > did not put that email into the cc field. > > gitgitgadget recognizes case-sensitive "Cc:" only[1]. > > > > [1]: > > https://github.com/gitgitgadget/gitgitgadget/blob/c4805370f59532aa438283431b8ea7d4484c530f/lib/patch-series.ts#L188 > > Thanks for everyone's patience while we improve gitgitgadget (and - in this > case - I learn how to use it). And let's please stop pretending that this GitGitGadget project is somebody else's problem. It is our best shot at addressing the *constant* pain point that is the code contribution process of Git. In other words: if you see something that you don't like about GitGitGadget, get your butts off the ground and contribute a fix. The code contribution process of GitGitGadget is very easy: open a PR at https://github.com/gitgitgadget/gitgitgadget Ciao, Johannes
Re: [PATCH 00/16] Consolidate reachability logic
Hi Peff, On Mon, 16 Jul 2018, Jeff King wrote: > On Mon, Jul 16, 2018 at 02:54:38PM +0100, Ramsay Jones wrote: > > > On 16/07/18 14:00, Derrick Stolee via GitGitGadget wrote: > > > There are many places in Git that use a commit walk to determine > > > reachability between commits and/or refs. A lot of this logic is > > > duplicated. > > [snip] ... > > > > This is not your problem, but I find these GitGitGadget > > submissions somewhat annoying. This series has been spewed > > all over my in-box in, what I assume, is commit date order. > > > > So, patches #4,5 dated 19/06, then #1,2,3 dated 25/06, > > then #15 dated 28/06, then #6,7 dated 12/07, then #8-16 > > dated 13/07, then 00/16 dated today. > > > > No I don't use a threaded display (I hate it), be even with > > that turned on, the patches still appear in the above order > > under the cover letter (but at least all together). > > Yeah, they're out of order in mutt's threaded display. And the > back-dating means there's a much higher chance of them getting blocked > as spam (e.g., some of the dates are from weeks ago). > > git-send-email uses the current time minus an offset, and then > monotonically increases for each patch: > > $time = time - scalar $#files; > ... > my $date = format_2822_time($time++); > > which seems to work pretty well in practice. It does mean the original > dates are lost. The committer date is not interesting at all (there will > be a new committer via "git am" anyway). The original author date is > potentially of interest, but could be included as an in-body header. > AFAIK send-email doesn't have such an option, though, and people are > fine with date-of-sending becoming the new author date. > > +cc Johannes as the GitGitGadget author Thanks for dumping even more work on my shoulders. I wanted to help with that insane process we have here, but in a more collaborative manner. This time I fixed it, but please do keep in mind that the decision to use the email transport for something it *was not designed for* (it was designed for humans talking to humans) is the culprit here. Next time, I will ask you to jump in, instead of putting the onus on me. I mean, seriously, what is this? "You can use *any* mail program to work with the Git mailing list, *any* mailer. As long as it is mutt. And as long as you spend hours and hours on tooling that oh BTW nobody else can use." Hopefully GitGitGadget will make this situation better. And hopefully not on the expense of my sanity. Ciao, Dscho
Re: [PATCH 00/16] Consolidate reachability logic
On Mon, Jul 16, 2018 at 2:56 PM Jeff King wrote: > On Mon, Jul 16, 2018 at 02:40:21PM -0400, Eric Sunshine wrote: > > On Mon, Jul 16, 2018 at 12:18 PM Jeff King wrote: > > > git-send-email uses the current time minus an offset, and then > > > monotonically increases for each patch: > > > > Junio pointed this out to gitgitgadget developers in [1], which led to > > an issue being opened[2]. That issue was merged today. > > > > [1]: > > https://public-inbox.org/git/xmqq7em7gg3j@gitster-ct.c.googlers.com/ > > [2]: https://github.com/gitgitgadget/gitgitgadget/pull/15 > > I was going to say "oh good, fixed", but it looks like it just merged > adding that line to the TODO list. :) Erm, right. I actually knew a couple days ago that that issue was just a change to the TODO list but forgot that important tidbit when I wrote the above "was merged today". Anyhow, at least it's on the radar.
Re: [PATCH 00/16] Consolidate reachability logic
On Mon, Jul 16, 2018 at 02:40:21PM -0400, Eric Sunshine wrote: > On Mon, Jul 16, 2018 at 12:18 PM Jeff King wrote: > > On Mon, Jul 16, 2018 at 02:54:38PM +0100, Ramsay Jones wrote: > > > This is not your problem, but I find these GitGitGadget > > > submissions somewhat annoying. This series has been spewed > > > all over my in-box in, what I assume, is commit date order. > > > > > > So, patches #4,5 dated 19/06, then #1,2,3 dated 25/06, > > > then #15 dated 28/06, then #6,7 dated 12/07, then #8-16 > > > dated 13/07, then 00/16 dated today. > > > > Yeah, they're out of order in mutt's threaded display. And the > > back-dating means there's a much higher chance of them getting blocked > > as spam (e.g., some of the dates are from weeks ago). > > > > git-send-email uses the current time minus an offset, and then > > monotonically increases for each patch: > > Junio pointed this out to gitgitgadget developers in [1], which led to > an issue being opened[2]. That issue was merged today. > > [1]: https://public-inbox.org/git/xmqq7em7gg3j@gitster-ct.c.googlers.com/ > [2]: https://github.com/gitgitgadget/gitgitgadget/pull/15 I was going to say "oh good, fixed", but it looks like it just merged adding that line to the TODO list. :) Still, it looks like wheels are in motion, which is nice. -Peff
Re: [PATCH 00/16] Consolidate reachability logic
On 7/16/2018 2:44 PM, Eric Sunshine wrote: On Mon, Jul 16, 2018 at 1:27 PM Stefan Beller wrote: Another pain point of the Gadget is that CC's in the cover letter do not work as I would imagine. The line CC: sbel...@google.com did not put that email into the cc field. gitgitgadget recognizes case-sensitive "Cc:" only[1]. [1]: https://github.com/gitgitgadget/gitgitgadget/blob/c4805370f59532aa438283431b8ea7d4484c530f/lib/patch-series.ts#L188 Thanks for everyone's patience while we improve gitgitgadget (and - in this case - I learn how to use it). Thanks, -Stolee
Re: [PATCH 00/16] Consolidate reachability logic
On Mon, Jul 16, 2018 at 1:27 PM Stefan Beller wrote: > Another pain point of the Gadget is that CC's in the cover letter > do not work as I would imagine. The line > > CC: sbel...@google.com > > did not put that email into the cc field. gitgitgadget recognizes case-sensitive "Cc:" only[1]. [1]: https://github.com/gitgitgadget/gitgitgadget/blob/c4805370f59532aa438283431b8ea7d4484c530f/lib/patch-series.ts#L188
Re: [PATCH 00/16] Consolidate reachability logic
On Mon, Jul 16, 2018 at 12:18 PM Jeff King wrote: > On Mon, Jul 16, 2018 at 02:54:38PM +0100, Ramsay Jones wrote: > > This is not your problem, but I find these GitGitGadget > > submissions somewhat annoying. This series has been spewed > > all over my in-box in, what I assume, is commit date order. > > > > So, patches #4,5 dated 19/06, then #1,2,3 dated 25/06, > > then #15 dated 28/06, then #6,7 dated 12/07, then #8-16 > > dated 13/07, then 00/16 dated today. > > Yeah, they're out of order in mutt's threaded display. And the > back-dating means there's a much higher chance of them getting blocked > as spam (e.g., some of the dates are from weeks ago). > > git-send-email uses the current time minus an offset, and then > monotonically increases for each patch: Junio pointed this out to gitgitgadget developers in [1], which led to an issue being opened[2]. That issue was merged today. [1]: https://public-inbox.org/git/xmqq7em7gg3j@gitster-ct.c.googlers.com/ [2]: https://github.com/gitgitgadget/gitgitgadget/pull/15
Re: [PATCH 00/16] Consolidate reachability logic
On Mon, Jul 16, 2018 at 02:54:38PM +0100, Ramsay Jones wrote: > On 16/07/18 14:00, Derrick Stolee via GitGitGadget wrote: > > There are many places in Git that use a commit walk to determine > > reachability between commits and/or refs. A lot of this logic is > > duplicated. > [snip] ... > > This is not your problem, but I find these GitGitGadget > submissions somewhat annoying. This series has been spewed > all over my in-box in, what I assume, is commit date order. > > So, patches #4,5 dated 19/06, then #1,2,3 dated 25/06, > then #15 dated 28/06, then #6,7 dated 12/07, then #8-16 > dated 13/07, then 00/16 dated today. > > No I don't use a threaded display (I hate it), be even with > that turned on, the patches still appear in the above order > under the cover letter (but at least all together). Yeah, they're out of order in mutt's threaded display. And the back-dating means there's a much higher chance of them getting blocked as spam (e.g., some of the dates are from weeks ago). git-send-email uses the current time minus an offset, and then monotonically increases for each patch: $time = time - scalar $#files; ... my $date = format_2822_time($time++); which seems to work pretty well in practice. It does mean the original dates are lost. The committer date is not interesting at all (there will be a new committer via "git am" anyway). The original author date is potentially of interest, but could be included as an in-body header. AFAIK send-email doesn't have such an option, though, and people are fine with date-of-sending becoming the new author date. +cc Johannes as the GitGitGadget author -Peff
Re: [PATCH 00/16] Consolidate reachability logic
On 16/07/18 14:00, Derrick Stolee via GitGitGadget wrote: > There are many places in Git that use a commit walk to determine > reachability between commits and/or refs. A lot of this logic is > duplicated. [snip] ... This is not your problem, but I find these GitGitGadget submissions somewhat annoying. This series has been spewed all over my in-box in, what I assume, is commit date order. So, patches #4,5 dated 19/06, then #1,2,3 dated 25/06, then #15 dated 28/06, then #6,7 dated 12/07, then #8-16 dated 13/07, then 00/16 dated today. No I don't use a threaded display (I hate it), be even with that turned on, the patches still appear in the above order under the cover letter (but at least all together). Annoyed. ATB, Ramsay Jones
[PATCH 00/16] Consolidate reachability logic
There are many places in Git that use a commit walk to determine reachability between commits and/or refs. A lot of this logic is duplicated. I wanted to achieve the following: 1. Consolidate several different commit walks into one file 2. Reduce duplicate reachability logic 3. Increase testability (correctness and performance) 4. Improve performance of reachability queries My approach is mostly in three parts: I. Move code to a new commit-reach.c file. II. Add a 'test-tool reach' command to test these methods directly. III. Modify the logic by improving performance and calling methods with similar logic but different prototypes. The 'test-tool reach' command is helpful to make sure I don't break anything as I change the logic, but also so I can test methods that are normally only exposed by other more complicated commands. For instance, ref_newer() is part of 'git push -f' and ok_to_give_up() is buried deep within fetch negotiation. Both of these methods have some problematic performance issues that are corrected by this series. As I discovered them, it was clear that it would be better to consolidate walk logic instead of discovering a new walk in another file hidden somewhere. For the ok_to_give_up() method, I refactored the method so I could pull the logic out of the depths of fetch negotiation. In the commit "commit-reach: make can_all_from_reach... linear" I discuss how the existing algorithm is quadratic and how we can make it linear. Also, we can use heuristic knowledge about the shape of the commit graph and the usual haves/wants to get some extra performance bonus. (The heuristic is to do a DFS with first-parents first, and stop on first found result. We expect haves/wants to include ref tips, which typically have their previous values in their first-parent history.) One major difference in this series versus the RFC is that I added a new method 'generation_numbers_enabled()' to detect if we have a commit-graph file with non-zero generation numbers. Using can_all_from_reach in is_descendant_of is only faster if we have generation numbers as a cutoff. Thanks, -Stolee This series is based on jt/commit-graph-per-object-store CC: sbel...@google.com Derrick Stolee (16): commit-reach: move walk methods from commit.c commit-reach: move ref_newer from remote.c commit-reach: move commit_contains from ref-filter upload-pack: make reachable() more generic upload-pack: refactor ok_to_give_up() upload-pack: generalize commit date cutoff commit-reach: move can_all_from_reach_with_flags test-reach: create new test tool for ref_newer test-reach: test in_merge_bases test-reach: test is_descendant_of test-reach: test get_merge_bases_many test-reach: test reduce_heads test-reach: test can_all_from_reach_with_flags commit-reach: replace ref_newer logic commit-reach: make can_all_from_reach... linear commit-reach: use can_all_from_reach Makefile | 2 + builtin/remote.c | 1 + commit-graph.c| 18 ++ commit-graph.h| 6 + commit-reach.c| 662 ++ commit-reach.h| 76 + commit.c | 358 --- fast-import.c | 1 + http-push.c | 1 + ref-filter.c | 147 +- remote.c | 50 +--- remote.h | 1 - t/helper/test-reach.c | 104 +++ t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/t6600-test-reach.sh | 208 + upload-pack.c | 58 +--- 17 files changed, 1095 insertions(+), 600 deletions(-) create mode 100644 commit-reach.c create mode 100644 commit-reach.h create mode 100644 t/helper/test-reach.c create mode 100755 t/t6600-test-reach.sh base-commit: 596e28576ef3ca69432dbe5953b7bdcd18a32876 Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-10%2Fderrickstolee%2Freach%2Frefactor-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-10/derrickstolee/reach/refactor-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/10 -- gitgitgadget
