{"thread":{"id":"61495","subject":"Re: [PATCH v2] merge: avoid write merge state when unable to write index","startedAt":"2024-05-17T03:59:36Z","lastAt":"2024-05-17T03:59:36Z","messageCount":1,"participants":["Kyle Zhao"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"494921","messageId":"CACzX-JMdDtZR6XoKEbn422e8ALng_pzkLCi3_Tb8qtXVrdiZew@mail.gmail.com","threadId":"61495","inReplyTo":null,"subject":"Re: [PATCH v2] merge: avoid write merge state when unable to write index","fromName":"Kyle Zhao","fromEmail":"goldofzky@gmail.com","sentAt":"2024-05-17T03:59:23Z","receivedAt":"2024-05-17T03:59:36Z","isPatch":true,"sender":{"key":"goldofzky@gmail.com","avatar":"https://gravatar.com/avatar/aa4fa27f0c1bed9c986ed596415799518e03a6e71a0931308a4de87e166e183a?d=mp&s=160"},"body":"> \"Currently, \" is superfluous in this project, as by convention our\n> proposed log message begins with an observation of the current\n> status (and \"what's wrong in it\" follows) in the present tense.\n\nDone.\n\n> My guess, from reading the tests, of the situation this patch\n> attempts to handle is something like this?\n>\n>    When running a merge while the index is locked (presumably by\n>   another process), the merge state is written in SUCH and SUCH\n>    files, the index is not updated, and then the merge fails.  This\n>    leaves the resulting state inconsistent.\n>\n>> If the user exec \"git commit\" instead of \"git merge --abort\" after this,\n>> a merge commit will be generated and all modifications from the source\n>> branch will be lost.\n>\n> I do not think this is accurate description of the \"an user action\n> can make it worse\".  In reality, if the user runs \"git commit\", a\n> safety kicks in and they get:\n>\n>    fatal: Unable to create '.../.git/index.lock': file exists.\n>\n> In fact, your \"How to reproduce\" below the three-dash line removes\n> the stale index.lock file before running \"git commit\".\n>\n>    From this state, if the index.lock file gets removed and the\n>    user runs \"git commit\", a merge commit is created, recording the\n>    tree from the inconsistent state the merge left.\n>\n> may be a better description of the breakage.\n>\n> But stepping back a bit, I do not think this extra paragraph is\n> needed at all.  If there were a competing process holding the\n> index.lock, it were in the process of updating the index, possibly\n> even to create a new commit.  If that process were indeed running\n> the \"git commit\" command, MERGE_HEAD and other state files we write\n> on our side will be taken into account by them and cause them to\n> record a merge, even though they may have been trying to record\n> something entirely different.  So regardless of what _this_ user,\n> whose merge failed due to a competing process that held the\n> index.lock file, does after the merge failure, the recorded history\n> can be hosed by the other process.\n>\n> In any case, to prevent the other \"git commit\" process from using\n> \"our\" MERGE_HEAD and other state files to record a wrong contents,\n> the right fix is to make sure everybody who takes the lock on the\n> index file does *not* create any other state files to be read by\n> others before it successfully takes the lock.  That will also\n> prevent \"git commit\" we run after a failed merge (and removing the\n> lockfile) from recording an incorrect merge.\n\nYes, your understanding is very correct.\n\nIn fact, there are many other situations besides the example I give\nthat will be affected.\n\n> I do not offhand know if returning 2 (aka \"I am not equipped to\n> handle this merg e at all\") is a good way to do so, but if it is,\n> then the patch given here is absolutely the right thing to do.\n\nIn the latest version I modified it to:\n    die(_(\"Unable to write index.\"));\n\nMaybe it's better. (Refer to the code elsewhere)\n\n> Do not use \"touch\" when the timestamp is not the thing we care\n> about.  In this case, you want that file to _exist_, and the right\n> way to do so is\n>\n> >.git/index.lock &&\n>\n> > which already appears in t7400 (it uses a no-op \":\" command, but\n> > that is not needed).\n\nDone.\n\nThanks.\n"}]}