Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: >> - if (strcmp(name, "body") && >> - !starts_with(name, "subject") && >> - !starts_with(name, "trailers") && >> - !starts_with(name, "contents")) >> + >> + if ((data->type != OBJ_TAG && >> + data->type != OBJ_COMMIT) || >> + (strcmp(name, "body") && >> + !starts_with(name, "subject") && >> + !starts_with(name, "trailers") && >> + !starts_with(name, "contents"))) > > We have 4 "real" object types, commit, tree, blob, tag. Do you really > mean "not tag or commit" here, don't you mean "is tree or blob" instead? > I.e. do we really want to pass OBJ_NONE etc. here? > >> continue; If somebody throws OBJ_NONE at us by mistake, we do not want to handle such an object and try to extract the subject member from it anyway, no? The intent of the code here, before the patch, is that "what we do after the control flow passes this point is about the body, subject, trailers, and contents request, so everybody else should go to the next iteration". The caller used to give us an object compatible with these four types of requests, now the caller may throw others, hence "by the way, we know these four kinds of requests make sense only for tags and commits, so everybody else should go to the next iteration, too" would be a natural thing to add. So in that sense, I prefer it over "we know these four types of requests do not make sense for blobs and trees".
