[RFH] unpack-trees: cache_entry lifetime issue?

["René Scharfe" <rene.scharfe@lsrfire.ath.cx>]

Hello,

below is the output of "git grep -hnW unpack_nondirectories",
which shows some parts of unpack-trees.c that I use as context to
ask: Should we check for o->merge in line 775, before using src[0]?

If o->merge is 0, the src[0] will be NULL right up to the call of
unpack_nondirectories() in line 772.  There it may be set (in line
582).  In that case we'll end up at line 779, where mark_ce_used()
is applied to it.

I suspect that this is unintended and that line 775 should rather
read "if (o->merge && src[0]) {".  Can someone with a better
understanding of unpack-trees confirm or refute that suspicion?

Thanks,
René


542:static int unpack_nondirectories(int n, unsigned long mask,
543-				 unsigned long dirmask,
544-				 struct cache_entry **src,
545-				 const struct name_entry *names,
546-				 const struct traverse_info *info)
547-{
548-	int i;
549-	struct unpack_trees_options *o = info->data;
550-	unsigned long conflicts;
551-
552-	/* Do we have *only* directories? Nothing to do */
553-	if (mask == dirmask && !src[0])
554-		return 0;
555-
556-	conflicts = info->conflicts;
557-	if (o->merge)
558-		conflicts >>= 1;
559-	conflicts |= dirmask;
560-
561-	/*
562-	 * Ok, we've filled in up to any potential index entry in src[0],
563-	 * now do the rest.
564-	 */
565-	for (i = 0; i < n; i++) {
566-		int stage;
567-		unsigned int bit = 1ul << i;
568-		if (conflicts & bit) {
569-			src[i + o->merge] = o->df_conflict_entry;
570-			continue;
571-		}
572-		if (!(mask & bit))
573-			continue;
574-		if (!o->merge)
575-			stage = 0;
576-		else if (i + 1 < o->head_idx)
577-			stage = 1;
578-		else if (i + 1 > o->head_idx)
579-			stage = 3;
580-		else
581-			stage = 2;

582-		src[i + o->merge] = create_ce_entry(info, names + i, stage);

583-	}
584-
585-	if (o->merge)
586-		return call_unpack_fn(src, o);
587-
588-	for (i = 0; i < n; i++)
589-		if (src[i] && src[i] != o->df_conflict_entry)
590-			add_entry(o, src[i], 0, 0);
591-	return 0;
592-}
593-
--
721=static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, struct name_entry *names, struct traverse_info *info)
722-{
723-	struct cache_entry *src[MAX_UNPACK_TREES + 1] = { NULL, };
724-	struct unpack_trees_options *o = info->data;
725-	const struct name_entry *p = names;
726-
727-	/* Find first entry with a real name (we could use "mask" too) */
728-	while (!p->mode)
729-		p++;
730-
731-	if (o->debug_unpack)
732-		debug_unpack_callback(n, mask, dirmask, names, info);
733-
734-	/* Are we supposed to look at the index too? */
735-	if (o->merge) {
736-		while (1) {
737-			int cmp;
738-			struct cache_entry *ce;
739-
740-			if (o->diff_index_cached)
741-				ce = next_cache_entry(o);
742-			else
743-				ce = find_cache_entry(info, p);
744-
745-			if (!ce)
746-				break;
747-			cmp = compare_entry(ce, info, p);
748-			if (cmp < 0) {
749-				if (unpack_index_entry(ce, o) < 0)
750-					return unpack_failed(o, NULL);
751-				continue;
752-			}
753-			if (!cmp) {
754-				if (ce_stage(ce)) {
755-					/*
756-					 * If we skip unmerged index
757-					 * entries, we'll skip this
758-					 * entry *and* the tree
759-					 * entries associated with it!
760-					 */
761-					if (o->skip_unmerged) {
762-						add_same_unmerged(ce, o);
763-						return mask;
764-					}
765-				}
766-				src[0] = ce;
767-			}
768-			break;
769-		}
770-	}
771-

772:	if (unpack_nondirectories(n, mask, dirmask, src, names, info) < 0)
773-		return -1;
774-
775-	if (src[0]) {
776-		if (ce_stage(src[0]))
777-			mark_ce_used_same_name(src[0], o);
778-		else
779-			mark_ce_used(src[0], o);
780-	}

781-
782-	/* Now handle any directories.. */
783-	if (dirmask) {
784-		unsigned long conflicts = mask & ~dirmask;
785-		if (o->merge) {
786-			conflicts <<= 1;
787-			if (src[0])
788-				conflicts |= 1;
789-		}
790-
791-		/* special case: "diff-index --cached" looking at a tree */
792-		if (o->diff_index_cached &&
793-		    n == 1 && dirmask == 1 && S_ISDIR(names->mode)) {
794-			int matches;
795-			matches = cache_tree_matches_traversal(o->src_index->cache_tree,
796-							       names, info);
797-			/*
798-			 * Everything under the name matches; skip the
799-			 * entire hierarchy.  diff_index_cached codepath
800-			 * special cases D/F conflicts in such a way that
801-			 * it does not do any look-ahead, so this is safe.
802-			 */
803-			if (matches) {
804-				o->cache_bottom += matches;
805-				return mask;
806-			}
807-		}
808-
809-		if (traverse_trees_recursive(n, dirmask, conflicts,
810-					     names, info) < 0)
811-			return -1;
812-		return mask;
813-	}
814-
815-	return mask;
816-}
817-

PS: These functions are a tad too long, aren't they? ;-)