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

[Calvin Wan <calvinwan@google.com>]

"Robert Coup via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 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!