git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 2/3] merge-recursive: Small code cleanup

From
Elijah Newren <newren@gmail.com>
Date
Sep 8, 2010, 06:24 UTC
Message-ID
<AANLkTim5AA7mnAhkbqJaFcUv9vniTVG7siOMxE+y=ehf@mail.gmail.com>
In-Reply-To
<EF9FEAB3A4B7D245B0801936B6EF4A254B6BBD@azsmsx503.amr.corp.intel.com>
On Tue, Sep 7, 2010 at 10:23 AM, Schalk, Ken <ken.schalk@intel.com> wrote:
Show 15 quoted lines
>>Also, in d5af510 (RE: [PATCH] Avoid rename/add conflict when contents are
>>identical 2010-09-01), a separate if-block was added to provide a special
>>case for the rename/add conflict case that can be resolved (namely when
>>the contents on the destination side are identical).  However, as a
>>separate if block, it's not immediately obvious that its code is related to
>>the subsequent code checking for a rename/add conflict.  We can combine and
>>simplify the check slightly.
>
> Originally I tried the fix the way you've re-structured it, just adding a test to the if around the rename/add conflict handling.  Unfortunately that didn't completely solve the problem in the case that originally motivated the fix (rename vs. rename+symlink, as in my initial post and my first attempt at adding a test to t/t3030-merge-recursive.sh).  That's why I changed it to a separate if block.
>
> The problem comes down in the code inside the "if(try_merge)" block below.  It merges the source of the rename on the other side with the renamed file, rather than the destination.  In the case with the symlink on the other side, this code merged a symlink with a regular file which resulted in a conflict.  I was trying to eliminate both conflicts in this case by avoiding the final else that sets try_merge=1.
>
> Your re-structuring will therefore only solve half the problem I was trying to solve.
>
> I suppose an alternative solution would have been to change the "if(try_merge)" code to merge with the destination of the rename on the other side, if it exists and is the same type.  However that clearly would have had a much more significant impact on other merge cases, so it didn't seem like a good choice to me.

Interesting...that means we probably should have stuck with the original testcase you suggested (though marking it with the SYMLINK dependence), since the new one doesn't fail with my modifications but the old one would. The typechange is critical. So I'll drop that portion of my patch.

Perhaps you could submit another patch changing your testcase back to using a symlink to make sure someone like me doesn't break your original testcase in the future?

Thanks, Elijah

Previous: Schalk, KenNext: Schalk, Ken
Message 11 of 14 in “cherry-picking a commit clobbers a file which is a directory in the target commit”
  1. NickSep 2, 2010
  2. NickSep 6, 2010
  3. Elijah NewrenSep 6, 2010
  4. 0/3 Fix resolvable rename + D/F conflict testcasesElijah Newren, Sep 6, 2010
  5. Elijah NewrenSep 6, 2010
  6. 1/3 t3509: Add rename + D/F conflict testcases that recursive strategy failsElijah Newren, Sep 6, 2010
  7. 2/3 merge-recursive: Small code cleanupElijah Newren, Sep 6, 2010
  8. Elijah NewrenSep 6, 2010
  9. Junio C HamanoSep 6, 2010
  10. Schalk, KenSep 7, 2010
  11. Elijah NewrenSep 8, 2010
  12. Schalk, KenSep 9, 2010
  13. Camille MoncelierOct 21, 2010
  14. 3/3 merge-recursive: D/F conflicts where was_a_dir/file -> was_a_dirElijah Newren, Sep 6, 2010

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.