i think we meant to update the memory of the client monitor, not c (in which case this line should be modified, not deleted).
the thing is that we normally update client memory in processCommandAndResetClient (after processCommand), so there's no need to call it per addReply, but in this case (feeding monitors), much like in pubsubPublishMessageInternal, we add a reply for the non-current client, so we need to call updateClientMemUsageAndBucket.

But monitors are never evicted (pubsub clients can be evicted), so updateClientMemUsageAndBucket() will always be NOP.

    int allow_eviction = clientEvictionAllowed(c);
    removeClientFromMemUsageBucket(c, allow_eviction);

    if (!allow_eviction) {
        return 0;    <- exit here
    }

interesting, that seems like a side effect of #11348.
in the original #8687, calling updateClientMemUsage used to update the client memory usage metrics.
now with #11348, it'll never do that for a monitor (and master / slave), even if client eviction is enabled.
considering the accurate memory accounting was mainly intended for the eviction, i suppose we're ok with this change.
@hpatro @yoav-steinberg what do you think?

Yeah, the statement is pretty much a no-op right now, I think we can safely clean it up.

@hpatro it is a no-op right now, but the question was if it should be.
i.e. i realized that your PR had implications on accurate memory reporting of certain types of clients even when client eviction is enabled.

i.e. it means that when enabling client eviction, the user enjoys accurate memory tracking of most clients, but monitor/master/slave remain inaccurate.
we can fix that if we want, but i'm leaning towards accepting it.

i wonder if we'll remember to re-introduce that line if we ever decide to change that..

@hpatro I see that you've handled this in #11348 by calling updateClientMemoryUsage() in clientCron() to update the client's memory usage even when eviction is disabled.

@hpatro to be clear, what i meant is that there's a benefit of accurate client memory tracking, not only for the purpose of client eviction.
i.e. when someone calls INFO, and sees a certain amount of memory being used, they'll know how much of it belongs to clients rather than having an outdated value.
in my previous understanding your PR meant that we have inaccurate reporting while client eviction is disabled, and we have accurate reporting when it is enabled.
but now i realize that for certain types of clients, it'll remain inaccurate regardless of the client eviction config.
i'm trying to decide i we wanna fix it.

@sundb yes, that's what he meant, that all we need to do is remove the line you removed in this PR.

when someone calls INFO, and sees a certain amount of memory being used, they'll know how much of it belongs to clients rather than having an outdated value.

I don't see much value of accurate memory usage date for just observability. @oranagra Are there any other benefits you can think of?
I think we should continue with our current solution and accept this PR to clean up the unnecessary call.

i can't think of other benefits, in some cases, when looking at INFO to diagnose an issue, it is important to have correct metrics.
however, the fact is that by default they're not correct, and people need to enable client-eviction to have them correct means that in most cases it's not gonna serve us.

so let's just add a comment in updateClientMemUsageAndBucket that explains that currently these types of clients are not accurately reported in INFO regardless of the config, and that if we someday want to improve that we need to re-add a call in replicationFeedMonitors.
@sundb can you do that? or do you want me to edit this PR?

@oranagra Please look at the last commit.
Not sure it's accurate, comment is much harder than code...