Okay...let me review patches 11-15. (Patches 16-20 deal with checkout
and might be better reviewed by someone who is already familiar with how
the existing merge performs checkout. If no one reviews it, I might come
back to it if I have time.)
> +/* Per entry merge function */
> +static void process_entry(struct merge_options *opt,
> + const char *path,
> + struct conflict_info *ci)
> +{
> + assert(!ci->merged.clean);
> + assert(ci->filemask >= 0 && ci->filemask <= 7);
I see below that this function doesn't handle ci->match_mask == 7 (and
it doesn't need to because, I believe, there is a function in one of the
earlier patches that optimizes the case wherein all 3 match with each
other). Maybe add an assert here for that too.
> +
> + if (ci->filemask == 0) {
> + /*
> + * This is a placeholder for directories that were recursed
> + * into; nothing to do in this case.
> + */
> + return;
> + }
> +
> + if (ci->df_conflict) {
> + die("Not yet implemented.");
> + }
> +
> + /*
> + * NOTE: Below there is a long switch-like if-elseif-elseif... block
> + * which the code goes through even for the df_conflict cases
> + * above. Well, it will once we don't die-not-implemented above.
> + */
> + if (ci->match_mask) {
> + ci->merged.clean = 1;
OK, looks straightforward so far. It's a clean merge if 2 match. (As I
said earlier, at this point in the code, it is not possible for 3 to
match.)
> + if (ci->match_mask == 6) {
> + /* stages[1] == stages[2] */
> + ci->merged.result.mode = ci->stages[1].mode;
> + oidcpy(&ci->merged.result.oid, &ci->stages[1].oid);
If OURS and THEIRS match, use one of them arbitrarily (because they are
the same anyway). OK.
> + } else {
> + /* determine the mask of the side that didn't match */
> + unsigned int othermask = 7 & ~ci->match_mask;
> + int side = (othermask == 4) ? 2 : 1;
BASE matches with either OURS or THEIRS, so use the side that doesn't
match. OK.
> +
> + ci->merged.is_null = (ci->filemask == ci->match_mask);
This works (if the non-matching bit in filemask is set, the file exists;
the comparison will be false and therefore is_null is false - and
correctly false because the file exists), but seems unnecessarily
clever. Couldn't you just check nullness of the OID (or through the
mode, like the line below it) and set it here?
Admittedly, the way you wrote it also verifies that filemask is what we
expect. I don't think it is important to verify it, but if you think it
is important, I think it is this verification that should go in the
assert statement.
> + ci->merged.result.mode = ci->stages[side].mode;
> + oidcpy(&ci->merged.result.oid, &ci->stages[side].oid);
> +
> + assert(othermask == 2 || othermask == 4);
> + assert(ci->merged.is_null == !ci->merged.result.mode);
> + }
> + } else if (ci->filemask >= 6 &&
> + (S_IFMT & ci->stages[1].mode) !=
> + (S_IFMT & ci->stages[2].mode)) {
> + /*
> + * Two different items from (file/submodule/symlink)
> + */
> + die("Not yet implemented.");
There are no matches, and OURS and THEIRS have different types. OK.
> + } else if (ci->filemask >= 6) {
> + /*
> + * TODO: Needs a two-way or three-way content merge, but we're
> + * just being lazy and copying the version from HEAD and
> + * leaving it as conflicted.
> + */
> + ci->merged.clean = 0;
> + ci->merged.result.mode = ci->stages[1].mode;
> + oidcpy(&ci->merged.result.oid, &ci->stages[1].oid);
OK.
> + } else if (ci->filemask == 3 || ci->filemask == 5) {
> + /* Modify/delete */
> + die("Not yet implemented.");
> + } else if (ci->filemask == 2 || ci->filemask == 4) {
> + /* Added on one side */
> + int side = (ci->filemask == 4) ? 2 : 1;
> + ci->merged.result.mode = ci->stages[side].mode;
> + oidcpy(&ci->merged.result.oid, &ci->stages[side].oid);
> + ci->merged.clean = !ci->df_conflict && !ci->path_conflict;
> + } else if (ci->filemask == 1) {
> + /* Deleted on both sides */
> + ci->merged.is_null = 1;
> + ci->merged.result.mode = 0;
> + oidcpy(&ci->merged.result.oid, &null_oid);
> + ci->merged.clean = !ci->path_conflict;
> + }
The rest is OK.
> +
> + /*
> + * If still unmerged, record it separately. This allows us to later
> + * iterate over just unmerged entries when updating the index instead
> + * of iterating over all entries.
> + */
> + if (!ci->merged.clean)
> + strmap_put(&opt->priv->unmerged, path, ci);
> +}
> +
> static void process_entries(struct merge_options *opt,
> struct object_id *result_oid)
> {
> - die("Not yet implemented.");
> + struct hashmap_iter iter;
> + struct strmap_entry *e;
> +
> + if (strmap_empty(&opt->priv->paths)) {
> + oidcpy(result_oid, opt->repo->hash_algo->empty_tree);
> + return;
> + }
> +
> + strmap_for_each_entry(&opt->priv->paths, &iter, e) {
> + /*
> + * WARNING: If ci->merged.clean is true, then ci does not
> + * actually point to a conflict_info but a struct merge_info.
> + */
> + struct conflict_info *ci = e->value;
> +
> + if (!ci->merged.clean)
> + process_entry(opt, e->key, e->value);
> + }
> +
> + die("Tree creation not yet implemented");
The rest looks straightforward.
