Hi,
On Fri, 18 Jul 2008, Alexander Gavrilov wrote:
> On Friday 18 July 2008 21:36:26 Johannes Schindelin wrote:
> > > What I'm unsure of is, should fast-export try to reuse commit
> > > marks for gitlinks where it happened to recognize the object, or
> > > always output the SHA as it is stored in the tree?
>
> > Are they commit marks? No. So they should be handled as marks, just
> > as those for blobs and trees.
> >
> > (They are commit marks in the _submodule_, but that does not matter
> > here.)
>
> Well, I was thinking of that unlikely case when the master module and
> the submodule are in the same repository and are exported together. But
> probably it is best to just output the SHA after all.
Exactly.
What you describe is a _possibility_. Requiring it would be a serious
mistake.
> > > for (i = 0; i < diff_queued_diff.nr; i++)
> > > - handle_object(diff_queued_diff.queue[i]->two->sha1);
> > > + if (!S_ISGITLINK(diff_queued_diff.queue[i]->two->mode))
> > > + handle_object(diff_queued_diff.queue[i]->two->sha1);
> >
> > Why? You do not want to export changes in the submodules?
>
> handle_object opens the sha as a blob and outputs it. As gitlinks aren't
> blobs, it won't work. And if the submodule is in a separate repository,
> there is nothing to open anyway.
So what should happen to them instead? Ignoring them?
Possible.
My earlier remark about the marks was this: you might want to mark SHA-1s
of gitlinks with a (shorter) number, but maybe that is just bullocks. At
the same time, it might be not.
> Simultaneously reading commits from the submodule repository is a whole
> different level of complexity. I'm afraid I'm not up to it yet.
Good news for you: I think it would be a serious mistake to read commits
from the submodule.
> > > else {
> > > struct object *object = lookup_object(spec->sha1);
> > > - printf("M %06o :%d %s\n", spec->mode,
> > > - get_object_mark(object), spec->path);
> > > + int mark = object ? get_object_mark(object) : 0;
> >
> > As I said, that looks wrong. Maybe we have to fake objects for the
> > gitlinks.
>
> I tried to avoid faking blobs and stick to the interface of the M
> command itself.
My point was: I do not see gitlinks handled _at all_.
I agree that they should not be handled the same as blobs, but i did not
have time to check that gitlinks are not just ignored by your patch.
Which would be wrong: you want the exported commits to contain them.
> > > @@ -1900,7 +1901,16 @@ static void file_change_m(struct branch *b)
> > > p = uq.buf;
> > > }
> > >
> > > - if (inline_data) {
> > > + if (S_ISGITLINK(mode)) {
> > > + if (inline_data)
> > > + die("Git links cannot be specified 'inline': %s",
> > > + command_buf.buf);
> > > + else if (oe) {
> > > + if (oe->type != OBJ_COMMIT)
> > > + die("Not a commit (actually a %s): %s",
> > > + typename(oe->type), command_buf.buf);
> >
> > How is that supposed to work? Do I understand correctly that you
> > require the user to construct a commit object for the gitlink? That
> > would be actively wrong.
>
> There are three forms of the M command:
>
> M mode inline some/path
> ...some data...
>
> M mode :mark some/path
>
> M mode SHA some/path
>
> For usual files the mark must be created by the 'blob' command,
> and the SHA must refer to an existing blob. This hunk makes fast-import
> require for gitlinks a commit mark instead, and accept the SHA without
> checking (as it is expected to be in another repository).
What's this commit mark thing? I hope you do not mean to ask the user to
construct a commit object in the _superproject's_ repository...
> > Oh, and your patch lacks test cases that demonstrate how you envisage
> > the whole thing to work.
>
> Ok, I'll make some tests tomorrow.
Maybe I should enhance on my point, to drive it home:
If you do _not_ include automated tests, other people have less reason to
trust that your patch does what you insist it does.
If you do not include tests, and have a sparse commit message (which you
do), people are left to guess what your patch tries to do, and do not even
have the chance to see something you wanted to work.
If you do not include tests, and your patch does something a reviewer
feels it should not to, or omits something a reviewer feels it _should_
do, there is less of an opportunity to see if this was intended: a
comprehensive test script would show what the submitter expects to
work alright.
So in a very real sense, it is advisable to write the test _first_, and
then the patch.
Not everything XP brought to this world is evil. (Oh yes, you guessed it,
I was talking about eXtreme Programming...)
Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
