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

Code before:
    bool keyremoved = false;

    if (getLongLongFromObjectOrReply(c, c->argv[fields_index - 1], &num_fields, NULL) != C_OK) return;

    /* Check that the parsed fields number matches the real provided number of fields */
    if (!num_fields || num_fields != (c->argc - fields_index)) {
        addReplyError(c, "numfields should be greater than 0 and match the provided number of fields");
        return;
    }

    /* Don't abort when the key cannot be found. Non-existing keys are empty
     * hashes, where HGETDEL should respond with a series of null bulks. */
    robj *o = lookupKeyWrite(c->db, c->argv[1]);
    if (checkType(c, o, OBJ_HASH)) return;

    bool hash_volatile_items = hashTypeHasVolatileFields(o);

    /* Reply with array of values and delete at the same time */
    addReplyArrayLen(c, num_fields);
    for (i = fields_index; i < c->argc; i++) {
        addHashFieldToReply(c, o, c->argv[i]->ptr);

        /* If hash doesn't exist, continue as already replied with NULL */
        if (o == NULL) continue;
        if (hashTypeDelete(o, c->argv[i]->ptr)) {
            deleted++;
            if (hashTypeLength(o) == 0) {
                if (hash_volatile_items) dbUntrackKeyWithVolatileItems(c->db, o);
                dbDelete(c->db, c->argv[1]);
                keyremoved = true;
                o = NULL;
            }
        }
    }

    if (deleted) {
        if (!keyremoved && hash_volatile_items != hashTypeHasVolatileFields(o)) {
            dbUpdateObjectWithVolatileItemsTracking(c->db, o);
        }
        signalModifiedKey(c, c->db, c->argv[1]);
        notifyKeyspaceEvent(NOTIFY_HASH, "hdel", c->argv[1], c->db->id);
        if (keyremoved) notifyKeyspaceEvent(NOTIFY_GENERIC, "del", c->argv[1], c->db->id);
        server.dirty += deleted;
    }
}

void hlenCommand(client *c) {
    robj *o;

    if ((o = lookupKeyReadOrReply(c, c->argv[1], shared.czero)) == NULL || checkType(c, o, OBJ_HASH)) return;


Review comment (performance):
NOTE: I know that I drove you to implement it this way, but now I thought of a reason why the old way might have been "useful". What should we do in case the same fields is provided multiple times? 
For example:
```
HMSET myhash f1 a f2 b f3 c
HGETDEL myhash fields 3 f1 f1 f1
```

in your previous implementation is would be:
```
"a", "a", "a"
```
which is slightly less efficient, but might fits better with common sense.
In the CURRENT implementation it would yield :
```
"a", nill, nill
```

BTW - Redis does as as the CURRENT implementation does (which means they return `"a", nill, nill`)
 
IMO we are doing the right thing just need to make sure we document it correctly

Code after:
    long long num_fields = 0;
    bool keyremoved = false;

    if (getLongLongFromObjectOrReply(c, c->argv[fields_index - 1], &num_fields, NULL) != C_OK) return;

    /* Check that the parsed fields number matches the real provided number of fields */
    if (!num_fields || num_fields != (c->argc - fields_index)) {
        addReplyError(c, "numfields should be greater than 0 and match the provided number of fields");
        return;
    }

    /* Don't abort when the key cannot be found. Non-existing keys are empty
     * hashes, where HGETDEL should respond with a series of null bulks. */
    robj *o = lookupKeyWrite(c->db, c->argv[1]);
    if (checkType(c, o, OBJ_HASH)) return;

    bool hash_volatile_items = hashTypeHasVolatileFields(o);

    /* Reply with array of values and delete at the same time */
    addReplyArrayLen(c, num_fields);
    for (i = fields_index; i < c->argc; i++) {
        addHashFieldToReply(c, o, c->argv[i]->ptr);

        /* If hash doesn't exist, continue as already replied with NULL */
        if (o == NULL) continue;
        if (hashTypeDelete(o, c->argv[i]->ptr)) {
            deleted++;
            if (hashTypeLength(o) == 0) {
                if (hash_volatile_items) dbUntrackKeyWithVolatileItems(c->db, o);
                dbDelete(c->db, c->argv[1]);
                keyremoved = true;
                o = NULL;
            }
        }
    }

    if (deleted) {
        if (!keyremoved && hash_volatile_items != hashTypeHasVolatileFields(o)) {
            dbUpdateObjectWithVolatileItemsTracking(c->db, o);
        }
        signalModifiedKey(c, c->db, c->argv[1]);
        notifyKeyspaceEvent(NOTIFY_HASH, "hdel", c->argv[1], c->db->id);
        if (keyremoved) notifyKeyspaceEvent(NOTIFY_GENERIC, "del", c->argv[1], c->db->id);
        server.dirty += deleted;
    }
}

void hlenCommand(client *c) {
    robj *o;

    if ((o = lookupKeyReadOrReply(c, c->argv[1], shared.czero)) == NULL || checkType(c, o, OBJ_HASH)) return;