git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Derrick Stolee <stolee@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v4 00/10] Improve path collision conflict resolutions
Date: Fri, 2 Nov 2018 13:06:32 -0700	[thread overview]
Message-ID: <CABPp-BG2rFEeKVe8ok+a-jLFvPBfnZs1b3Mp2Jfi2JgNZcO8gA@mail.gmail.com> (raw)
In-Reply-To: <232094ce-6872-4039-06a6-af40c709cac0@gmail.com>

On Fri, Nov 2, 2018 at 12:09 PM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 11/2/2018 2:53 PM, Elijah Newren wrote:
> > Major question:
> >    * You'll note that I edited the last two patches to mark them as RFC.
> >      To be honest, I'm not sure what to do with these.  They improve code
> >      coverage of new code, but the same gaps existed in the old code;
> >      they only show up in the coverage-diff because I essentially moved
> >      code around to a new improved function.  Since the new code doesn't
> >      really add new abilities but rather just shifts the handling of
> >      these conflicts to a common function, they shouldn't need any more
> >      testcases than previously and modifying the existing patches thus
> >      feels slightly misleading.  That line of thought leads me to believe
> >      that perhaps putting them in a separate combined patch of their own
> >      with a decent commit message is the right way to go.  On the other
> >      hand, squashing them to the commits they're marked as fixups for
> >      shows which logical part of the code the tests are related to, which
> >      seems like a good thing.  So what's the right way to handle these?
>
> I appreciate the effort you made to improve test coverage! It's
> unfortunate that this portion wasn't covered earlier, because we could
> have broken it and not noticed until a release.

Yes, I agree...except that I think we might not have noticed until a
couple releases down the road; these things tend to not come up a lot
in practice.  (Which may make it even more important to pay attention
to code coverage.)

> I think making them separate commits is fine, and the comment on the
> test case is helpful. The fact that you only had to change the commit
> timestamps in order to get the coverage makes me reexamine the code and
> realize that maybe the "right" thing to do is to reduce our code clones.
> (This is also how I was looking at the wrong block of the patch when
> talking about it not being covered.) I'll look at the patch and see if I
> can contribute a concrete code suggestion there.

Yeah, I had the same feeling, _again_, while re-looking at the tests
and code as well.  I think the history of how we got here goes
something like this:

  * there is some fairly simple code to handle these rename/rename
(1to2 and 2to1) cases, with logic for handling each side being a
neary-copy of each other.
  * someone does some analysis about trying to remove duplication and
notes that there are 3-4 pieces that change; adding logic to change
out those pieces and rewrite it as a loop adds some complexity, which
isn't worth it given the simple code
  * additional issues are discovered, such as D/F conflicts or
inappropriate handling of untracked or dirty files, and due to
merge-recursive's bad design[1], the fixes have to be sprinkled
*everywhere* throughout the whole code base.  Lather, rinse, repeat a
few times.
  * Now, although the original analysis of removing the duplication
was correct given the amount of code that exited at the time, the
weights have changed as new code was added to both codepaths.  But the
original analysis is long since forgotten, the code is more complex,
and we have to think about whether fixing it now is worth it if we're
going to rewrite it all anyway to fix that fundamental design flaw[2].

[1] https://public-inbox.org/git/xmqqd147kpdm.fsf@gitster.mtv.corp.google.com/
[2] https://public-inbox.org/git/xmqqk1ydkbx0.fsf@gitster.mtv.corp.google.com/


> Aside: I hope that I am not being annoying by poking around with the
> test coverage reports. It does give me a way to target my review
> efforts, especially into changes that touch areas outside my expertise
> (like this one).

