Re: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts

Thanks for splitting these changes into smaller, more manageable chunks. A couple non-code comments below apply to both patches in this 2-patch series even though I'm responding only to this patch. (The actual code changes in the other patch looked fine and the patch was easily digested.)

On Fri, Mar 23, 2018 at 12:39 AM, Wink Saville <wink@saville.com> wrote:

Nit: On this project, the summary line is not capitalized, so: s/Update/update/

On this project, commit messages are written in imperative mood. The commit message Junio suggested[1] said "are structured", which makes for a better imperative mood fit.

ECANTPARSE: This paragraph is grammatically corrupt.

?  "When {something}..." but then what?

?  "...when exiting with a "return" is executed"

Junio's proposed commit message[1] called this a "workaround", not a "fix", and, indeed, "workaround" better characterizes that change. If anything, _this_ patch is a (more correct) "fix" for that workaround.

Junio's version, using imperative mood, said "Move the call...".

ECANTPARSE: Grammatically corrupt.

?  "where the ... library and allows..."

Overall, Junio's proposed message followed project practice (imperative mood) more closely and felt somewhat more coherent (despite the run-on sentence in the first paragraph and the apparent incorrect explanation of top-level "return" misbehavior -- the in-code comment says top-level "return" was essentially a no-op in broken shells, whereas he said it exited the shell).

Perhaps the following re-write addresses the above concerns:

    Due to historical reasons, the backend scriptlets for "git rebase"
    are structured a bit unusually. As originally designed,
    dot-sourcing them from "git rebase" was sufficient to invoke the
    specific backend.

    However, it was later discovered that some shell implementations
    (e.g. FreeBSD 9.x) misbehaved by continuing to execute statements
    following a top-level "return" rather than returning control to
    the next statement in "git rebase" after dot-sourcing the
    scriptlet. To work around this shortcoming, the whole body of
    git-rebase--$backend.sh was made into a shell function
    git_rebase__$backend, and then the very last line of the scriptlet
    called that function.

    A more normal architecture is for a dot-sourced scriptlet merely
    to define functions (thus acting as a function library), and for
    those functions to be called by the script doing the dot-sourcing.
    Migrate to this arrangement by moving the git_rebase__$backend
    call from the end of a scriptlet into "git rebase" itself.

    While at it, remove the large comment block from each scriptlet
    explaining this historic anomaly since it serves no purpose under
    the new normalized architecture in which a scriptlet is merely a
    function library.

Despite its name, on this project, a Reviewed-by: does not mean merely that a person looked at and commented on a patch. Rather, it is a way for a person to say "I have studied and understood the patch and feel that it is ready for inclusion in the project." Reviewed-by:'s are therefore always given explicitly by the reviewer and Junio adds them to a patch when queuing. (Reviewed-by:'s are not always given, though, even when a reviewer has not found problems with a patch. For instance, even if I review and comment on this or subsequent patches, I will not give a Reviewed-by: since I'm not an area expert, thus wouldn't feel comfortable stating that the patch is correct.)

Consequently, these Reviewed-by: lines should be dropped. (You can, on the other hand, add Helped-by:'s when appropriate.)

The patch itself makes sense and seems straightforward. See one minor comment below...

[1]: https://public-inbox.org/git/xmqqefkbltxv.fsf@gitster-ct.c.googlers.com/

The new comment merely repeats what the two lines of code themselves already state clearly, thus the comment is an unnecessary distraction; it does not aid in understanding the code.