| Message ID | 20200110195533.6416-1-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fetch: emphasize failure during submodule fetch | expand |
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);
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);
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);
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(+)