Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Jonathan Tan <jonathantanmy@google.com>
To: newren@gmail.com
Cc: git@vger.kernel.org, Jonathan Tan <jonathantanmy@google.com>
Subject: Re: [PATCH v2 11/20] merge-ort: add a preliminary simple process_entries() implementation
Date: Wed, 11 Nov 2020 11:51:11 -0800
Message-ID: <20201111195111.3115407-1-jonathantanmy@google.com> (raw)
In-Reply-To: <20201102204344.342633-12-newren@gmail.com>

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.

  reply index

Thread overview: 84+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-02 20:43 [PATCH v2 00/20] fundamentals of merge-ort implementation Elijah Newren
2020-11-02 20:43 ` [PATCH v2 01/20] merge-ort: setup basic internal data structures Elijah Newren
2020-11-06 22:05   ` Jonathan Tan
2020-11-06 22:45     ` Elijah Newren
2020-11-09 20:55       ` Jonathan Tan
2020-11-02 20:43 ` [PATCH v2 02/20] merge-ort: add some high-level algorithm structure Elijah Newren
2020-11-02 20:43 ` [PATCH v2 03/20] merge-ort: port merge_start() from merge-recursive Elijah Newren
2020-11-11 13:52   ` Derrick Stolee
2020-11-11 16:22     ` Elijah Newren
2020-11-02 20:43 ` [PATCH v2 04/20] merge-ort: use histogram diff Elijah Newren
2020-11-11 13:54   ` Derrick Stolee
2020-11-11 16:47     ` Elijah Newren
2020-11-11 16:51       ` Derrick Stolee
2020-11-11 17:03         ` Elijah Newren
2020-11-02 20:43 ` [PATCH v2 05/20] merge-ort: add an err() function similar to one from merge-recursive Elijah Newren
2020-11-11 13:58   ` Derrick Stolee
2020-11-11 17:07     ` Elijah Newren
2020-11-11 17:10       ` Derrick Stolee
2020-11-02 20:43 ` [PATCH v2 06/20] merge-ort: implement a very basic collect_merge_info() Elijah Newren
2020-11-06 22:19   ` Jonathan Tan
2020-11-06 23:10     ` Elijah Newren
2020-11-09 20:59       ` Jonathan Tan
2020-11-11 14:38   ` Derrick Stolee
2020-11-11 17:02     ` Elijah Newren
2020-11-02 20:43 ` [PATCH v2 07/20] merge-ort: avoid repeating fill_tree_descriptor() on the same tree Elijah Newren
2020-11-11 14:51   ` Derrick Stolee
2020-11-11 17:13     ` Elijah Newren
2020-11-11 17:21       ` Eric Sunshine
2020-11-02 20:43 ` [PATCH v2 08/20] merge-ort: compute a few more useful fields for collect_merge_info Elijah Newren
2020-11-06 22:52   ` Jonathan Tan
2020-11-06 23:41     ` Elijah Newren
2020-11-09 22:04       ` Jonathan Tan
2020-11-09 23:05         ` Elijah Newren
2020-11-02 20:43 ` [PATCH v2 09/20] merge-ort: record stage and auxiliary info for every path Elijah Newren
2020-11-06 22:58   ` Jonathan Tan
2020-11-07  0:26     ` Elijah Newren
2020-11-09 22:09       ` Jonathan Tan
2020-11-09 23:08         ` Elijah Newren
2020-11-11 15:26   ` Derrick Stolee
2020-11-11 18:16     ` Elijah Newren
2020-11-11 22:06       ` Elijah Newren
2020-11-12 18:23         ` Derrick Stolee
2020-11-12 18:39       ` Derrick Stolee
2020-11-02 20:43 ` [PATCH v2 10/20] merge-ort: avoid recursing into identical trees Elijah Newren
2020-11-11 15:31   ` Derrick Stolee
2020-11-02 20:43 ` [PATCH v2 11/20] merge-ort: add a preliminary simple process_entries() implementation Elijah Newren
2020-11-11 19:51   ` Jonathan Tan [this message]
2020-11-12  1:48     ` Elijah Newren
2020-11-02 20:43 ` [PATCH v2 12/20] merge-ort: have process_entries operate in a defined order Elijah Newren
2020-11-11 16:09   ` Derrick Stolee
2020-11-11 18:58     ` Elijah Newren
2020-11-02 20:43 ` [PATCH v2 13/20] merge-ort: step 1 of tree writing -- record basenames, modes, and oids Elijah Newren
2020-11-11 20:01   ` Jonathan Tan
2020-11-11 20:24     ` Elijah Newren
2020-11-12 20:39       ` Jonathan Tan
2020-11-02 20:43 ` [PATCH v2 14/20] merge-ort: step 2 of tree writing -- function to create tree object Elijah Newren
2020-11-11 20:47   ` Jonathan Tan
2020-11-11 21:21     ` Elijah Newren
2020-11-02 20:43 ` [PATCH v2 15/20] merge-ort: step 3 of tree writing -- handling subdirectories as we go Elijah Newren
2020-11-12 20:15   ` Jonathan Tan
2020-11-12 22:30     ` Elijah Newren
2020-11-24 20:19       ` Elijah Newren
2020-11-25  2:07         ` Jonathan Tan
2020-11-26 18:13           ` Elijah Newren
2020-11-30 18:41             ` Jonathan Tan
2020-11-02 20:43 ` [PATCH v2 16/20] merge-ort: basic outline for merge_switch_to_result() Elijah Newren
2020-11-02 20:43 ` [PATCH v2 17/20] merge-ort: add implementation of checkout() Elijah Newren
2020-11-02 20:43 ` [PATCH v2 18/20] tree: enable cmp_cache_name_compare() to be used elsewhere Elijah Newren
2020-11-02 20:43 ` [PATCH v2 19/20] merge-ort: add implementation of record_unmerged_index_entries() Elijah Newren
2020-11-02 20:43 ` [PATCH v2 20/20] merge-ort: free data structures in merge_finalize() Elijah Newren
2020-11-03 14:49 ` [PATCH v2 00/20] fundamentals of merge-ort implementation Derrick Stolee
2020-11-03 16:36   ` Elijah Newren
2020-11-07  6:06     ` Elijah Newren
2020-11-07 15:02       ` Derrick Stolee
2020-11-07 19:39         ` Elijah Newren
2020-11-09 12:30           ` Derrick Stolee
2020-11-09 17:13             ` Elijah Newren
2020-11-09 19:51               ` Derrick Stolee
2020-11-09 22:44                 ` Elijah Newren
2020-11-11 17:08 ` Derrick Stolee
2020-11-11 18:35   ` Elijah Newren
2020-11-11 20:48     ` Derrick Stolee
2020-11-11 21:18       ` Elijah Newren
2020-11-29  7:43 [PATCH " Elijah Newren via GitGitGadget
2020-12-04 20:47 ` [PATCH v2 " Elijah Newren via GitGitGadget
2020-12-04 20:48   ` [PATCH v2 11/20] merge-ort: add a preliminary simple process_entries() implementation Elijah Newren via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201111195111.3115407-1-jonathantanmy@google.com \
    --to=jonathantanmy@google.com \
    --cc=git@vger.kernel.org \
    --cc=newren@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git