[PATCH] fetch: emphasize failure during submodule fetch

[PATCH] fetch: emphasize failure during submodule fetch

[Emily Shaffer]

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(+)

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);
-- 
2.25.0.rc1.283.g88dfdc4193-goog

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

[Junio C Hamano]

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

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

[Emily Shaffer]

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 v2] fetch: emphasize failure during submodule fetch

[Emily Shaffer]

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, and where.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
V1's approach was to show a generic error based on the output status of
fetch_populated_submodules(); with a long set of submodules, this is
only marginally better than showing the error inline. For v2, instead
we're gathering a list of submodules which failed during the parallel
processing.

The contents of stderr at the time fetch_finish() is called is not
available to us; 'err' on the input is for providing output only. So,
gather the submodule name only.

 - Emily

 submodule.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/submodule.c b/submodule.c
index 9da7181321..13bc9354bc 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1280,10 +1280,13 @@ struct submodule_parallel_fetch {
 	/* Pending fetches by OIDs */
 	struct fetch_task **oid_fetch_tasks;
 	int oid_fetch_tasks_nr, oid_fetch_tasks_alloc;
+
+	struct strbuf submodules_with_errors;
+	pthread_mutex_t submodule_errors_mutex;
 };
 #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, \
 		  STRING_LIST_INIT_DUP, \
-		  NULL, 0, 0}
+		  NULL, 0, 0, STRBUF_INIT, PTHREAD_MUTEX_INITIALIZER}
 
 static int get_fetch_recurse_config(const struct submodule *submodule,
 				    struct submodule_parallel_fetch *spf)
