Re: [PATCH v3 3/3] repack -ad: prune the list of shallow commits

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Oct 24, 2018, 08:02 UTC
Message-ID: <nycvar.QRO.7.76.6.1810241001470.4546@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqqin1sov48.fsf@gitster-ct.c.googlers.com>

Hi Junio,

On Wed, 24 Oct 2018, Junio C Hamano wrote:

Show 31 quoted lines

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > diff --git a/builtin/repack.c b/builtin/repack.c
> > index c6a7943d5..9217fc832 100644
> > --- a/builtin/repack.c
> > +++ b/builtin/repack.c
> > @@ -549,6 +549,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> >  		if (!po_args.quiet && isatty(2))
> >  			opts |= PRUNE_PACKED_VERBOSE;
> >  		prune_packed_objects(opts);
> > +
> > +		if (!keep_unreachable &&
> > +		    (!(pack_everything & LOOSEN_UNREACHABLE) ||
> > +		     unpack_unreachable) &&
> > +		    is_repository_shallow(the_repository))
> > +			prune_shallow(0, 1);
> 
> The logic looks correct (and the reasoning behind it, which was
> explained in the log message, was sound).  prune_shallow(0, 1)
> however is not easily understandable.
> 
> There are only two callsites of this function after these three
> patches, and the areas of the code that have these calls are in
> relatively quiescent parts in the whole tree, so it shouldn't be too
> distracting to do an update to make the function take a flag word,
> so that we can make callsites more immediately obvious, i.e.
> 
> 	prune_shallow(PRUNE_SHALLOW_QUICK)
> 
> It of course can be left as a low-hanging fruit loose-end.

I almost did that, but then decided not to clutter the previous patch with this change (and global constant).

Having said that, since you also had this idea, I will make that change. It will read nicer, you are right.

Ciao, Dscho

Show 15 quoted lines

> 
> > diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
> > index 2d0031703..777c9d1dc 100755
> > --- a/t/t5537-fetch-shallow.sh
> > +++ b/t/t5537-fetch-shallow.sh
> > @@ -186,7 +186,7 @@ EOF
> >  	test_cmp expect actual
> >  '
> >  
> > -test_expect_failure '.git/shallow is edited by repack' '
> > +test_expect_success '.git/shallow is edited by repack' '
> >  	git init shallow-server &&
> >  	test_commit -C shallow-server A &&
> >  	test_commit -C shallow-server B &&
>