archive mirror
 help / color / mirror / Atom feed
From: Jonathan Tan <>
Subject: Re: Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones?
Date: Wed, 15 Apr 2020 19:05:09 -0700	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <20200414235057.GA6863@syl.local>

> Here's a patch that I didn't sign-off on that fixes the problem for me.
> --- >8 ---
> Subject: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate
> In bd0b42aed3 (fetch-pack: do not take shallow lock unnecessarily,
> 2019-01-10), the author noted that 'is_repository_shallow' produces
> visible side-effect(s) by setting 'is_shallow' and 'shallow_stat'.

Thanks for the reference to my commit. When I wrote that, if I remember
correctly, I found it difficult to be able to guarantee that the
clearing of is_shallow and shallow_stat was performed upon every
commit-lock (which creates a new shallow file) and
<unlink+rollback-lock> (which removes the shallow file, potentially
changing the repo state from shallow to none), so I added the NEEDSWORK
instead. I see that this patch makes some progress in solving that.

> @@ -1630,6 +1630,7 @@ static void update_shallow(struct fetch_pack_args *args,
>  		if (*alternate_shallow_file == '\0') { /* --unshallow */
>  			unlink_or_warn(git_path_shallow(the_repository));
>  			rollback_lock_file(&shallow_lock);
> +			reset_repository_shallow(the_repository);
>  		} else
>  			commit_lock_file(&shallow_lock);
>  		alternate_shallow_file = NULL;

Here, do you need a reset in the "else" branch as well? (I.e. in both
branches, so you can put it outside the "if" block.) I didn't look at it
too closely, but I would think that when the shallow_lock file is
committed, there might have been no shallow beforehand, changing the
state from no-shallow to shallow.

> @@ -411,6 +411,7 @@ void prune_shallow(unsigned options)
>  			die_errno("failed to write to %s",
>  				  get_lock_file_path(&shallow_lock));
>  		commit_lock_file(&shallow_lock);
> +		reset_repository_shallow(the_repository);
>  	} else {
>  		unlink(git_path_shallow(the_repository));
>  		rollback_lock_file(&shallow_lock);

And the opposite case here - reset in the "else" branch because the
state could have changed from shallow to no-shallow.

In any case, I think the commit message should discuss why
reset_repository_shallow() is added only on the unlink+rollback side in
one "if" statement, but only on the opposite "commit" side in another
"if" statement.

  parent reply	other threads:[~2020-04-16  2:05 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-14 20:22 Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones? Elijah Newren
2020-04-14 20:31 ` Taylor Blau
2020-04-14 20:31 ` Derrick Stolee
2020-04-14 23:50   ` Taylor Blau
2020-04-15  0:07     ` Taylor Blau
2020-04-15 11:55     ` Derrick Stolee
2020-04-15 15:55       ` Taylor Blau
2020-04-15 18:07       ` Elijah Newren
2020-04-16  2:05     ` Jonathan Tan [this message]
2020-04-15 20:54 ` Jonathan Nieder
2020-04-15 22:54   ` Elijah Newren
2020-04-16  0:47     ` Taylor Blau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).