Re: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts
On Fri, Mar 23, 2018 at 5:01 PM, Junio C Hamano wrote: > Eric Sunshine writes: >> (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). > > My bad, almost entirely. Sorry. No apology necessary. That minor error aside, your proposed commit message gave just the right amount of detail for a person (me) not at all familiar with the topic to be able to understand it fully and intuit the patch content before even reading the patch proper. That's a good commit message.
Re: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts
On Fri, Mar 23, 2018 at 1:51 PM, Junio C Hamano wrote: > Wink Saville writes: > >> Here is one possibility: >> >> git format-patch --cover-letter --rfc --thread -v 5 >> --to=git@vger.kernel.org --cc=sunsh...@sunshineco.com >> --cc=johannes.schinde...@gmx.de -o patches/v5 master..v5-2 > > Sounds sensible. > >> If this was the first version then the above would seem to be a >> reasonable choice. > > My personal preference (both as a reviewer and an occasional > multi-patch series submitter) is to use a cover letter for a larger > series (e.g. more than 3-5 patches), regardless of the iteration. > In fact, a submitter tends to have _more_ things to say in the cover > letter for v2 and subsequent iteration than the original iteration. > > The motivation behind the series may not change so greatly but will > be refined as iterations go on, and you want help those who missed > the earlier iteration understand what you are doing with the updated > cover letter. Also cover letter is the ideal place to outline where > to find older iterations and their discussion and summarize what > changed since these earlier attempts in this round. > >> But this is version 5 and maybe I don't need --cover-letter which, I >> think means I >> don't want to use --thread. If that's the case should I add --in-reply-to? >> But >> that leads to the question. from which message should I get the Message-Id? > > The most typical practice I've seen around here is that v5's cover > is made in-reply-to v4's cover. > Make sense
Re: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts
Eric Sunshine writes: >> When it was discovered that some shell implementations > ... > ECANTPARSE: This paragraph is grammatically corrupt. > > ECANTPARSE: Grammatically corrupt. > ... > (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). My bad, almost entirely. Sorry.
Re: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts
Wink Saville writes: > Here is one possibility: > > git format-patch --cover-letter --rfc --thread -v 5 > --to=git@vger.kernel.org --cc=sunsh...@sunshineco.com > --cc=johannes.schinde...@gmx.de -o patches/v5 master..v5-2 Sounds sensible. > If this was the first version then the above would seem to be a > reasonable choice. My personal preference (both as a reviewer and an occasional multi-patch series submitter) is to use a cover letter for a larger series (e.g. more than 3-5 patches), regardless of the iteration. In fact, a submitter tends to have _more_ things to say in the cover letter for v2 and subsequent iteration than the original iteration. The motivation behind the series may not change so greatly but will be refined as iterations go on, and you want help those who missed the earlier iteration understand what you are doing with the updated cover letter. Also cover letter is the ideal place to outline where to find older iterations and their discussion and summarize what changed since these earlier attempts in this round. > But this is version 5 and maybe I don't need --cover-letter which, I > think means I > don't want to use --thread. If that's the case should I add --in-reply-to? But > that leads to the question. from which message should I get the Message-Id? The most typical practice I've seen around here is that v5's cover is made in-reply-to v4's cover.
Re: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts
On Fri, Mar 23, 2018 at 10:12 AM, Johannes Schindelin wrote: > Hi Wink, > > On Thu, 22 Mar 2018, Wink Saville wrote: > >> The backend scriptlets for "git rebase" were structured in a >> 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). >> >> 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. >> >> Here the call is moved to "git rebase" side, instead of at the >> 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. >> >> Signed-off-by: Wink Saville >> Reviewed-by: Junio C Hamano >> Reviewed-by: Eric Sunsine >> --- >> git-rebase--am.sh | 11 --- >> git-rebase--interactive.sh | 11 --- >> git-rebase--merge.sh | 11 --- >> git-rebase.sh | 2 ++ > > The patch makes sense to me. > > Thanks, > Johannes Junio, Eric and Johannes, thanks for the help!!! I've created v5 with the two patches, what is the suggested format-patch/send-email command(s)? Here is one possibility: git format-patch --cover-letter --rfc --thread -v 5 --to=git@vger.kernel.org --cc=sunsh...@sunshineco.com --cc=johannes.schinde...@gmx.de -o patches/v5 master..v5-2 If this was the first version then the above would seem to be a reasonable choice. But this is version 5 and maybe I don't need --cover-letter which, I think means I don't want to use --thread. If that's the case should I add --in-reply-to? But that leads to the question. from which message should I get the Message-Id? More likely I'm totally wrong and should do something completely different, advice appreciated. -- Wink
Re: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts
Hi Wink, On Thu, 22 Mar 2018, Wink Saville wrote: > The backend scriptlets for "git rebase" were structured in a > 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). > > 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. > > Here the call is moved to "git rebase" side, instead of at the > 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. > > Signed-off-by: Wink Saville > Reviewed-by: Junio C Hamano > Reviewed-by: Eric Sunsine > --- > git-rebase--am.sh | 11 --- > git-rebase--interactive.sh | 11 --- > git-rebase--merge.sh | 11 --- > git-rebase.sh | 2 ++ The patch makes sense to me. Thanks, Johannes
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 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
> Reviewed-by: Junio C Hamano
> Reviewed-by: Eric Sunsine
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@gitster-ct.c.googlers.com/
> ---
> diff --git a/git-rebase.sh b/git-rebase.sh
> index a1f6e5de6..4595a31
