Re: [PATCH] disable grafts during fetch/push/bundle
On Wed, Mar 19, 2014 at 03:39:42PM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: On Fri, Mar 07, 2014 at 08:08:37AM +0100, Christian Couder wrote: Be it graft or replace, I do not think we want to invite people to use these mechansims too lightly to locally rewrite their history willy-nilly without fixing their mistakes at the object layer with commit --amend, rebase, bfg, etc. in the longer term. So in that sense, adding a command to make it easy is not something I am enthusiastic about. On the other hand, if the user does need to use graft or replace (perhaps to prepare for casting the fixed history in stone with filter-branch), it would be good to help them avoid making mistakes while doing so and tool support may be a way to do so. So, ... I am of two minds. ... I do not think the features we are talking about are significantly more dangerous than git replace is in the first place. If we want to make people aware of the dangers, perhaps git-replace.1 is the right place to do it. Sure. So should we take the four-patch series for git replace --edit? I think that is certainly going in the right direction, but it is missing documentation and tests still. Aside from a one-liner bug (which Christian pointed out on the list), I do not think it will _hurt_ anybody. But it probably should be finished before seeing the light of day. I'd be happy if you wanted to pick it up for pu or even next waiting and do that finishing in-tree. Otherwise, I may eventually get to it and re-roll the whole completed series. -Peff -- 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] disable grafts during fetch/push/bundle
Jeff King p...@peff.net writes: On Fri, Mar 07, 2014 at 08:08:37AM +0100, Christian Couder wrote: Be it graft or replace, I do not think we want to invite people to use these mechansims too lightly to locally rewrite their history willy-nilly without fixing their mistakes at the object layer with commit --amend, rebase, bfg, etc. in the longer term. So in that sense, adding a command to make it easy is not something I am enthusiastic about. On the other hand, if the user does need to use graft or replace (perhaps to prepare for casting the fixed history in stone with filter-branch), it would be good to help them avoid making mistakes while doing so and tool support may be a way to do so. So, ... I am of two minds. ... I do not think the features we are talking about are significantly more dangerous than git replace is in the first place. If we want to make people aware of the dangers, perhaps git-replace.1 is the right place to do it. Sure. So should we take the four-patch series for git replace --edit? -- 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] disable grafts during fetch/push/bundle
On Fri, Mar 07, 2014 at 08:08:37AM +0100, Christian Couder wrote: Be it graft or replace, I do not think we want to invite people to use these mechansims too lightly to locally rewrite their history willy-nilly without fixing their mistakes at the object layer with commit --amend, rebase, bfg, etc. in the longer term. So in that sense, adding a command to make it easy is not something I am enthusiastic about. On the other hand, if the user does need to use graft or replace (perhaps to prepare for casting the fixed history in stone with filter-branch), it would be good to help them avoid making mistakes while doing so and tool support may be a way to do so. So, ... I am of two minds. Maybe if we add a new command (or maybe a script) with a name long and cryptic-looking enough like git create-replacement-object it will scare casual users from touching it, while power users will be happy to benefit from it. I do not think the features we are talking about are significantly more dangerous than git replace is in the first place. If we want to make people aware of the dangers, perhaps git-replace.1 is the right place to do it. -Peff -- 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] disable grafts during fetch/push/bundle
On 03/05/2014 08:18 PM, Junio C Hamano wrote: Jeff King p...@peff.net writes: On Wed, Mar 05, 2014 at 10:49:24AM -0800, Junio C Hamano wrote: ... the plan, at least in my mind, has always been exactly that: grafts were a nice little attempt but is broken---if you really wanted to muck with the history without rewriting (which is still discouraged, by the way), do not use graft, but use replace. I certainly had in the back of my mind that grafts were a lesser form of replace, and that eventually we could get rid of the former. Perhaps my question should have been: why haven't we deprecated grafts yet?. Given that we discourage grafts strongly and replace less so (but still discourage it), telling the users that biting the bullet and rewriting the history is _the_ permanent solution, I think it is understandable why nobody has bothered to. Replace objects are better than grafts in *almost* every dimension. The exception is that it is dead simple to create grafts, whereas I always have to break open the man pages to remember how to create a replace object that does the same thing. So I think a helpful step towards deprecating grafts would be to offer a couple of convenience features to help people kick the grafts habit: * A tool that converts grafts (i.e., the grafts read from $GIT_DIR/info/grafts) into the equivalent replacements. * A tool that creates a new replacement object that is the equivalent of a graft. I.e., it should do, using replace references, the equivalent of the following command: echo SHA1 [PARENT1...] $GIT_DIR/info/grafts These features could be added to git replace or could be built into a new git grafts command. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- 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] disable grafts during fetch/push/bundle
On Thu, Mar 6, 2014 at 9:42 AM, Michael Haggerty mhag...@alum.mit.edu wrote: On 03/05/2014 08:18 PM, Junio C Hamano wrote: Jeff King p...@peff.net writes: On Wed, Mar 05, 2014 at 10:49:24AM -0800, Junio C Hamano wrote: ... the plan, at least in my mind, has always been exactly that: grafts were a nice little attempt but is broken---if you really wanted to muck with the history without rewriting (which is still discouraged, by the way), do not use graft, but use replace. I certainly had in the back of my mind that grafts were a lesser form of replace, and that eventually we could get rid of the former. Perhaps my question should have been: why haven't we deprecated grafts yet?. Given that we discourage grafts strongly and replace less so (but still discourage it), telling the users that biting the bullet and rewriting the history is _the_ permanent solution, I think it is understandable why nobody has bothered to. Replace objects are better than grafts in *almost* every dimension. The exception is that it is dead simple to create grafts, whereas I always have to break open the man pages to remember how to create a replace object that does the same thing. So I think a helpful step towards deprecating grafts would be to offer a couple of convenience features to help people kick the grafts habit: * A tool that converts grafts (i.e., the grafts read from $GIT_DIR/info/grafts) into the equivalent replacements. Yeah, I sent a kind of rough draft of a script to do that last year to the mailing list, but I didn't take the time to convert it to a real script or command. * A tool that creates a new replacement object that is the equivalent of a graft. I.e., it should do, using replace references, the equivalent of the following command: echo SHA1 [PARENT1...] $GIT_DIR/info/grafts Yeah, maybe it can be a git create-replace-ref command and it could have a --convert-graft-file option to convert an existing graft file. There have been discussions about such a command already some time ago. These features could be added to git replace or could be built into a new git grafts command. I think Junio previously said that it was better if such features were not part of git replace. But maybe I misunderstood his subtle saying. And I don't think git grafts is a good name. It looks too much like we are encouraging people to use grafts. Thanks, Christian. -- 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] disable grafts during fetch/push/bundle
On Thu, Mar 06, 2014 at 09:42:46AM +0100, Michael Haggerty wrote: Replace objects are better than grafts in *almost* every dimension. The exception is that it is dead simple to create grafts, whereas I always have to break open the man pages to remember how to create a replace object that does the same thing. So I think a helpful step towards deprecating grafts would be to offer a couple of convenience features to help people kick the grafts habit: I agree that better tool support would make git replace more pleasant to use. * A tool that converts grafts (i.e., the grafts read from $GIT_DIR/info/grafts) into the equivalent replacements. I don't know if this is strictly necessary, if we make your command below pleasant to use. I.e., it should just be: while read sha1 parents; do git replace --graft $sha1 $parents done .git/info/grafts We can wrap that in git replace --convert-grafts, but I do not think grafts are so common that there would be a big demand for it. * A tool that creates a new replacement object that is the equivalent of a graft. I.e., it should do, using replace references, the equivalent of the following command: echo SHA1 [PARENT1...] $GIT_DIR/info/grafts These features could be added to git replace or could be built into a new git grafts command. I think it would be nice to have a set of mode options for git-replace to do basic editing of a sha1 and install the result (technically you could split the editing into a separate command, but I do not see the point in editing a sha1 and then _not_ replacing it). Perhaps: # pretty-print sha1 based on type, start $EDITOR, create a # type-appropriate object from the result (e.g., using hash-object, # mktree, or mktag), and then set up the object as a replacement for # SHA1 git replace --edit SHA1 # ditto, but replace the $EDITOR step with the parent list git replace --graft SHA1 PARENT1 PARENT2 # ...or remove entries from a tree git replace --remove-entry SHA1 foo bar -Peff -- 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] disable grafts during fetch/push/bundle
On 03/06/2014 04:56 PM, Jeff King wrote: On Thu, Mar 06, 2014 at 09:42:46AM +0100, Michael Haggerty wrote: Replace objects are better than grafts in *almost* every dimension. The exception is that it is dead simple to create grafts, whereas I always have to break open the man pages to remember how to create a replace object that does the same thing. So I think a helpful step towards deprecating grafts would be to offer a couple of convenience features to help people kick the grafts habit: I agree that better tool support would make git replace more pleasant to use. * A tool that converts grafts (i.e., the grafts read from $GIT_DIR/info/grafts) into the equivalent replacements. I don't know if this is strictly necessary, if we make your command below pleasant to use. I.e., it should just be: while read sha1 parents; do git replace --graft $sha1 $parents done .git/info/grafts We can wrap that in git replace --convert-grafts, but I do not think grafts are so common that there would be a big demand for it. It's probably easier to wrap it than to explain to Windows users what they have to do. * A tool that creates a new replacement object that is the equivalent of a graft. I.e., it should do, using replace references, the equivalent of the following command: echo SHA1 [PARENT1...] $GIT_DIR/info/grafts These features could be added to git replace or could be built into a new git grafts command. I think it would be nice to have a set of mode options for git-replace to do basic editing of a sha1 and install the result (technically you could split the editing into a separate command, but I do not see the point in editing a sha1 and then _not_ replacing it). If modifying without replacing is needed, it would be pretty easy to add an option --stdout that writes the SHA1 of the modified object to stdout instead of creating a replace reference. That way what you want 95% of the time is the default but there is still an escape hatch. Perhaps: # pretty-print sha1 based on type, start $EDITOR, create a # type-appropriate object from the result (e.g., using hash-object, # mktree, or mktag), and then set up the object as a replacement for # SHA1 git replace --edit SHA1 # ditto, but replace the $EDITOR step with the parent list git replace --graft SHA1 PARENT1 PARENT2 # ...or remove entries from a tree git replace --remove-entry SHA1 foo bar I like this idea a lot, especially the pretty-printer round-tripping. git replace could support some of the options that git filter-branch can take, like --env-filter, --msg-filter, etc. (at least if the target is a commit object). All of this would make it possible to build up the changes that you want to integrate via filter-branch piecemeal instead of having to have a single monster filter-branch invocation. For example, for c in $(git rev-list --all --before=2007-01-01 --author=root@localhost) do git replace --env-filter 'export AUTHOR_EMAIL=j...@example.com' $c done # Make some more changes to other commits... # And when everything is done and checked: git filter-branch --all --tag-name-filter=cat To me this is easier to construct than the equivalent filter-branch invocation, and can be faster because its processing can be more easily limited to the commits that need it. Of course to really gain speed, there should be a C program that bakes in replace references by traversing the object tree rather than processing each commit separately, like filter-branch. I predict that this approach would have most of the speed of BFG. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- 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] disable grafts during fetch/push/bundle
On Thu, Mar 06, 2014 at 05:41:27PM +0100, Michael Haggerty wrote: We can wrap that in git replace --convert-grafts, but I do not think grafts are so common that there would be a big demand for it. It's probably easier to wrap it than to explain to Windows users what they have to do. How would Windows users get a graft file in the first-place? There's no GUI for it! ;) It should be easy to do --convert-grafts, though, and I think it fits into the scheme we're discussing below. I think it would be nice to have a set of mode options for git-replace to do basic editing of a sha1 and install the result (technically you could split the editing into a separate command, but I do not see the point in editing a sha1 and then _not_ replacing it). If modifying without replacing is needed, it would be pretty easy to add an option --stdout that writes the SHA1 of the modified object to stdout instead of creating a replace reference. That way what you want 95% of the time is the default but there is still an escape hatch. Agreed. I had originally though that perhaps something like this should be part of hash-object, and that replace should farm out the work. But thinking on it more, it doesn't really make sense as part of hash-object. Perhaps: # pretty-print sha1 based on type, start $EDITOR, create a # type-appropriate object from the result (e.g., using hash-object, # mktree, or mktag), and then set up the object as a replacement for # SHA1 git replace --edit SHA1 Here's a rough series that gets us this far: [1/4]: replace: refactor command-mode determination [2/4]: replace: use OPT_CMDMODE to handle modes [3/4]: replace: factor object resolution out of replace_object [4/4]: replace: add --edit option It shouldn't be too hard to do --graft or --convert-grafts on top. I also noticed that doing: git replace foo foo is less than friendly (we notice the cycle, but just barf). It's especially easy to do with git replace --edit, if you just exit the editor without making changes. Or if you make changes to an already-replaced object to revert it back, in which case we would want to notice and delete the replacement. So I think we want to have git replace foo foo silently converted into git replace -d foo (but without an error if there is no existing replacement), and then --edit will just do the right thing, as it's built on top. I also noticed that the diff engine does not play well with replacements of blobs. When we are diffing the trees, we see that the sha1 for path foo is the same on either side, and do not look further, even though feeding those sha1s to builtin_diff would fetch the replacements. I think compare_tree_entry would have to learn lookup_replace_object (and I suspect it would make tree diffs noticeably slower when you have even one replace ref). git replace could support some of the options that git filter-branch can take, like --env-filter, --msg-filter, etc. (at least if the target is a commit object). All of this would make it possible to build up the changes that you want to integrate via filter-branch piecemeal instead of having to have a single monster filter-branch invocation. For example, Right. I was tempted to suggest that, too, but I think it can get rather tricky, as you need to replace in a loop, and sometimes the exact objects you need aren't obvious. For example, a common use of --index-filter is to remove a single file. But to remove foo/bar/baz, you would need to loop over each commit, find the tree for foo/bar, and then remove the baz entry in Still, I really like the workflow of having decent replace tools, followed by cementing the changes into place with a filter-branch run (which, btw, does not yet know how to cement trees and blobs into place). It lets you work on the filtering incrementally, and even share or work collaboratively on it by pushing refs/replace). And as you mention, it could be a heck of a lot faster than what we have now. -Peff -- 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] disable grafts during fetch/push/bundle
Jeff King p...@peff.net writes: I also noticed that the diff engine does not play well with replacements of blobs. When we are diffing the trees, we see that the sha1 for path foo is the same on either side, and do not look further, even though feeding those sha1s to builtin_diff would fetch the replacements. Sorry, I do not quite understand. In git diff A B -- path, if the object name recorded for A:path and B:path are the same, but the replacement mechanism maps the object name for that blob object to some other blob object, wouldn't the result be empty because both sides replace to the same thing anyway? -- 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] disable grafts during fetch/push/bundle
On Thu, Mar 06, 2014 at 11:00:08AM -0800, Junio C Hamano wrote:
Jeff King p...@peff.net writes:
I also noticed that the diff engine does not play well with replacements
of blobs. When we are diffing the trees, we see that the sha1 for path
foo is the same on either side, and do not look further, even though
feeding those sha1s to builtin_diff would fetch the replacements.
Sorry, I do not quite understand.
In git diff A B -- path, if the object name recorded for A:path
and B:path are the same, but the replacement mechanism maps the
object name for that blob object to some other blob object, wouldn't
the result be empty because both sides replace to the same thing
anyway?
Oh, right, I was being stupid. I did:
git replace --edit HEAD:some-file
and expected git show to find the diff. But that doesn't make sense.
On top of that, I need to do:
git replace --edit HEAD^{tree}
to replace the sha1 of the entry in the tree. In which case diff would
find it just fine.
-Peff
--
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] disable grafts during fetch/push/bundle
On 03/07/2014 12:01 AM, Philip Oakley wrote: From: Jeff King p...@peff.net On Thu, Mar 06, 2014 at 05:41:27PM +0100, Michael Haggerty wrote: We can wrap that in git replace --convert-grafts, but I do not think grafts are so common that there would be a big demand for it. It's probably easier to wrap it than to explain to Windows users what they have to do. How would Windows users get a graft file in the first-place? There's no GUI for it! ;) Now, now... It's dead easy using the git-gui and Notepad++, you can see and confirm the sha1's, copy and paste, and the graft file is a very easy format, so even wimps (windows, icons, menus, pointers aka mouse) folks can do it. (It worked for me when I needed it ;-) I didn't mean to insult all Windows users in general. I was only referring to the fact that since the default Windows command line is not a POSIX shell, even an experienced Windows user might have trouble figuring out how to execute a shell loop. Putting this functionality in a git command or script, by contrast, would make it work universally, no fuss, no muss. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- 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] disable grafts during fetch/push/bundle
Michael Haggerty mhag...@alum.mit.edu writes: I didn't mean to insult all Windows users in general. I was only referring to the fact that since the default Windows command line is not a POSIX shell, even an experienced Windows user might have trouble figuring out how to execute a shell loop. Putting this functionality in a git command or script, by contrast, would make it work universally, no fuss, no muss. ;-) Be it graft or replace, I do not think we want to invite people to use these mechansims too lightly to locally rewrite their history willy-nilly without fixing their mistakes at the object layer with commit --amend, rebase, bfg, etc. in the longer term. So in that sense, adding a command to make it easy is not something I am enthusiastic about. On the other hand, if the user does need to use graft or replace (perhaps to prepare for casting the fixed history in stone with filter-branch), it would be good to help them avoid making mistakes while doing so and tool support may be a way to do so. So, ... I am of two minds. -- 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] disable grafts during fetch/push/bundle
From: Michael Haggerty mhag...@alum.mit.edu On 03/07/2014 12:01 AM, Philip Oakley wrote: From: Jeff King p...@peff.net On Thu, Mar 06, 2014 at 05:41:27PM +0100, Michael Haggerty wrote: We can wrap that in git replace --convert-grafts, but I do not think grafts are so common that there would be a big demand for it. It's probably easier to wrap it than to explain to Windows users what they have to do. How would Windows users get a graft file in the first-place? There's no GUI for it! ;) Now, now... It's dead easy using the git-gui and Notepad++, you can see and confirm the sha1's, copy and paste, and the graft file is a very easy format, so even wimps (windows, icons, menus, pointers aka mouse) folks can do it. (It worked for me when I needed it ;-) I didn't mean to insult all Windows users in general. I was only referring to the fact that since the default Windows command line is not a POSIX shell, even an experienced Windows user might have trouble figuring out how to execute a shell loop. I'd missed that aspect about the shell loop. I was mainly pointing out current awkwardness of creating the replace object, relative to grafts - There was an initial attempt by Christian, but it became quite hard to make it robust to sha1's embedded in commit messages. Putting this functionality in a git command or script, by contrast, would make it work universally, no fuss, no muss. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- 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] disable grafts during fetch/push/bundle
On Fri, Mar 7, 2014 at 12:39 AM, Junio C Hamano gits...@pobox.com wrote: Michael Haggerty mhag...@alum.mit.edu writes: I didn't mean to insult all Windows users in general. I was only referring to the fact that since the default Windows command line is not a POSIX shell, even an experienced Windows user might have trouble figuring out how to execute a shell loop. Putting this functionality in a git command or script, by contrast, would make it work universally, no fuss, no muss. ;-) Be it graft or replace, I do not think we want to invite people to use these mechansims too lightly to locally rewrite their history willy-nilly without fixing their mistakes at the object layer with commit --amend, rebase, bfg, etc. in the longer term. So in that sense, adding a command to make it easy is not something I am enthusiastic about. On the other hand, if the user does need to use graft or replace (perhaps to prepare for casting the fixed history in stone with filter-branch), it would be good to help them avoid making mistakes while doing so and tool support may be a way to do so. So, ... I am of two minds. Maybe if we add a new command (or maybe a script) with a name long and cryptic-looking enough like git create-replacement-object it will scare casual users from touching it, while power users will be happy to benefit from it. -- 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] disable grafts during fetch/push/bundle
Jeff King p...@peff.net writes: On Tue, Mar 04, 2014 at 12:52:18PM -0800, Junio C Hamano wrote: I do not recall any past discussion on this topic, and searching the archive only shows people echoing what I said above. Is this something we've promised to work in the past? The history lesson is coming solely from my recollection of a discussion I and Linus had on the list back when we were doing the original graft and thinking about the interaction between it and the object/history transfer; especially the only add new ones, but hide the ones that the user wants to be hidden part is something suggested by Linus but we couldn't implement back then, IIRC. Perhaps the right response is grafts are broken, use git-replace instead. But then should we think about deprecating grafts? I am sort of surprised to hear that question, actually ;-) I didn't say that in the message you are responding to because I somehow thought that everybody has been in agreement with these two lines for a long while. Ever since I suggested the replace as an alternative grafts done right and outlined how it should work to Christian while sitting next to him in one of the early GitTogether, the plan, at least in my mind, has always been exactly that: grafts were a nice little attempt but is broken---if you really wanted to muck with the history without rewriting (which is still discouraged, by the way), do not use graft, but use replace. -- 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] disable grafts during fetch/push/bundle
Jeff King p...@peff.net writes: On Wed, Mar 05, 2014 at 10:49:24AM -0800, Junio C Hamano wrote: ... the plan, at least in my mind, has always been exactly that: grafts were a nice little attempt but is broken---if you really wanted to muck with the history without rewriting (which is still discouraged, by the way), do not use graft, but use replace. I certainly had in the back of my mind that grafts were a lesser form of replace, and that eventually we could get rid of the former. Perhaps my question should have been: why haven't we deprecated grafts yet?. Given that we discourage grafts strongly and replace less so (but still discourage it), telling the users that biting the bullet and rewriting the history is _the_ permanent solution, I think it is understandable why nobody has bothered to. -- 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] disable grafts during fetch/push/bundle
On Wed, Mar 05, 2014 at 11:18:17AM -0800, Junio C Hamano wrote: Jeff King p...@peff.net writes: On Wed, Mar 05, 2014 at 10:49:24AM -0800, Junio C Hamano wrote: ... the plan, at least in my mind, has always been exactly that: grafts were a nice little attempt but is broken---if you really wanted to muck with the history without rewriting (which is still discouraged, by the way), do not use graft, but use replace. I certainly had in the back of my mind that grafts were a lesser form of replace, and that eventually we could get rid of the former. Perhaps my question should have been: why haven't we deprecated grafts yet?. Given that we discourage grafts strongly and replace less so (but still discourage it), telling the users that biting the bullet and rewriting the history is _the_ permanent solution, I think it is understandable why nobody has bothered to. Perhaps the patch below would help discourage grafts more? The notable place in the documentation where grafts are still used is git-filter-branch.txt. But since the example there is about cementing rewritten history, it might be OK to leave. I used outdated below. We could also up the ante to deprecated. -- 8 -- Subject: [PATCH] docs: mark info/grafts as outdated We should be encouraging people to use git-replace instead. Signed-off-by: Jeff King p...@peff.net --- Documentation/gitrepository-layout.txt | 4 Documentation/glossary-content.txt | 4 2 files changed, 8 insertions(+) diff --git a/Documentation/gitrepository-layout.txt b/Documentation/gitrepository-layout.txt index aa03882..17d2ea6 100644 --- a/Documentation/gitrepository-layout.txt +++ b/Documentation/gitrepository-layout.txt @@ -176,6 +176,10 @@ info/grafts:: per line describes a commit and its fake parents by listing their 40-byte hexadecimal object names separated by a space and terminated by a newline. ++ +Note that the grafts mechanism is outdated and can lead to problems +transferring objects between repositories; see linkgit:git-replace[1] +for a more flexible and robust system to do the same thing. info/exclude:: This file, by convention among Porcelains, stores the diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt index 378306f..be0858c 100644 --- a/Documentation/glossary-content.txt +++ b/Documentation/glossary-content.txt @@ -176,6 +176,10 @@ current branch integrates with) obviously do not work, as there is no you can make Git pretend the set of def_parent,parents a def_commit,commit has is different from what was recorded when the commit was created. Configured via the `.git/info/grafts` file. ++ +Note that the grafts mechanism is outdated and can lead to problems +transferring objects between repositories; see linkgit:git-replace[1] +for a more flexible and robust system to do the same thing. [[def_hash]]hash:: In Git's context, synonym for def_object_name,object name. -- 1.8.5.2.500.g8060133 -- 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] disable grafts during fetch/push/bundle
Jeff King p...@peff.net writes: On Wed, Mar 05, 2014 at 11:18:17AM -0800, Junio C Hamano wrote: Given that we discourage grafts strongly and replace less so (but still discourage it), telling the users that biting the bullet and rewriting the history is _the_ permanent solution, I think it is understandable why nobody has bothered to. Perhaps the patch below would help discourage grafts more? The notable place in the documentation where grafts are still used is git-filter-branch.txt. But since the example there is about cementing rewritten history, it might be OK to leave. Thanks; I agree with the reasoning. -- 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] disable grafts during fetch/push/bundle
Jeff King p...@peff.net writes: When we are creating a pack to send to a remote, we should make sure that we are not respecting grafts or replace refs. Otherwise, we may end up sending a broken pack to the other side that does not contain all objects (either omitting them entirely, or using objects that the other side does not have as delta bases). We already make an attempt to do the right thing in several places by turning off read_replace_refs. However, we missed at least one case (during bundle creation), and we do nothing anywhere to handle grafts. Doing nothing for grafts has been pretty much a deliberate omission. Because we have no way to transfer how histories are grafted together, people cloning from a repository that grafts away a commit that records a mistakenly committed sekrit will end up with a disjoint history, instead of exposing the sekrit to them, and are expected to join the history by recreating grafts (perhaps a README of such a project instructs them to do so). That was deemed far better than exposing the hidden history, I think. And replace tries to do the right thing was an attempt to rectify that misfeature of grafts in that we now do have a way to transfer how the history is grafted together, so that project README does not have to instruct the fetcher of doing anything special. It _might_ be a misfeature, however, for the object connectivity layer to expose a part of the history replaced away to the party that fetches from such a repository. Ideally, the right thing ought to be to include history that would be omitted if we did not have the replacement (i.e. adding parents the underlying commit does not record), while not following the history that replacement wants to hide (i.e. excluding the commits replacement commits overlay). -- 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] disable grafts during fetch/push/bundle
On Tue, Mar 4, 2014 at 12:48 PM, Jeff King p...@peff.net wrote:
diff --git a/commit.c b/commit.c
index 6bf4fe0..886dbfe 100644
--- a/commit.c
+++ b/commit.c
@@ -114,6 +114,11 @@ static unsigned long parse_commit_date(const char *buf,
const char *tail)
static struct commit_graft **commit_graft;
static int commit_graft_alloc, commit_graft_nr;
+int commit_grafts_loaded(void)
+{
+ return !!commit_graft_nr;
+}
Did you mean !!commit_graft ?
static int commit_graft_pos(const unsigned char *sha1)
{
int lo, hi;
--
1.8.5.2.500.g8060133
--
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] disable grafts during fetch/push/bundle
On Tue, Mar 04, 2014 at 06:36:07PM -0500, Eric Sunshine wrote:
On Tue, Mar 4, 2014 at 12:48 PM, Jeff King p...@peff.net wrote:
diff --git a/commit.c b/commit.c
index 6bf4fe0..886dbfe 100644
--- a/commit.c
+++ b/commit.c
@@ -114,6 +114,11 @@ static unsigned long parse_commit_date(const char
*buf, const char *tail)
static struct commit_graft **commit_graft;
static int commit_graft_alloc, commit_graft_nr;
+int commit_grafts_loaded(void)
+{
+ return !!commit_graft_nr;
+}
Did you mean !!commit_graft ?
Shouldn't they produce the same results?
-Peff
--
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] disable grafts during fetch/push/bundle
On Tue, Mar 04, 2014 at 12:52:18PM -0800, Junio C Hamano wrote: We already make an attempt to do the right thing in several places by turning off read_replace_refs. However, we missed at least one case (during bundle creation), and we do nothing anywhere to handle grafts. Doing nothing for grafts has been pretty much a deliberate omission. Because we have no way to transfer how histories are grafted together, people cloning from a repository that grafts away a commit that records a mistakenly committed sekrit will end up with a disjoint history, instead of exposing the sekrit to them, and are expected to join the history by recreating grafts (perhaps a README of such a project instructs them to do so). That was deemed far better than exposing the hidden history, I think. I see your point, but I would be tempted to say that the person trying to hide a secret with grafting is simply wrong to do so. You need to cement that history with a rewrite if you want to share with people. I do not recall any past discussion on this topic, and searching the archive only shows people echoing what I said above. Is this something we've promised to work in the past? I'm certainly sympathetic to systems failing to a secure default rather than doing something that the user does not expect. But at the same time, if using grafts for security isn't something people reasonably expect, then failing only hurts the non-security cases. And replace tries to do the right thing was an attempt to rectify that misfeature of grafts in that we now do have a way to transfer how the history is grafted together, so that project README does not have to instruct the fetcher of doing anything special. Perhaps the right response is grafts are broken, use git-replace instead. But then should we think about deprecating grafts? Again, this patch was spurred by a real user with a graft trying to push and getting a confusing error message. It _might_ be a misfeature, however, for the object connectivity layer to expose a part of the history replaced away to the party that fetches from such a repository. Ideally, the right thing ought to be to include history that would be omitted if we did not have the replacement (i.e. adding parents the underlying commit does not record), while not following the history that replacement wants to hide (i.e. excluding the commits replacement commits overlay). I don't really think it's worth the complexity. It's fairly common knowledge (or at least I think so) that replace refs are a _view_ onto the history. When you share the history graph, you share the true objects. You can _also_ share your views in replace/refs, but it is up to the client to fetch them. If you want to hide things, then you need to rewrite the true objects, end of story. I dunno. Maybe there are people who have different expectations. -Peff -- 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] disable grafts during fetch/push/bundle
On Tue, Mar 4, 2014 at 7:37 PM, Jeff King p...@peff.net wrote:
On Tue, Mar 04, 2014 at 06:36:07PM -0500, Eric Sunshine wrote:
On Tue, Mar 4, 2014 at 12:48 PM, Jeff King p...@peff.net wrote:
diff --git a/commit.c b/commit.c
index 6bf4fe0..886dbfe 100644
--- a/commit.c
+++ b/commit.c
@@ -114,6 +114,11 @@ static unsigned long parse_commit_date(const char
*buf, const char *tail)
static struct commit_graft **commit_graft;
static int commit_graft_alloc, commit_graft_nr;
+int commit_grafts_loaded(void)
+{
+ return !!commit_graft_nr;
+}
Did you mean !!commit_graft ?
Shouldn't they produce the same results?
Yes they should, but the use of !! seemed to imply that you wanted to
apply it to the pointer value. (If you indeed intended to use
commit_graft_nr, then 'return commit_graft_nr', without !!, would have
been sufficient and idiomatic C.)
--
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] disable grafts during fetch/push/bundle
On Tue, Mar 04, 2014 at 08:00:44PM -0500, Eric Sunshine wrote:
+int commit_grafts_loaded(void)
+{
+ return !!commit_graft_nr;
+}
Did you mean !!commit_graft ?
Shouldn't they produce the same results?
Yes they should, but the use of !! seemed to imply that you wanted to
apply it to the pointer value. (If you indeed intended to use
commit_graft_nr, then 'return commit_graft_nr', without !!, would have
been sufficient and idiomatic C.)
I just wanted to normalize the return value to a boolean 0/1. Even when
the input is an int, it eliminates surprises when you try to assign the
result to a bitfield or other smaller-width type.
-Peff
--
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] disable grafts during fetch/push/bundle
On Tue, Mar 4, 2014 at 8:05 PM, Jeff King p...@peff.net wrote:
On Tue, Mar 04, 2014 at 08:00:44PM -0500, Eric Sunshine wrote:
+int commit_grafts_loaded(void)
+{
+ return !!commit_graft_nr;
+}
Did you mean !!commit_graft ?
Shouldn't they produce the same results?
Yes they should, but the use of !! seemed to imply that you wanted to
apply it to the pointer value. (If you indeed intended to use
commit_graft_nr, then 'return commit_graft_nr', without !!, would have
been sufficient and idiomatic C.)
I just wanted to normalize the return value to a boolean 0/1. Even when
the input is an int, it eliminates surprises when you try to assign the
result to a bitfield or other smaller-width type.
Thanks for the explanation.
--
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