@@ -1547,7 +1550,10 @@ static int fetch_finish(int retvalue, struct strbuf *err,
 	struct string_list_item *it;
 	struct oid_array *commits;
 
-	if (retvalue)
+	if (!task || !task->sub)
+		BUG("callback cookie bogus");
+
+	if (retvalue) {
 		/*
 		 * NEEDSWORK: This indicates that the overall fetch
 		 * failed, even though there may be a subsequent fetch
@@ -1557,8 +1563,11 @@ static int fetch_finish(int retvalue, struct strbuf *err,
 		 */
 		spf->result = 1;
 
-	if (!task || !task->sub)
-		BUG("callback cookie bogus");
+		pthread_mutex_lock(&spf->submodule_errors_mutex);
+		strbuf_addf(&spf->submodules_with_errors, "\t%s\n",
+			    task->sub->name);
+		pthread_mutex_unlock(&spf->submodule_errors_mutex);
+	}
 
 	/* Is this the second time we process this submodule? */
 	if (task->commits)
@@ -1627,6 +1636,11 @@ int fetch_populated_submodules(struct repository *r,
 				   &spf,
 				   "submodule", "parallel/fetch");
 
+	if (spf.submodules_with_errors.len > 0)
+		fprintf(stderr, "Errors during submodule fetch:\n%s",
+			spf.submodules_with_errors.buf);
+
+
 	argv_array_clear(&spf.args);
 out:
 	free_submodules_oids(&spf.changed_submodule_names);
-- 
2.25.0.rc1.283.g88dfdc4193-goog

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

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> @@ -1280,10 +1280,13 @@ struct submodule_parallel_fetch {
>  	/* Pending fetches by OIDs */
>  	struct fetch_task **oid_fetch_tasks;
>  	int oid_fetch_tasks_nr, oid_fetch_tasks_alloc;
> +
> +	struct strbuf submodules_with_errors;
> +	pthread_mutex_t submodule_errors_mutex;

Hmph, it is kind of surprising that we need a new mutex for this.

Isn't the task_finish handler, which is what accesses the
with_errors field this patch adds, called by pp_collect_finished()
one at a time, is it?

It seems oid_fetch_tasks[] array is also a shared resource in this
structure among the parallel fetch tasks, but there is no protection
against simultaneous access to it.  Am I missing what makes the new
field different?  Somewhat puzzled...

Other than that, I think this is a vast improvement relative to the
initial round.  I wonder if we want to _("i18n/l10n") the message,
though.

Thanks.


>  #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, \
>  		  STRING_LIST_INIT_DUP, \
> -		  NULL, 0, 0}
> +		  NULL, 0, 0, STRBUF_INIT, PTHREAD_MUTEX_INITIALIZER}
>  
>  static int get_fetch_recurse_config(const struct submodule *submodule,
>  				    struct submodule_parallel_fetch *spf)
> @@ -1547,7 +1550,10 @@ static int fetch_finish(int retvalue, struct strbuf *err,
>  	struct string_list_item *it;
>  	struct oid_array *commits;
>  
> -	if (retvalue)
> +	if (!task || !task->sub)
> +		BUG("callback cookie bogus");
> +
> +	if (retvalue) {
>  		/*
>  		 * NEEDSWORK: This indicates that the overall fetch
>  		 * failed, even though there may be a subsequent fetch
> @@ -1557,8 +1563,11 @@ static int fetch_finish(int retvalue, struct strbuf *err,
>  		 */
>  		spf->result = 1;
>  
> -	if (!task || !task->sub)
> -		BUG("callback cookie bogus");
> +		pthread_mutex_lock(&spf->submodule_errors_mutex);
> +		strbuf_addf(&spf->submodules_with_errors, "\t%s\n",
> +			    task->sub->name);
> +		pthread_mutex_unlock(&spf->submodule_errors_mutex);
> +	}
>  
>  	/* Is this the second time we process this submodule? */
>  	if (task->commits)
> @@ -1627,6 +1636,11 @@ int fetch_populated_submodules(struct repository *r,
>  				   &spf,
>  				   "submodule", "parallel/fetch");
>  
> +	if (spf.submodules_with_errors.len > 0)
> +		fprintf(stderr, "Errors during submodule fetch:\n%s",
> +			spf.submodules_with_errors.buf);
> +
> +
>  	argv_array_clear(&spf.args);
>  out:
>  	free_submodules_oids(&spf.changed_submodule_names);

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

[Emily Shaffer]

On Thu, Jan 16, 2020 at 10:23:58AM -0800, Junio C Hamano wrote:
> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > @@ -1280,10 +1280,13 @@ struct submodule_parallel_fetch {
> >  	/* Pending fetches by OIDs */
> >  	struct fetch_task **oid_fetch_tasks;
> >  	int oid_fetch_tasks_nr, oid_fetch_tasks_alloc;
> > +
> > +	struct strbuf submodules_with_errors;
> > +	pthread_mutex_t submodule_errors_mutex;
> 
> Hmph, it is kind of surprising that we need a new mutex for this.
> 
> Isn't the task_finish handler, which is what accesses the
> with_errors field this patch adds, called by pp_collect_finished()
> one at a time, is it?

Hm. It is called by pp_collect_finished() one at a time, but while other
processes may still be running. So I guess that is OK - spf might still
be read by other tasks but this field of it won't be touched by anybody
simultaneously. Ok, I'm convinced.

> It seems oid_fetch_tasks[] array is also a shared resource in this
> structure among the parallel fetch tasks, but there is no protection
> against simultaneous access to it.  Am I missing what makes the new
> field different?  Somewhat puzzled...

I think it's similar. As I understand it, it looks something like this:

  loop forever:
    can i start a new process?
      get_next_task cb (blocking)
      start work cb (nonblocking unless it failed to start)
    process stderr in/out once (blocking)
    is anybody done? (blocking)
      task_finished cb (blocking) <- My change is in here
        did fetch by ref fail? (blocking)
          put fetch by OID onto the process list (blocking)
    is everybody done?
      break

That is, everything but the work unit itself is blocking and runs in a
single threaded infinite loop. So since oid_fetch_tasks is read in
get_next_task callback and modified in the task_finished callback, those
areas don't need thread protection.

Thanks for poking me to think it through better. I'll remove the mutex
and include a short note about why it's not needed in the commit message.
I suppose if I wanted to try and catch more precise error information
during the actual work, then I would need it, but I'm not sure it's
necessary or trivial because of how the stdout/stderr is handled for
cohesive printing.

> Other than that, I think this is a vast improvement relative to the
> initial round.  I wonder if we want to _("i18n/l10n") the message,
> though.

Sure, sorry to have missed it.

Thanks for the thoughtful review. Will send a reroll in a moment.

 - Emily

> 
> 
> >  #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, \
> >  		  STRING_LIST_INIT_DUP, \
> > -		  NULL, 0, 0}
> > +		  NULL, 0, 0, STRBUF_INIT, PTHREAD_MUTEX_INITIALIZER}
> >  
> >  static int get_fetch_recurse_config(const struct submodule *submodule,
> >  				    struct submodule_parallel_fetch *spf)
> > @@ -1547,7 +1550,10 @@ static int fetch_finish(int retvalue, struct strbuf *err,
> >  	struct string_list_item *it;
> >  	struct oid_array *commits;
> >  
> > -	if (retvalue)
> > +	if (!task || !task->sub)
> > +		BUG("callback cookie bogus");
> > +
> > +	if (retvalue) {
> >  		/*
> >  		 * NEEDSWORK: This indicates that the overall fetch
> >  		 * failed, even though there may be a subsequent fetch
> > @@ -1557,8 +1563,11 @@ static int fetch_finish(int retvalue, struct strbuf *err,
> >  		 */
> >  		spf->result = 1;
> >  
> > -	if (!task || !task->sub)
> > -		BUG("callback cookie bogus");
> > +		pthread_mutex_lock(&spf->submodule_errors_mutex);
> > +		strbuf_addf(&spf->submodules_with_errors, "\t%s\n",
> > +			    task->sub->name);
> > +		pthread_mutex_unlock(&spf->submodule_errors_mutex);
> > +	}
> >  
> >  	/* Is this the second time we process this submodule? */
> >  	if (task->commits)
> > @@ -1627,6 +1636,11 @@ int fetch_populated_submodules(struct repository *r,
> >  				   &spf,
> >  				   "submodule", "parallel/fetch");
> >  
> > +	if (spf.submodules_with_errors.len > 0)
> > +		fprintf(stderr, "Errors during submodule fetch:\n%s",
> > +			spf.submodules_with_errors.buf);
> > +
> > +
> >  	argv_array_clear(&spf.args);
> >  out:
> >  	free_submodules_oids(&spf.changed_submodule_names);

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

[Emily Shaffer]

On Thu, Jan 16, 2020 at 01:55:26PM -0800, Emily Shaffer wrote:
> On Thu, Jan 16, 2020 at 10:23:58AM -0800, Junio C Hamano wrote:
> > Emily Shaffer <emilyshaffer@google.com> writes:
> > 
> > > @@ -1280,10 +1280,13 @@ struct submodule_parallel_fetch {
> > >  	/* Pending fetches by OIDs */
> > >  	struct fetch_task **oid_fetch_tasks;
> > >  	int oid_fetch_tasks_nr, oid_fetch_tasks_alloc;
> > > +
> > > +	struct strbuf submodules_with_errors;
> > > +	pthread_mutex_t submodule_errors_mutex;
> > 
> > Hmph, it is kind of surprising that we need a new mutex for this.
> > 
> > Isn't the task_finish handler, which is what accesses the
> > with_errors field this patch adds, called by pp_collect_finished()
> > one at a time, is it?
> 
> Hm. It is called by pp_collect_finished() one at a time, but while other
> processes may still be running. So I guess that is OK - spf might still
> be read by other tasks but this field of it won't be touched by anybody
> simultaneously. Ok, I'm convinced.
> 
> > It seems oid_fetch_tasks[] array is also a shared resource in this
> > structure among the parallel fetch tasks, but there is no protection
> > against simultaneous access to it.  Am I missing what makes the new
> > field different?  Somewhat puzzled...
> 
> I think it's similar. As I understand it, it looks something like this:
> 
>   loop forever:
>     can i start a new process?
>       get_next_task cb (blocking)
>       start work cb (nonblocking unless it failed to start)
>     process stderr in/out once (blocking)
>     is anybody done? (blocking)
>       task_finished cb (blocking) <- My change is in here
>         did fetch by ref fail? (blocking)
>           put fetch by OID onto the process list (blocking)
>     is everybody done?
>       break

Ah, as I look deeper I realize that it's a child process, not a thread,
so this code becomes even simpler to understand. I think then I don't
need to worry about thread safety at all here.

 - Emily

[PATCH v3] fetch: emphasize failure during submodule fetch

[Emily Shaffer]

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, and where.

Because fetch_finish() is only called synchronously by
run_processes_parallel, mutexing is not required around
submodules_with_errors.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
Since v2, removed mutexing as run_processes_parallel() creates
subprocesses, not threads, and is in fact synchronous. Also added
translation marker for the error message.

 - Emily

 submodule.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/submodule.c b/submodule.c
index 9da7181321..ee74acee27 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1280,10 +1280,12 @@ struct submodule_parallel_fetch {
 	/* Pending fetches by OIDs */
 	struct fetch_task **oid_fetch_tasks;
 	int oid_fetch_tasks_nr, oid_fetch_tasks_alloc;
+
+	struct strbuf submodules_with_errors;
 };
 #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, \
 		  STRING_LIST_INIT_DUP, \
-		  NULL, 0, 0}
+		  NULL, 0, 0, STRBUF_INIT}
 
 static int get_fetch_recurse_config(const struct submodule *submodule,
 				    struct submodule_parallel_fetch *spf)
