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

From: Calvin Wan <calvinwan@google.com>
Date: Mar 9, 2022, 00:27 UTC
Message-ID: <20220309002729.3581315-1-calvinwan@google.com>
In-Reply-To: <pull.1138.v3.git.1646406274.gitgitgadget@gmail.com>

"Robert Coup via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 11 quoted lines

> If a filter is changed on a partial clone repository, for example from
> blob:none to blob:limit=1m, there is currently no straightforward way to
> bulk-refetch the objects that match the new filter for existing local
> commits. This is because the client will report commits as "have" during
> fetch negotiation and any dependent objects won't be included in the
> transferred pack. Another use case is discussed at [1].
> 
> This patch series introduces a --refetch option to fetch & fetch-pack to
> enable doing a full fetch without performing any commit negotiation with the
> remote, as a fresh clone does. It builds upon cbe566a071 ("negotiator/noop:
> add noop fetch negotiator", 2020-08-18).

Hi Robert,

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!

Overall I think your patch is well written and the implementation accomplishes what you describe in your cover letter, however, 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. I’m not sure if the config is the best location to store the last used filter, but we can use it as an example for now. The tradeoff here is balancing between having an additional config variable and having the user know to specify a parameter to fetch after changing the config. And in the future, if there are other use cases for needing a fetch that bypasses commit negotiation (I know you described one such possible use case in v2), the ‘--refetch’ option can easily be readded to hook into this patch.

Looking forward to hearing your thoughts!