Re: [PATCH v2 21/25] fetch: define shallow boundary with --shallow-exclude

[Eric Sunshine]

On Mon, Feb 15, 2016 at 3:15 AM, Duy Nguyen  wrote:
> On Mon, Feb 15, 2016 at 12:52:26AM -0500, Eric Sunshine wrote:
>> Yes, dropping 'const' was implied. I didn't examine it too deeply, but
>> it did not appear as if there would be any major fallout from dropping
>> 'const'. It feels a bit cleaner to keep it all self-contained than to
>> have that somewhat oddball static string_list*, but it's not such a
>> big deal that I'd insist upon a rewrite.
>
> Dropping 'const' is not a big deal. But before we do that, how about
> this instead? I think the code looks better, and the compiler can
> still catch invalid updates to deepen_not.

I like this better, too.

> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index 0402e27..07570be 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -50,6 +50,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
> *prefix)
> struct child_process *conn;
> struct fetch_pack_args args;
> struct sha1_array shallow = SHA1_ARRAY_INIT;
> +   struct string_list deepen_not = STRING_LIST_INIT_DUP;
>
> packet_trace_identity("fetch-pack");
>
> @@ -108,6 +109,10 @@ int cmd_fetch_pack(int argc, const char **argv, const 
> char *prefix)
> args.deepen_since = xstrdup(arg);
> continue;
> }
> +   if (skip_prefix(arg, "--shallow-exclude=", )) {
> +   string_list_append(_not, arg);
> +   continue;
> +   }
> if (!strcmp("--no-progress", arg)) {
> args.no_progress = 1;
> continue;
> @@ -135,6 +140,8 @@ int cmd_fetch_pack(int argc, const char **argv, const 
> char *prefix)
> }
> usage(fetch_pack_usage);
> }
> +   if (deepen_not.nr)
> +   args.deepen_not = _not;
>
> if (i < argc)
> dest = argv[i++];
> --
> 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 v2 21/25] fetch: define shallow boundary with --shallow-exclude

[Duy Nguyen]

On Mon, Feb 15, 2016 at 12:52:26AM -0500, Eric Sunshine wrote:
> Yes, dropping 'const' was implied. I didn't examine it too deeply, but
> it did not appear as if there would be any major fallout from dropping
> 'const'. It feels a bit cleaner to keep it all self-contained than to
> have that somewhat oddball static string_list*, but it's not such a
> big deal that I'd insist upon a rewrite.

Dropping 'const' is not a big deal. But before we do that, how about
this instead? I think the code looks better, and the compiler can
still catch invalid updates to deepen_not.

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 0402e27..07570be 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -50,6 +50,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
struct child_process *conn;
struct fetch_pack_args args;
struct sha1_array shallow = SHA1_ARRAY_INIT;
+   struct string_list deepen_not = STRING_LIST_INIT_DUP;
 
packet_trace_identity("fetch-pack");
 
@@ -108,6 +109,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
args.deepen_since = xstrdup(arg);
continue;
}
+   if (skip_prefix(arg, "--shallow-exclude=", )) {
+   string_list_append(_not, arg);
+   continue;
+   }
if (!strcmp("--no-progress", arg)) {
args.no_progress = 1;
continue;
@@ -135,6 +140,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
}
usage(fetch_pack_usage);
}
+   if (deepen_not.nr)
+   args.deepen_not = _not;
 
if (i < argc)
dest = argv[i++];
--
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 v2 21/25] fetch: define shallow boundary with --shallow-exclude

[Eric Sunshine]

