On Thu, 2 Aug 2018 13:43:05 -0700
Stefan Beller <sbeller@xxxxxxxxxx> wrote:
> On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@xxxxxx> wrote:
> >
> > git submodule commands can now access .gitmodules from the current
> > branch even when it's not in the working tree, add some tests for that
> > scenario.
> >
> > Signed-off-by: Antonio Ospite <ao2@xxxxxx>
> > ---
[...]
> > +NOTE: "git mv" and "git rm" are still supposed to work even without
> > +a .gitmodules file, as stated in the t3600-rm.sh and t7001-mv.sh tests.
>
> "supposed to work" != "tested that it works" ?
"git mv submod new_submod" and "git rm submod" are actually expected to
work without the .gitmodules file, and there are tests about that in
t3600-rm.sh and t7001-mv.sh:
t3600-rm.sh:
'rm does not complain when no .gitmodules file is found'
t7001-mv.sh:
'git mv moves a submodule with a .git directory and no .gitmodules'
'mv does not complain when no .gitmodules file is found'
> I am not sure what the NOTE wants to tell me? (Should I review those
> tests to double check them now? or do we just want to tell future readers
> of this test there are other tangent tests to this?)
>
Admittedly the NOTE is not useful without any context: during the
development of "submodule--helper config --stage" I initially assumed
that "git mv" and "git rm" should fail if .gitmodules was not available,
because these commands modify .gitmodules and I added code for that in
stage_updated_gitmodules().
But then later I found out that my assumption was wrong and that git has
tests to verify that these operations on submodules succeed even when
.gitmodules does not exist, which was a little of a surprise to me.
So I removed all my code that was conflicting with git assumptions, and
added the NOTE. However I guess that was primarily a note to myself, and
it should have not slipped in the public patches.
I think I will remove the note, it can be confusing and does not really
add anything, and even less considering that "submodule--helper config
--stage" is going to be dropped.
[...]
> > +test_expect_success 'not adding submodules when the gitmodules config is not checked out' '
> > + (cd super &&
> > + test_must_fail git submodule add ../new_submodule
> > + )
> > +'
> > +
> > +# "git add" in the test above fails as expected, however it still leaves the
> > +# cloned tree in there and adds a config entry to .git/config. This is because
> > +# no cleanup is done by cmd_add in git-submodule.sh when "git
> > +# submodule--helper config" fails to add a new config setting.
> > +#
> > +# If we added the following commands to the test above:
> > +#
> > +# rm -rf .git/modules/new_submodule &&
> > +# git reset HEAD new_submodule &&
> > +# rm -rf new_submodule
>
> Alternatively we could check for the existence of .gitmodules
> before starting all these things?
>
You mean in cmd_add(), before doing anything?
The following would anticipates the same check which makes "git submodule
add" fail:
diff --git a/git-submodule.sh b/git-submodule.sh
index ff258e2e8c..b261175143 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -159,6 +159,11 @@ cmd_add()
shift
done
+ if test ! -e .gitmodules && git cat-file -e HEAD:.gitmodules
+ then
+ die "$(eval_gettext "please make sure that the .gitmodules file in the current branch is checked out")"
+ fi
+
if test -n "$reference_path"
then
is_absolute_path "$reference_path" ||
This refers to .gitmodules explicitly but we said that we do not care
about that for now, if opaque access was ever needed in the future,
something like "submodule--helper config --is-writeable" could be added.
> I think it is okay to not clean up if we check all "regular" or rather expected
> things such as a non-writable .gitmodules file before actually doing it.
> (This is similar to 'checkout' that walks the whole tree and checks if the
> checkout is possible given the dirtyness of the tree, to either abort early
> or pull through completely. In catastrophic problems such as a full disk
> we'd still die in the middle of work)
>
> > +#
> > +# then the repository would be in a clean state and the test below would pass.
> > +#
> > +# Maybe cmd_add should do the cleanup from above itself when failing to add
> > +# a submodule.
> > +test_expect_failure 'init submodule after adding failed when the gitmodules config is not checked out' '
>
> So this comment and test is about explaining why we can fail mid way through,
> which we could not before unless we had the catastrophic event.
>
> I think we should check for a "writable" .gitmodules file at the beginning,
> which is if (G || (!G && !H)) [using the notation from the cover letter]?
>
> > + (cd super &&
> > + git submodule init
With the change from above this last test passes.
BTW the check I am using here and in the code of submodule--helper,
corresponds indeed to the boolean expression you mentioned, but
simplified and negated.
Thanks,
Antonio
--
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
