patchclone: Make use of the strip_suffix() helper method

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 builtin/clone.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

On Thu, Jul 09, 2015 at 03:33:46PM +0000, Sebastian Schuberth wrote:

Yay, always glad to see complicated string handling like this go away. As the resulting conditional blocks are one-liners, I think you can drop the curly braces, which will match our usual style:

  if (is_bundle)
	strip_suffix(start, ".bundle", &len);
  else
	strip_suffix(start, ".git", &len);

If you wanted to get really fancy, I think you could put a ternary operator in the middle of the strip_suffix call. That makes it clear that "len" is set in all code paths, but I think some people find ternary operators unreadable. :)

This one can also be simplified using xstrfmt to:

  if (is_bare)
	dir = xstrfmt("%.*s.git", len, start);

Do we still need to cast "len" to an int to use it with "%.*" (it is defined by the standard as an int, not a size_t)?

-Peff

On Thu, Jul 9, 2015 at 7:00 PM, Jeff King <peff@peff.net> wrote:

I like the idea about the ternary operator, will do.

Nice, will also do.

I think we're more on the safe side by keeping the cast, so I'll do that, too.

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 builtin/clone.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

Sebastian Schuberth <sschuberth@gmail.com> writes:

We seem not to capitalize the first word on the subject line.

Please don't.

This looks vastly nicer than the original.

This however I had to read twice.  I'd say

	if (is_bare)
        	dir = xstrfmt(...);
	else
        	dir = xstrndup(...);

is much easier to read.

On Thu, Jul 9, 2015 at 8:05 PM, Junio C Hamano <gitster@pobox.com> wrote:

Will change that.

This seems to come from submitgit, I've filed an issue about it:

https://github.com/rtyley/submitgit/issues/17

What content type(s) would you accept? Only text/plain?

That's what I had locally before. Will revert to that.

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 builtin/clone.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 builtin/clone.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

Sebastian Schuberth <sschuberth@gmail.com> writes:

The last one needs to be adjusted with s/end - start/len/. The last-minute rewrite without testing shows; your first two patches correctly used "len" ;-)

No need to resend.  Will locally tweak before queuing.

Thanks.

On Thu, Jul 9, 2015 at 11:21 PM, Junio C Hamano <gitster@pobox.com> wrote:

Doh, you're right, sorry for that.

Thanks!

On Thu, 09 Jul 2015 at 20:24:08, Sebastian Schuberth wrote:

I am currently on vacation and cannot bisect or debug this but I am pretty confident that this patch changes the behaviour of directory name guessing. With Git 2.4.6, cloning http://foo.bar/foo.git/ results in a directory named foo and with Git 2.5.0, the resulting directory is called foo.git.

Note how the end variable is decreased when the repository name ends with a slash but that isn't taken into account when simply using strip_suffix() later...

Is this intended?

On Tue, Aug 4, 2015 at 6:34 AM, Lukas Fleischer <lfleischer@lfos.de> wrote:

I did not intend this change in behavior, and I can confirm that reverting my patch restores the original behavior. Thanks for bringing this to my attention, I'll work on a patch.

On Tue, Aug 04, 2015 at 09:31:18AM +0200, Sebastian Schuberth wrote:

I think this regression is in v2.4.8, as well. We should be able to use a running "len" instead of the "end" pointer in the earlier part, and then use strip_suffix_mem later (to strip from our already-reduced length, rather than the full NUL-terminated string). Like this:

On Tue, Aug 04, 2015 at 06:42:46PM -0400, Jeff King wrote:

[snip]

It shouldn't be hard rebasing my work onto this. If it's being applied I'll come up with a new version.

Patrick

On Wed, Aug 05, 2015 at 08:08:52AM +0200, Patrick Steinhardt wrote:

Thanks, it is always nice when contributors are flexible and easy to work with. :)

Hopefully the new tests I've added can help you out, as well.

-Peff

On Wed, Aug 05, 2015 at 04:41:48AM -0400, Jeff King wrote:

You're welcome. And yes, your tests help me quite a lot here. Got tedious to always set up the chroot. Guess I'll still send my fixes for the chroot-tests as a separate patch series, even though I don't require them anymore.

Short question on how to proceed: should I mention that my patch series builds upon your patches or just include them in my series?

Patrick

On Wed, Aug 05, 2015 at 11:06:03AM +0200, Patrick Steinhardt wrote:

Yeah, fixes for t1509 are definitely welcome.

I think we'll want to merge my patches separately, due to the regression, so you should not include them. So hopefully the sequence is:

  1. Junio picks them up as jk/guess-repo-name-regression or similar.

  2. They are merged to 'next', and then eventually to 'maint'.

  3. Mention the topic branch name (whatever Junio ends up picking for
     it) when you post your patches. Junio can then apply on top in his
     repo.

  4. If re-rolls keep going past step 2, then it becomes a non-issue, as
     'maint' gets merged to 'master'.

-Peff

On Tue, Aug 04, 2015 at 06:42:46PM -0400, Jeff King wrote:

