Re: [PATCH] upload-pack: allow shallow fetching from a read-only repository

> This is not introduced by your patch, but I notice that we do not seem
> to do anything with the tempfiles when the program dies prematurely.
> We've started collecting stale shallow_XXXXXX files in our server repos.
>
> For the writable cases, should we be using the lockfile API to take
> shallow.lock? It looks like we do take a lock on it, but only after the
> fetch, and then we have to do a manual check for it having moved (by the
> way, shouldn't we do that check under the lock? I think
> setup_alternate_shallow has a race condition).
>
> I guess the reason to take the lock at the last minute is that the whole
> fetch is heavyweight. But if the fetches are going to conflict, perhaps
> it is better to have lock contention at the beginning, rather than
> failing only at the end. I don't know very much about the shallow
> system; does each operation rewrite the shallow file, or is it often
> left untouched (in which case two simultaneous fetches could coexist
> most of the time).

> At any rate, if we used the lockfile API, it handles tempfile cleanup
> automatically. Otherwise, I think we need something like the patch
> below (in the interest of simplicity, I just drop all of the explicit
> unlinks and let them use the atexit handler to clean up; you could also
> add a function to explicitly unlink the tempfile and clear the handler).

>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 85bba35..ac1d9b5 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -833,8 +833,6 @@ static void execute_commands(struct command *commands,
>                         error("BUG: run 'git fsck' for safety.\n"
>                               "If there are errors, try to remove "
>                               "the reported refs above");
> -               if (alt_shallow_file && *alt_shallow_file)
> -                       unlink(alt_shallow_file);
>         }
>  }
>
> @@ -1087,10 +1085,6 @@ static void update_shallow_info(struct command *commands,
>                         cmd->skip_update = 1;
>                 }
>         }
> -       if (alt_shallow_file && *alt_shallow_file) {
> -               unlink(alt_shallow_file);
> -               alt_shallow_file = NULL;
> -       }
>         free(ref_status);
>  }
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 90fdd49..ae8550e 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -947,17 +947,6 @@ static void update_shallow(struct fetch_pack_args *args,
>         if (!si->shallow || !si->shallow->nr)
>                 return;
>
> -       if (alternate_shallow_file) {
> -               /*
> -                * The temporary shallow file is only useful for
> -                * index-pack and unpack-objects because it may
> -                * contain more roots than we want. Delete it.
> -                */
> -               if (*alternate_shallow_file)
> -                       unlink(alternate_shallow_file);
> -               free((char *)alternate_shallow_file);
> -       }
> -
>         if (args->cloning) {
>                 /*
>                  * remote is shallow, but this is a clone, there are
> diff --git a/shallow.c b/shallow.c
> index bbc98b5..0f2bb48 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -8,6 +8,7 @@
>  #include "diff.h"
>  #include "revision.h"
>  #include "commit-slab.h"
> +#include "sigchain.h"
>
>  static int is_shallow = -1;
>  static struct stat shallow_stat;
> @@ -216,27 +217,49 @@ int write_shallow_commits(struct strbuf *out, int use_pack_protocol,
>         return write_shallow_commits_1(out, use_pack_protocol, extra, 0);
>  }
>
> +static struct strbuf shallow_tmpfile = STRBUF_INIT;
> +
> +static void remove_shallow_tmpfile(void)
> +{
> +       if (shallow_tmpfile.len) {
> +               unlink_or_warn(shallow_tmpfile.buf);
> +               strbuf_reset(&shallow_tmpfile);
> +       }
> +}
> +
> +static void remove_shallow_tmpfile_on_signal(int signo)
> +{
> +       remove_shallow_tmpfile();
> +       sigchain_pop(signo);
> +       raise(signo);
> +}
> +
>  char *setup_temporary_shallow(const struct sha1_array *extra)
>  {
>         struct strbuf sb = STRBUF_INIT;
>         int fd;
>
>         if (write_shallow_commits(&sb, 0, extra)) {
> -               struct strbuf path = STRBUF_INIT;
> -               strbuf_addstr(&path, git_path("shallow_XXXXXX"));
> -               fd = xmkstemp(path.buf);
> +               strbuf_addstr(&shallow_tmpfile, git_path("shallow_XXXXXX"));
> +               fd = xmkstemp(shallow_tmpfile.buf);
> +
> +               /* XXX can there be multiple shallow tempfiles in one program?
> +                * In that case, we would need to maintain a list */
> +               atexit(remove_shallow_tmpfile);
> +               sigchain_push_common(remove_shallow_tmpfile_on_signal);
> +
>                 if (write_in_full(fd, sb.buf, sb.len) != sb.len)
>                         die_errno("failed to write to %s",
> -                                 path.buf);
> +                                 shallow_tmpfile.buf);
>                 close(fd);
>                 strbuf_release(&sb);
> -               return strbuf_detach(&path, NULL);
> +               return shallow_tmpfile.buf;
>         }
>         /*
>          * is_repository_shallow() sees empty string as "no shallow
>          * file".
>          */
> -       return xstrdup("");
> +       return shallow_tmpfile.buf;
>  }
>
>  void setup_alternate_shallow(struct lock_file *shallow_lock,
> diff --git a/upload-pack.c b/upload-pack.c
> index 0c44f6b..55c1f71 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -242,11 +242,6 @@ static void create_pack_file(void)
>                 error("git upload-pack: git-pack-objects died with error.");
>                 goto fail;
>         }
> -       if (shallow_file) {
> -               if (*shallow_file)
> -                       unlink(shallow_file);
> -               free(shallow_file);
> -       }
>
>         /* flush the data */
>         if (0 <= buffered) {