[PATCH 2 of 5 flagprocessor v6] revlog: add 'raw' argument to revision and _addrevision
raf at durin42.com
Fri Dec 30 14:46:27 EST 2016
On Dec 30, 2016 12:30, "Pierre-Yves David" <pierre-yves.david at ens-lyon.org> wrote:
> On 12/30/2016 06:09 PM, Augie Fackler wrote:
>> On Dec 30, 2016, at 12:04 PM, Rémi Chaintron <remi.chaintron at gmail.com> wrote:
>>> Following discussion with marmoute on IRC, I'll drop the rawrevision() method for now.
>> OOC, what’s the rationale for preferring the boolean parameter instead of making the code structure more self-documenting? I legitimately don’t see why the parameter version is preferable design-wise.
> What about you check the IRC log so that remi and I can focus on getting this things out? The log are still warm and fresh.
I *did* read the IRC log, which I’ve quoted at the bottom of this message for future reference (since #mercurial isn’t logged, so until now this was only privately archived by our respective clients.)
Now, I just re-read that IRC conversation and still didn't see a *reason*, just that it's your aesthetic opinion and you find my link unconvincing (it's not the best example when compared to the specific case we have here, but instead is more a defense of the general "boolean parameters tend to be a design smell" principle). I'm happy to discuss my reasoning further, but am not happy to hear my design sense discarded because you don't like the look of the resulting API with no engineering reasoning.
I’d like Remi to move along with your design in the name of Facebook making progress and not burning out on your reviews, but in parallel with that I’d like to have this design discussion so that I can better understand why you’re insisting (or preferring? I’m not actually sure based on word choices, but you seem pretty strongly opposed to the two-methods approach) on this particular design.
IRC transcript from today in #mercurial, times America/New_York:
> 11:57 < marmoute> remi17: if rawrevision was a whole complex and different
> things on its own I would get why we had a new method.
> 11:57 < remi17> You and durin42 seem to disagree on what's best here, but a)
> if I get any cycles on this, this will be refactored later
> on anyway (but I'd understand if you do not want to bet on
> my cycles) b) it's not something I'm feeling strongy enough
> about to push either way c) I'd rather coin flip rather than
> having you guys losing time on this specific bit :)
> 11:58 < remi17> marmoute: I completely get your argument, it's 100% code
> style tastes/opinions at that point
> 11:58 < marmoute> But given how similar the codepath(basically just forward
> the boolean far down to some flag processing, I really
> don't see the point (and things from the link Augie
> provided is mostly too far away from the actual case here)
> 11:59 < marmoute> remi17: So, lets drop it (unless you want to actually do
> all that refactoring now ;-) )
> 11:59 < remi17> so if you feel strongly about this, and durin42 is happy
> with waiting for me to take a stab at it later after the
> censor clean up, I'll proceed with raw=True
> 11:59 < remi17> wilco
> 12:00 < marmoute> I'm okay with processing with dropping the method and
> using raw=True for now.
> 12:01 < marmoute> remi17: can you drop a minimal reply to your email saying
> that we'll drop it as per IRC discussion (so that people
> following the email have the context)
> 12:09 < marmoute> remi17: in patch 4, I don't see how the flag will be
> processed with different action on read and on write
> because the two call seems undistinguishable
> 12:12 < remi17> marmoute: the flag is not processed with different actions
> 12:12 < remi17> the order in which flags are processed is reversed between
> read and write operations
> 12:13 < marmoute> Yep, I got that part
> 12:13 < marmoute> But 'lfs' need to do different things on read and on
> write, right ?
> 12:13 < remi17> yeah, that's something we cleared while VC-ing
> 12:14 < marmoute> But I'm confused about the implementation in patch 4.
> 12:14 < marmoute> Because I don't see how any information is passed to allow
> having a different behavior
> 12:15 < remi17> We wrap around addrevision() to add flags and transform the
> contents on local commits
> 12:15 < marmoute> I also know that
> 12:16 < marmoute> remi17: the flag processor is not called at all during
> 'addrevision' ?
> 12:17 < marmoute> (well, _addrevision)
> Quits: remi17
> 12:24 < marmoute> hum _addrevision call processflag, but that is in order to
> retrieve and check the base text.
> 12:24 < marmoute> hum no
> 12:26 < marmoute> That code seems censors related.
> 12:31 < marmoute> hum remi fanished.
> 12:31 < marmoute> s/f/v/
More information about the Mercurial-devel