Not annoying at all; I think it's a very valuable thing you're doing.
And I think these tests make things better (there have definitely been
cases in the past where a merge one way would work and a merge the
other way would have funny bugs).  I was just unsure about what the
best way to present it amongst the other changes was.

  reply	other threads:[~2018-11-02 20:06 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-14  2:05 [RFC PATCH v2 0/7] Improve path collision conflict resolutions Elijah Newren
2018-10-14  2:05 ` [RFC PATCH v2 1/7] Add testcases for consistency in file collision conflict handling Elijah Newren
2018-10-14  2:05 ` [RFC PATCH v2 2/7] t6036, t6042: testcases for rename collision of already conflicting files Elijah Newren
2018-10-14  2:05 ` [RFC PATCH v2 3/7] merge-recursive: new function for better colliding conflict resolutions Elijah Newren
2018-10-14  2:05 ` [RFC PATCH v2 4/7] merge-recursive: fix rename/add conflict handling Elijah Newren
2018-10-14  2:05 ` [RFC PATCH v2 5/7] merge-recursive: improve handling for rename/rename(2to1) conflicts Elijah Newren
2018-10-14  2:05 ` [RFC PATCH v2 6/7] merge-recursive: use handle_file_collision for add/add conflicts Elijah Newren
2018-10-14  2:05 ` [RFC PATCH v2 7/7] merge-recursive: improve rename/rename(1to2)/add[/add] handling Elijah Newren
2018-10-19 19:31 ` [PATCH v3 0/8] Improve path collision conflict resolutions Elijah Newren
2018-10-19 19:31   ` [PATCH v3 1/8] Add testcases for consistency in file collision conflict handling Elijah Newren
2018-10-19 19:31   ` [PATCH v3 2/8] t6036, t6042: testcases for rename collision of already conflicting files Elijah Newren
2018-10-31 14:01     ` Derrick Stolee
2018-11-01  6:57       ` Elijah Newren
2018-10-19 19:31   ` [PATCH v3 3/8] merge-recursive: increase marker length with depth of recursion Elijah Newren
2018-10-19 19:31   ` [PATCH v3 4/8] merge-recursive: new function for better colliding conflict resolutions Elijah Newren
2018-10-31 13:53     ` Derrick Stolee
2018-10-31 13:57       ` Derrick Stolee
2018-11-01  6:56         ` Elijah Newren
2018-10-19 19:31   ` [PATCH v3 5/8] merge-recursive: fix rename/add conflict handling Elijah Newren
2018-10-19 19:31   ` [PATCH v3 6/8] merge-recursive: improve handling for rename/rename(2to1) conflicts Elijah Newren
2018-10-19 19:31   ` [PATCH v3 7/8] merge-recursive: use handle_file_collision for add/add conflicts Elijah Newren
2018-10-19 19:31   ` [PATCH v3 8/8] merge-recursive: improve rename/rename(1to2)/add[/add] handling Elijah Newren
2018-10-31 15:08     ` Derrick Stolee
2018-11-01  7:01       ` Elijah Newren
2018-11-02 17:27         ` Elijah Newren
2018-11-02 17:30           ` Derrick Stolee
2018-11-02 18:53   ` [PATCH v4 00/10] Improve path collision conflict resolutions Elijah Newren
2018-11-02 18:53     ` [PATCH v4 01/10] Add testcases for consistency in file collision conflict handling Elijah Newren
2018-11-02 18:53     ` [PATCH v4 02/10] t6036, t6042: testcases for rename collision of already conflicting files Elijah Newren
2018-11-02 18:53     ` [PATCH v4 03/10] merge-recursive: increase marker length with depth of recursion Elijah Newren
2018-11-02 18:53     ` [PATCH v4 04/10] merge-recursive: new function for better colliding conflict resolutions Elijah Newren
2018-11-02 18:53     ` [PATCH v4 05/10] merge-recursive: fix rename/add conflict handling Elijah Newren
2018-11-02 18:53     ` [PATCH v4 06/10] merge-recursive: improve handling for rename/rename(2to1) conflicts Elijah Newren
2018-11-02 18:53     ` [PATCH v4 07/10] merge-recursive: use handle_file_collision for add/add conflicts Elijah Newren
2018-11-02 18:53     ` [PATCH v4 08/10] merge-recursive: improve rename/rename(1to2)/add[/add] handling Elijah Newren
2018-11-02 20:01       ` [PATCH] merge-recursive: combine error handling Derrick Stolee
2018-11-02 18:53     ` [RFC PATCH v4 09/10] fixup! merge-recursive: fix rename/add conflict handling Elijah Newren
2018-11-02 19:05     ` [PATCH v4 10/10] fixup! merge-recursive: improve rename/rename(1to2)/add[/add] handling Elijah Newren
2018-11-02 19:09     ` [PATCH v4 00/10] Improve path collision conflict resolutions Derrick Stolee
2018-11-02 20:06       ` Elijah Newren [this message]
2018-11-08  4:40         ` [PATCH v5 " Elijah Newren
2018-11-08  4:40           ` [PATCH v5 01/10] Add testcases for consistency in file collision conflict handling Elijah Newren
2018-11-08  4:40           ` [PATCH v5 02/10] t6036, t6042: testcases for rename collision of already conflicting files Elijah Newren
2018-11-08  4:40           ` [PATCH v5 03/10] merge-recursive: increase marker length with depth of recursion Elijah Newren
2018-11-08  4:40           ` [PATCH v5 04/10] merge-recursive: new function for better colliding conflict resolutions Elijah Newren
2018-11-08  4:40           ` [PATCH v5 05/10] merge-recursive: fix rename/add conflict handling Elijah Newren
2018-11-08  4:40           ` [PATCH v5 06/10] merge-recursive: improve handling for rename/rename(2to1) conflicts Elijah Newren
2018-11-08  4:40           ` [PATCH v5 07/10] merge-recursive: use handle_file_collision for add/add conflicts Elijah Newren
2018-11-08  4:40           ` [PATCH v5 08/10] merge-recursive: improve rename/rename(1to2)/add[/add] handling Elijah Newren
2018-11-08  4:40           ` [PATCH v5 09/10] t6036, t6043: increase code coverage for file collision handling Elijah Newren
2018-11-08  4:40           ` [PATCH v5 10/10] merge-recursive: combine error handling Elijah Newren
2018-11-08  5:25             ` Junio C Hamano

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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=CABPp-BG2rFEeKVe8ok+a-jLFvPBfnZs1b3Mp2Jfi2JgNZcO8gA@mail.gmail.com \
    --to=newren@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=stolee@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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).