Re: [PATCH v4] git-apply: apply submodule changes
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 13, 2007, 20:26 UTC
- Message-ID
- <7vmywvfag3.fsf@assigned-by-dhcp.cox.net>
- In-Reply-To
- <20070813171349.GL999MdfPADPa@greensroom.kotnet.org>
Sven Verdoolaege <skimo@kotnet.org> writes:
Show 31 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;
> +}
> @@ -1994,20 +2029,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;
> ...
> 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);
> }I think the logic here sounds sane. The read_file_or_gitlink() abstraction in the first if() part was so nice that I was hoping we could do something similar in this "else if" part, without hiding the assignment to patch->fragments as an obscure side effect of calling read_gitlink_or_skip(). That assignment is a gitlink specific hack so it might be easier to read if this part is written like:
} else if (patch->old_name && S_ISGITLINK(patch->old_mode)) {
if (ce)
size = snprintf(....)
else {
/* we cannot apply gitlink mods without index */
size = 0;
patch->fragments = NULL;
}
} else if (patch->old_name) {
... original code here ...Show 17 quoted lines
> @@ -2055,6 +2087,20 @@ static int check_to_create_blob(const char *new_name, int ok_if_exists)
> return 0;
> }
>
> +/* Check that the directory corresponding to a gitlink is either
> + * empty or a git repo.
> + */
> +static int verify_gitlink_clean(const char *path)
> +{
> + unsigned char sha1[20];
> +
> + if (!rmdir(path)) {
> + mkdir(path, 0777);
> + return 0;
> + }
> + return resolve_gitlink_ref(path, "HEAD", sha1);
> +}Is it the responsibility of the caller of this function to make sure that path is either absent or is a directory? Can there be a regular file at path, which would cause rmdir to fail? I do not think it is sensible to call resolve_gitlink_ref() on such a path, so let's say the caller will make sure that path is gitlink in the current (i.e. in index) tree before calling this function.
Show 10 quoted lines
> static int check_patch(struct patch *patch, struct patch *prev_patch)
> {
> struct stat st;
> @@ -2096,8 +2142,15 @@ static int check_patch(struct patch *patch, struct patch *prev_patch)
> lstat(old_name, &st))
> return -1;
> }
> - if (!cached)
> + if (!cached) {
> changed = ce_match_stat(ce, &st, 1);Here, ce_match_stat() sees if the index and work tree match. You could have a regular file there but you haven't checked (which is not a crime, yet).
> + if (S_ISGITLINK(patch->old_mode)) {I think the rmdir/mkdir sequence should be done only when ce is a gitlink. Perhaps it is just the matter of:
if (S_ISGITLINK(patch->old_mode) &&
S_ISGITLINK(ntohl(ce->ce_mode))) {
...
}Show 6 quoted lines
> + changed &= TYPE_CHANGED; > + if (!changed && > + verify_gitlink_clean(patch->old_name)) > + changed |= TYPE_CHANGED; > + } > + }
This part is very confusing. You discard all changes other than TYPE_CHANGED, and give TYPE_CHANGED and nothing else if gitlink is not clean. I suspect "changed &= ~TYPE_CHANGED" might be what you meant, but I do not know what you are trying to do here.