From: Kaartic Sivaraam Date: Thu, 16 Nov 2017 17:02:42 GMT Subject: Re: [PATCH 3/4] branch: correctly reject refs/heads/{-dash,HEAD} Message-ID: In-Reply-To: On Thursday 16 November 2017 08:27 PM, Junio C Hamano wrote: > Kaartic Sivaraam writes: > >> I guess this series is not yet ready for 'next'. When I tried to apply >> this patch it doesn't seem to be applying cleanly. I get some >> conflicts in 'sha1_name.c' possibly as a consequence of the changes to >> the file that aren't accounted by the patch. > > Oh, it is totally expected that this patch (and others) may not > apply on 'next' without conflict resolution, as this topic, as all > the other topics, are designed to apply cleanly to either 'master' > or 'maint' or one of the older 'maint-*' series, if it is a bugfix > topic. A patch series that only applies cleanly to 'next' would be > useless---it would mean all the topics that are already in 'next' > that interacts with it must graduate first before it can go in. Thanks for the explanation. Seems I still have to become accustomed to the workflow. > Make it a habit to build on 'master' or older and then merge to > higher integration branches to make sure it fits with others. > Though I don't clearly understand how to do that, I'll let experience teach me the same (if possible). :-) > What you could do is to inspect origin/pu branch after you fetch, > and look at the commit that merges this topic to learn how the > conflicts are resolved (the contrib/rerere-train.sh script may help > this process). > Sure thing. >>> + if (*name == '-' || >>> + !strcmp(sb->buf, "refs/heads/HEAD")) >> >> I guess this check should be made more consistent. Possibly either of, > > Among these two, this one > >> if (starts_with(sb->buf, "refs/heads/-") || >> !strcmp(sb->buf, "refs/heads/HEAD")) > > has more chance to be correct. Also, if we were to check the result > of expansion in sb->buf[], I would think that we should keep a > separate variable that points at &sb->buf[11] and compare the > remainder against fixed strings, as we already know sb->buf[] starts > with "refs/heads/" due to our splicing in the fixed string. > > Because the point of using strbuf_branchname() is to allow us expand > funny notations like @{-1} to refs/heads/foo, and the result of > expansion is what eventually matters, checking name[] is wrong, I > would think, even though I haven't thought things through. > > In any case, I would say thinking this part through should be left > as a theme for a follow-on patch, and not within the scope of this > one. After all, checking *name against '-' was part of the original > code, so it is safer to keep the thing we are not touching bug-to-bug > compatible and fixing things one step at a time (the one fix we made > with this patch is to make sure we store refs/heads/-dash in sb when > we reject name=="-dash"). > Good point. This is better for a follow-up patch as there's a possibility that we might be introducing new bugs which weren't present previously as a consequence of changing that conditional (bug-to-bug compatibility, good term that (possibly) summarizes my long-winded explanation ;-)