git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/9] remerge diff proof of concept/RFC
@ 2014-02-04 22:17 Thomas Rast
  2014-02-04 22:17 ` [PATCH 1/9] merge-recursive: remove dead conditional in update_stages() Thomas Rast
                   ` (9 more replies)
  0 siblings, 10 replies; 11+ messages in thread
From: Thomas Rast @ 2014-02-04 22:17 UTC (permalink / raw)
  To: git

Hi,

This may look intimidating, but it's actually 3.5 separate things:

  merge-recursive: remove dead conditional in update_stages()
  merge-recursive: internal flag to avoid touching the worktree
  merge-recursive: -Xindex-only to leave worktree unchanged

These are unchanged from tr/merge-recursive-index-only.  I'm just
resending them here because it's been a while, the rest depends on it,
and it makes it more obvious how patch 8 fits in.

  pretty: refactor add_merge_info() into parts
  log: add a merge base inspection option

This seemed a good idea when I wrote it, but the more I think about
it, the less useful it appears to me.  I left it here as a
demonstration that computing the merge bases as we go is feasible.

  combine-diff: do not pass revs->dense_combined_merges redundantly
  Fold all merge diff variants into an enum

Some initial refactoring.

  merge-recursive: allow storing conflict hunks in index
  log --remerge-diff: show what the conflict resolution changed

The real meat.

I'll only describe the last patches here.

