Jan Engelhardt (jengelh@inai.de) wrote:
On Saturday 2014-01-11 17:32, Adam Spiers wrote:
are available in the git repository at: git://git.inai.de/obs-service-tar_scm master for you to fetch changes up to 3c38cd32941dfc51ac1245c9de91fd63e29969fe: tar_scm: support retrieving the git tag offset (2014-01-11 16:26:46 +0100)
However, please can you submit this to a fork on github,
I don't use github, sorry.
This is not a personal choice, it's how the project operates. github and Travis are an integral part of the development workflow for tar_scm (and many other openSUSE projects). I can't imagine why you wouldn't use github, but if you don't then you'll find it a lot harder to contribute in general. Personally I will not merge a branch for which I cannot conduct a code review in public, and I would encourage all the other maintainers to adopt the same stance, because transparent peer review is a Very Good Thing. Suitable vehicles for public review include github and gerrit (where available), and git format-patch for an email-based workflow. However in the tar_scm case, it has to be github because we already have it tied to Travis for automated test runs.
- It's missing a corresponding addition to the test suite. That's a show-stopper ;-)
Surely you can address that, because I don't write python a lot either.
I could but I don't have time. Besides, the person who writes the code should also write the tests. It's called Test-Driven Development.
- I don't think "@REVISION@" is the best choice of placeholder name; it's too misleading. Something like @TAG_OFFSET@ would be better.
I would have picked %r because that already gets you a number with svn/hg, but %r is prone to clash with git itself in the future, so *throws up hands in the air*.
Like I said, @TAG_OFFSET@ or similar is a big improvement.
- In general I agree that shell variables could be quoted during expansion. But quoting variables like $? and $# is overkill, since they will never be the empty string. Furthermore, in bash the quotes are redundant when you are using the double [[ ]] parentheses:
Alas, the original programmer did not think of using [[ ]] everywhere. So, safest option: [[ and ".
No, the logical conclusion is to use either a) [ ] and "", or b) [[ ]] and no "". Using "" with [[ ]] is redundant and unnecessarily noisy. I agree that consistent use of either [ ] or [[ ]] rather than a mixture of both would be nice, but that's totally orthogonal to the discussion of quoting.
Specifying the basetag also has the advantage that @REVISION@ does not go back to 0 without the bspkg maintainer noticing, which is also a property that %ct seemed to hold in case a new git tag (e.g. v0.3.3) was created while _service was left at versionformat=0.3.2.something.
Ah OK, you've changed your design! Yes, this is *much* better than some fluffy git describe --match=GLOB logic; finally it addresses the main flaw in your scheme.
I would not call it a flaw.
It was a flaw for the reasons which I have already stated several times - in short, it was too brittle due to the possibility of new tags being added to the upstream project. However, your new idea of fixing the tag string completely addresses that, which I'm very happy about :)
There's still a small risk that upstream repositions the tag in question,
That is (even more) haphazard than repositioning a branch head, because tags - unlike branch heads - don't get force-updated when a client runs git-fetch.
Agreed! -- To unsubscribe, e-mail: opensuse-packaging+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-packaging+owner@opensuse.org