patchdisallow refs containing successive slashes

When creating branches using names starting with '/' or containing a '//', a leading slash would just vanish and successive slashes were 'compressed' into just one slash.

This behaviour comes from the refs being stored as files in subdirectories where the filesystem compresses each '//' in a pathname to a '/' . So disallowing refs to contain '//' fixes these problems and prohibits branch names to start with a '/' or to contain '//'.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---

I became aware of this issue while looking into problems occuring when a user created a branch starting with a '/' in git gui (e.g. "/foo"). Strange things happen, while git gui shows the current branch as "/foo" under the hood a branch "foo" (without the slash) had been created. But then you can't delete "/foo" from git gui, because a branch of that name doesn't exist.

IMHO branch names should not be allowed to start with a '/' or contain a '//' (branches with trailing slashes are not allowed since commit 03feddd6). So this implies that refs should never contain a '//' in the first place.

This behaviour has been changed the last time by commit 03feddd. The version before that commit didn't allow '//' to be part of a ref (but still allowed a trailing '/'). The newer version explicitly states "/* tolerate duplicated slashes */" in the added code. But i couldn't find any hints on the list or on the interweb *why* these were allowed. So this patch might break something i can't see right now, even though the test suite runs fine ...

Thoughts? Objections?

 Documentation/git-check-ref-format.txt |    2 ++
 refs.c                                 |    7 +++++--
 t/t1600-check-ref-format.sh            |   19 +++++++++++++++++++
 3 files changed, 26 insertions(+), 2 deletions(-)
 create mode 100755 t/t1600-check-ref-format.sh

Jens Lehmann <Jens.Lehmann@web.de> writes:

Hmm.  We already do that without your patch.

    $ git branch /foo//bar
    $ git for-each-ref --format='%(refname)'
    refs/heads/foo/bar
    refs/heads/master
    $ git branch -d /foo//bar
    Deleted branch /foo//bar (was deadbeef)
    $ git for-each-ref --format='%(refname)'
    refs/heads/master

Perhaps an interface to give a cleaned-up version, e.g.

    $ git check-ref-format --print refs/heads//foo/bar
    refs/heads/foo/bar

is what you want in order to fix git-gui?  I dunno.

Junio C Hamano schrieb:

Yes, but git changes the name silently from '/foo//bar' to 'foo/bar'!

The automagical removal of some slashes leads to strange behaviour when using such names:

    $ git checkout -b foo/bar
    Switched to a new branch 'foo/bar'
    $ git checkout -b /foo//bar
    fatal: A branch named '/foo//bar' already exists.
    $ git for-each-ref --format='%(refname)'
    refs/heads/foo/bar
    refs/heads/master

git claims wrongly that "A branch named '/foo//bar' already exists.".

And that same happens to tag and repo names, leading to other strange effects:

    $ git tag /foo//bar
    $ git tag
    foo/bar

That is not the tagname the user provided.

    $ git remote add /foo//bar git://git.kernel.org/pub/scm/git/git.git
    $ git remote show
    warning: Config remote shorthand cannot begin with '/': /foo//bar.url
    warning: Config remote shorthand cannot begin with '/': /foo//bar.fetch
    $ git fetch /foo//bar
    warning: Config remote shorthand cannot begin with '/': /foo//bar.url
    warning: Config remote shorthand cannot begin with '/': /foo//bar.fetch
    fatal: 'foo/bar' does not appear to be a git repository
    fatal: The remote end hung up unexpectedly

Note: git fetch uses both versions of the remote name in its output.

Yes, one solution could be to fix every application handling branch, tag or repo names to mimic the namechange done in the bowels of git. But i think it is not worth the hassle. Every application i tested (git gui, gitk and a handful of git commands) just assumes - and i think rightfully so - that refnames will not change while they are being created (And they do call "git check-ref-format" or strbuf_check_branch_ref() and friends to make sure they have a valid refname, but it gets changed nonetheless).

With this patch you get an error every time you try to create such a refname in the first place and the strange effects and the ambiguity of refnames ('/foo//bar' is a synonym for 'foo/bar' right now) just go away.

The motivation for this patch was a confused user at my dayjob. IMHO most people don't use '/foo//bar' on purpose but the extra slashes get there through typos, copy & paste errors and such. Then generating an error might just be the Right Thing to do to avoid the problems with the changed refname afterwards.