On Mon, Feb 15, 2016 at 12:52 AM, Eric Sunshine  wrote:
> On Sun, Feb 14, 2016 at 10:53 PM, Duy Nguyen  wrote:
>> On Fri, Feb 5, 2016 at 12:26 PM, Eric Sunshine  
>> wrote:
>>> Hmm, can't this be simplified to:
>>>
>>> if (skip_prefix(arg, "--shallow-exclude=", )) {
>>> if (!args.deepen_not) {
>>> args.deepen_not = xmalloc(sizeof(*args.deepen_not));
>>> string_list_init(args.deepen_not, 1);
>>> }
>>> string_list_append(args.deepen_not, value);
>>> continue;
>>> }
>>
>> args.deepen_not is const, so no, the compiler will complain at
>> string_list_init and string_list_append. Dropping "const" is one
>> option, if you prefer.
>
> Yes, dropping 'const' was implied. I didn't examine it too deeply, but
> it did not appear as if there would be any major fallout from dropping
> 'const'. It feels a bit cleaner to keep it all self-contained than to
> have that somewhat oddball static string_list*, but it's not such a
> big deal that I'd insist upon a rewrite.
>
>>> Or, perhaps even better, declare it as plain 'struct string_list
>>> deepen_not' in struct fetch_pack_args, rather than as a pointer, and
>>> then in cmd_fetch_pack():
>>>
>>> memset(, 0, sizeof(args));
>>> args.uploadpack = "git-upload-pack";
>>> string_list_init(_not, 1);
>>
>> There's another place fetch_pack_args variable is declared, in
>> fetch_refs_via_pack(), and we would need to string_list_copy() from
>> transport->data->options.deepen_not and then free it afterward. So I
>> think it's not really worth it.

Upon re-reading, I suppose this also is an argument for keeping the
static string_list* rather than dropping the 'const'...(?)
--
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 v2 21/25] fetch: define shallow boundary with --shallow-exclude

[Eric Sunshine]

On Sun, Feb 14, 2016 at 10:53 PM, Duy Nguyen  wrote:
> On Fri, Feb 5, 2016 at 12:26 PM, Eric Sunshine  
> wrote:
>> On Thu, Feb 4, 2016 at 4:03 AM, Nguyễn Thái Ngọc Duy  
>> wrote:
>>> Signed-off-by: Nguyễn Thái Ngọc Duy 
>>> ---
>>> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
>>> @@ -109,6 +109,16 @@ int cmd_fetch_pack(int argc, const char **argv, const 
>>> char *prefix)
>>> +   if (skip_prefix(arg, "--shallow-exclude=", )) {
>>> +   static struct string_list *deepen_not;
>>> +   if (!deepen_not) {
>>> +   deepen_not = xmalloc(sizeof(*deepen_not));
>>> +   string_list_init(deepen_not, 1);
>>> +   args.deepen_not = deepen_not;
>>> +   }
>>> +   string_list_append(deepen_not, value);
>>> +   continue;
>>> +   }
>>
>> Hmm, can't this be simplified to:
>>
>> if (skip_prefix(arg, "--shallow-exclude=", )) {
>> if (!args.deepen_not) {
>> args.deepen_not = xmalloc(sizeof(*args.deepen_not));
>> string_list_init(args.deepen_not, 1);
>> }
>> string_list_append(args.deepen_not, value);
>> continue;
>> }
>
> args.deepen_not is const, so no, the compiler will complain at
> string_list_init and string_list_append. Dropping "const" is one
> option, if you prefer.

Yes, dropping 'const' was implied. I didn't examine it too deeply, but
it did not appear as if there would be any major fallout from dropping
'const'. It feels a bit cleaner to keep it all self-contained than to
have that somewhat oddball static string_list*, but it's not such a
big deal that I'd insist upon a rewrite.

>> Or, perhaps even better, declare it as plain 'struct string_list
>> deepen_not' in struct fetch_pack_args, rather than as a pointer, and
>> then in cmd_fetch_pack():
>>
>> memset(, 0, sizeof(args));
>> args.uploadpack = "git-upload-pack";
>> string_list_init(_not, 1);
>
> There's another place fetch_pack_args variable is declared, in
> fetch_refs_via_pack(), and we would need to string_list_copy() from
> transport->data->options.deepen_not and then free it afterward. So I
> think it's not really worth it.

