public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Junio C Hamano <gitster@pobox•com>
To: Ray Donnelly <mingw.android@gmail•com>
Cc: git@vger•kernel.org, Johannes Schindelin <johannes.schindelin@gmx•de>
Subject: Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption
Date: Sun, 04 Oct 2015 10:21:08 -0700	[thread overview]
Message-ID: <xmqqwpv21rej.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <CAOYw7dv4iPQ4cq4Ab1ZeThrp=u51T5v387a1Y8QPO-yj=fyMcg@mail.gmail.com> (Ray Donnelly's message of "Sun, 4 Oct 2015 15:51:22 +0100")

Ray Donnelly <mingw.android@gmail•com> writes:

>> Some callers of this function in real code (i.e. not the one you are
>> removing the check) do seem to depend on that condition, e.g. the
>> codepath in clone that leads to add_to_alternates_file() wants to
>> make sure it does not add an duplicate, so it may end up not noticing
>> /foo/bar and /foo/bar/ are the same thing, no?  There may be others.
>
> Enforcing that normalize_path_copy() removes any trailing '/' (apart
> from the root directory) breaks other things that assume it doesn't
> mess with trailing '/'s, for example filtering in ls-tree. Any
> suggestions for what to do about this? Would a flag be appropriate as
> to whether to do this part or not? Though I'll admit I don't like the
> idea of adding flags to modify the behavior of something that's meant
> to "normalize" something. Alternatively, I could go through all the
> breakages and try to fix them up?

I agree with you that "normalize" should "normalize".  Making sure
that all the callers expect the same kind of normalization would be
a lot of work but I do think that is the best approach in the long
run.  Thanks for the ls-tree example, by the way, did you find it by
code inspection?  I do not think it is reasonable to expect the test
coverage for this to be 100%, so the "try to fix them up" would have
to involve a lot of manual work both in fixing and reviewing,
unfortunately.

The first step of the "best approach" would be to make a note on
normalize_path_copy() by adding a NEEDSWORK: comment to describe the
situation.

Thanks.

next prev parent reply	other threads:[~2015-10-04 17:21 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-10-03 12:44 [PATCH 1/2] test-path-utils.c: remove incorrect assumption Ray Donnelly
2015-10-03 15:38 ` Ray Donnelly
2015-10-03 17:13 ` Junio C Hamano
2015-10-04 14:51   ` Ray Donnelly
2015-10-04 17:21     ` Junio C Hamano [this message]
2015-10-04 23:36       ` Ray Donnelly
2015-10-08 20:42         ` Ray Donnelly
2015-10-09  1:05           ` Junio C Hamano
2015-10-09 10:12             ` Ray Donnelly

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqwpv21rej.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox$(echo .)com \
    --cc=git@vger$(echo .)kernel.org \
    --cc=johannes.schindelin@gmx$(echo .)de \
    --cc=mingw.android@gmail$(echo .)com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox