Re: [PATCH v2 0/6] git-submodule.sh: convert part of cmd_update to C
> > To allow me to protest in a timely manner, I wanted to teach GitGitGadget > (which is the main reason I work on range-diff, as you undoubtedly suspect > by now) to warn me about such instances. I did not suspect that GGG is the prime motivation for range diff; as it proves useful (a) on its own, e.g. looking at the updates in pu and (b) is helping the current workflow very much.
Re: [PATCH v2 0/6] git-submodule.sh: convert part of cmd_update to C
Hi Stefan, On Tue, 17 Jul 2018, Stefan Beller wrote: > > A tangent. > > > > Because this "-- " is a conventional signature separator, MUAs like > > Emacs message-mode seems to omit everything below it from the quote > > while responding, making it cumbersome to comment on the tbdiff. > > > > Something to think about if somebody is contemplating on adding more > > to format-patch's cover letter. > > +cc Eric who needs to think about this tangent, then. > https://public-inbox.org/git/20180530080325.37520-1-sunsh...@sunshineco.com/ I think this is just a natural fall-out from the users' choice of mail program. Personally, I have no difficulty commenting on anything below the `--` separator. FWIW GitGitGadget follows the example of the `base-commit` footer and places this information *above* the `--` separator. > > >> 1: d4e1ec45740 ! 1: bbc8697a8ca git-submodule.sh: align error > > >> reporting for update mode to use path > > >> @@ -6,7 +6,6 @@ > > >> on its path, so let's do that for invalid update modes, too. > > >> > > >> Signed-off-by: Stefan Beller > > >> -Signed-off-by: Junio C Hamano > > >> > > >> diff --git a/git-submodule.sh b/git-submodule.sh > > >> --- a/git-submodule.sh > > > > This is quite unfortunate. I wonder if it is easy to tell > > range-diff that certain differences in the log message are to be > > ignored so that we can show that the first patch is unchanged in a > > case like this. This series has 4 really changed ones with 2 > > otherwise unchanged ones shown all as changed, which is not too bad, > > but for a series like sb/diff-colro-move-more reroll that has 9 > > patches, out of only two have real updated patches, showing > > otherwise unchanged 7 as changed like this hunk does would make the > > cover letter useless. It is a shame that adding range-diff to the > > cover does have so much potential. > > Actually I thought it was really cool, i.e. when using your queued branch > instead of my last sent branch, I can see any edits *you* did > (including fixing up typos or applying at slightly different bases). This is probably a good indicator that the practice on insisting on signing off on every patch, rather than just the merge commit, is something to re-think. Those are real changes relative to the original commit, after all, and if they are not desired, they should not be made. > The sign offs are a bit unfortunate as they are repetitive. > I have two conflicting points of view on that: > > (A) This sign off is inherent to the workflow. So we could > change the workflow, i.e. you pull series instead of applying them. > I think this "more in git, less in email" workflow would find supporters, > such as DScho (cc'd). > > The downside is that (1) you'd have to change your > workflow, i.e. instead of applying the patches at the base you think is > best for maintenance you'd have to tell the author "please rebase to $X"; > but that also has upsides, such as "If you want to have your series integrated > please merge with $Y and $Z" (looking at the object store stuff). > > The other (2) downside is that everyone else (authors, reviewers) have > to adapt as well. For authors this might be easy to adapt (push instead > of sending email sounds like a win). For reviewers we'd need to have > an easy way to review things "stored in git" and not exposed via email, > which is not obvious how to do. > > (B) The other point of view that I can offer is that we teach range-diff > to ignore certain patterns. Maybe in combination with interpret-trailers > this can be an easy configurable thing, or even a default to ignore > all sign offs? I thought about that myself. The reason: I was surprised, a couple of times, when I realized long after the fact, that some of my patches were changed without my knowledge nor blessing before being merged into `master`. To allow me to protest in a timely manner, I wanted to teach GitGitGadget (which is the main reason I work on range-diff, as you undoubtedly suspect by now) to warn me about such instances. The range-diff patch series has simmered too long at this stage, though, and I did not try to address such a "ignore " feature *specifically* so that the range-diff command could be available sooner than later. I already missed one major version, please refrain from forcing me to miss another one. Ciao, Dscho
Re: [PATCH v2 0/6] git-submodule.sh: convert part of cmd_update to C
Eric Sunshine writes: > I did consider placing the range-diff before the diffstat, however, > what convinced me to position range-diff last was that the diffstat is > usually short and easy to skip over both visually and via scrolling, > whereas the range-diff often is long and noisy, thus more difficult to > skip for someone more interested in the diffstat. I find that quite sensible, from a reader's point of view.
Re: [PATCH v2 0/6] git-submodule.sh: convert part of cmd_update to C
On Wed, Jul 18, 2018 at 3:34 PM Stefan Beller wrote: > On Tue, Jul 17, 2018 at 11:59 AM Eric Sunshine > wrote: > > The "git-format-patch --range-diff" option implemented by that patch > > series (and its upcoming re-roll) place the range-diff before the "-- > > " signature line, so this isn't a problem. > > Now that I grow more accustomed to range-diffs, I wonder > if we want to have them even *before* the short stat in the > cover letter. (I usually scroll over the short stat just to take it > as a FYI on what to expect, whereas the range-diff can already > be reviewed, hence seems more useful that the stats) I did consider placing the range-diff before the diffstat, however, what convinced me to position range-diff last was that the diffstat is usually short and easy to skip over both visually and via scrolling, whereas the range-diff often is long and noisy, thus more difficult to skip for someone more interested in the diffstat. So, the choice was deliberate. However, I also considered making it configurable, but that's probably something that can be done later, if needed, once we get more experience with the feature. It's also worth taking Duy's wish[1] for customization into account, as well, before designing such capability. [1]: https://public-inbox.org/git/CACsJy8D=6faepo5m4cc7kzyggaw1aosskuwaunqbfh0nr-y...@mail.gmail.com/
Re: [PATCH v2 0/6] git-submodule.sh: convert part of cmd_update to C
Hi Eric, On Tue, Jul 17, 2018 at 11:59 AM Eric Sunshine wrote: > > On Tue, Jul 17, 2018 at 2:53 PM Stefan Beller wrote: > > > A tangent. > > > > > > Because this "-- " is a conventional signature separator, MUAs like > > > Emacs message-mode seems to omit everything below it from the quote > > > while responding, making it cumbersome to comment on the tbdiff. > > > > > > Something to think about if somebody is contemplating on adding more > > > to format-patch's cover letter. > > > > +cc Eric who needs to think about this tangent, then. > > https://public-inbox.org/git/20180530080325.37520-1-sunsh...@sunshineco.com/ > > The "git-format-patch --range-diff" option implemented by that patch > series (and its upcoming re-roll) place the range-diff before the "-- > " signature line, so this isn't a problem. > > I think Junio's tangent was targeted more at humans blindly plopping > the copy/pasted range-diff at the end of the cover letter without > taking the "-- " signature line into account. (Indeed, Gmail hides > everything below the "-- " line by default, as well.) Awesome thanks! (I missed that hint given by Junio) Now that I grow more accustomed to range-diffs, I wonder if we want to have them even *before* the short stat in the cover letter. (I usually scroll over the short stat just to take it as a FYI on what to expect, whereas the range-diff can already be reviewed, hence seems more useful that the stats) Thanks, Stefan
Re: [PATCH v2 0/6] git-submodule.sh: convert part of cmd_update to C
Stefan Beller writes: >> As I most often edit the log message and material below three-dash >> lines (long) _after_ format-patch produced files, I do not think it >> is a win to force me to push and ask to pull > > Ah, that is an interesting workflow. Do you keep patch files/emails > around locally, only to (long after) add a message and resend it? The time I "finish" working on a series and commit is typically much closer to the time I format-patch the result, but it will take time for me to actually mail out these files. It will take time to re-review and re-think if the explanation in them makes sense to readers, and sending half-edited patch is something I try to avoid as it would be a waste of time for everybody involved (including me who would then need to respond to messages that helpfully point out silly typoes, in addition to those who spots them). I am trained myself not to touch these files after sending them out, as comparing them with a newer iteration of the correspoinding files was one way to see what has (and hasn't) changed between iterations before I learned "git tbdiff", and that habit persists.
Re: [PATCH v2 0/6] git-submodule.sh: convert part of cmd_update to C
On Tue, Jul 17, 2018 at 12:52 PM Junio C Hamano wrote: > > (A) This sign off is inherent to the workflow. So we could > > change the workflow, i.e. you pull series instead of applying them. > > I think this "more in git, less in email" workflow would find supporters, > > such as DScho (cc'd). > > Sign-off is inherent to the project, in the sense that we want to > see how the change flowed recorded in the commits. > > With a pull-request based workflow, until Git is somehow improved so > that a "pull" becomes "fetch and rebase what got fetched on top of > my stuff, and add my sign-off while at it" (which is the opposite of > the usual "pull --rebase"), and here is where our thoughts did not align. I imagined a git-pull as of today (fetch + merge), where you in the role of the maintainer tells me in the role of an author to base my series on top of $X, such that there is no need for rebase on your end, (you would also not sign off each commit, but only the resulting merge) Even merge conflicts could be handed off to the authors instead of burdening you. > we would lose the ability to fully "use" > Git to run this project, as we would lose most sign-offs, unlike the > e-mail based workflow, which we can use Git fully to have it do its > job of recording necessary information. I think all needed information would still be there, but there would be an actual change as authors would be committers (as they commit locally and we keep it all in Git to get it to you and you keep it in Git to put it into the blessed repository) > And much more importantly, when "pull-request" based workflow is > improved enough so that your original without my sign-off (and you > shouldn't, unless you are relaying my changes) becomes what I > pulled, which does have my sign-off, range-diff that compares both > histories does need to deal with a pair of commits with only one > side having a sign-off. So switching the tool is not a proper > solution to work around the "sign-off noise" we observed. I do not view it as work around, but "another proper workflow that has advantages and disadvantages, one of the advantages is that it would enable us to work with this tool". > One side > having a sign-off while the other side does not is inherent to what > we actively want, [in the current workflow that has proven good for 10 years] > and you are letting your tail wag your dog by > suggesting to discard it, which is disappointing. I am suggesting to continue thinking about workflows in general, as there are many; all of them having advantages and disadvantages. I am not sure if workflows can be adapted easily via improving the current workflow continually or if sometimes a workflow has to be rethought to to changes in the landscape of available tools. When the Git project started, an email based workflow was chosen, precisely because Git was not available. Now that it has gained wide spread adoption (among its developers at least) the workflow could adapt to that. > > The other (2) downside is that everyone else (authors, reviewers) have > > to adapt as well. For authors this might be easy to adapt (push instead > > of sending email sounds like a win). > > As I most often edit the log message and material below three-dash > lines (long) _after_ format-patch produced files, I do not think it > is a win to force me to push and ask to pull Ah, that is an interesting workflow. Do you keep patch files/emails around locally, only to (long after) add a message and resend it? I try to keep any contribution of mine in Git as long as possible as that helps me tracking and fixing errors in my code and log messages. > > (B) The other point of view that I can offer is that we teach range-diff > > to ignore certain patterns. Maybe in combination with interpret-trailers > > this can be an easy configurable thing, or even a default to ignore > > all sign offs? > > That indicates the same direction as I alluded to in the message you > are responding to, I guess, which is a good thing. Yes, I imagined this is the approach we'll be taking. I think we would want to give %(trailers:none) or rather "ignore-sign-offs" to the inner diffs. Thanks, Stefan
Re: [PATCH v2 0/6] git-submodule.sh: convert part of cmd_update to C
Stefan Beller writes: > Actually I thought it was really cool, i.e. when using your queued branch > instead of my last sent branch, I can see any edits *you* did > (including fixing up typos or applying at slightly different bases). Absolutely. I did not say that there needs a mode to ignore log message. > The sign offs are a bit unfortunate as they are repetitive. > I have two conflicting points of view on that: > > (A) This sign off is inherent to the workflow. So we could > change the workflow, i.e. you pull series instead of applying them. > I think this "more in git, less in email" workflow would find supporters, > such as DScho (cc'd). Sign-off is inherent to the project, in the sense that we want to see how the change flowed recorded in the commits. With a pull-request based workflow, until Git is somehow improved so that a "pull" becomes "fetch and rebase what got fetched on top of my stuff, and add my sign-off while at it" (which is the opposite of the usual "pull --rebase"), we would lose the ability to fully "use" Git to run this project, as we would lose most sign-offs, unlike the e-mail based workflow, which we can use Git fully to have it do its job of recording necessary information. And much more importantly, when "pull-request" based workflow is improved enough so that your original without my sign-off (and you shouldn't, unless you are relaying my changes) becomes what I pulled, which does have my sign-off, range-diff that compares both histories does need to deal with a pair of commits with only one side having a sign-off. So switching the tool is not a proper solution to work around the "sign-off noise" we observed. One side having a sign-off while the other side does not is inherent to what we actively want, and you are letting your tail wag your dog by suggesting to discard it, which is disappointing. > The other (2) downside is that everyone else (authors, reviewers) have > to adapt as well. For authors this might be easy to adapt (push instead > of sending email sounds like a win). As I most often edit the log message and material below three-dash lines (long) _after_ format-patch produced files, I do not think it is a win to force me to push and ask to pull. > (B) The other point of view that I can offer is that we teach range-diff > to ignore certain patterns. Maybe in combination with interpret-trailers > this can be an easy configurable thing, or even a default to ignore > all sign offs? That indicates the same direction as I alluded to in the message you are responding to, I guess, which is a good thing.
Re: [PATCH v2 0/6] git-submodule.sh: convert part of cmd_update to C
On Tue, Jul 17, 2018 at 2:53 PM Stefan Beller wrote: > > A tangent. > > > > Because this "-- " is a conventional signature separator, MUAs like > > Emacs message-mode seems to omit everything below it from the quote > > while responding, making it cumbersome to comment on the tbdiff. > > > > Something to think about if somebody is contemplating on adding more > > to format-patch's cover letter. > > +cc Eric who needs to think about this tangent, then. > https://public-inbox.org/git/20180530080325.37520-1-sunsh...@sunshineco.com/ The "git-format-patch --range-diff" option implemented by that patch series (and its upcoming re-roll) place the range-diff before the "-- " signature line, so this isn't a problem. I think Junio's tangent was targeted more at humans blindly plopping the copy/pasted range-diff at the end of the cover letter without taking the "-- " signature line into account. (Indeed, Gmail hides everything below the "-- " line by default, as well.)
Re: [PATCH v2 0/6] git-submodule.sh: convert part of cmd_update to C
> A tangent. > > Because this "-- " is a conventional signature separator, MUAs like > Emacs message-mode seems to omit everything below it from the quote > while responding, making it cumbersome to comment on the tbdiff. > > Something to think about if somebody is contemplating on adding more > to format-patch's cover letter. +cc Eric who needs to think about this tangent, then. https://public-inbox.org/git/20180530080325.37520-1-sunsh...@sunshineco.com/ > > >> 2.18.0.203.gfac676dfb9-goog > >> > >> 1: d4e1ec45740 ! 1: bbc8697a8ca git-submodule.sh: align error reporting > >> for update mode to use path > >> @@ -6,7 +6,6 @@ > >> on its path, so let's do that for invalid update modes, too. > >> > >> Signed-off-by: Stefan Beller > >> -Signed-off-by: Junio C Hamano > >> > >> diff --git a/git-submodule.sh b/git-submodule.sh > >> --- a/git-submodule.sh > > This is quite unfortunate. I wonder if it is easy to tell > range-diff that certain differences in the log message are to be > ignored so that we can show that the first patch is unchanged in a > case like this. This series has 4 really changed ones with 2 > otherwise unchanged ones shown all as changed, which is not too bad, > but for a series like sb/diff-colro-move-more reroll that has 9 > patches, out of only two have real updated patches, showing > otherwise unchanged 7 as changed like this hunk does would make the > cover letter useless. It is a shame that adding range-diff to the > cover does have so much potential. Actually I thought it was really cool, i.e. when using your queued branch instead of my last sent branch, I can see any edits *you* did (including fixing up typos or applying at slightly different bases). The sign offs are a bit unfortunate as they are repetitive. I have two conflicting points of view on that: (A) This sign off is inherent to the workflow. So we could change the workflow, i.e. you pull series instead of applying them. I think this "more in git, less in email" workflow would find supporters, such as DScho (cc'd). The downside is that (1) you'd have to change your workflow, i.e. instead of applying the patches at the base you think is best for maintenance you'd have to tell the author "please rebase to $X"; but that also has upsides, such as "If you want to have your series integrated please merge with $Y and $Z" (looking at the object store stuff). The other (2) downside is that everyone else (authors, reviewers) have to adapt as well. For authors this might be easy to adapt (push instead of sending email sounds like a win). For reviewers we'd need to have an easy way to review things "stored in git" and not exposed via email, which is not obvious how to do. (B) The other point of view that I can offer is that we teach range-diff to ignore certain patterns. Maybe in combination with interpret-trailers this can be an easy configurable thing, or even a default to ignore all sign offs? Thanks, Stefan
Re: [PATCH v2 0/6] git-submodule.sh: convert part of cmd_update to C
Stefan Beller writes: >> v2: >> addressed review comments, renaming the struct, improving the commit message. >> >> v1: >> https://public-inbox.org/git/20180712194754.71979-1-sbel...@google.com/ >> I thought about writing it all in one go, but the series got too large, >> so let's chew one bite at a time. >> >> Thanks, >> Stefan >> >> Stefan Beller (6): >> git-submodule.sh: align error reporting for update mode to use path >> git-submodule.sh: rename unused variables >> builtin/submodule--helper: factor out submodule updating >> builtin/submodule--helper: store update_clone information in a struct >> builtin/submodule--helper: factor out method to update a single >> submodule >> submodule--helper: introduce new update-module-mode helper >> >> builtin/submodule--helper.c | 152 >> git-submodule.sh| 22 +- >> 2 files changed, 122 insertions(+), 52 deletions(-) >> >> -- A tangent. Because this "-- " is a conventional signature separator, MUAs like Emacs message-mode seems to omit everything below it from the quote while responding, making it cumbersome to comment on the tbdiff. Something to think about if somebody is contemplating on adding more to format-patch's cover letter. >> 2.18.0.203.gfac676dfb9-goog >> >> 1: d4e1ec45740 ! 1: bbc8697a8ca git-submodule.sh: align error reporting >> for update mode to use path >> @@ -6,7 +6,6 @@ >> on its path, so let's do that for invalid update modes, too. >> >> Signed-off-by: Stefan Beller >> -Signed-off-by: Junio C Hamano >> >> diff --git a/git-submodule.sh b/git-submodule.sh >> --- a/git-submodule.sh This is quite unfortunate. I wonder if it is easy to tell range-diff that certain differences in the log message are to be ignored so that we can show that the first patch is unchanged in a case like this. This series has 4 really changed ones with 2 otherwise unchanged ones shown all as changed, which is not too bad, but for a series like sb/diff-colro-move-more reroll that has 9 patches, out of only two have real updated patches, showing otherwise unchanged 7 as changed like this hunk does would make the cover letter useless. It is a shame that adding range-diff to the cover does have so much potential.
[PATCH v2 0/6] git-submodule.sh: convert part of cmd_update to C
v2:
addressed review comments, renaming the struct, improving the commit message.
v1:
https://public-inbox.org/git/20180712194754.71979-1-sbel...@google.com/
I thought about writing it all in one go, but the series got too large,
so let's chew one bite at a time.
Thanks,
Stefan
Stefan Beller (6):
git-submodule.sh: align error reporting for update mode to use path
git-submodule.sh: rename unused variables
builtin/submodule--helper: factor out submodule updating
builtin/submodule--helper: store update_clone information in a struct
builtin/submodule--helper: factor out method to update a single
submodule
submodule--helper: introduce new update-module-mode helper
builtin/submodule--helper.c | 152
git-submodule.sh| 22 +-
2 files changed, 122 insertions(+), 52 deletions(-)
--
2.18.0.203.gfac676dfb9-goog
1: d4e1ec45740 ! 1: bbc8697a8ca git-submodule.sh: align error reporting for
update mode to use path
@@ -6,7 +6,6 @@
on its path, so let's do that for invalid update modes, too.
Signed-off-by: Stefan Beller
-Signed-off-by: Junio C Hamano
diff --git a/git-submodule.sh b/git-submodule.sh
--- a/git-submodule.sh
2: 9c5ec3fccea ! 2: 7e26af17578 git-submodule.sh: rename unused variables
@@ -14,8 +14,12 @@
using its own function starting in 48308681b07 (git submodule update:
have a dedicated helper for cloning, 2016-02-29), its removal was
missed.
+A later patch in this series also touches the communication between
+the submodule helper and git-submodule.sh, but let's have this as
+a preparatory patch, as it eases the next patch, which stores the
+raw data instead of the line printed for this communication.
+
Signed-off-by: Stefan Beller
-Signed-off-by: Junio C Hamano
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
--- a/builtin/submodule--helper.c
3: a3fb4e5539f ! 3: 3e8d22b0c70 builtin/submodule--helper: factor out
submodule updating
@@ -7,7 +7,6 @@
most of it is still in git-submodule.sh.
Signed-off-by: Stefan Beller
-Signed-off-by: Junio C Hamano
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
--- a/builtin/submodule--helper.c
4: e680684139d ! 4: 5e0a39015df builtin/submodule--helper: store update_clone
information in a struct
@@ -11,7 +11,6 @@
struct.
Signed-off-by: Stefan Beller
-Signed-off-by: Junio C Hamano
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
--- a/builtin/submodule--helper.c
@@ -20,7 +19,7 @@
return 0;
}
-+struct submodule_update_clone_information {
++struct update_clone_data {
+ const struct submodule *sub;
+ struct object_id oid;
+ unsigned just_cloned;
@@ -36,8 +35,8 @@
- /* Machine-readable status lines to be consumed by git-submodule.sh */
- struct string_list projectlines;
+ /* to be consumed by git-submodule.sh */
-+ struct submodule_update_clone_information *submodule_lines;
-+ int submodule_lines_nr; int submodule_lines_alloc;
++ struct update_clone_data *update_clone;
++ int update_clone_nr; int update_clone_alloc;
/* If we want to stop as fast as possible and return an error */
unsigned quickstop : 1;
@@ -58,12 +57,12 @@
- strbuf_addf(, "dummy %s %d\t%s\n",
- oid_to_hex(>oid), needs_cloning, ce->name);
- string_list_append(>projectlines, sb.buf);
-+ ALLOC_GROW(suc->submodule_lines, suc->submodule_lines_nr + 1,
-+ suc->submodule_lines_alloc);
-+ oidcpy(>submodule_lines[suc->submodule_lines_nr].oid, >oid);
-+ suc->submodule_lines[suc->submodule_lines_nr].just_cloned =
needs_cloning;
-+ suc->submodule_lines[suc->submodule_lines_nr].sub = sub;
-+ suc->submodule_lines_nr++;
++ ALLOC_GROW(suc->update_clone, suc->update_clone_nr + 1,
++ suc->update_clone_alloc);
++ oidcpy(>update_clone[suc->update_clone_nr].oid, >oid);
++ suc->update_clone[suc->update_clone_nr].just_cloned = needs_cloning;
++ suc->update_clone[suc->update_clone_nr].sub = sub;
++ suc->update_clone_nr++;
if (!needs_cloning)
goto cleanup;
@@ -83,11 +82,11 @@
- for_each_string_list_item(item, >projectlines)
- fprintf(stdout, "%s", item->string);
-+ for (i = 0; i < suc->submodule_lines_nr; i++) {
++ for (i = 0; i < suc->update_clone_nr; i++) {
+ strbuf_addf(, "dummy %s %d\t%s\n",
-+ oid_to_hex(>submodule_lines[i].oid),
-+ suc->submodule_lines[i].just_cloned,
-+ suc->submodule_lines[i].sub->path);
++
