On Tue, Jan 30, 2018 at 05:30:04PM +0100, Patryk Obara wrote: > > I don't have a strong opinion on this, but it does feel a little funny > > to add this extension now, before we quite know what the code that uses > > it is going to look like (or maybe we're farther along there than I > > realize). > > Code using this is already in master - in the result of overwriting > data->hash_algo, every piece of code, that was modernised starts using > the selected hash algorithm (through the_hash_algo) instead of hardcoded > sha-1. Right, that part seems pretty simple. But in the long run, is that going to be enough for the hash transition? My impression is that the transition document is going to require a more nuanced view than "this is the hash algorithm for this repo". Putting code in master is OK; we can always refactor it. But once we add and document a user-facing config option like this, we have to support it forever. So that's really the step I was wondering about: are we sure this is what the user-facing config is going to look like? > > What do we gain by doing this now as opposed to later? By the design of > > the extension code, we should complain on older versions anyway. And by > > doing it now we carry a small risk that it might not give us the > > interface we want, and it will be slightly harder to paper over this > > failed direction. > > Mostly convenience for developers, who want to work on transition. There's > no need to re-compile only for changing default hashing algorithm (which is > useful for testing and debugging). I could carry this patch around to every > NewHash-related branch, that I work on but it's annoying me already ;) OK, that makes some sense to me. Even if we may end up with a more nuanced config later, this is useful for getting the first step done: just making a standalone NewHash repo without worrying about interoperation with existing history. > > I originally wrote it the other way out of an abundance of > > backward-compatibility. After all "extension.*" doesn't mean anything in > > format 0, and somebody _could_ have added such a config key for their > > own purposes. But that's a pretty weak argument, and if we are going to > > start marking some extensions as forbidden there, we might as well do > > them all. > > What about users, who are using new version of Git, but have it > misconfigured with preciousObjects and repo format 0? That's why I decided > to make repo format check specific to objectFormat extension (initially I > made it generic to all extensions). But that's sort of my point. It appears to be working, but the prior-version safety they think they have is not there. I think we're better off erring on the side of caution here, and letting them know forcefully that their config is bogus. > At the same time... there's extension.partialclone in pu and it does not > have check on repo format. IMHO it should (and we should just do it by enforcing it for all extensions automatically). -Peff