Context: last summer (yes, it's been a while) I investigated some
angles of looking at merges that make it easier to investigate
mismerges.  The problem space is: it is exceedingly hard to find
instances of

  git merge -s ours foo
  git merge -Xours foo
  manually resolve some hunks in favor of one side

after some time has passed.  The only ways I found with existing
machinery involve too much manual inspection.

At the time I made some scripts that look at the problem in various
ways:

  https://github.com/trast/evilmergediff

This series explores another angle, which I call "remerge diff".  It
works by re-doing the merge in core, using features from patches 1-3
and 7.  Likely that will result in conflicts, which are formatted in
the usual <<<<<<< way.  Then it diffs this "remerge" against the
merge's tree that is recorded in history.

This tends to give nice results in practice; I pasted a particularly
pretty example at the bottom.


However, this is RFC because nontrivial merges can easily lead to
states that I cannot handle with the current -- pretty neat and
simple, I think -- implementation.

For example, a delete/modify conflict does not result in a nice
conflict-hunk formatted file, it just leaves 2 (out of 3) stages in
the index.  I can then not write a tree from that.

This could be fixed by adding a pass that turns a missing stage 2 or 3
into an empty file at that stage, so that the resulting file looks
like

  <<<<<<<
  =======
  ... stage 3 content goes here ...
  >>>>>>>

(analogously for stage 3 missing).

But that still doesn't handle at least directory/file conflicts.

So I would welcome comments, and/or better ideas, on the following
proposed resolution:

* Implement what I described last, to take care of delete/modify
  conflicts.

* Punt on more complex conflicts, by removing those files from the
  index, and emitting a warning about those files instead.

Note that even without delete/modify handling, this covers the vast
majority of merges: among some 8090 merges I sampled from git.git,
there are only 13 that trigger the "BUG: ..." message, and 34
octopuses.


Example run:

  $ git show --remerge-diff a39b15b4f6a3f08b67b17d968935d177821e680f
  [...]
  diff --git a/builtin/ls-files.c b/builtin/ls-files.c
  index 0f4473e..175e6e3 100644
  --- a/builtin/ls-files.c
  +++ b/builtin/ls-files.c
  @@ -490,16 +490,9 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
   		OPT_BOOLEAN('u', "unmerged", &show_unmerged,
   			N_("show unmerged files in the output")),
   		OPT_BOOLEAN(0, "resolve-undo", &show_resolve_undo,
  -<<<<<<< f12e49ae877ad0644b9b9939b7cb742da98691d2
   			    N_("show resolve-undo information")),
  -		{ OPTION_CALLBACK, 'x', "exclude", &dir.exclude_list[EXC_CMDL], N_("pattern"),
  +		{ OPTION_CALLBACK, 'x', "exclude", &exclude_list, N_("pattern"),
   			N_("skip files matching pattern"),
  -=======
  -			    "show resolve-undo information"),
  -		{ OPTION_CALLBACK, 'x', "exclude",
  -			&exclude_list, "pattern",
  -			"skip files matching pattern",
  ->>>>>>> 72aeb18772deeb386da7dd8997b969877bd29e41
   			0, option_parse_exclude },
   		{ OPTION_CALLBACK, 'X', "exclude-from", &dir, N_("file"),
   			N_("exclude patterns are read from <file>"),
  diff --git a/dir.c b/dir.c
  index 8c58ce4..cf1e6b0 100644
  --- a/dir.c
  +++ b/dir.c
  @@ -1674,14 +1674,14 @@ void free_pathspec(struct pathspec *pathspec)
   	pathspec->items = NULL;
   }
   
  -<<<<<<< f12e49ae877ad0644b9b9939b7cb742da98691d2
   int limit_pathspec_to_literal(void)
   {
   	static int flag = -1;
   	if (flag < 0)
   		flag = git_env_bool(GIT_LITERAL_PATHSPECS_ENVIRONMENT, 0);
   	return flag;
  -=======
  +}
  +
   /*
    * Frees memory within dir which was allocated for exclude lists and
    * the exclude_stack.  Does not free dir itself.
  @@ -1710,5 +1710,4 @@ void clear_directory(struct dir_struct *dir)
   		free(stk);
   		stk = prev;
   	}
  ->>>>>>> 72aeb18772deeb386da7dd8997b969877bd29e41
   }




 Documentation/merge-strategies.txt |   9 +++
 Documentation/rev-list-options.txt |  14 ++++
 builtin/diff-files.c               |   5 +-
 builtin/diff-tree.c                |   2 +-
 builtin/diff.c                     |  12 +--
 builtin/fmt-merge-msg.c            |   2 +-
 builtin/log.c                      |   9 +--
 builtin/merge.c                    |   1 -
 combine-diff.c                     |  13 ++--
 commit.h                           |   1 +
 diff-lib.c                         |  13 ++--
 diff.h                             |   6 +-
 log-tree.c                         |  69 +++++++++++++++++-
 merge-recursive.c                  |  52 +++++++------
 merge-recursive.h                  |   3 +
 pretty.c                           |  41 +++++++----
 revision.c                         |  17 ++---
 revision.h                         |  26 ++++++-
 submodule.c                        |   3 +-
 t/t3030-merge-recursive.sh         |  33 +++++++++
 t/t4202-log.sh                     |  34 +++++++++
 t/t4213-log-remerge-diff.sh        | 145 +++++++++++++++++++++++++++++++++++++
 22 files changed, 426 insertions(+), 84 deletions(-)
 create mode 100755 t/t4213-log-remerge-diff.sh

-- 
1.9.rc2.232.gdd31389

^ permalink raw reply	[flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-02-04 23:05 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-02-04 22:17 [PATCH 0/9] remerge diff proof of concept/RFC Thomas Rast
2014-02-04 22:17 ` [PATCH 1/9] merge-recursive: remove dead conditional in update_stages() Thomas Rast
2014-02-04 22:17 ` [PATCH 2/9] merge-recursive: internal flag to avoid touching the worktree Thomas Rast
2014-02-04 22:17 ` [PATCH 3/9] merge-recursive: -Xindex-only to leave worktree unchanged Thomas Rast
2014-02-04 22:17 ` [POC PATCH 4/9] pretty: refactor add_merge_info() into parts Thomas Rast
2014-02-04 22:17 ` [POC PATCH 5/9] log: add a merge base inspection option Thomas Rast
2014-02-04 22:17 ` [PATCH 6/9] combine-diff: do not pass revs->dense_combined_merges redundantly Thomas Rast
2014-02-04 22:17 ` [PATCH 7/9] Fold all merge diff variants into an enum Thomas Rast
2014-02-04 22:17 ` [PATCH 8/9] merge-recursive: allow storing conflict hunks in index Thomas Rast
2014-02-04 22:17 ` [RFC PATCH 9/9] log --remerge-diff: show what the conflict resolution changed Thomas Rast
2014-02-04 23:04 ` [PATCH 0/9] remerge diff proof of concept/RFC Junio C Hamano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).