Repository: valkey-io/valkey
File: src/t_hash.c (C)

Code before:
    long long expiry = EXPIRY_NONE;

    if (getLongLongFromObjectOrReply(c, c->argv[3], &incr, NULL) != C_OK) return;
    if ((o = hashTypeLookupWriteOrCreate(c, c->argv[1])) == NULL) return;
    if (hashTypeGetValue(o, objectGetVal(c->argv[2]), &vstr, &vlen, &value, &expiry) == C_OK) {
        if (vstr) {
            if (string2ll((char *)vstr, vlen, &value) == 0) {
                addReplyError(c, "hash value is not an integer");
                return;
            }
        } /* Else hashTypeGetValue() already stored it into &value */
    } else {
        value = 0;
    }

    oldvalue = value;
    if ((incr < 0 && oldvalue < 0 && incr < (LLONG_MIN - oldvalue)) ||
        (incr > 0 && oldvalue > 0 && incr > (LLONG_MAX - oldvalue))) {
        addReplyError(c, "increment or decrement would overflow");
        return;
    }
    value += incr;
    new = sdsfromlonglong(value);
    bool has_volatile_fields = hashTypeHasVolatileFields(o);
    bool expired_overitten = false;
    hashTypeSet(o, objectGetVal(c->argv[2]), new, expiry, HASH_SET_TAKE_VALUE, &expired_overitten);
    if (has_volatile_fields != hashTypeHasVolatileFields(o)) {
        dbUpdateObjectWithVolatileItemsTracking(c->db, o);
    }
    signalModifiedKey(c, c->db, c->argv[1]);
    /* In case we overitten an expired field, we need to act as if it was just expired */
    if (expired_overitten) {
        server.stat_expiredfields++;
        notifyKeyspaceEvent(NOTIFY_HASH, "hexpired", c->argv[1], c->db->id);
    }
    notifyKeyspaceEvent(NOTIFY_HASH, "hincrby", c->argv[1], c->db->id);
    server.dirty++;
    addReplyLongLong(c, value);

    /* Always replicate HINCRBY as an HSET or HSETEX command with the final value
     * when hash has volatile item, since we do not know what will the field state be when the command reach the replica.
     * HSET is used to override the resulting value and HSETEX is used in order to maintain the expiration time on the target. */
    if (has_volatile_fields) {
        char buf[MAX_LONG_DOUBLE_CHARS];
        int len = ld2string(buf, sizeof(buf), value, LD_STR_HUMAN);
        robj *newobj = createRawStringObject(buf, len);
        if (expiry == EXPIRY_NONE) {
            rewriteClientCommandArgument(c, 0, shared.hset);
            rewriteClientCommandArgument(c, 3, newobj);
            decrRefCount(newobj);
        } else {

Review comment (suggestion):
Shouldn't expiry be dependent on whether or not the old expiry is in the past? If the expiry is in the past, I guess we want EXPIRY_NONE (HINCRBY on expired-but-not-cleaned should be like HINCRBY on non existent)

Code after:
    long long expiry = EXPIRY_NONE;

    if (getLongLongFromObjectOrReply(c, c->argv[3], &incr, NULL) != C_OK) return;
    if ((o = hashTypeLookupWriteOrCreate(c, c->argv[1])) == NULL) return;
    if (hashTypeGetValue(o, objectGetVal(c->argv[2]), &vstr, &vlen, &value, &expiry) == C_OK) {
        if (vstr) {
            if (string2ll((char *)vstr, vlen, &value) == 0) {
                addReplyError(c, "hash value is not an integer");
                return;
            }
        } /* Else hashTypeGetValue() already stored it into &value */
    } else {
        value = 0;
    }

    oldvalue = value;
    if ((incr < 0 && oldvalue < 0 && incr < (LLONG_MIN - oldvalue)) ||
        (incr > 0 && oldvalue > 0 && incr > (LLONG_MAX - oldvalue))) {
        addReplyError(c, "increment or decrement would overflow");
        return;
    }
    value += incr;
    new = sdsfromlonglong(value);
    bool has_volatile_fields = hashTypeHasVolatileFields(o);
    bool expired_overwritten = false;
    hashTypeSet(o, objectGetVal(c->argv[2]), new, expiry, HASH_SET_TAKE_VALUE, &expired_overwritten);
    if (has_volatile_fields != hashTypeHasVolatileFields(o)) {
        dbUpdateObjectWithVolatileItemsTracking(c->db, o);
    }
    signalModifiedKey(c, c->db, c->argv[1]);
    /* In case we overitten an expired field, we need to act as if it was just expired */
    if (expired_overwritten) {
        server.stat_expiredfields++;
        notifyKeyspaceEvent(NOTIFY_HASH, "hexpired", c->argv[1], c->db->id);
    }
    notifyKeyspaceEvent(NOTIFY_HASH, "hincrby", c->argv[1], c->db->id);
    server.dirty++;
    addReplyLongLong(c, value);

    /* Always replicate HINCRBY as an HSET or HSETEX command with the final value
     * when hash has volatile item, since we do not know what will the field state be when the command reach the replica.
     * HSET is used to override the resulting value and HSETEX is used in order to maintain the expiration time on the target. */
    if (has_volatile_fields) {
        char buf[MAX_LONG_DOUBLE_CHARS];
        int len = ld2string(buf, sizeof(buf), value, LD_STR_HUMAN);
        robj *newobj = createRawStringObject(buf, len);
        if (expiry == EXPIRY_NONE) {
            rewriteClientCommandArgument(c, 0, shared.hset);
            rewriteClientCommandArgument(c, 3, newobj);
            decrRefCount(newobj);
        } else {