Re: [PATCH] maintenance: add prune-remote-refs task

From: Shubham Kanodia <shubham.kanodia10@gmail.com>
Date: Dec 28, 2024, 16:24 UTC
Message-ID: <CAG=Um+0rnj3B3QoTZFCZWNCcYMtoiLnn=nov7GuHRH-aDydARQ@mail.gmail.com>
In-Reply-To: <xmqqr05r4wu5.fsf@gitster.g>

On Sat, Dec 28, 2024 at 9:35 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

>
> Shubham Kanodia <shubham.kanodia10@gmail.com> writes:
>
> >> Hmph, is there a reason why you need two loops, instead of
> >> for-each-remote calling a function that does the run_command()
> >> thing?
> >
> > It can be collapsed into one.
>
> Sorry, but that is not an answer, as my question was not a
> suggestion to change anything.
>
> It was a question asking you if there was a specific reason why the
> code was structured the way it was written.  If there is another way
> to write it, you need to answer why the alternative wasn't picked.

There wasn't a good reason for doing it that way. I guess I was trying to understand the second argument for `for_each_remote` would be best used if the command was called directly (while avoiding a compilation warning), but looking at a few other usages of `for_each_remote` I realised that it could just be marked unused in this case (since we aren't doing anything with it).

I should've probably looked deeper and learnt from existing patterns (e.g. `maintenance_task_prefetch`) — which I have in my last patch.

Show 14 quoted lines

> >> This loop does not stop at the first error, but returns a non-zero
> >> error after noticing even a single remote fail to run prune, which
> >> sounds like a seneible design.  Would an error percolate up the same
> >> way when two different tasks run and one of them fails in the
> >> control folow in "git maintenance"?  Just want to see if we are
> >> being consistent with the surrounding code.
> >
> > Fair point. I'll make the process flow identical to the prefetch refs
> > task that works similarly across remotes.
> > It returns as soon as the first remote fails (without necessarily
> > affecting other tasks).
>
> ... and the first failure signals the caller a failure?  That would
> match what you did in your new feature, which is perfect.

Exactly — the first failing remote will signal that the `prune-remote-refs` task has failed via an immediate `return 1`. The maintenance command uses this to register the exit code of the top level command to 1, while continuing to execute all other tasks anyway.

Thanks, Shubham K