Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist

[Heiko Voigt]

On Thu, Oct 06, 2016 at 10:20:16AM -0700, Stefan Beller wrote:
> On Thu, Oct 6, 2016 at 2:23 AM, Heiko Voigt  wrote:
> > On Wed, Oct 05, 2016 at 03:53:25PM +0200, Heiko Voigt wrote:
> >> On Tue, Oct 04, 2016 at 02:03:58PM -0700, Stefan Beller wrote:
> >> > Jeff,
> >> > thanks for the suggestions, both git_path(..) as well as checking the 
> >> > config,
> >> > this seems quite readable to me:
> >>
> >> When reading the discussion I thought the same: What about the
> >> "old-style" repositories. I like this one. Checking both locations
> >> is nice.
> >
> > BTW, since it seems we all agree on the direction. Should we add some
> > tests?
> >
> 
> Good call. What do we want to test for?
> * Correctness in case of submodules? (just push and get rejected)
>   I think that is easy to do.
> * Performance with [no, lots of] submodules? That seems harder to me.
> 
> I'll add a test for the correctness part and resend.

Well I though about the following tests:

 * Without submodules: Make sure submodule processing is disabled by
   default
 * With new-style submodules: Make sure submodules are processed by
   default
 * With old-style submodules: Make sure submodules are processed by
   default

But I have to admit that I did not think about the "how can we do that".
But performance seems to be the only thing that is exposing the
processing when we have no submodules, so it seems we can only easily do
the tests with submodules.

Cheers Heiko

Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist

[Stefan Beller]

On Thu, Oct 6, 2016 at 2:23 AM, Heiko Voigt  wrote:
> On Wed, Oct 05, 2016 at 03:53:25PM +0200, Heiko Voigt wrote:
>> On Tue, Oct 04, 2016 at 02:03:58PM -0700, Stefan Beller wrote:
>> > Jeff,
>> > thanks for the suggestions, both git_path(..) as well as checking the 
>> > config,
>> > this seems quite readable to me:
>>
>> When reading the discussion I thought the same: What about the
>> "old-style" repositories. I like this one. Checking both locations
>> is nice.
>
> BTW, since it seems we all agree on the direction. Should we add some
> tests?
>

Good call. What do we want to test for?
* Correctness in case of submodules? (just push and get rejected)
  I think that is easy to do.
* Performance with [no, lots of] submodules? That seems harder to me.

I'll add a test for the correctness part and resend.

Thanks,
Stefan

Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist

[Heiko Voigt]

On Wed, Oct 05, 2016 at 03:53:25PM +0200, Heiko Voigt wrote:
> On Tue, Oct 04, 2016 at 02:03:58PM -0700, Stefan Beller wrote:
> > Jeff,
> > thanks for the suggestions, both git_path(..) as well as checking the 
> > config,
> > this seems quite readable to me:
> 
> When reading the discussion I thought the same: What about the
> "old-style" repositories. I like this one. Checking both locations
> is nice.

BTW, since it seems we all agree on the direction. Should we add some
tests?

Cheers Heiko

Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist

[Stefan Beller]

On Wed, Oct 5, 2016 at 8:47 AM, Jeff King  wrote:
> On Tue, Oct 04, 2016 at 02:03:58PM -0700, Stefan Beller wrote:
>
>> thanks for the suggestions, both git_path(..) as well as checking the config,
>> this seems quite readable to me:
>>
>>  builtin/push.c | 14 +-
>>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> Yeah, this seems like a good compromise to me.
>
> I did have one other thought, but I don't think it's worth pursuing now.
> We care about finding gitlinks in objects we're pushing; is there any
> process which is already looking at those objects? Genreally, the
> "pack-objects" doing the push has to. Not to actually push the objects,
> which often are sent blindly off disk, but to determine the set of
> reachable objects in the first place. So in theory we could prepare the
> list of objects to pack, and as a side effect it could say "and here are
> gitlinks referenced by those objects". But that doesn't work if bitmaps
> are in effect, because then we don't access the objects directly at all.
> I think you could solve that by extending the bitmap format to include a
> bit for gitlinks that are reachable (but not necessarily included in the
> pack).
>
> So I don't think that's worth thinking too much about now, but it might
> be an interesting optimization down the road.
>
> -Peff

That sounds like what we'd want down the road. In my imagination the
submodules of the future may live completely inside the superproject,
i.e. they do not necessarily have their own URL. They can be obtained
via fetching the superproject, that automagically also transmits the objects
of the submodules.

Thanks for the hint w.r.t. the bimaps!
Stefan

Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist

