From: Junio C Hamano <hidden> Date: 2016-06-15 22:48:00
Jens Lehmann [off-list ref] writes:
Until now a submodule only showed up as changed in the supermodule when
the last commit in the submodule differed from the one in the index or
the last commit of the superproject. A dirty working tree or a detached
HEAD in a submodule were just ignored when looking at it from the
superproject.
This patch shows these changes when using git status or one of the diff
commands which compare against the working tree in the superproject.
Signed-off-by: Jens Lehmann <redacted>
---
This is the first version of a patch letting git status and the git
diff family show dirty working directories and a detached HEAD in
directories. It is not intended to be merged in its present form but
to be used as a starting point for discussion if this is going in
the right direction.
What the patch does:
* It makes git show submodules as modified in the superproject when
one or more of these conditions are met:
a) The submodule contains untracked files
b) The submodule contains modified files
c) The submodules HEAD is not on a local or remote branch
That can be seen when using either "git status", "git diff[-files]"
& "git diff[-index] HEAD" (and with "git gui" & gitk).
If the submodule is checked out, _and_ if the HEAD there, either detached
or not, does not agree with what the "other" one records (i.e. the commit
recorded in an entry in the index, or in the tree, that you are comparing
your work tree against), then it also should be considered modified. I
don't think your (a)-(c) cover this case.
Also I don't understand why you want to treat (c) any specially at all.
Even if (c) is something we _should_ report, please do not call that as
"detached" in its implementation. "detached HEAD" has a very precise
technical meaning, and can point at the same commit as a local or a remote
tracking branch, which is very different from the definition your
implementation seems to use.
* This behavior is not configurable but activated by default. A config
option is needed here.
I doubt it.
My gut feeling is that this should be _always_ on for a submodule
directory that has been "submodule init/update". The user is interested
in that particular submodule, and any change to it should be reported for
both classes of users. Theose who meant to use the submodule read-only
need to be able to notice that they accidentally made the submodule dirty
before making a commit in the superproject. Those who wanted to work in
submodule needs to know if the state is in sync with what they expect
before making a commit in the superproject.
That of course is provided if the unconditional check does not trigger for
submodules that the user hasn't "submodue init"ed; I think you did that
correctly at the beginning of your is_submodule_modified() implementation.
+static int is_submodule_head_detached(const char *path)
+{
I don't understand why you should care which branch the submodule happens
to be on, as long as the next commit you make in the superproject records
the commit that is checked out in the submodule.
Of course you may want to be careful when "pushing" the superproject
results out (i.e. you would want to push out the history leading to that
commit at the submodule HEAD in the submodule history), so that the people
who are pulling from the repository you are pushing into will have
everything available.
But the thing is, in a distributed environment, the submodule HEAD being
at the tip of _some_ branch (either local or remote) you have doesn't mean
anything to help them. IOW, for protect others, you would need a check
when you _push out_ (either in 'push' or on the receiving end).
So I'd suggest dropping this condition in "status/diff" that is about
preparing to make the next commit in your _local_ history.
If "must be reachable from somewhere" is a condition worth caring about in
some context other than "status/diff", you can do an equivalent of:
$ git rev-parse HEAD --not --all | git rev-list --stdin
and see if anything comes out (in which case you have commits that are not
reachable from any of your refs other than the detached HEAD).
But that is not "is HEAD detached?"; it is something else. "Dangling",
perhaps, as that is how "git fsck" call commits that are not reachable
from any of your refs ("fsck" considers HEAD a part of refs, so it is not
strictly correct but it is much closer).
* It makes git show submodules as modified in the superproject when
one or more of these conditions are met:
a) The submodule contains untracked files
b) The submodule contains modified files
c) The submodules HEAD is not on a local or remote branch
That can be seen when using either "git status", "git diff[-files]"
& "git diff[-index] HEAD" (and with "git gui" & gitk).
If the submodule is checked out, _and_ if the HEAD there, either detached
or not, does not agree with what the "other" one records (i.e. the commit
recorded in an entry in the index, or in the tree, that you are comparing
your work tree against), then it also should be considered modified. I
don't think your (a)-(c) cover this case.
Right, i did not to add the current (and unchanged) behavior to this
list, i just wrote down the new cases (and these new cases only come
into play when the submodule has been checked out).
Also I don't understand why you want to treat (c) any specially at all.
To avoid possible loss of commits.
Before doing something like "git checkout -f" or "git reset --hard", it
is a good idea to check via "git status" if you have local changes. I
hope checkout and reset will recurse into submodules in the near future.
when they do, all commits in the submodule which are not on any branch
are lost (at least when the reflog expired). Or the remote branch the
user thinks the submodule is tracking has been deleted or rebased. You
might want to know that before e.g. committing it in the superproject.
Maybe compare it to new or modified files in a git repo: They don't
necessarily pose a problem when committing, you might be able to push
and clone the repo somewhere else and nothing breaks. But you wanna
know about these new and modifies files, in case you just forgot to add
them. So i think the HEAD of a submodule not on any branch is a bit like
a new or modified file in a regular repo, both will not show up in a
different repo than yours unless you do something about it. And a
modification is lost by a checkout or reset just as the dangling commits
will be.
Yes, this test can't provide 100% safety against loss of commits, but at
least we should try to warn if we can detect it. Does it give false
positives (saying the submodules HEAD is dangling when it shouldn't)?
I doubt it. Does it give false negatives? Yes, but we can't do anything
about that due to the distributed nature of git.
Even if (c) is something we _should_ report, please do not call that as
"detached" in its implementation.
Correct, that term is misleading in this context. Maybe call it
something like "The submodule contains a HEAD not on any branch"
then? Or "The submodule has a dangling HEAD"?
quoted
* This behavior is not configurable but activated by default. A config
option is needed here.
I doubt it.
My gut feeling is that this should be _always_ on for a submodule
directory that has been "submodule init/update". The user is interested
in that particular submodule, and any change to it should be reported for
both classes of users. Theose who meant to use the submodule read-only
need to be able to notice that they accidentally made the submodule dirty
before making a commit in the superproject. Those who wanted to work in
submodule needs to know if the state is in sync with what they expect
before making a commit in the superproject.
Yes, me too thinks it should default to on for every initialized
submodule.
But this is a major change in behavior, so it might be a good idea to be
able to turn it off (e.g. if it breaks scripts). Maybe a config option
really isn't such a bright idea, but what about having something like a
"--no-dirty-submodules" command line option?
That of course is provided if the unconditional check does not trigger for
submodules that the user hasn't "submodue init"ed; I think you did that
correctly at the beginning of your is_submodule_modified() implementation.
Yes, that's what that test is for. Will add a comment there.
But the thing is, in a distributed environment, the submodule HEAD being
at the tip of _some_ branch (either local or remote) you have doesn't mean
anything to help them. IOW, for protect others, you would need a check
when you _push out_ (either in 'push' or on the receiving end).
This is something on my TODO list: Add a change to "git push" to assert
that all HEADs of initialized submodules lie on a /remote/ branch before
doing the push in the superproject.
So I'd suggest dropping this condition in "status/diff" that is about
preparing to make the next commit in your _local_ history.
I would rather have this patch merged without c) than not at all. But i
think it is a worthwhile and rather cheap test. And i would prefer to
change the default behavior of "git status" only once now and not again
later.
From: Junio C Hamano <hidden> Date: 2016-06-15 22:48:00
Jens Lehmann [off-list ref] writes:
To avoid possible loss of commits.
... Or the remote branch the
user thinks the submodule is tracking has been deleted or rebased. You
might want to know that before e.g. committing it in the superproject.
The interaction between recursive reset/checkout and gc in submodule
repository is something I didn't think about thoroughly, but I think you
are solving the issue in a wrong place at a wrong level.
We have so far neglected the object reachability analysis across
repositories. When people use "clone --reference" to take advantage of
alternate object store, we tell them not to use any repository that
rewinds its branches as reference. Putting it the other way around, we
tell owners of repositories that somebody borrows objects from not to run
gc, because objects they themselves do not use may be used by others who
borrow from them. But we don't _enforce_ it; it is merely a social
convention. We might want to do something about it, perhaps by having
typed back-pointers from a borrowed repository to borrowing repositories.
I think commits in submodules that are pointed at by _some_ superproject
commits are in the same situation. Just like some objects in a borrowed
repository may have to be protected by refs in borrowing repositories,
some commits in a submodule repository (here I am only talking about
repositories that serve as distribution points---a private repository of a
user who is not interested in that particular submodule is excluded from
this discussion and may not follow the same rules) may have to be
protected by commits reachable by refs in its superproject repository.
The right solution to your "gc might soon remove objects from submodule
repository even when commits in the superproject uses them" may be to
teach "gc" run in the submodule repository to behave better. After all,
even if the commit were reachable from some of the refs in the submodule
when you created a commit that points at it at the superproject level,
next "git submodule update" might rewind that ref in the submodule that
reached the commit and make it unreachable; your check to be careful at
commit time wouldn't buy you much, even if the check were 100% accurate.
Also in _your_ repository you may have the submodule commit with your
check at the time you make commit in the superproject, but if you forget
to push it out from the submodule before you push the commit out from the
superproject, other people will suffer from the same issue you are trying
to guard against.
In any case, this "where in the submodule's history does the commit being
recorded lie" does not have anything to do with "diff/status" that lets
you know "has it been _modified_?" at the level of the superproject. As
you mentioned, it is more similar to "Untracked files" section in "status"
output of a flat project. It doesn't belong to "diff", nor to "Changed
but not updated" section.
quoted
quoted
* This behavior is not configurable but activated by default. A config
option is needed here.
I doubt it.
My gut feeling is that this should be _always_ on for a submodule
directory that has been "submodule init/update". The user is interested
in that particular submodule, and any change to it should be reported for
both classes of users. Those who meant to use the submodule read-only
need to be able to notice that they accidentally made the submodule dirty
before making a commit in the superproject. Those who wanted to work in
submodule needs to know if the state is in sync with what they expect
before making a commit in the superproject.
Yes, me too thinks it should default to on for every initialized
submodule.
But this is a major change in behavior, so it might be a good idea to be
able to turn it off (e.g. if it breaks scripts).
If scripts are broken by this change, I think it is actually a good thing.
They've been operating happily as if everything is clean when some
submodule directories are *not*, and your patch starts showing something
closer to the reality, the changes they should care about. If on the
other hand the scripts are willing to commit the index while leaving local
modifications behind, they will not be checking with "diff" output
(instead they will be checking "diff --cached") and you won't change the
output with your patch for that codepath, so they will keep working
happily.
quoted
But the thing is, in a distributed environment, the submodule HEAD being
at the tip of _some_ branch (either local or remote) you have doesn't mean
anything to help them. IOW, for protect others, you would need a check
when you _push out_ (either in 'push' or on the receiving end).
The right place to do this check is at the receiving end. Just like they
can be (and by default are) configured to reject a non ff push into the
repository, the receiving end of the superproject can inspect the incoming
history and make sure that the commits bound at submodule paths are all
available to others that might want to fetch those superproject commits
from it in associated submodule repositories. It would forbid people from
first pushing superproject and then pushing submodule projects, but I
think that is a better workflow anyway (you make sure prerequisites are
available in the submodule repositories to others first, and then make the
superproject commit that depends on the submodules).
You may also need to check at the receiving end when pushing into the
submodule repository; if the push is a non ff one, it _might_ lose commits
that are still needed by the associated superproject.
quoted
So I'd suggest dropping this condition in "status/diff" that is about
preparing to make the next commit in your _local_ history.
I would rather have this patch merged without c) than not at all. But i
think it is a worthwhile and rather cheap test.
Did I ever say "drop (c) because it is *too expensive*"?
I suggested to drop it because it is a _pointless_ check in the context of
the codepath, even though I was too polite to put it that bluntly in the
message you are responding to.
I think this topic of yours overall is going in the right direction. I
have no issue with the intent to show submodules with local changes in
"Changed but not updated" section of status, or do the "+sha1-dirty" thing
in diff.
Thanks.
Until now a submodule only then showed up as modified in the supermodule
when the last commit in the submodule differed from the one in the index
or the diffed against commit of the superproject. A dirty work tree
containing new untracked or modified files in a submodule was
undetectable when looking at it from the superproject.
Now git status and git diff (against the work tree) in the superproject
will also display submodules as modified when they contain untracked or
modified files, even if the compared ref matches the HEAD of the
submodule.
Signed-off-by: Jens Lehmann <redacted>
---
Thanks for your review, here is the updated patch. Changes to the RFC
version are:
- Removed check for a dangling HEAD (now the testsuite runs fine)
- Reworded the commit message
- Inlined is_submodule_working_directory_dirty() into
is_submodule_modified()
- The new code will only be called when refs did match (when they
didn't the submodule will already show up as modified)
What do you think?
diff-lib.c | 4 ++-
submodule.c | 49 +++++++++++++++++++++++++++++++++++++++++++
submodule.h | 1 +
t/t7506-status-submodule.sh | 31 ++++++++++++++++++++++++++-
4 files changed, 83 insertions(+), 2 deletions(-)
@@ -159,7 +159,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)continue;}-if(ce_uptodate(ce)||ce_skip_worktree(ce))+if((ce_uptodate(ce)&&!S_ISGITLINK(ce->ce_mode))||ce_skip_worktree(ce))continue;/* If CE_VALID is set, don't look at workdir for file removal */
@@ -176,6 +176,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)continue;}changed=ce_match_stat(ce,&st,ce_option);+if(S_ISGITLINK(ce->ce_mode)&&!changed)+changed=is_submodule_modified(ce->name);if(!changed){ce_mark_uptodate(ce);if(!DIFF_OPT_TST(&revs->diffopt,FIND_COPIES_HARDER))
@@ -112,3 +113,51 @@ void show_submodule_summary(FILE *f, const char *path,}strbuf_release(&sb);}++intis_submodule_modified(constchar*path)+{+intlen;+structchild_processcp;+constchar*argv[]={+"status",+"--porcelain",+NULL,+};+char*env[3];+structstrbufbuf=STRBUF_INIT;++strbuf_addf(&buf,"%s/.git/",path);+if(!is_directory(buf.buf)){+strbuf_release(&buf);+/* The submodule is not checked out, so it is not modified */+return0;++}+strbuf_reset(&buf);++strbuf_addf(&buf,"GIT_WORK_TREE=%s",path);+env[0]=strbuf_detach(&buf,NULL);+strbuf_addf(&buf,"GIT_DIR=%s/.git",path);+env[1]=strbuf_detach(&buf,NULL);+env[2]=NULL;++memset(&cp,0,sizeof(cp));+cp.argv=argv;+cp.env=(constchar*const*)env;+cp.git_cmd=1;+cp.no_stdin=1;+cp.out=-1;+if(start_command(&cp))+die("Could not run git status --porcelain");++len=strbuf_read(&buf,cp.out,1024);+close(cp.out);++if(finish_command(&cp))+die("git status --porcelain failed");++free(env[0]);+free(env[1]);+strbuf_release(&buf);+returnlen!=0;+}
@@ -23,6 +27,31 @@ test_expect_success 'commit --dry-run -a clean' 'gitcommit--dry-run-a|grep"nothing to commit"'++echo"changed">sub/foo+test_expect_success'status with modified file in submodule''+gitstatus|grep"modified: sub"+'+test_expect_success'status with modified file in submodule (porcelain)''+gitstatus--porcelain>output&&+diffoutput-<<-EOF+Msub+EOF+'+(cdsub&&gitcheckoutfoo)++echo"content">sub/new-file+test_expect_success'status with untracked file in submodule''+gitstatus|grep"modified: sub"+'+test_expect_success'status with untracked file in submodule (porcelain)''+gitstatus--porcelain>output&&+diffoutput-<<-EOF+Msub+EOF+'+rmsub/new-file+ test_expect_success'rm submodule contents''rm-rfsub/*sub/.git'