@@ -1547,7 +1549,10 @@ static int fetch_finish(int retvalue, struct strbuf *err,
 	struct string_list_item *it;
 	struct oid_array *commits;
 
-	if (retvalue)
+	if (!task || !task->sub)
+		BUG("callback cookie bogus");
+
+	if (retvalue) {
 		/*
 		 * NEEDSWORK: This indicates that the overall fetch
 		 * failed, even though there may be a subsequent fetch
@@ -1557,8 +1562,9 @@ static int fetch_finish(int retvalue, struct strbuf *err,
 		 */
 		spf->result = 1;
 
-	if (!task || !task->sub)
-		BUG("callback cookie bogus");
+		strbuf_addf(&spf->submodules_with_errors, "\t%s\n",
+			    task->sub->name);
+	}
 
 	/* Is this the second time we process this submodule? */
 	if (task->commits)
@@ -1627,6 +1633,11 @@ int fetch_populated_submodules(struct repository *r,
 				   &spf,
 				   "submodule", "parallel/fetch");
 
+	if (spf.submodules_with_errors.len > 0)
+		fprintf(stderr, _("Errors during submodule fetch:\n%s"),
+			spf.submodules_with_errors.buf);
+
+
 	argv_array_clear(&spf.args);
 out:
 	free_submodules_oids(&spf.changed_submodule_names);
-- 
2.25.0.rc1.283.g88dfdc4193-goog

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

[Johannes Schindelin]

Hi Emily,

On Thu, 16 Jan 2020, Emily Shaffer wrote:

> 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, and where.
>
> Because fetch_finish() is only called synchronously by
> run_processes_parallel, mutexing is not required around
> submodules_with_errors.
>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
> Since v2, removed mutexing as run_processes_parallel() creates
> subprocesses, not threads, and is in fact synchronous. Also added
> translation marker for the error message.

Excellent. For Git for Windows' ever-green branch based on `pu`, I had to
introduce this:

-- snip --
Subject: [PATCH] fixup??? fetch: emphasize failure during submodule fetch

This would be the first user of `PTHREAD_MUTEX_INITIALIZER`, but as we
have a hard time to emulate this with critical sections in Windows,
let's not do that.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 submodule.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/submodule.c b/submodule.c
index a65db54cece..ec54f4db2cd 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1286,7 +1286,7 @@ struct submodule_parallel_fetch {
 };
 #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, \
 		  STRING_LIST_INIT_DUP, \
-		  NULL, 0, 0, STRBUF_INIT, PTHREAD_MUTEX_INITIALIZER}
+		  NULL, 0, 0, STRBUF_INIT}

 static int get_fetch_recurse_config(const struct submodule *submodule,
 				    struct submodule_parallel_fetch *spf)