[Jeff King]

On Tue, Oct 04, 2016 at 02:03:58PM -0700, Stefan Beller wrote:

> thanks for the suggestions, both git_path(..) as well as checking the config,
> this seems quite readable to me:
> 
>  builtin/push.c | 14 +-
>  1 file changed, 13 insertions(+), 1 deletion(-)

Yeah, this seems like a good compromise to me.

I did have one other thought, but I don't think it's worth pursuing now.
We care about finding gitlinks in objects we're pushing; is there any
process which is already looking at those objects? Genreally, the
"pack-objects" doing the push has to. Not to actually push the objects,
which often are sent blindly off disk, but to determine the set of
reachable objects in the first place. So in theory we could prepare the
list of objects to pack, and as a side effect it could say "and here are
gitlinks referenced by those objects". But that doesn't work if bitmaps
are in effect, because then we don't access the objects directly at all.
I think you could solve that by extending the bitmap format to include a
bit for gitlinks that are reachable (but not necessarily included in the
pack).

So I don't think that's worth thinking too much about now, but it might
be an interesting optimization down the road.

-Peff

Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist

[Heiko Voigt]

On Tue, Oct 04, 2016 at 02:03:58PM -0700, Stefan Beller wrote:
> Jeff,
> thanks for the suggestions, both git_path(..) as well as checking the config,
> this seems quite readable to me:

When reading the discussion I thought the same: What about the
"old-style" repositories. I like this one. Checking both locations
is nice.

Cheers Heiko

[PATCHv3 1/2] push: change submodule default to check when submodules exist

[Stefan Beller]

When working with submodules, it is easy to forget to push the submodules.
The setting 'check', which checks if any existing submodule is present on
at least one remote of the submodule remotes, is designed to prevent this
mistake.

Flipping the default to check for submodules is safer than the current
default of ignoring submodules while pushing.

However checking for submodules requires additional work[1], which annoys
users that do not use submodules, so we turn on the check for submodules
based on a cheap heuristic, the existence of the .git/modules directory.
That directory doesn't exist when no submodules are used and is only
created and populated when submodules are cloned/added.

When the submodule directory doesn't exist, a user may have changed the
gitlinks via plumbing commands. Currently the default is to not check.
RECURSE_SUBMODULES_DEFAULT is effectively RECURSE_SUBMODULES_OFF currently,
though it may change in the future. When no submodules exist such a check
is pointless as it would fail anyway, so let's just turn it off.

[1] 
https://public-inbox.org/git/CA+55aFyos78qODyw57V=w13ux5-8svbqobjfaq22k+xkpwv...@mail.gmail.com/

Signed-off-by: Stefan Beller 
---

Jeff,
thanks for the suggestions, both git_path(..) as well as checking the config,
this seems quite readable to me:

 builtin/push.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/builtin/push.c b/builtin/push.c
index 3bb9d6b..65bb1df 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -3,6 +3,7 @@
  */
 #include "cache.h"
 #include "refs.h"
+#include "dir.h"
 #include "run-command.h"
 #include "builtin.h"
 #include "remote.h"
@@ -22,6 +23,7 @@ static int deleterefs;
 static const char *receivepack;
 static int verbosity;
 static int progress = -1;
+static int has_submodules;
 static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static enum transport_family family;
 
@@ -31,6 +33,14 @@ static const char **refspec;
 static int refspec_nr;
 static int refspec_alloc;
 
+static void preset_submodule_default(void)
+{
+   if (has_submodules || file_exists(git_path("modules")))
+   recurse_submodules = RECURSE_SUBMODULES_CHECK;
+   else
+   recurse_submodules = RECURSE_SUBMODULES_OFF;
+}
+
 static void add_refspec(const char *ref)
 {
refspec_nr++;
@@ -495,7 +505,8 @@ static int git_push_config(const char *k, const char *v, 
void *cb)
const char *value;
if (!git_config_get_value("push.recursesubmodules", ))
recurse_submodules = 
parse_push_recurse_submodules_arg(k, value);
-   }
+   } else if (starts_with(k, "submodule.") && ends_with(k, ".url"))
+   has_submodules = 1;
 
return git_default_config(k, v, NULL);
 }
@@ -552,6 +563,7 @@ int cmd_push(int argc, const char **argv, const char 
*prefix)
};
 
packet_trace_identity("push");
+   preset_submodule_default();
git_config(git_push_config, );
argc = parse_options(argc, argv, prefix, options, push_usage, 0);
set_push_cert_flags(, push_cert);
-- 
2.10.0.129.g35f6318