Looks like "git clone --bare host:foo/.git" is broken, too. I've added some tests to cover the recently broken cases, as well as some obvious normal cases (which the patch I sent earlier break!). And as a bonus, we can easily cover Patrick's root-repo problems (so people will actually run the tests, unlike the stuff in t1509. :) ).

This is crap, of course. Our "len" variable is computed from the start of "repo", of which "start" is a subset. So we are indexing way out of bounds here.

As it turns out, this actually makes things simpler. We can stop using "len" entirely in the early part, and leave it as-is with pointer math (the patch I sent earlier did not really make anything simpler, anyway). And then we can just compute the length of "start" here, minus everything we've stripped off the end (i.e., "len = end - start").

Here are the patches.

  [1/2]: clone: add tests for output directory
  [2/2]: clone: use computed length in guess_dir_name

-Peff

When we run "git clone $url", clone guesses from the $url what to name the local output directory. We don't have any test coverage of this, so let's add some basic tests.

This reveals a few problems:

  - cloning "foo.git/" does not properly remove the ".git";
    this is a recent regression from 7e837c6 (clone:
    simplify string handling in guess_dir_name(), 2015-07-09)

  - likewise, cloning foo/.git does not seem to handle the
    bare case (we should end up in foo.git, but we try to
    use foo/.git on the local end), which also comes from
    7e837c6.

  - cloning the root is not very smart about URL parsing,
    and usernames and port numbers may end up in the
    directory name

All of these tests are marked as failures.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5603-clone-dirname.sh | 69 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100755 t/t5603-clone-dirname.sh

Commit 7e837c6 (clone: simplify string handling in guess_dir_name(), 2015-07-09) changed clone to use strip_suffix instead of hand-rolled pointer manipulation. However, strip_suffix will strip from the end of a NUL-terminated string, and we may have already stripped some characters (like directory separators, or "/.git"). This leads to commands like:

  git clone host:foo.git/

failing to strip the ".git".

We must instead convert our pointer arithmetic into a computed length and feed that to strip_suffix_mem, which will then reduce the length further for us.

It would be nicer if we could drop the pointer manipulation entirely, and just continually strip using strip_suffix. But that doesn't quite work for two reasons:

  1. The early suffixes we're stripping are not constant; we
     need to look for is_dir_sep, which could be one of
     several characters.

  2. Mid-way through the stripping we compute the pointer
     "start", which shows us the beginning of the pathname.
     Which really give us two lengths to work with: the
     offset from the start of the string, and from the start
     of the path. By using pointers for the early part, we
     can just compute the length from "start" when we need
     it.

Signed-off-by: Jeff King <peff@peff.net>
---
I suspect you _could_ clean up this logic further, but I
really wanted to do the minimal fix for the regression.
Especially because Patrick is hopefully going to sweep
through and make it all more robust soon enough. :)

 builtin/clone.c          | 3 ++-
 t/t5603-clone-dirname.sh | 6 +++---
 2 files changed, 5 insertions(+), 4 deletions(-)

On 8/5/2015 10:39, Jeff King wrote:

Thanks a lot Peff for fixing my bugs, I should have known that you'll be able to come up with something much sooner than I would ;-)

This all looks good to me!

Regards, Sebastian

Jeff King <peff@peff.net> writes:

Sorry, my fault; I should have been much less trusting while queuing a patch like that offending one that was meant to be a no-op.

Thanks.

On Wed, Aug 05, 2015 at 10:19:56AM -0700, Junio C Hamano wrote:

I reviewed it, too. :-/

I actually did give some thought to that while working on the fix. Why did we miss what in retrospect was a pretty obvious bug? I saw two interesting bits:

  1. From the diff context, it looked like a perfectly reasonable
     change; the shrinking of the "end" pointer happened further up
     in the function.

     So I guess the lesson is not to trust reading just the diff, and
     to really read the whole of the modified function. But that's easy
     to say in retrospect; most of the time the bits outside the context
     aren't interesting, and we can't afford to read the whole code
     base for each patch. It's a judgement call where to stop looking at
     the surrounding context of a given change (e.g., the function, the
     callers, their callers, etc).

  2. We didn't have any test coverage in this area; when I wrote even
     basic tests, it caught the problem.

     I hate to set a rule like "if you are cleaning something up, make
     sure there is decent test coverage". Lots of trivial-looking
     patches really are trivial, and it doesn't make sense to insist the
     submitter add a new battery of tests.

So I dunno. This was definitely preventable, but that is all in retrospect. Bugs will happen, and we usually catch them while cooking. The biggest pain is that this slipped through to a release, and that may just be a measure of how few people were impacted (the cases it affected were relatively obscure).

-Peff

Sebastian Schuberth <sschuberth@gmail.com> writes:

I could take anything, even chicken scratches on a piece of paper, for a small change like this.

But let's make sure we see text/plain out of "submitgit", as the whole point of it is to allow people generate the common denominator format out of a commit pushed to GitHub.

Thanks for letting them know.