Re: [PATCH 20/20] fetch: add --deepen=<N> to extend shallow boundary by <N> commits

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 4, 2016, 09:45 UTC
Message-ID: <CAPig+cTiN+y0KXLjoK0EeyxXk6DOMG16A2gQ_a9PsU__DA-LUg@mail.gmail.com>
In-Reply-To: <1451391043-28093-21-git-send-email-pclouds@gmail.com>

On Tue, Dec 29, 2015 at 7:10 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

Show 12 quoted lines

> In git-fetch, --depth argument is always relative with the latest
> remote refs. This makes it a bit difficult to cover this use case,
> where the user wants to make the shallow history, say 3 levels
> deeper. It would work if remote refs have not moved yet, but nobody
> can guarantee that, especially when that use case is performed a
> couple months after the last clone or "git fetch --depth". Also,
> modifying shallow boundary using --depth does not work well with
> clones created by --since or --not.
> [...]
>
> Helped-by: Duy Nguyen <pclouds@gmail.com>
> Helped-By: Eric Sunshine <sunshine@sunshineco.com>

s/By/by/

> Helped-By: Junio C Hamano <gitster@pobox.com>

Ditto.

Show 9 quoted lines

> Signed-off-by: Dongcan Jiang <dongcan.jiang@gmail.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
> @@ -13,6 +13,11 @@
> +--deepen=<depth>::
> +       Similar to --depth, except it specifies the number of commits
> +       from the current shallow boundary instead of from the tip of
> +       reach remote branch history.

Did you mean s/reach/each/ ?

Show 13 quoted lines

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> @@ -1185,6 +1189,15 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>         /* no need to be strict, transport_set_option() will validate it again */
>         if (depth && atoi(depth) < 1)
>                 die(_("depth %s is not a positive number"), depth);
> +       if (deepen_relative) {
> +               struct strbuf sb = STRBUF_INIT;
> +               if (deepen_relative < 0)
> +                       die(_("Negative depth in --deepen is not supported"));
> +               if (depth)
> +                       die(_("--deepen and --depth are mutually exclusive"));
> +               strbuf_addf(&sb, "%d", deepen_relative);
> +               depth = strbuf_detach(&sb, NULL);

Maybe replace:

    struct strbuf sb = STRBUF_INIT;
    ...
    strbuf_addf(&sb, "%d", deepen_relative);
    depth = strbuf_detach(&sb, NULL);

with:

    depth = xstrfmt("%d", deepen_relative);

Show 7 quoted lines

> +       }
>         if (depth || deepen_since || deepen_not.nr)
>                 deepen = 1;
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> @@ -708,4 +708,16 @@ test_expect_success 'fetching a one-level ref works' '
> +test_expect_success 'fetching deepen' '
> +       git clone . deepen --depth=1 && (

Style: ( on its own line.

> +               cd deepen &&
> +               git fetch .. foo --depth=1
> +               git show foo

Are these git-show instances merely for checking if a ref is valid? If so, perhaps git-rev-parse would be clearer?

> +               test_must_fail git show foo~
> +               git fetch .. foo --deepen=1
> +               git show foo~
> +               test_must_fail git show foo~2

&&-chain heavily broken in subshell.

> +       )
> +'