v2, 6 partsgit-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-sbeller@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(-)

All other error messages in cmd_update are reporting the submodule based on its path, so let's do that for invalid update modes, too.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

The 'mode' variable is not used in cmd_update for its original purpose, rename it to 'dummy' as it only serves the purpose to abort quickly documenting this knowledge.

The variable 'stage' is also not used any more in cmd_update, so remove it.

This went unnoticed as first each function used the commonly used submodule listing, which was converted in 74703a1e4df (submodule: rewrite `module_list` shell function in C, 2015-09-02). When cmd_update was 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 <sbeller@google.com>
---
 builtin/submodule--helper.c | 5 ++---
 git-submodule.sh            | 4 ++--
 2 files changed, 4 insertions(+), 5 deletions(-)

Separate the command line parsing from the actual execution of the command within the repository. For now there is not a lot of execution as most of it is still in git-submodule.sh.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 59 +++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 26 deletions(-)

The information that is printed for update_submodules in 'submodule--helper update-clone' and consumed by 'git submodule update' is stored as a string per submodule. This made sense at the time of 48308681b07 (git submodule update: have a dedicated helper for cloning, 2016-02-29), but as we want to migrate the rest of the submodule update into C, we're better off having access to the raw information in a helper struct.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

In a later patch we'll find this method handy.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

This chews off a bit of the shell part of the update command in git-submodule.sh. When writing the C code, keep in mind that the submodule--helper part will go away eventually and we want to have a C function that is able to determine the submodule update strategy, it as a nicety, make determine_submodule_update_strategy accessible for arbitrary repositories.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 61 +++++++++++++++++++++++++++++++++++++
 git-submodule.sh            | 16 +---------
 2 files changed, 62 insertions(+), 15 deletions(-)

Various compilers warn about the potential insecurity of the above call:

      CC builtin/submodule--helper.o
  builtin/submodule--helper.c: In function ‘module_update_module_mode’:
  builtin/submodule--helper.c:1502:2: error: format not a string literal and no format arguments [-Werror=format-security]
    fprintf(stdout, submodule_strategy_to_string(&update_strategy));
    ^
  cc1: all warnings being treated as errors
  Makefile:2261: recipe for target 'builtin/submodule--helper.o' failed
  make: *** [builtin/submodule--helper.o] Error 1

I think it should either use an explicit format string:

  fprintf(stdout, "%s", submodule_strategy_to_string(&update_strategy));

or, perhaps better yet, simply use fputs().

SZEDER Gábor <szeder.dev@gmail.com> writes:

Sounds good.

Stefan Beller <sbeller@google.com> writes:

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.

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.

+cc Eric who needs to think about this tangent, then. https://public-inbox.org/git/20180530080325.37520-1-sunshine@sunshineco.com/

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

On Tue, Jul 17, 2018 at 2:53 PM Stefan Beller <sbeller@google.com> 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.

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.)

Hi Eric, On Tue, Jul 17, 2018 at 11:59 AM Eric Sunshine <sunshine@sunshineco.com> wrote:

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

On Wed, Jul 18, 2018 at 3:34 PM Stefan Beller <sbeller@google.com> wrote:

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-YrdA@mail.gmail.com/

Eric Sunshine <sunshine@sunshineco.com> writes:

I find that quite sensible, from a reader's point of view.

Stefan Beller <sbeller@google.com> writes:

Absolutely. I did not say that there needs a mode to ignore log message.

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.

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.

That indicates the same direction as I alluded to in the message you are responding to, I guess, which is a good thing.

On Tue, Jul 17, 2018 at 12:52 PM Junio C Hamano <gitster@pobox.com> wrote:

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.

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)

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".

[in the current workflow that has proven good for 10 years]

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.

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.

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

Stefan Beller <sbeller@google.com> writes:

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.

Hi Stefan,

On Tue, 17 Jul 2018, Stefan Beller wrote:

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.

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.

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 <regex>" 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

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.