Re: [PATCH v3] git-apply: apply submodule changes
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 12, 2007, 18:16 UTC
- Message-ID
- <7vwsw0ipp2.fsf@assigned-by-dhcp.cox.net>
- In-Reply-To
- <20070812142340.GA10399MdfPADPa@greensroom.kotnet.org>
Sven Verdoolaege <skimo@kotnet.org> writes:
Show 12 quoted lines
> diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt > index f03f661..804fdc3 100644 > --- a/Documentation/git-apply.txt > +++ b/Documentation/git-apply.txt > @@ -171,6 +171,20 @@ apply.whitespace:: > When no `--whitespace` flag is given from the command > line, this configuration item is used as the default. > > +Submodules > +---------- > +If the patch contains any changes to submodules then gitlink:git-apply[1] > +behaves as follows.
perhaps "as follows wrt the submodules"...
Show 27 quoted lines
> diff --git a/builtin-apply.c b/builtin-apply.c
> index da27075..eef596b 100644
> --- a/builtin-apply.c
> +++ b/builtin-apply.c
> @@ -1984,6 +1984,40 @@ static int apply_fragments(struct buffer_desc *desc, struct patch *patch)
> return 0;
> }
>
> +static int read_file_or_gitlink(struct cache_entry *ce, char **buf_p,
> + unsigned long *size_p)
> +{
> + if (!ce)
> + return 0;
> +
> + if (S_ISGITLINK(ntohl(ce->ce_mode))) {
> + *buf_p = xmalloc(100);
> + *size_p = snprintf(*buf_p, 100,
> + "Subproject commit %s\n", sha1_to_hex(ce->sha1));
> + } else {
> + enum object_type type;
> + *buf_p = read_sha1_file(ce->sha1, &type, size_p);
> + if (!*buf_p)
> + return -1;
> + }
> +
> + return 0;
> +}Ok, read_file_or_gitlink() expects ce taken from the current index and fills *buf_p with the preimage to be patched from it.
Show 13 quoted lines
> +static int read_gitlink_or_skip(struct patch *patch, struct cache_entry *ce,
> + char *buf, unsigned long alloc)
> +{
> + if (ce)
> + return snprintf(buf, alloc,
> + "Subproject commit %s\n", sha1_to_hex(ce->sha1));
> +
> + /* We can't apply the submodule change without an index, so just
> + * skip the patch itself and only create/remove directory.
> + */
> + patch->fragments = NULL;
> + return 0;
> +}Hmmmm... see below.
Show 11 quoted lines
> static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *ce)
> {
> char *buf;
> @@ -1994,20 +2028,17 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *
> alloc = 0;
> buf = NULL;
> if (cached) {
> + if (read_file_or_gitlink(ce, &buf, &size))
> + return error("read of %s failed", patch->old_name);
> + alloc = size;
> }This part is consistent with the read_file_or_gitlink() semantics above...
Show 10 quoted lines
> 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...".
- what is alloc at this point? it is based on the size of directory st->st_size.
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.
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.