Re: [PATCH v3 0/7] fetch: add repair: full refetch without negotiation (was: "refiltering")

From: Robert Coup <robert@coup.net.nz>
Date: Mar 9, 2022, 09:57 UTC
Message-ID: <CACf-nVeEBDQse0coA7QpQmQ92y9kDwXoTmayD8_NY2OHNZ5v+g@mail.gmail.com>
In-Reply-To: <20220309002729.3581315-1-calvinwan@google.com>

Hi Calvin,

On Wed, 9 Mar 2022 at 00:27, Calvin Wan <calvinwan@google.com> wrote:

>
> This is my first time sending a review to the list, so forgive me for any
> mistakes I make or conventions missed. Feedback about my review would be well
> appreciated!

I'm as new as you to contributing to Git :-)

> Overall I think your patch is well written and the implementation accomplishes
> what you describe in your cover letter,

Thanks! I think if you're completely content you can add your Reviewed-By as described at https://git-scm.com/docs/SubmittingPatches#commit-trailers

Show 7 quoted lines

> I would like to discuss another
> possible design I thought of. Currently, the user has to know to run ‘--refetch’
> after changing the partial clone filter configuration in order to fetch the
> commits that match the new filter. Ideally I believe this behavior should be
> default so therefore instead of adding an option, if git knew what filter was
> last used in the fetch, it could automatically ‘refetch’ everything if there is
> a change between the last used filter and the default filter.

So, if you do a partial clone using `git clone --filter=...` then the filter is saved into the config at `remote.<name>.partialclonefilter` and is re-used by default for subsequent fetches from that remote. But there's nothing to stop `git fetch --filter=...` being run multiple times with different filters to carefully setup a repository for a particular use case, or any notion that there has to be "one" filter in place for a remote.

Running `git fetch --filter=...` doesn't update the remote's partial clone filter in the config, and IMO it shouldn't for the above reason.

I think having `git config remote.<name>.partialclonefilter <new-filter>` print out something to the user along the lines of "Your change to/removal of the filter won't fetch in additional objects associated with existing commits, you can do this with `fetch --refetch <remote>`" could be helpful, but after a very quick look I can't see anything like that at the moment for other config settings (ie. no plumbing in place to easily reuse), and I'm not motivated to add such plumbing.

Cheers,

Rob :)