Sven Verdoolaege <skimo@xxxxxxxxxx> writes:
> +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 ...
> @@ -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.
> 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))) {
...
}
> + 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.
-
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