Okay.

>> if (skip_prefix(arg, "--shallow-exclude=", )) {
>> string_list_append(args.deepen_not, value);
>> continue;
>> }
--
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 v2 21/25] fetch: define shallow boundary with --shallow-exclude

[Duy Nguyen]

On Fri, Feb 5, 2016 at 12:26 PM, Eric Sunshine  wrote:
> On Thu, Feb 4, 2016 at 4:03 AM, Nguyễn Thái Ngọc Duy  
> wrote:
>> Signed-off-by: Nguyễn Thái Ngọc Duy 
>> ---
>> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
>> @@ -109,6 +109,16 @@ int cmd_fetch_pack(int argc, const char **argv, const 
>> char *prefix)
>> +   if (skip_prefix(arg, "--shallow-exclude=", )) {
>> +   static struct string_list *deepen_not;
>> +   if (!deepen_not) {
>> +   deepen_not = xmalloc(sizeof(*deepen_not));
>> +   string_list_init(deepen_not, 1);
>> +   args.deepen_not = deepen_not;
>> +   }
>> +   string_list_append(deepen_not, value);
>> +   continue;
>> +   }
>
> Hmm, can't this be simplified to:
>
> if (skip_prefix(arg, "--shallow-exclude=", )) {
> if (!args.deepen_not) {
> args.deepen_not = xmalloc(sizeof(*args.deepen_not));
> string_list_init(args.deepen_not, 1);
> }
> string_list_append(args.deepen_not, value);
> continue;
> }

args.deepen_not is const, so no, the compiler will complain at
string_list_init and string_list_append. Dropping "const" is one
option, if you prefer.

> Or, perhaps even better, declare it as plain 'struct string_list
> deepen_not' in struct fetch_pack_args, rather than as a pointer, and
> then in cmd_fetch_pack():
>
> memset(, 0, sizeof(args));
> args.uploadpack = "git-upload-pack";
> string_list_init(_not, 1);

There's another place fetch_pack_args variable is declared, in
fetch_refs_via_pack(), and we would need to string_list_copy() from
transport->data->options.deepen_not and then free it afterward. So I
think it's not really worth it.

> if (skip_prefix(arg, "--shallow-exclude=", )) {
> string_list_append(args.deepen_not, value);
> continue;
> }
-- 
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 v2 21/25] fetch: define shallow boundary with --shallow-exclude

[Eric Sunshine]

On Thu, Feb 4, 2016 at 4:03 AM, Nguyễn Thái Ngọc Duy  wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> @@ -109,6 +109,16 @@ int cmd_fetch_pack(int argc, const char **argv, const 
> char *prefix)
> +   if (skip_prefix(arg, "--shallow-exclude=", )) {
> +   static struct string_list *deepen_not;
> +   if (!deepen_not) {
> +   deepen_not = xmalloc(sizeof(*deepen_not));
> +   string_list_init(deepen_not, 1);
> +   args.deepen_not = deepen_not;
> +   }
> +   string_list_append(deepen_not, value);
> +   continue;
> +   }

Hmm, can't this be simplified to:

if (skip_prefix(arg, "--shallow-exclude=", )) {
if (!args.deepen_not) {
args.deepen_not = xmalloc(sizeof(*args.deepen_not));
string_list_init(args.deepen_not, 1);
}
string_list_append(args.deepen_not, value);
continue;
}

Or, perhaps even better, declare it as plain 'struct string_list
deepen_not' in struct fetch_pack_args, rather than as a pointer, and
then in cmd_fetch_pack():

memset(, 0, sizeof(args));
args.uploadpack = "git-upload-pack";
string_list_init(_not, 1);

...

