2011/11/13 Junio C Hamano <gitster@xxxxxxxxx>: > The above log message with the callchain would be a perfect explanation > for a patch to fix builtin/merge.c and nothing else, but among 50+ > callsites of resolve_ref(), only 5 places are modified in this patch, and > it is not explained in the commit log message at all how they were chosen. > Were the other 40+ or so places inspected and deemed to be OK? Or is this > "I just did a few of them in addition to the immediate need of fixing what > was reported, and the rest are left as an exercise to the readers" patch? I went through all of them. Most of them only checks if return value is NULL, or does one-time string comparison. Others do xstrdup() to save the return value. Will update the patch message to reflect this. > Some variables that receive xstrdup()'ed result with this patch are > globals whose lifetime lasts while the process is alive, and I do not > think it is a huge problem that this patch does not free it, but it seems > the patch does not free some other function scope variables once their use > is done even when it is fairly easy to do so. Will fix. > How much overhead would it incur if we return xstrdup()'ed memory from the > resolve_ref() and make it the caller's responsibility to free it? Would it > make the calling site too ugly? Given a lot of callsites do "if (resolve_ref(...)) ...", yes it may look ugly. Though if you don't mind a bigger patch, we could turn const char *resolve_ref(const char *path, const char *sha1, int reading, int *flag); to int resolve_ref(const char *path, const char *sha1, int reading, int *flag, char **ref); where *ref is the current return value and is only allocated by resolve_ref() if ref != NULL. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html