On Sun, Aug 12, 2007 at 11:16:09AM -0700, Junio C Hamano wrote:
> Sven Verdoolaege <skimo@xxxxxxxxxx> writes:
> > else if (patch->old_name) {
> > size = xsize_t(st->st_size);
> > alloc = size + 8192;
> > buf = xmalloc(alloc);
> > - if (read_old_data(st, patch->old_name, &buf, &alloc, &size))
> > + if (S_ISGITLINK(patch->old_mode))
> > + size = read_gitlink_or_skip(patch, ce, buf, alloc);
> > + else if (read_old_data(st, patch->old_name, &buf, &alloc, &size))
> > return error("read of %s failed", patch->old_name);
> > }
>
> read_old_data() gets the lstat information from the current
> filesystem data at old_name, and gives the preimage to be
> patched, and naturally it bombs out if it is a directory, but
> when we are applying a change to gitlink, the patch expects
> old_name to be a directory.
>
> So you introduced read_gitlink_or_skip() to work it around. But
> this makes me wonder...
>
> - what does ce have to do in this codepath? read_old_data()
> does not care about what is in the index (in fact, in the
> index the entry can be a symlink when the path on the
> filesystem is a regular file, and it reads from the regular
> file as asked--it does not even look at ce by design).
> if you have a regular file there in the current version, ce
> would say it is a regular file blob and you would not want
> read_gitlink_or_skip() to say "Subproject commit xyz...".
Hmmm... the documentation says that if --index is in effect
then the file to be patched in the work tree is supposed to be
up-to-date. Then for files it shouldn't matter if the data
comes from the index or not. For submodules it's crucial to
look at the index (if it is available), since that's the
only place you can find the current state of the submodule.
(Remember that git currently doesn't automatically update submodules.)
If I specify --index (e.g., through git rebase), then I really
wouldn't want the patch to apply if the old submodule commit
in the patch doesn't match the submodule commit in the index.
Would you?
> - what is alloc at this point? it is based on the size of
> directory st->st_size.
I assume you mean "size". For submodules, the value isn't used,
except to test that the "file" is empty (size==0) on delete.
So it seems safe to set it to zero for submodules in any case.
> I think dropping fragments for a patch that tries to modify a
> gitlink here is fine, but that can be done regardless of what ce
> is.
As explained above, I don't think it would a good idea to
just drop gitlink patches if --index is specified.
> The type-mismatch case to attempt to apply gitlink patch to a
> regular blob is covered much earlier in check_patch(). It
> complains if st_mode does not match patch->old_mode; I think you
> need to adjust it a bit to:
>
> - allow gitlink patch to a path that currently has nothing (no
> submodule checked out) or a directory that has ".git/"
> (i.e. submodule checked out).
>
> - reject gitlink patch otherwise.
Are you talking about the case where --index is specified?
Otherwise, I don't think we can make any assumptions on
what's inside the subdirectory.
skimo
-
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
