Re: [PATCH] fetch: emphasize failure during submodule fetch

[Emily Shaffer <emilyshaffer@google.com>]

On Fri, Jan 10, 2020 at 12:18:30PM -0800, Junio C Hamano wrote:
> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > In cases when a submodule fetch fails when there are many submodules, the error
> > from the lone failing submodule fetch is buried under activity on the other
> > submodules if more than one fetch fell back on fetch-by-oid. Call out a failure
> > late so the user is aware that something went wrong.
> >
> > Example without this change:
> 
> >   $ git pull --rebase
> >   remote: Counting objects: 1591, done
> >   remote: Finding sources: 100% (4317/4317)
> >   remote: Total 4317 (delta 1923), reused 4252 (delta 1923)
> >   Receiving objects: 100% (4317/4317), 2.09 MiB | 8.15 MiB/s, done.
> >   Resolving deltas: 100% (1923/1923), completed with 101 local objects.
> >   From https://android.googlesource.com/platform/superproject
> >   [snip ~100 lines]
> >   From https://android.googlesource.com/platform/prebuilts/fullsdk/platforms/android-29
> >    * branch            a97149980b7d8acf48392af591b35689f7205d9e -> FETCH_HEAD
> >   From https://android.googlesource.com/platform/prebuilts/fullsdk-darwin/platform-tools
> >    * branch            98f9454af8ca210818eff4f502097c471d7327b5 -> FETCH_HEAD
> >   From https://android.googlesource.com/platform/prebuilts/checkstyle
> >    * branch            6fb3e23f05ed186908ea9f48d6692220891363b0 -> FETCH_HEAD
> >    * branch            f21d92f6339f0993a946b25fa2172c2ceb5e332b -> FETCH_HEAD
> >   From https://android.googlesource.com/platform/prebuilts/androidx/studio
> >    * branch            bed5e7b5866b8698bbcd1879134b03ac312a2ba8 -> FETCH_HEAD
> >   From https://android.googlesource.com/platform/prebuilts/androidx/internal
> >    * branch                179375220f834de5dfbee169f4c2f948d850a203 -> FETCH_HEAD
> >    * branch                1dcf3ceef9a86001c693fa34b3513f0c4af26178 -> FETCH_HEAD
> >    * branch                2ea3ccef4c98f5de1b74affd1dda33f5b2834a45 -> FETCH_HEAD
> >    * branch                a09de09c3814c3d31cc770d5351b92d29ea624ae -> FETCH_HEAD
> >    * branch                d2ae6add8b2c0e28899e4faeb2d6889ceefb0b62 -> FETCH_HEAD
> >    * branch                e244e2a5f7d98f47f75d06ef57ef1c6c5701a38d -> FETCH_HEAD
> >   Auto packing the repository in background for optimum performance.
> >   See "git help gc" for manual housekeeping.
> >   From https://android.googlesource.com/platform/prebuilts/androidx/external
> >    * branch              c3df2fa7f3e63b8714ac8d24f86a26cc50ee4af5 -> FETCH_HEAD
> >   fatal: remote error: want c5bd7796550b3742772c8fb8c73a1311013b5159 not valid
> >   From https://android.googlesource.com/platform/external/noto-fonts
> >    * branch            02969d3046f6944a5a211d2331d1c82736487f01 -> FETCH_HEAD
> >    * branch            9ee45fcd0b8bb8621c1cdbc6de5fe7502eff7393 -> FETCH_HEAD
> >   From https://android.googlesource.com/platform/external/dokka
> >    * branch            03a8ed966a7b847931a0ee20327f989837aaff13 -> FETCH_HEAD
> >    * branch            cb1684602b5b4e18385d890c972764c55d177704 -> FETCH_HEAD
> >    * branch            fd4521e89ab0e01447dda9b42be2b9bbc000f02f -> FETCH_HEAD
> >   From https://android.googlesource.com/platform/external/doclava
> >    * branch            04ddf3962f0cd40c81a2e144f27f497223782457 -> FETCH_HEAD
> >    * branch            44bf22680e939b21a21a365f6038d5883d5163c8 -> FETCH_HEAD
> >    * branch            66f673f4a3865f3b4ab645655a6484101dbd051f -> FETCH_HEAD
> >
> > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> > ---
> > As hinted by the snippet in the commit-message (should I remove it? I
> > think it's a poignant example, I couldn't see the fatal without grepping
> > even after being told it was there) this manifested to an end user via
> > 'git pull'.
> 
> It indeed is too noisy, especially without showing what happens with
> this patch.

Sure, it makes sense. I'll take it out in next round.

> 
> Is it clear to the users that a block of lines starting "From $URL"
> and ending before the next "From $AnotherURL" is about the same
> repository, including error messages?

Well, for me - and the bug reporter - the "fatal" line visually blends
in with the "From" next to it. I think once you see the "fatal" line
it's clear where it's coming from, sure.

I wonder if the line order still holds with -j specified, though.

> 
> > diff --git a/builtin/fetch.c b/builtin/fetch.c
> > index b4c6d921d0..0c19781cb9 100644
> > --- a/builtin/fetch.c
> > +++ b/builtin/fetch.c
> > @@ -1857,6 +1857,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> >  						    verbosity < 0,
> >  						    max_children);
> >  		argv_array_clear(&options);
> > +		if (result)
> > +			fprintf(stderr, _("Failure during submodule fetch.\n"));
> 
> How does a user find out which submodule had trouble with after
> seeing this message?  Or is it something you still need to find by
> scrolling back?

The handiest way is probably the latter; maybe there is some way to
achieve the former, but my submodule fu isn't strong enough for me to
answer from the top of my head.

> 
> If the latter, I am not sure if there is much point to add a
> half-way solution like this.  It is a different story if "fetch"
> exits with success status when this happens, but I do not think the
> "result" that is non-zero is being lost before the function returns,
> so...

I agree, although I do find it irritating that there's no final
success/failure log line from 'git fetch'. I personally don't run 'echo
$?' after every step in my Git workflow.

It's less trivial (a low bar) to try and point out the submodule(s) which
had an issue by this point, but I can give it a shot if you are open to
the change.

> 
> >  	}
> >  
> >  	string_list_clear(&list, 0);