Re: [PATCH v3] push: respect --no-thin
Duy Nguyen writes: > transport_get() actually sets thin option to 1 by default. Yeah, I missed your message in the nearby thread. It indeed does. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] push: respect --no-thin
On Mon, Aug 12, 2013 at 12:59 PM, Junio C Hamano wrote: >> @@ -15,7 +15,7 @@ static const char * const push_usage[] = { >> NULL, >> }; >> >> -static int thin; >> +static int thin = 1; >> static int deleterefs; >> static const char *receivepack; >> static int verbosity; >> @@ -313,8 +313,7 @@ static int push_with_options(struct transport >> *transport, int flags) >> if (receivepack) >> transport_set_option(transport, >>TRANS_OPT_RECEIVEPACK, receivepack); >> - if (thin) >> - transport_set_option(transport, TRANS_OPT_THIN, "yes"); >> + transport_set_option(transport, TRANS_OPT_THIN, thin ? "yes" : NULL); >> >> if (verbosity > 0) >> fprintf(stderr, _("Pushing to %s\n"), transport->url); > > Hmm, I am utterly confused. > > How does the original code have thin as an non-overridable default? > The variable is initialized to 0, parse-options specifies it as > OPT_BOOLEAN, and TRANS_OPT_THIN is set only if "thin" is set. > > Updated code flips the default to "1" but unconditionally uses > TRANS_OPT_THIN to set it to either "yes" or NULL. While it is not > wrong per-se, do_push() (i.e. the caller of this function) grabs the > transport from transport_get() which initializes the transport with > the thin option disabled by default, transport_get() actually sets thin option to 1 by default. If I don't misread the code, the first version of transport.c already flipped "thin" from 0 (in push.c) to 1 (in transport.c), see 9b28851 (Push code for transport library - 2007-09-10). The funny thing is that commit was just one day after Shawn flipped 'thin' from 1 to 0 in push.c in a4503a1. > so it is not immediately > obvious to me why "always call TRANS_OPT_THIN but set it explicitly > to NULL when --no-thin is asked" is necessary or improvement. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] push: respect --no-thin
Nguyễn Thái Ngọc Duy writes: > Over the time the default value for --thin has been switched between > on and off. As of now it's always on, even if --no-thin is given. > Correct the code to respect --no-thin. > > receive-pack learns about --no-thin only for testing purposes, hence > no document update. Please name it "--reject-thin-pack-for-testing" to make it crystal clear that a documentation patch to "document undocumented option" is unwanted. > diff --git a/builtin/push.c b/builtin/push.c > index 04f0eaf..333a1fb 100644 > --- a/builtin/push.c > +++ b/builtin/push.c > @@ -15,7 +15,7 @@ static const char * const push_usage[] = { > NULL, > }; > > -static int thin; > +static int thin = 1; > static int deleterefs; > static const char *receivepack; > static int verbosity; > @@ -313,8 +313,7 @@ static int push_with_options(struct transport *transport, > int flags) > if (receivepack) > transport_set_option(transport, >TRANS_OPT_RECEIVEPACK, receivepack); > - if (thin) > - transport_set_option(transport, TRANS_OPT_THIN, "yes"); > + transport_set_option(transport, TRANS_OPT_THIN, thin ? "yes" : NULL); > > if (verbosity > 0) > fprintf(stderr, _("Pushing to %s\n"), transport->url); Hmm, I am utterly confused. How does the original code have thin as an non-overridable default? The variable is initialized to 0, parse-options specifies it as OPT_BOOLEAN, and TRANS_OPT_THIN is set only if "thin" is set. Updated code flips the default to "1" but unconditionally uses TRANS_OPT_THIN to set it to either "yes" or NULL. While it is not wrong per-se, do_push() (i.e. the caller of this function) grabs the transport from transport_get() which initializes the transport with the thin option disabled by default, so it is not immediately obvious to me why "always call TRANS_OPT_THIN but set it explicitly to NULL when --no-thin is asked" is necessary or improvement. Puzzled -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] push: respect --no-thin
Over the time the default value for --thin has been switched between on and off. As of now it's always on, even if --no-thin is given. Correct the code to respect --no-thin. receive-pack learns about --no-thin only for testing purposes, hence no document update. Signed-off-by: Nguyễn Thái Ngọc Duy --- v3 gets rid of seq in the test. builtin/push.c | 5 ++--- builtin/receive-pack.c | 8 +++- t/t5516-fetch-push.sh | 16 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/builtin/push.c b/builtin/push.c index 04f0eaf..333a1fb 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -15,7 +15,7 @@ static const char * const push_usage[] = { NULL, }; -static int thin; +static int thin = 1; static int deleterefs; static const char *receivepack; static int verbosity; @@ -313,8 +313,7 @@ static int push_with_options(struct transport *transport, int flags) if (receivepack) transport_set_option(transport, TRANS_OPT_RECEIVEPACK, receivepack); - if (thin) - transport_set_option(transport, TRANS_OPT_THIN, "yes"); + transport_set_option(transport, TRANS_OPT_THIN, thin ? "yes" : NULL); if (verbosity > 0) fprintf(stderr, _("Pushing to %s\n"), transport->url); diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index e3eb5fc..da60817 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -38,6 +38,7 @@ static int quiet; static int prefer_ofs_delta = 1; static int auto_update_server_info; static int auto_gc = 1; +static int fix_thin = 1; static const char *head_name; static void *head_name_to_free; static int sent_capabilities; @@ -869,7 +870,8 @@ static const char *unpack(int err_fd) keeper[i++] = "--stdin"; if (fsck_objects) keeper[i++] = "--strict"; - keeper[i++] = "--fix-thin"; + if (fix_thin) + keeper[i++] = "--fix-thin"; keeper[i++] = hdr_arg; keeper[i++] = keep_arg; keeper[i++] = NULL; @@ -975,6 +977,10 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) stateless_rpc = 1; continue; } + if (!strcmp(arg, "--no-thin")) { + fix_thin = 0; + continue; + } usage(receive_pack_usage); } diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 4691d51..3cfd1cd 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1172,4 +1172,20 @@ test_expect_success 'push --follow-tag only pushes relevant tags' ' test_cmp expect actual ' +test_expect_success 'push --no-thin must produce non-thin pack' ' + cat >>path1 <<\EOF && +keep base version of path1 big enough, compared to the new changes +later, in order to pass size heuristics in +builtin/pack-objects.c:try_delta() +EOF + git commit -am initial && + git init no-thin && + git --git-dir=no-thin/.git config receive.unpacklimit 0 && + git push no-thin/.git refs/heads/master:refs/heads/foo && + echo modified >> path1 && + git commit -am modified && + git repack -adf && + git push --no-thin --receive-pack="git receive-pack --no-thin" no-thin/.git refs/heads/master:refs/heads/foo +' + test_done -- 1.8.2.83.gc99314b -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html