Yann Dirson [off-list ref] writes:
On Fri, Nov 07, 2008 at 12:19:14PM -0800, Junio C Hamano wrote:
quoted
I am afraid that this is totally unacceptable, as you yourself mentioned,
the end result is unapplicable with any existing tool and would confuse
viewers like gitk and gitweb.
Well, other tools will still have to be taught about a new syntax, if
they're going to use the new flag - just like it was for --rename.
You are mistaken. For a patch, you are dealing with two different
parties: producer and consumer. If you are adding new feature to the
producer, the output format should be desigend to allow the consumer tell
that it is something it does not know how to handle.
Marking a non patch with "diff --git" to trigger the logic to signal the
beginning of a patch to git-apply (and perhaps other tools) is a non
starter.
And for this "we are giving a patch that your git-apply can apply and gitk
can show, but by the way we also think the whole directory foo moved to
new location bar" is merely an additional information. You should still
be able to apply the patch with tools that are unaware of this new
directory move detection feature.
On Fri, Nov 07, 2008 at 01:11:40PM -0800, Junio C Hamano wrote:
Yann Dirson [off-list ref] writes:
quoted
On Fri, Nov 07, 2008 at 12:19:14PM -0800, Junio C Hamano wrote:
quoted
I am afraid that this is totally unacceptable, as you yourself mentioned,
the end result is unapplicable with any existing tool and would confuse
viewers like gitk and gitweb.
Well, other tools will still have to be taught about a new syntax, if
they're going to use the new flag - just like it was for --rename.
You are mistaken. For a patch, you are dealing with two different
parties: producer and consumer. If you are adding new feature to the
producer, the output format should be desigend to allow the consumer tell
that it is something it does not know how to handle.
Agreed.
Marking a non patch with "diff --git" to trigger the logic to signal the
beginning of a patch to git-apply (and perhaps other tools) is a non
starter.
This is not what I meant.
And for this "we are giving a patch that your git-apply can apply and gitk
can show, but by the way we also think the whole directory foo moved to
new location bar" is merely an additional information. You should still
be able to apply the patch with tools that are unaware of this new
directory move detection feature.
I hope I just miss your point. Letting unaware tools handle such a
patch "the right way" would imply just adding the information "dir foo
moved to bar", and not removing the individual file moves, which goes
in the way of the exact reason why I have started to work on this.
Compare this to the addition of the "file rename" feature (correct me
if I'm wrong): it was added without bothering whether plain old
"patch" can deal with it, and when feeding a git diff to patch(1) I
cannot expect it to DTRT (indeed it applies the content change but not
the rename part, without complaining, and an unsuspecting user would
just be shot in the foot): we are precisely *not* able to apply the
patch with tools that are unaware of this new file rename feature.
This is precisely to limit this problem in the future that I proposed
this "diff extension" stuff in my last mail: limit how the "backward
compatibility" argument can cripple innovation.
Yann Dirson [off-list ref] writes:
I hope I just miss your point. Letting unaware tools handle such a
patch "the right way" would imply just adding the information "dir foo
moved to bar", and not removing the individual file moves, which goes
in the way of the exact reason why I have started to work on this.
If your change is to move a/{1,2,3} to b/{1,2,3} and without content
change to a/{1,2} to b/{1,2}, then do you want to say "a/ moved to b/
and by the way here is the content change from a/3 to b/3" without saying
anything about a/{1,2} and b/{1,2}?
Two points.
* I do not think it is a good idea to begin with. If you are to apply
such a patch (with git-apply that is updated with your patch to
understand that notation) to the exact tree that has only {1,2,3} under
a/, you would get an expected result. But if the recipient of your
patch has a/4 (or lacks a/2), there is no cue in the patch that
automatically moving a/4 to b/4 may or may not be what is sane (or the
patch is unapplicable in general).
* If you give at least the names of paths that were moved without any
content changes as I suggested, at least the recipient of your patch
can catch the case where his tree is structurally different from what
you used to prepare the patch for by noticing the a/2 in the patch that
he does not have.
In addition, if you keep the movements for the paths whose contents did
not change, existing tools are perfectly capable of applying (or showing)
the output. I seriously doubt that keeping 4 lines per perfectly moved
paths is too much a price to pay to keep backward compatibility.
Compare this to the addition of the "file rename" feature (correct me
if I'm wrong): it was added without bothering whether plain old
"patch" can deal with it,...
Sorry, but that's an old history whie git-diff output format was rapidly
being developed, when we did not have that many users, and when we did not
have an old version of git-apply that did not understand the new feature
in majority of user's hands.
We do not have that kind of luxury anymore. git is much more widespread
now and the majority of people use pre-1.6.1 git now (including me ;-)).
On Fri, Nov 07, 2008 at 03:43:19PM -0800, Junio C Hamano wrote:
Yann Dirson [off-list ref] writes:
quoted
I hope I just miss your point. Letting unaware tools handle such a
patch "the right way" would imply just adding the information "dir foo
moved to bar", and not removing the individual file moves, which goes
in the way of the exact reason why I have started to work on this.
If your change is to move a/{1,2,3} to b/{1,2,3} and without content
change to a/{1,2} to b/{1,2}, then do you want to say "a/ moved to b/
and by the way here is the content change from a/3 to b/3" without saying
anything about a/{1,2} and b/{1,2}?
Two points.
* I do not think it is a good idea to begin with. If you are to apply
such a patch (with git-apply that is updated with your patch to
understand that notation) to the exact tree that has only {1,2,3} under
a/, you would get an expected result. But if the recipient of your
patch has a/4 (or lacks a/2), there is no cue in the patch that
automatically moving a/4 to b/4 may or may not be what is sane (or the
patch is unapplicable in general).
Sure in theory. But in practice I do not remember one time when, if
all files from one dir are moved in one branch, the files added on
another in the same dir were not bound to be moved as well.
Anway, if we feel git-apply should not decide without the user
knowing, we can make it refuse by default, with options to do either
way, and one option to ask for each doubtful file instead.
* If you give at least the names of paths that were moved without any
content changes as I suggested, at least the recipient of your patch
can catch the case where his tree is structurally different from what
you used to prepare the patch for by noticing the a/2 in the patch that
he does not have.
Right.
In addition, if you keep the movements for the paths whose contents did
not change, existing tools are perfectly capable of applying (or showing)
the output. I seriously doubt that keeping 4 lines per perfectly moved
paths is too much a price to pay to keep backward compatibility.
OK, so I realize we need 2 things here: one format for diff-exporting
with complete info, and one for human viewing (which is, again, the
primary reason why I needed this feature, so I'm not very keen on
letting all this work finally not being useful for me :). Commands
for saving/mailing patches could issue a bold warning if the user
specifies the for-human-viewing flag.
quoted
Compare this to the addition of the "file rename" feature (correct me
if I'm wrong): it was added without bothering whether plain old
"patch" can deal with it,...
Sorry, but that's an old history whie git-diff output format was rapidly
being developed, when we did not have that many users, and when we did not
have an old version of git-apply that did not understand the new feature
in majority of user's hands.
We do not have that kind of luxury anymore. git is much more widespread
now and the majority of people use pre-1.6.1 git now (including me ;-)).
I was talking about exchanging patches with the non-git part of the
world. The point is that eg. GNU patch still happily accepts
git-generated files but produces nonsense using some, exactly because
it ignores meaningful data which (by design ?) appear to it to be
legal to ignore.