Skip to content

Commit ca3abe4

Browse files
peffgitster
authored andcommitted
run_diff_files(): de-mystify the size of combine_diff_path struct
We allocate a combine_diff_path struct with space for 5 parents. Why 5? The history is not particularly enlightening. The allocation comes from b4b1550 (Don't instantiate structures with FAMs., 2006-06-18), which just switched to xmalloc from a stack struct with 5 elements. That struct changed to 5 from 4 in 2454c96 (combine-diff: show mode changes as well., 2006-02-06), when we also moved from storing raw sha1 bytes to the combine_diff_parent struct. But no explanation is given. That 4 comes from the earliest code in ea726d0 (diff-files: -c and --cc options., 2006-01-28). One might guess it is for the 4 stages we can store in the index. But this code path only ever diffs the current state against stages 2 and 3. So we only need two slots. And it's easy to see this is still the case. We fill the parent slots by subtracting 2 from the ce_stage() values, ignoring values below 2. And since ce_stage() is only 2 bits, there are 4 values, and thus we need 2 slots. Let's use the correct value (saving a tiny bit of memory) and add a comment explaining what's going on (saving a tiny bit of programmer brain power). Arguably we could use: 1 + (STAGEMASK >> STAGESHIFT) - 2 which lets the compiler enforce that we will not go out-of-bounds if we see an unexpected value from ce_stage(). But that is more confusing to explain, and the constant "2" is baked into other parts of the function. It is a fundamental constant, not something where somebody might bump a macro and forget to update this code. Signed-off-by: Jeff King <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent 30f7414 commit ca3abe4

File tree

1 file changed

+6
-1
lines changed

1 file changed

+6
-1
lines changed

diff-lib.c

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,8 +166,13 @@ void run_diff_files(struct rev_info *revs, unsigned int option)
166166
wt_mode = 0;
167167
}
168168

169+
/*
170+
* Allocate space for two parents, which will come from
171+
* index stages #2 and #3, if present. Below we'll fill
172+
* these from (stage - 2).
173+
*/
169174
dpath = combine_diff_path_new(ce->name, ce_namelen(ce),
170-
wt_mode, null_oid(), 5);
175+
wt_mode, null_oid(), 2);
171176

172177
while (i < entries) {
173178
struct cache_entry *nce = istate->cache[i];

0 commit comments

Comments
 (0)