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@xxxxxxxxxxx> wrote:
> rebase: Update invocation of rebase dot-sourced scripts
Nit: On this project, the summary line is not capitalized, so: s/Update/update/
> The backend scriptlets for "git rebase" were structured in a
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.
> bit unusual way for historical reasons. Originally, it was
> designed in such a way that dot-sourcing them from "git
> rebase" would be sufficient to invoke the specific backend.
>
> When it was discovered that some shell implementations
> (e.g. FreeBSD 9.x) misbehaved when exiting with a "return"
> is executed at the top level of a dot-sourced script (the
> original was expecting that the control returns to the next
> command in "git rebase" after dot-sourcing the scriptlet).
ECANTPARSE: This paragraph is grammatically corrupt.
? "When {something}..." but then what?
? "...when exiting with a "return" is executed"
> To fix this issue the whole body of git-rebase--$backend.sh
> was made into a shell function git_rebase__$backend and then
> the last statement of the scriptlet would invoke the function.
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.
> Here the call is moved to "git rebase" side, instead of at the
Junio's version, using imperative mood, said "Move the call...".
> end of each scriptlet. This give us a more normal arrangement
> where the scriptlet function library and allows multiple functions
> to be implemented in a scriptlet.
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.
> Signed-off-by: Wink Saville <wink@xxxxxxxxxxx>
> Reviewed-by: Junio C Hamano <gitster@xxxxxxxxx>
> Reviewed-by: Eric Sunsine <sunsine@xxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxxxxxx/
> ---
> diff --git a/git-rebase.sh b/git-rebase.sh
> index a1f6e5de6..4595a316a 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -196,7 +196,9 @@ run_specific_rebase () {
> export GIT_EDITOR
> autosquash=
> fi
> + # Source the code and invoke it
> . git-rebase--$type
> + git_rebase__$type
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.
