| Message ID | 20200804220113.5909-1-alipman88@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce --first-parent flag for git bisect | expand |
Aaron Lipman <alipman88@gmail.com> writes: > OK, here's take 4! Responding to Junio's feedback, first: > >> That function [cmd_bisect__helper()] is supposed to be a thin shim >> layer whose only reason of its existence is to serve as an interface >> with the scripted version of "git bisect". When everything is >> migrated from the shell script, cmd_bisect__helper() should disappear. >> ... >> This is going backwards, as far as I can tell. If anything, I'd >> rather see cmd_bisect__helper() get fixed so that it does not parse >> "--first-parent" (and similarly, "--no-checkout" that you imitated) >> into first_parent_only (and no_checkout) variables and passed as >> parameters to bisect_start(). > > Thanks for the explanation, Junio. (And for bearing with me while I > gain familiarity with git's codebase.) Now that I've taken some time > to examine how git-bisect.sh and cmd_bisect__helper() fit together, > the correct approach is much more clear. I did notice that no_checkout variable in the top-level cmd_bisect__helper() was convenient to have in the current code structure, and I didn't think how it should be fixed deeply enough, so for the purpose of this series, I do not mind if it is left untouched. It's just that it was disturbing to see that addition of "--first-parent" wanted to add yet another variable to the top-level, and that the new variable was totally unneeded. > As no_checkout was also passed to bisect_next_all() [via implicitly > checking for the existence of .git/BISECT_HEAD when calling > bisect_next() in git-bisect.sh], I've removed that parameter and > instead check for .git/BISECT_HEAD in bisect_next_all() to determine > whether no-checkout mode is on. > > This means no-checkout mode can no longer be enabled by "git > bisect--helper --next-all --no-checkout" Unlike "start", "next-all" is not really a command in the end-user's mind ("git bisect next" is, though). It merely is just an interim "convenience" interface for the remaining part of the scripted "git bisect" to call into. As long as the caller can do the same thing as before, it should be OK, I would think.
On Wed, Aug 5, 2020 at 12:04 AM Aaron Lipman <alipman88@gmail.com> wrote: > > OK, here's take 4! Responding to Junio's feedback, first: [...] > Martin, thanks for your suggestions [...] It's better to have the people you are replying to as recipients of your emails (in the "To:" field). I have added them into "Cc:". > > (Signed-off-by: Martin Ågren <martin.agren@gmail.com>, FWIW.) > > I'm still getting used to the conventions - should I add your name as > a signed-off-by tag, a thanks-to tag, or both? We often use the following trailers: - "Helped-by:" when someone helped you - "Suggested-by:" when someone suggested the main idea in the patch - "Reported-by:" when someone reported an issue fixed by the patch - "Acked-by:" when someone explicitly acked the patch - "Reviewed-by:" when someone explicitly gave their "Reviewed-by:" If your patch is based on a patch from someone else, you can also keep the "Signed-off-by:" and other trailers that the person already put in the commit message. If you haven't made a lot of changes to a patch initially from someone else you can also keep them as the author. Thanks, Christian.
Hi Aaron, By the way, welcome to the list! On Wed, 5 Aug 2020 at 00:02, Aaron Lipman <alipman88@gmail.com> wrote: > > Martin, thanks for your suggestions - I've moved the commit updating > "git bisect run" tests to 1/5, updated the commit message, and > included the changes you provided. (I held off on additional > indentation corrections, as it kinda felt like unnecessary code > churn/outside the scope of abstracting platform-specific details.) Ok, great, I think it's fine to stop there. Those spots really are in a much better shape now, thanks. > > (Signed-off-by: Martin Ågren <martin.agren@gmail.com>, FWIW.) > > I'm still getting used to the conventions - should I add your name as > a signed-off-by tag, a thanks-to tag, or both? What I meant was "oh, and here's my Signed-off-by for the record" so that we wouldn't need a round-trip of "cool, can I have your S-o-b on that?". Of course, I expressed that in such a lousy, compact way that we needed a round-trip *anyway*. :-/ And I'm not even sure we need a "Signed-off-by". I just posted the patch form of "If you look for chmod, you'll find a few more spots where you can do the exact same transformation, and you might want to look up test_expect_code." I see you added "Thanks-to" and I would have been fine with no mention at all. I think that patch looks good now, thanks. No need to think about the trailers for that patch now, I think you can safely focus on the later patches from now on. ;-) Martin
Christian Couder <christian.couder@gmail.com> writes: > On Wed, Aug 5, 2020 at 12:04 AM Aaron Lipman <alipman88@gmail.com> wrote: >> >> OK, here's take 4! Responding to Junio's feedback, first: > > [...] > >> Martin, thanks for your suggestions > > [...] > > It's better to have the people you are replying to as recipients of > your emails (in the "To:" field). I have added them into "Cc:". > >> > (Signed-off-by: Martin Ågren <martin.agren@gmail.com>, FWIW.) >> >> I'm still getting used to the conventions - should I add your name as >> a signed-off-by tag, a thanks-to tag, or both? > > We often use the following trailers: > > - "Helped-by:" when someone helped you > - "Suggested-by:" when someone suggested the main idea in the patch > - "Reported-by:" when someone reported an issue fixed by the patch > - "Acked-by:" when someone explicitly acked the patch > - "Reviewed-by:" when someone explicitly gave their "Reviewed-by:" > > If your patch is based on a patch from someone else, you can also keep > the "Signed-off-by:" and other trailers that the person already put in > the commit message. If you haven't made a lot of changes to a patch > initially from someone else you can also keep them as the author. OK. It appears that the patches themselves do not have fundamental issues, perhaps other than test style updates for [2/5]? Aaron, Eric, can we have the hopefully final update to close this topic, if that is the case? Thanks.
On Fri, Aug 7, 2020 at 5:05 PM Junio C Hamano <gitster@pobox.com> wrote: > OK. It appears that the patches themselves do not have fundamental > issues, perhaps other than test style updates for [2/5]? > > Aaron, Eric, can we have the hopefully final update to close this > topic, if that is the case? I have not personally reviewed any of these patches, nor have I been following the topic. My one contribution to this thread[1] was merely to point out minor style violations in [2/5] which I noticed while quickly scrolling through the email. Consequently, I'm not in a position to say whether or not there are any fundamental issues with the changes made by any of these patches. [1]: https://lore.kernel.org/git/CAPig+cQumRSCQA3Et5=h7SD7zqMm_Z6LJVUTonkewR=hNR55Ug@mail.gmail.com/
Eric Sunshine <sunshine@sunshineco.com> writes: > Consequently, I'm not in a > position to say whether or not there are any fundamental issues with > the changes made by any of these patches. Sorry, that wasn't what I meant. A reroll would be done with an updated test, so I was asking your eyeball once again on that.
On Fri, Aug 7, 2020 at 6:07 PM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > Consequently, I'm not in a > > position to say whether or not there are any fundamental issues with > > the changes made by any of these patches. > > Sorry, that wasn't what I meant. A reroll would be done with an > updated test, so I was asking your eyeball once again on that. I eyeballed the test in the re-rolled [2/5] from a purely style perspective (not a content perspective), and it looks fine. I could say (and could previously have said) something about the lost exit codes from the $(git rev-parse ...) invocations within the here-doc in order to save Denton the trouble of eradicating them, but that seems relatively minor. Re-thinking the unindented here-docs on the other tests, I wonder now if that they might be easier to read if indented with \-EOF (rather than using plain EOF), as well: test_output_expect_success '--first-parent' 'git rev-list ...' <<-\EOF E ... e8 EOF but I doubt that such a minor style change is worth a re-roll (and I wouldn't ask Aaron to re-roll just for that).