if (skip_prefix(arg, "--shallow-exclude=", )) {
string_list_append(args.deepen_not, value);
continue;
}
--
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 v2 21/25] fetch: define shallow boundary with --shallow-exclude

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/fetch-options.txt |  5 +
 Documentation/git-fetch-pack.txt|  5 +
 Documentation/gitremote-helpers.txt |  4 
 builtin/fetch-pack.c| 10 ++
 builtin/fetch.c | 17 -
 fetch-pack.c| 15 ++-
 fetch-pack.h|  1 +
 remote-curl.c   |  9 +
 transport-helper.c  | 27 +++
 transport.c |  4 
 transport.h |  6 ++
 11 files changed, 101 insertions(+), 2 deletions(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 8738d3d..7aa1285 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -18,6 +18,11 @@
Deepen or shorten the history of a shallow repository to
include all reachable commits after .
 
+--shallow-exclude=::
+   Deepen or shorten the history of a shallow repository to
+   exclude commits reachable from a specified remote branch or tag.
+   This option can be specified multiple times.
+
 --unshallow::
If the source repository is complete, convert a shallow
repository to a complete one, removing all the limitations
diff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt
index 99e6257..4d15b04 100644
--- a/Documentation/git-fetch-pack.txt
+++ b/Documentation/git-fetch-pack.txt
@@ -91,6 +91,11 @@ be in a separate packet, and the list must end with a flush 
packet.
Deepen or shorten the history of a shallow'repository to
include all reachable commits after .
 
+--shallow-exclude=::
+   Deepen or shorten the history of a shallow repository to
+   exclude commits reachable from a specified remote branch or tag.
+   This option can be specified multiple times.
+
 --no-progress::
Do not show the progress.
 
diff --git a/Documentation/gitremote-helpers.txt 
b/Documentation/gitremote-helpers.txt
index 9971d9a..75bb638 100644
--- a/Documentation/gitremote-helpers.txt
+++ b/Documentation/gitremote-helpers.txt
@@ -418,6 +418,10 @@ set by Git if the remote helper has the 'option' 
capability.
 'option deepen-since ::
Deepens the history of a shallow repository based on time.
 
+'option deepen-not ::
+   Deepens the history of a shallow repository excluding ref.
+   Multiple options add up.
+
 'option followtags' {'true'|'false'}::
If enabled the helper should automatically fetch annotated
tag objects if the object the tag points at was transferred
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 192c1ae..1a49184 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -109,6 +109,16 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
args.deepen_since = xstrdup(value);
continue;
}
+   if (skip_prefix(arg, "--shallow-exclude=", )) {
+   static struct string_list *deepen_not;
+   if (!deepen_not) {
+   deepen_not = xmalloc(sizeof(*deepen_not));
+   string_list_init(deepen_not, 1);
+   args.deepen_not = deepen_not;
+   }
+   string_list_append(deepen_not, value);
+   continue;
+   }
if (!strcmp("--no-progress", arg)) {
args.no_progress = 1;
continue;
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7d4d082..11b444b 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -41,6 +41,7 @@ static int max_children = 1;
 static const char *depth;
 static const char *deepen_since;
 static const char *upload_pack;
+struct string_list deepen_not = STRING_LIST_INIT_NODUP;
 static struct strbuf default_rla = STRBUF_INIT;
 static struct transport *gtransport;
 static struct transport *gsecondary;
@@ -50,6 +51,13 @@ static int shown_url = 0;
 static int refmap_alloc, refmap_nr;
 static const char **refmap_array;
 
+static int option_parse_deepen_not(const struct option *opt,
+  const char *arg, int unset)
+{
+   string_list_append(_not, arg);
+   return 0;
+}
+
 static int option_parse_recurse_submodules(const struct option *opt,
   const char *arg, int unset)
 {
@@ -118,6 +126,9 @@ static struct option builtin_fetch_options[] = {
   N_("deepen history of shallow clone")),
OPT_STRING(0, "shallow-since", _since, N_("time"),
   N_("deepen history of shallow repository based on time")),
+   { OPTION_CALLBACK, 0, "shallow-exclude", NULL, N_("revision"),
+   N_("deepen history of shallow clone by excluding rev"),
+