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

Re: [PATCH v4 4/6] patch-id: make it stable against hunk reordering

From
Junio C Hamano <gitster@pobox.com>
Date
Apr 23, 2014, 22:05 UTC
Message-ID
<xmqqzjjbwvk9.fsf@gitster.dls.corp.google.com>
In-Reply-To
<20140423175717.GB28308@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> writes:
Show 7 quoted lines
> On Wed, Apr 23, 2014 at 10:39:23AM -0700, Junio C Hamano wrote:
>> Are these three patches the same as what has been queued on
>> mt/patch-id-stable topic and cooking in 'next' for a few weeks?
>
> Not exactly - at your request I implemented git config
> options to control patch id behaviour.
> Documentation and tests updated to match.

After comparing the patches 4-6 and the one that has been in 'next' for a few weeks, I tried to like the new one, but I couldn't.

The new one, if I am reading the patch correctly, makes the stable variant the default if

 - the configuration explicitly asks to use it; or
 - diff.orderfile, which is a new configuration that did not exist,
   is used.

At the first glance, the latter makes it look as if that will not hurt any existing users/repositories, but the thing is, the producer of the patches is different from the consumer of the patches. There needs to be a way for a consumer to say "I need stable variant" on the patches when computing "patch-id" on a patch that arrived. As long as two different producers use two different orders, the consumer of the patches from these two sources is forced to use the stable variant if some sort of cache is kept keyed with the patch-ids.

But "diff.orderfile" configuration is all and only about the producer, and does not help the case at all.

Compared to it, the previous version forced people who do not have a particular reason to choose between the variants to use the new "stable" variant, which was a lot simpler solution.

The reason why I merged the previous one to 'next' was because I wanted to see if the behaviour change is as grave as I thought, or I was just being unnecessary cautious. If there is no third-party script that precomputes something and stores them under these hashes that breaks with this change, I do not see any reason not to make the new "stable" one the default.

I however suspect that the ideal "stable" implementation may be different from what you implemented. What if we compute a patch-id on a reordered patch as if the files came in the "usual" order? That would be another way to achieve the stable-ness for sane cases (i.e. no funny "you could split one patch with two hunks into two patches with one hunk each twice mentioning the same path" which is totally an uninteresting use case---diff.orderfile would not even produce such a patch) and a huge difference would be that it would produce the same patch-id as existing versions of Git, if the patch is not reordered. Is that asking for a moon (I admit that I haven't looked at the code involved too deeply)?

Previous: Michael S. TsirkinNext: Michael S. Tsirkin
Message 11 of 15 in “diff: add a config option to control orderfile”
  1. 1/6 diff: add a config option to control orderfileMichael S. Tsirkin, Apr 23, 2014
  2. 2/6 test: add test_write_lines helperMichael S. Tsirkin, Apr 23, 2014
  3. Junio C HamanoApr 23, 2014
  4. Michael S. TsirkinApr 23, 2014
  5. 3/6 tests: new test for orderfile optionsMichael S. Tsirkin, Apr 23, 2014
  6. Junio C HamanoApr 23, 2014
  7. Michael S. TsirkinApr 23, 2014
  8. 4/6 patch-id: make it stable against hunk reorderingMichael S. Tsirkin, Apr 23, 2014
  9. Junio C HamanoApr 23, 2014
  10. Michael S. TsirkinApr 23, 2014
  11. Junio C HamanoApr 23, 2014
  12. Michael S. TsirkinApr 24, 2014
  13. Junio C HamanoApr 24, 2014
  14. 5/6 patch-id: document new behaviourMichael S. Tsirkin, Apr 23, 2014
  15. 6/6 patch-id-test: test stable and unstable behaviourMichael S. Tsirkin, Apr 23, 2014

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.