fetch: emphasize failure during submodule fetch
diff mbox series

Message ID 20200110195533.6416-1-emilyshaffer@google.com
State New
Headers show
Series
  • fetch: emphasize failure during submodule fetch
Related show

Commit Message

Emily Shaffer Jan. 10, 2020, 7:55 p.m. UTC
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'. As I was tracking down the right place to "bump" the error
line, I noticed that 'git pull' is made out of run_command() calls, ever
since it was adopted from bash ~5 years ago. Is there interest in
refactoring it to use library calls instead, or do folks consider 'pull'
to be such a thin layer over 'git fetch && git (rebase|merge)' that it's
not worth the effort?

 - Emily

 builtin/fetch.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Junio C Hamano Jan. 10, 2020, 8:18 p.m. UTC | #1
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.

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?

> 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?

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

>  	}
>  
>  	string_list_clear(&list, 0);
Emily Shaffer Jan. 10, 2020, 11:01 p.m. UTC | #2
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);

Patch
diff mbox series

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"));
 	}
 
 	string_list_clear(&list, 0);