Re: whither merge-tree?
On 23.02.2016 at 06:02 Jeff King wrote: Let's wait and see how many "please don't"s we hear, perhaps, before deciding to go 3.? I'm guessing we won't see much either way. Even Stefan, the original reporter, does not seem to actively be using it, but rather relaying a report. I _am_ actively using it. Maybe I was unclear on that topic. I'm in favour of keeping it, because this means I don't have to rewrite Chris' Code in order to be able to use the Python library that uses merge-tree (Acidfs). But as a sensible human being I want what's best in the long run. I leave that up to you as I have no way of assessing that. So that's a "please don't" leave the code as-is but provide a (transitional) solution that fixes the reported bug and has the best chances of not causing any more headaches :) We'd probably get more response by doing 2 for now, then adding a deprecation warning to the manpage (and possibly the program itself) for the next release. A deprecation warning would be very welcome. Thanks, Stefan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] merge_blobs: use strbuf instead of manually-sized mmfile_t
On 2016-02-16 at 22:27 Junio C Hamano wrote: Three, I know the existence of the program is not more than "we could do something like this" illustration by Linus, and its output is in no way _designed_ to be so. We know today that it does not do Well, then it is just really sad that the manpage doesn't say so. This should be corrected immediately in order to prevent someone to build more (e.g.) libraries on top of it. Stefan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] merge_blobs: use strbuf instead of manually-sized mmfile_t
On 2016-02-16 at 21:35 Jeff King wrote: Yeah, I agree there isn't a great solution in git here. Using "git merge" is definitely wrong if you don't want to touch HEAD or have a working directory. If you _just_ care about doing the tree-level merge without content-level merging inside blobs, you can do it in the index like: export GIT_INDEX_FILE=temp.index base=$(git merge-base $ours $theirs) git read-tree -i -m --aggressive $base $ours $theirs If you want to do content-level resolving on top of that, you can do it with: git merge-index git-merge-one-file -a though it will need a temp directory to write out conflicts and resolved content. That's an interesting alternative, I'll give it a try! I don't think merge-tree is at all the wrong tool, in the sense that it is being used as designed. But it is using merge code that is different from literally the rest of git. That means you're going to run into weird bugs (like this one), and places where it does not behave the same. This add/add case, for instance, is usually a conflict in a normal git merge (try your test case with "git merge"), but merge-tree tries to do a partial content merge with a base that never actually existed[1]. Thank you for clarifying, I understand. So I worry that merge-tree's existence is a disservice to people like Chris, because there is no disclaimer that it is effectively unmaintained. I agree, I don't want to advocate continuing development under these conditions. So merge-blobs.c:generate_common_file() is definitely buggy, but I think the bug gets unintentionally canceled out by the follow-on three-way merge. Which is...good, I guess? Well I don't know how to handle all this with respect to my original problem, but that's completely off topic. Anyway: Thanks! Stefan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] merge_blobs: use strbuf instead of manually-sized mmfile_t
Thank you for working on this! Let me just address two things from an outsider's perspective: On 2016-02-16 at 06:50, Jeff King wrote: Yeah, maybe. There were two reasons I avoided adding a test. One, I secretly hoped that by dragging my feet we could get consensus on just ripping out merge-tree entirely. ;) Since I cannot comment on the code quality or maturity of merge-tree, but would appreciate if this tool worked as expected, let me quote Chris' comment[1] on why he uses merge-tree instead of just "git merge", hoping that it has an impact upon your decision whether to keep the code or "ripping it out": "One might ask, why not use the porcelain 'git merge' command? One reason is, in the context of the two phase commit protocol, we'd rather do pretty much everything we possibly can in the voting stage, leaving ourselves with nothing to do in the finish phase except updating the head to the commit we just created, and possibly updating the working directory--operations that are guaranteed to work. Since 'git merge' will update the head, we'd prefer to do it during the final phase of the commit, but we can't guarantee it will work. There is not a convenient way to do a merge dry run during the voting phase. Although I can conceive of ways to do the merge during the voting phase and roll back to the previous head if we need to, that feels a little riskier. Doing the merge ourselves, here, also frees us from having to work with a working directory, required by the porcelain 'git merge' command. This means we can use bare repositories and/or have transactions that use a head other than the repositories 'current' head." Two, I'm not sure what the test output _should_ be. I think this case is buggy. So we can check that we don't corrupt the heap, which is nice, What do you mean exactly by "this case is buggy"? Doesn't make sense to me. Stefan [1] https://github.com/Pylons/acidfs/blob/master/acidfs/__init__.py -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: malloc memory corruption on merge-tree with leading newline
Addendum: Problem occurs with version 2.7.1 as well as version 1.9.1. On 15/02/16 22:39, Stefan Frühwirth wrote: in one specific circumstance, git-merge-tree exits with a segfault caused by "*** Error in `git': malloc(): memory corruption (fast)": Stefan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
malloc memory corruption on merge-tree with leading newline
Hi, in one specific circumstance, git-merge-tree exits with a segfault caused by "*** Error in `git': malloc(): memory corruption (fast)": There has to be at least one commit first (as far as I can tell it doesn't matter what content). Then create a tree containing a file with a leading newline character (\n) followed by some random string, and another tree with a file containing a string without leading newline. Now merge trees: Segmentation fault. There is a test case[1] kindly provided by chrisrossi, which he crafted after I discovered the problem[2] in the context of Pylons/acidfs. Best, Stefan [1] https://gist.github.com/chrisrossi/f09c8bed70b364f9f12e [2] https://github.com/Pylons/acidfs/issues/3 For in-line reference, here's the test case: git init bug cd bug echo b > a git add a git commit -m "first commit" echo > b echo -n a >> b git add b git commit -m "second commit, first branch" git checkout HEAD~1 git checkout -b other echo -n a > b git add b git commit -m "second commit, second branch" git merge-tree HEAD~1 master other cd .. rm -rf bug -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