@@ -1614,6 +1614,7 @@ int fetch_populated_submodules(struct repository *r,
 	spf.default_option = default_option;
 	spf.quiet = quiet;
 	spf.prefix = prefix;
+	pthread_mutex_init(&spf.submodule_errors_mutex, NULL);

 	if (!r->worktree)
 		goto out;
-- snap --

I look forward to v3 hitting `pu` and making this change obsolete.

Thanks,
Dscho

>
>  - Emily
>
>  submodule.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 9da7181321..ee74acee27 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1280,10 +1280,12 @@ struct submodule_parallel_fetch {
>  	/* Pending fetches by OIDs */
>  	struct fetch_task **oid_fetch_tasks;
>  	int oid_fetch_tasks_nr, oid_fetch_tasks_alloc;
> +
> +	struct strbuf submodules_with_errors;
>  };
>  #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, \
>  		  STRING_LIST_INIT_DUP, \
> -		  NULL, 0, 0}
> +		  NULL, 0, 0, STRBUF_INIT}
>
>  static int get_fetch_recurse_config(const struct submodule *submodule,
>  				    struct submodule_parallel_fetch *spf)
> @@ -1547,7 +1549,10 @@ static int fetch_finish(int retvalue, struct strbuf *err,
>  	struct string_list_item *it;
>  	struct oid_array *commits;
>
> -	if (retvalue)
> +	if (!task || !task->sub)
> +		BUG("callback cookie bogus");
> +
> +	if (retvalue) {
>  		/*
>  		 * NEEDSWORK: This indicates that the overall fetch
>  		 * failed, even though there may be a subsequent fetch
> @@ -1557,8 +1562,9 @@ static int fetch_finish(int retvalue, struct strbuf *err,
>  		 */
>  		spf->result = 1;
>
> -	if (!task || !task->sub)
> -		BUG("callback cookie bogus");
> +		strbuf_addf(&spf->submodules_with_errors, "\t%s\n",
> +			    task->sub->name);
> +	}
>
>  	/* Is this the second time we process this submodule? */
>  	if (task->commits)
> @@ -1627,6 +1633,11 @@ int fetch_populated_submodules(struct repository *r,
>  				   &spf,
>  				   "submodule", "parallel/fetch");
>
> +	if (spf.submodules_with_errors.len > 0)
> +		fprintf(stderr, _("Errors during submodule fetch:\n%s"),
> +			spf.submodules_with_errors.buf);
> +
> +
>  	argv_array_clear(&spf.args);
>  out:
>  	free_submodules_oids(&spf.changed_submodule_names);
> --
> 2.25.0.rc1.283.g88dfdc4193-goog
>
>