Jens Lehmann <Jens.Lehmann@web.de> writes:

That cuts both ways.

When the users make typoes (e.g. /foo//bar) you can accept the only sane correction (e.g. foo/bar) instead of rejecting, since the only thing the user can do after getting such a rejection is to correct it to that corrected name (e.g. foo/bar) himself and re-issue the command anyway. You can push the "hassle" down to the user, or you can fix the tool to remove the hassle from the user.

Besides, by rejecting what we used to accept you are breaking people's expectations. So I am moderately negative, unless you can say your "every application" is literally _tons_.

Junio C Hamano wrote:
 

Yes, making check-ref-format stricter without changing its users to massage their input would be a regression.

The problem Jens described is a git-gui bug. In lib/branch_delete.tcl, line 57, the list of deletable branches is populated as follows:

	foreach h [load_all_heads] {
		if {$h ne $current_branch} {
			$w_heads insert end $h
		}
	}

Since slashes coalesce, a user-supplied new branch name is not canonical and checking "$h eq $current_branch" does not actually check if they are the same branch. git-gui should be using the branch name as output by "git check-ref-format --branch" after the branch is created.

But what about other scripts that assume each branch has only one possible name? Maybe they could be forced to fix up the name early on by making check-ref-format reject names with "//" in them and providing a "git check-ref-format --print" to help.

Upside: scripts would complain loudly instead of failing subtly on input
with extra slashes if they forget to use "git check-ref-format --print"
when appropriate.  Downside: users, including the 22 callers of
check_ref_format() in git, would have to be checked and probably changed
to avoid regressions.

Never having made that typo, I find it hard to consider such a change worth the trouble, but it sounds doable.

Regards, Jonathan

Quoting Junio C Hamano <gitster@pobox.com> writes:

Isn't 1.7.0 a good release to break such people's expectations?

Junio C Hamano wrote:

The following packages do exactly that. I tried to use it to fix git gui, but my Tcl-fu was not up to it, at least for tonight.

It is not obvious to me that this interface is the one that would be most helpful for git gui: probably a command to just munge the branch name would be more convenient, though less general.

Is the --print option useful? Right now, it seems that its main use is documentation. But in the future, it would be very nice if this command could be used to transform Unicode ref names to some appropriate normalization form, to make Unicode ref names usable in Mac OS X and less confusing everywhere.

I look forward to your thoughts.

Jonathan Nieder (4):
  Add tests for git check-ref-format
  Documentation: describe check-ref-format --branch in more detail
  check-ref-format: add option to print canonical name
  check-ref-format: Simplify --print implementation

 Documentation/git-check-ref-format.txt |   32 ++++++++++++----
 builtin-check-ref-format.c             |   31 ++++++++++++++++
 t/t1402-check-ref-format.sh            |   61 ++++++++++++++++++++++++++++++++
 3 files changed, 116 insertions(+), 8 deletions(-)
 create mode 100644 t/t1402-check-ref-format.sh

The "git check-ref-format" command is a basic command various porcelains rely on. Test its functionality to make sure it does not unintentionally change.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t1402-check-ref-format.sh |   44 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 44 insertions(+), 0 deletions(-)
 create mode 100644 t/t1402-check-ref-format.sh

Unless one already knew, it was not obvious what sort of shorthand "git check-ref-format --branch" expands. Explain it.

The --branch argument is not optional.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/git-check-ref-format.txt |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

Tolerating empty path components in ref names means each ref does not have a unique name. This creates difficulty for porcelains that want to see if two branches are equal. Add a helper associating to each ref a canonical name.

If a user asks a porcelain to create a ref "refs/heads//master", the porcelain can run "git check-ref-format --print refs/heads//master" and only deal with "refs/heads/master" from then on.

In the future, it would be very nice if this command could be modified to transform Unicode ref names to some appropriate normalization form, to make Unicode ref names usable in Mac OS X, too, and less confusing everywhere.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/git-check-ref-format.txt |   25 +++++++++++++++++++------
 builtin-check-ref-format.c             |   10 ++++++++++
 t/t1402-check-ref-format.sh            |   17 +++++++++++++++++
 3 files changed, 46 insertions(+), 6 deletions(-)

Jonathan Nieder <jrnieder@gmail.com> wrote:

What about '/refs/heads/foo'?  Shouldn't that drop the leading /?

I actually had someone enter that into Gerrit Code Review once, exposing a bug I have yet to fix that permits that as a valid branch name. *sigh*

FWIW, I think this is heading in the right direction. Rather than teaching the UIs how clean up a name, give us a tool to do the normalization and validation, and let us call it when we get user input.

"Shawn O. Pearce" <spearce@spearce.org> writes:

I understand that you prefer the latter between "there is no tool; the caller is responsibile to make sure it feeds us canonical representation" and "there is a tool that makes a slightly malformed string into canonical form for the callers to use before calling us." And that would be my preference between these two as well.

But that is based on the current behaviour of accepting slightly malformed and silently making it canonical. If we throw a third alternative, Jonathan's original patch, that did "we reject malformed string", in the mix, what would be your preference?

I moderately favour the "tool to canonicalize is given, and it would be a bug for the caller not to use it" approach this series takes primarily because that approach won't break scripts that do something like this to make a new branch, optionally grouped by the owner (e.g. 'sp/smart-http' or 'next'):

	owner=$1 topic=$2
	branch=$owner/$topic
        git branch "$branch"

This currently works as expected as long as it does not later try to compare for-each-ref output with "refs/heads/$branch" and expects a match. With the third approach, the optional username grouping becomes mandatory for such a script, as 'git branch' will error out. To keep the grouping still optinal, such a script needs to be written perhaps like:

	if test -z "$2"
        then
		branch="$1"
	else
        	branch="$1/$2"
	fi

and that would be a hassle for the scripters, but this _could_ be a kind of backward incompatible tightening we might want to consider for 1.7.0, as somebody suggested earlier.

But now I have spelled this out, I do not see much upside for rejecting, and more importantly, I think it would be an independent issue. We can reject or just keep normalizing silently, and a tool to show the normalized name would be useful and necessary regardless of that.

Junio C Hamano <gitster@pobox.com> wrote:

...

I agree with the last paragraph here, we shouldn't reject, but instead keep the current state but encourage tools to use the new canonical print tool to clean up a name if they want to hang onto the string the user entered and it needs to exactly match for-each-ref sort of output.

Jonathan Nieder <jrnieder@gmail.com> writes:

I do not disagree with a desire to help fixing the unicode insanity on that platform, but I suspect that check-ref-format is a wrong place to tackle the issue. You would need a similar filter for outputs from the likes of ls-files and "diff --name-only", iow, anything that deal with pathnames, no?

It would have be something like "check-ref-format --print | iconv ..." pipeline (conceptually, if not forcing the pipeline to the end users, that is).

Also are people happy with "--print"? I was waiting for others to reword it to "--normalize" or something like that. In your documentation you explain it to "canonicalize", and in your tests you name the output "normalized". I am fine with either wording, but would like to see us using only one, not both.

Thanks.

Junio C Hamano wrote:

GNU iconv does not write the various Unicode normalization forms, so it would have to be something like "check-ref-format --print | charlint ..." instead. Regardless of the filesystem, it seems reasonable to consider é (U+00e9) and é (U+0065 + U+0301) the same character when it appears in a ref name, and one way to achieve this would be to pick one normalization form and stick to it. This does not seem so different from stripping out empty path components.

As a side effect, that would deal with OS X’s strange handling of unicode filenames for .git/refs/*. Now that I think about it, if fighting OS X were the only problem that needed to be solved, I don’t think I’d like this solution so much. The analogous solution to the also unsolved issue of case insensitive filesystems is to force all ref names to lowercase. Do we want to do that? (The case insensitivity issue might not be as bad, since the relevant filesystems will at least _preserve_ the case of filenames in .git/refs. Should we copy them and smudge new ref names to match known ones that differ only in case? Just thinking about the problem makes me cringe.)

Coping with the unicode filename craziness in the working tree is a separate issue, though probably a more important one. I think Linus set up a framework for solving it in <http://thread.gmane.org/gmane.comp.version-control.git/77827>.

Regards, Jonathan

normalize_path_copy() is a complicated function, but most of its functionality will never apply to a ref name that has been checked with check_ref_format().

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin-check-ref-format.c |   25 +++++++++++++++++++++++--
 1 files changed, 23 insertions(+), 2 deletions(-)

Jonathan Nieder wrote:

s/packages/patches/

I better sleep...

Sorry for the confusion, Jonathan