Re: [PATCH 16/18] entry.c: update submodules when interesting
Stefan Beller writes: >> In addition to mkdir(), would we also need the .git file that point >> into superproject's .git/modules/ too? > > The move_head function takes care of it > Both creation as well as deletion are handled in the move_head function, > when either new or old is NULL. Oh, if it does the creation of directory and adjusting of .git gitfile, and handles other anomalies, I have no problem with it. It was just that this codepath tried to handle some kind of anomaly (namely, the user originally thought the submodule needs to be changed to a regular file and then changed mind and wants to restore it from the index) and yet not doing the full anomaly handling (like "not even an empty directory exists" case) that confused me.
Re: [PATCH 16/18] entry.c: update submodules when interesting
On Tue, Mar 7, 2017 at 3:04 PM, Junio C Hamano wrote:
> Junio C Hamano writes:
>
>>> +if (!is_submodule_populated_gently(ce->name, &err)) {
>>> +struct stat sb;
>>> +if (lstat(ce->name, &sb))
>>> +die(_("could not stat file '%s'"),
>>> ce->name);
>>> +if (!(st.st_mode & S_IFDIR))
>>> +unlink_or_warn(ce->name);
>>
>> We found that the path ce->name is supposed to be a submodule that
>> is checked out, we found that the submodule is not yet populated, we
>> tried to make sure what is on that path, and its failure would cause
>> us to die(). All that is sensible.
>> ...
>> But if that unlink fails, shouldn't we die, just like the case where
>> we cannot tell what is at the path ce->name?
>>
>> And if that unlink succeeds, instead of having an empty directory,
>> we start the "move-head" call to switch from NULL to ce->oid without
>> having any directory. Wouldn't we want to mkdir() here (and remove
>> mkdir() in submodule_move_head() if there is one---if there isn't
>> then I do not think this codepath would work)?
>
> In addition to mkdir(), would we also need the .git file that point
> into superproject's .git/modules/ too?
The move_head function takes care of it
Both creation as well as deletion are handled in the move_head function,
when either new or old is NULL.
We can drag that out of that function and have it at the appropriate places
manually.
Why do you think it is better design to have mkdir here and not in move_head?
(For me having everything in move_head was easier to understand,
maybe it is not for others)
Thanks,
Stefan
Re: [PATCH 16/18] entry.c: update submodules when interesting
Junio C Hamano writes:
>> +if (!is_submodule_populated_gently(ce->name, &err)) {
>> +struct stat sb;
>> +if (lstat(ce->name, &sb))
>> +die(_("could not stat file '%s'"),
>> ce->name);
>> +if (!(st.st_mode & S_IFDIR))
>> +unlink_or_warn(ce->name);
>
> We found that the path ce->name is supposed to be a submodule that
> is checked out, we found that the submodule is not yet populated, we
> tried to make sure what is on that path, and its failure would cause
> us to die(). All that is sensible.
> ...
> But if that unlink fails, shouldn't we die, just like the case where
> we cannot tell what is at the path ce->name?
>
> And if that unlink succeeds, instead of having an empty directory,
> we start the "move-head" call to switch from NULL to ce->oid without
> having any directory. Wouldn't we want to mkdir() here (and remove
> mkdir() in submodule_move_head() if there is one---if there isn't
> then I do not think this codepath would work)?
In addition to mkdir(), would we also need the .git file that point
into superproject's .git/modules/ too?
Re: [PATCH 16/18] entry.c: update submodules when interesting
Stefan Beller writes:
> Signed-off-by: Stefan Beller
> ---
> entry.c | 30 ++
> 1 file changed, 30 insertions(+)
>
> diff --git a/entry.c b/entry.c
> index c6eea240b6..d2b512da90 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -2,6 +2,7 @@
> #include "blob.h"
> #include "dir.h"
> #include "streaming.h"
> +#include "submodule.h"
>
> static void create_directories(const char *path, int path_len,
> const struct checkout *state)
> @@ -146,6 +147,7 @@ static int write_entry(struct cache_entry *ce,
> unsigned long size;
> size_t wrote, newsize = 0;
> struct stat st;
> + const struct submodule *sub;
>
> if (ce_mode_s_ifmt == S_IFREG) {
> struct stream_filter *filter = get_stream_filter(ce->name,
> @@ -203,6 +205,10 @@ static int write_entry(struct cache_entry *ce,
> return error("cannot create temporary submodule %s",
> path);
> if (mkdir(path, 0777) < 0)
> return error("cannot create submodule directory %s",
> path);
> + sub = submodule_from_ce(ce);
> + if (sub)
> + return submodule_move_head(ce->name,
> + NULL, oid_to_hex(&ce->oid),
> SUBMODULE_MOVE_HEAD_FORCE);
> break;
> default:
> return error("unknown file mode for %s in index", path);
> @@ -259,7 +265,31 @@ int checkout_entry(struct cache_entry *ce,
> strbuf_add(&path, ce->name, ce_namelen(ce));
>
> if (!check_path(path.buf, path.len, &st, state->base_dir_len)) {
> + const struct submodule *sub;
> unsigned changed = ce_match_stat(ce, &st,
> CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE);
> + /*
> + * Needs to be checked before !changed returns early,
> + * as the possibly empty directory was not changed
> + */
> + sub = submodule_from_ce(ce);
> + if (sub) {
> + int err;
> + if (!is_submodule_populated_gently(ce->name, &err)) {
> + struct stat sb;
> + if (lstat(ce->name, &sb))
> + die(_("could not stat file '%s'"),
> ce->name);
> + if (!(st.st_mode & S_IFDIR))
> + unlink_or_warn(ce->name);
We found that the path ce->name is supposed to be a submodule that
is checked out, we found that the submodule is not yet populated, we
tried to make sure what is on that path, and its failure would cause
us to die(). All that is sensible.
Then we want to make sure the filesystem entity at the path
currently is a directory and otherwise unlink (i.e. we may have an
unrelated file that is not tracked there, or perhaps the user earlier
decided that replacing the submodule with a single file is a good
idea, but then getting rid of the change with "checkout -f" before
doing "git add" on that file). That is also sensible.
But if that unlink fails, shouldn't we die, just like the case where
we cannot tell what is at the path ce->name?
And if that unlink succeeds, instead of having an empty directory,
we start the "move-head" call to switch from NULL to ce->oid without
having any directory. Wouldn't we want to mkdir() here (and remove
mkdir() in submodule_move_head() if there is one---if there isn't
then I do not think this codepath would work)?
If we had a directory here, but if that directory is not empty,
should we proceed? I am assuming (I haven't carefully read
"move-head") that it is OK because the "run an equivalent of
'checkout --detach ce->oid'" done by "move-head" would notice a
possible information loss to overwrite an untracked path (everything
is "untracked" as the head moves from NULL to ce->oid in this case),
and prevent it from happening.
Am I reading the design correctly?
> + return submodule_move_head(ce->name,
> + NULL, oid_to_hex(&ce->oid),
> + SUBMODULE_MOVE_HEAD_FORCE);
> + } else
> + return submodule_move_head(ce->name,
> + "HEAD", oid_to_hex(&ce->oid),
> + SUBMODULE_MOVE_HEAD_FORCE);
> + }
> +
> if (!changed)
> return 0;
> if (!state->force) {
