[PATCH v2 0/2] Fix reference name format check and normalization

[PATCH v2 0/2] Fix reference name format check and normalization

[Michael Haggerty]

This patch series consists of "Do not allow refnames to start with a
slash" as previously submitted (and improved by Junio) and a second
patch that forbids DEL characters in reference names and adds tests of
control characters in reference names.  It should be applied to maint.

Michael Haggerty (2):
  check-ref-format --print: Normalize refnames that start with slashes
  Forbid DEL characters in reference names

 builtin/check-ref-format.c  |    6 +++---
 refs.c                      |    2 +-
 t/t1402-check-ref-format.sh |    9 +++++++++
 3 files changed, 13 insertions(+), 4 deletions(-)

-- 
1.7.6.8.gd2879

[PATCH v2 1/2] check-ref-format --print: Normalize refnames that start with slashes

[Michael Haggerty]

And add tests that such refnames are accepted and normalized
correctly.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---

This patch includes the simplification suggested by Junio on the
mailing list.

 builtin/check-ref-format.c  |    6 +++---
 t/t1402-check-ref-format.sh |    6 ++++++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index ae3f281..0723cf2 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -12,8 +12,8 @@ static const char builtin_check_ref_format_usage[] =
 "   or: git check-ref-format --branch <branchname-shorthand>";
 
 /*
- * Replace each run of adjacent slashes in src with a single slash,
- * and write the result to dst.
+ * Remove leading slashes and replace each run of adjacent slashes in
+ * src with a single slash, and write the result to dst.
  *
  * This function is similar to normalize_path_copy(), but stripped down
  * to meet check_ref_format's simpler needs.
@@ -21,7 +21,7 @@ static const char builtin_check_ref_format_usage[] =
 static void collapse_slashes(char *dst, const char *src)
 {
 	char ch;
-	char prev = '\0';
+	char prev = '/';
 
 	while ((ch = *src++) != '\0') {
 		if (prev == '/' && ch == prev)
diff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh
index 1b0f82f..7563043 100755
--- a/t/t1402-check-ref-format.sh
+++ b/t/t1402-check-ref-format.sh
@@ -18,6 +18,9 @@ invalid_ref 'foo'
 valid_ref 'foo/bar/baz'
 valid_ref 'refs///heads/foo'
 invalid_ref 'heads/foo/'
+valid_ref '/heads/foo'
+valid_ref '///heads/foo'
+invalid_ref '/foo'
 invalid_ref './foo'
 invalid_ref '.refs/foo'
 invalid_ref 'heads/foo..bar'
@@ -70,7 +73,10 @@ invalid_ref_normalized() {
 
 valid_ref_normalized 'heads/foo' 'heads/foo'
 valid_ref_normalized 'refs///heads/foo' 'refs/heads/foo'
+valid_ref_normalized '/heads/foo' 'heads/foo'
+valid_ref_normalized '///heads/foo' 'heads/foo'
 invalid_ref_normalized 'foo'
+invalid_ref_normalized '/foo'
 invalid_ref_normalized 'heads/foo/../bar'
 invalid_ref_normalized 'heads/./foo'
 invalid_ref_normalized 'heads\foo'
-- 
1.7.6.8.gd2879

Re: [PATCH v2 1/2] check-ref-format --print: Normalize refnames that start with slashes

[Michael Haggerty]

On 08/27/2011 06:12 AM, Michael Haggerty wrote:
> And add tests that such refnames are accepted and normalized
> correctly.

...oops, I just noticed that you have committed a this same patch to pu
already, but with a better log message.  Please retain that version.

Patch 2/2, however, is new.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v2 1/2] check-ref-format --print: Normalize refnames that start with slashes

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> On 08/27/2011 06:12 AM, Michael Haggerty wrote:
>> And add tests that such refnames are accepted and normalized
>> correctly.
>
> ...oops, I just noticed that you have committed a this same patch to pu
> already, but with a better log message.  Please retain that version.

Thanks. Very much appreciated.

It sometimes gets frustrating to see a re-rolled submission that ignores
the fix-ups to messages and patches I make locally before queued to 'pu'.

It is easy for me to say that they should fetch 'pu' to see what is queued
before resubmitting, but I've been wondering if there is a better way to
communicate back such differences, so that submitters can easily sanity
check to see if my fix-ups are sensible, and to ensure that the re-rolled
patches do not discard them by mistake before submitting.

I could post what are queued in new topics back to the list as part of
ack, but that would make the list too noisy to read.

git project patch workflow

[Michael Haggerty]

On 08/27/2011 08:30 PM, Junio C Hamano wrote:
> It sometimes gets frustrating to see a re-rolled submission that ignores
> the fix-ups to messages and patches I make locally before queued to 'pu'.
> 
> It is easy for me to say that they should fetch 'pu' to see what is queued
> before resubmitting [...]

I usually try to do this, but it is awkward (or maybe I am doing it
wrong).  If I run git fetch and notice that one of the upstream branches
was updated, then I still have to do a semi-manual step to see whether
any of the new commits are from a patch series that I submitted,
possibly which version of the patch series they are from, and whether
the patch or commit message was edited by you.  Sometimes it is not even
clear which branch you will choose to apply a patch series to (maint vs
master).  Contrast that, for example, with the ease of determining in
github whether a patch series has made it upstream.

In this particular case your email mentioned your code improvement, and
I obviously checked that your change made sense and squashed it onto my
own patch.  But your email didn't mention your changes to the commit
message, so I did not think to look for any.  Given that we, the
unwashed masses, have to transact all of our git patches through the
mailing list, I naively assumed that you would also mention your changes
on the mailing list.

I am amazed at the volume, quality, promptness, and patience of your
feedback (thanks!) and we should all bend over backwards to make your
life easier even at the expense of some extra work for ourselves.  Junio
doesn't scale :-)  So my suggestions below should only be considered if
they do not increase your bureaucratic work at the expense of your
technical work.

> [...] but I've been wondering if there is a better way to
> communicate back such differences, so that submitters can easily sanity
> check to see if my fix-ups are sensible, and to ensure that the re-rolled
> patches do not discard them by mistake before submitting.
> 
> I could post what are queued in new topics back to the list as part of
> ack, but that would make the list too noisy to read.

I personally would find it helpful if you would mention any changes that
you made in an acknowledgment email.  Ideally, the email would also
mention the branch name that you have chosen for the feature branch, to
make it easier to grep for the commit in your repo where it was merged
into pu and in your "What's cooking" messages.  In your case it would be
enough to *mention* that you made a change because we can see the change
itself in your repo.  I wouldn't worry too much about mailing list
traffic, because the acknowledgement would presumably be in the same
thread as the original patch series and thus easily ignorable by people
who are not interested in the subject.


Taking a step back, the real irony is that the git project hardly use
the distributed features of git *for the development of git itself*.  I
was flabbergasted when I first realized that fact.  Why is that?  It
seems that the kind of fork/pull request/merge workflow that github
makes so easy would be more convenient (and if not, the dogfooding would
force us to *make* it more convenient).  I personally found it vastly
easier to contribute my first patch to a github-based project than to git.

What would it take to make a git-centered workflow meet all of the
requirements that are satisfied by the mailing-list centered workflow
that is used currently?

ISTM that if there were a pull request procedure that automatically sent
PATCH emails to the mailing list for review (including the
non-log-message comments), then we would already be 95% of the way
there.  The goal would be to make git the definitive store of
information, and the patch emails a side-product.  That's the real
reason I started the thread "'git format-patch' should get more
information out of git": if all necessary information were in git then
there would be no need for all changes to be serialized through the
mailing list.

1. Author creates v1 of patch series
2. Author pushes changes to any publicly-visible git repo, including
non-log-message comments as notes
3. Author creates v1 tag and submits pull request, which results in
emails being sent to the mailing list as now (but with links to the git
repo and tag containing the patches)
4. Reviewers submit feedback via the mailing list.  They can either work
directly with the patch emails or can fetch the patches directly from
the author.  The feedback is handled informally as now, but might
include links to changed versions in their own public repo).
5. Author responds to feedback via the mailing list as now.  Goto 1 if
needed, creating tags for v2, v3 etc. of the patch series.
6. When satisfied, maintainer pulls branch to shared repo.  *The
author's commits retain their SHA1s.*

Pie in the sky?

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v2 1/2] check-ref-format --print: Normalize refnames that start with slashes

[Jeff King]

On Sat, Aug 27, 2011 at 11:30:26AM -0700, Junio C Hamano wrote:

> It sometimes gets frustrating to see a re-rolled submission that ignores
> the fix-ups to messages and patches I make locally before queued to 'pu'.
> 
> It is easy for me to say that they should fetch 'pu' to see what is queued
> before resubmitting, but I've been wondering if there is a better way to
> communicate back such differences, so that submitters can easily sanity
> check to see if my fix-ups are sensible, and to ensure that the re-rolled
> patches do not discard them by mistake before submitting.

FWIW, I never do this, nor did I realize I was expected to. It takes
effort on the part of the submitter, which means they're probably not
inclined to do so unless there are changes that they know are pending.
I.e., if I see changes as replies on the list, I grab them and put them
into my re-roll. But I am not in the habit of poking through pu, trying
to match up equivalent patches, and then comparing them against my
versions, just on the off chance that some change has been made that
wasn't mentioned on the list. I always assumed that you did one of:

  1. Comment on the patch via email, just as any other reviewer, so it
     can go into the re-roll.

  2. Fix-up the patch or commit message during "am", with the assumption
     that it is ready to be merged at least to "next", at which point
     re-rolls are no longer OK, anyway.

I mentioned "it takes effort" above. I don't mean "submitters shouldn't
be expected to put in extra effort". But we should make sure the effort
is well-spent, which means:

  1. Giving them some indication that you tweaked things during
     application. It doesn't have to be an inclusive list. Even saying
     "Thanks, applied with some spelling fixes" instead of your usual
     "Thanks" is enough (actually, I think you frequently do so
     already).

  2. Having better tool support for picking out the topics. Right now
     we don't know the name of the topic branch you choose without
     hunting for it in pu (or seeing it later in a What's Cooking
     message). And even if we do, picking the tip commit out of pu
     requires a bit of scripting. Have you considered publishing the
     tips of topic branches you apply? Probably it makes sense to keep
     them out of refs/heads/ in git.git, but even having them available
     in refs/topics/ would allow interested parties to fetch them.

  3. Having better tool support for comparing two sets of commits. The
     ideal interface (to me) for this workflow would be something like:

         $ git compare-series my-topic origin/my-topic origin
         Patch 1: first patch subject...OK
         Patch 2: second patch subject...

         diff --git a/hello.c b/hello.c
         index cef8b34..4f08083 100644
         --- a/hello.c
         +++ b/hello.c
         @@ -1,6 +1,6 @@
          #include <stdio.h>
          int main(void)
          {
         -  printf("hello wrold\n!");
         +  printf("hello world\n!");
            return 0;
          }
         Accept change from upstream [y,n,q]?

         Patch 3: third patch subject...

         diff --git a/COMMIT_MSG b/COMMIT_MSG
         index 54c8fa2..fd7b9be 100644
         --- a/COMMIT_MSG
         +++ b/COMMIT_MSG
         @@ -1,3 +1,3 @@
          third patch subject

         -This patch has a commit message with a tpyo in it.
         +This patch has a commit message with a typo in it.

         Accept commit message update from upstream [y,n,q]?

     where the implementation would be something like:

       a. Get two series of commits as $3..$1 and $3..$2.

       b. Try to match commits from series one to series two, ending up
          with some ordered list of pairs like the one below (entries on
          the left would be commit sha1s from series 1; entries on the
          right would be commit sha1s from series 2).

            (P1, P1)  (an unmodified version of a patch)
            (P2, P2') (a modified version of a patch)
            (P3, )    (dropped P3 in series 2)
            (, P4)    (added P4 in series 2)

          The matching would probably involve some text similarity
          analysis of the commit messages (or possibly the patch
          itself, though that can get confused if an early patch is
          tweaked with a change that cascades through the series).

       c. Do a sort of interactive rebase over this list. For unmodified
          pairs, take the commit from either. For modified pairs,
          checkout the commit from series 1, then "checkout -p" the
          commit from series 2 on top of it. The resulting commit is
          then applied to the intermediate rebase result. For modified
          commit messages, do a "git add -p" in a one-off sub-repository
          with only COMMIT_MSG in it, and then use the result as the
          final commit message. For dropped patches, show the patch from
          series 1 and say "Drop this patch?" For added patches, do the
          same (but with "Add this patch?").

          I'm not sure how reordering of patches would be handled, if at
          all. Maybe just as a deletion and an addition.

Anyway, that's just an idea I had while writing this message. I wouldn't
be surprised if there are a ton of awful corner cases I didn't think
about, or that somebody has a much better way of accomplishing the same
thing.

I often end up with something close to this by rebasing my topic
branches on top of master. Once your version hits master, then I see
your changes as conflicts. It's a bit annoying, though, because the
conflicts are frequently annoying to resolve. E.g., you fix a typo in
the line, and then every patch in the series after that which modified
the same line (or even a nearby one) ends up conflicting.

-Peff

Re: [PATCH v2 1/2] check-ref-format --print: Normalize refnames that start with slashes

[Jeff King]

On Mon, Aug 29, 2011 at 02:50:12PM -0400, Jeff King wrote:

>   3. Having better tool support for comparing two sets of commits. The
>      ideal interface (to me) for this workflow would be something like:

BTW, this discussion is obviously about comparing what was applied
upstream with what was submitted, but I think such a tool could have
other purposes, too. It should be able to provide a much nicer interdiff
between two versions of the same series (e.g., for reviewers looking at
a re-roll). You could even use it as maintainer to apply a re-roll,
giving you a chance to review the differences as you apply.

-Peff

Re: [PATCH v2 1/2] check-ref-format --print: Normalize refnames that start with slashes

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Aug 29, 2011 at 02:50:12PM -0400, Jeff King wrote:
>
>>   3. Having better tool support for comparing two sets of commits. The
>>      ideal interface (to me) for this workflow would be something like:
>
> BTW, this discussion is obviously about comparing what was applied
> upstream with what was submitted, but I think such a tool could have
> other purposes, too. It should be able to provide a much nicer interdiff
> between two versions of the same series (e.g., for reviewers looking at
> a re-roll).

I agree with it in principle, but I am not going to write one any time
soon. I think the most difficult case is when the submitter based a fix on
'next' or 'pu' and I fixed it up so that it applies to 'maint'. It is not
trivial to express conflict resolution in such a case, as what your tool
has to do its work are (1) the original patch that may apply to
"somewhere" (the tool could ask "am -3" to help it out), and (2) the patch
that is queued somewhere in 'pu'.

Re: [PATCH v2 1/2] check-ref-format --print: Normalize refnames that start with slashes

[Jeff King]

On Mon, Aug 29, 2011 at 01:56:35PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Mon, Aug 29, 2011 at 02:50:12PM -0400, Jeff King wrote:
> >
> >>   3. Having better tool support for comparing two sets of commits. The
> >>      ideal interface (to me) for this workflow would be something like:
> >
> > BTW, this discussion is obviously about comparing what was applied
> > upstream with what was submitted, but I think such a tool could have
> > other purposes, too. It should be able to provide a much nicer interdiff
> > between two versions of the same series (e.g., for reviewers looking at
> > a re-roll).
> 
> I agree with it in principle, but I am not going to write one any time
> soon.

Heh. I didn't expect you to. :) I may make an attempt at something, but
I'd be happy if somebody else has ideas, too.

> I think the most difficult case is when the submitter based a fix on
> 'next' or 'pu' and I fixed it up so that it applies to 'maint'. It is not
> trivial to express conflict resolution in such a case, as what your tool
> has to do its work are (1) the original patch that may apply to
> "somewhere" (the tool could ask "am -3" to help it out), and (2) the patch
> that is queued somewhere in 'pu'.

The nastiest part of such a thing is not necessarily comparing the
patches afterwards, but the actual conflicts you have to resolve to
convince "am -3" to apply the patch on "master" in the first place.

Consider this toy example, which accidentally bases a topic off of
"next" instead of "master":

  git init repo &&
  cd repo &&
  perl -le 'print "line $_" for (1..20)' >file &&
  git add file &&
  git commit -m base &&
  git checkout -b next &&
  perl -pi -e 's/10/$& unrelated topic on next/' file &&
  git commit -a -m 'unrelated topic' &&
  git checkout -b our-topic next && # oops!
  perl -pi -e 's/11/$& our topic/' file &&
  git commit -a -m 'our topic' &&
  git format-patch --stdout -1 >patch &&
  git checkout master

Obviously "git am" will fail to apply the patch. You can do it with "am
-3", but get a conflict like:

  ++<<<<<<< HEAD
   +line 10
   +line 11
  ++=======
  + line 10 unrelated topic on next
  + line 11 our topic
  ++>>>>>>> our topic

which is obviously easy to resolve in this toy example, but harder in
the real world (I assume in really hard cases, you just complain to the
submitter to rebase it properly).

But assuming you resolve that properly to:

  line 10
  line 11 our topic

what does the original submitter see? If they try to rebase their topic
onto master, they'll get this pretty sane conflict:

  ++<<<<<<< HEAD
   +line 10
  ++=======
  + line 10 unrelated topic on next
  ++>>>>>>> our topic

I think the tool behavior I outlined in the previous message would
yield:

  diff --git a/file b/file
  --- a/file
  +++ b/file
   line 8
   line 9
  -line 10 unrelated topic on next
  +line 10
   line 11 our topic
   line 12
  Accept this change [y,n,q]?

That is, it would look at the final versions of what each patch
produces, and try to apply the differences from one to the other, and
then use the result to apply on top of our rebase-in-progress.

So really, I think the ugliness of what the submitter would see is no
uglier than what you had to do during "git am". If you make trivial
changes, they'll see trivial changes. If you had to pick apart large
chunks of code into their constituent topics, then that's what the
submitter will see when reviewing the differences.

But note that I didn't actually run the above "this is what my tool
should do" through an actual script. I haven't written anything yet, so
I am just guessing, and there may be more complex cases where it breaks
down.

-Peff

Re: [PATCH v2 1/2] check-ref-format --print: Normalize refnames that start with slashes

[Johannes Sixt]

Am 29.08.2011 20:50, schrieb Jeff King:
>      Have you considered publishing the
>      tips of topic branches you apply?

Try 'git remote add github git://github.com/gitster/git' and drop your
jaws on the wealth of branches you get on every 'git fetch github'.

-- Hannes

Re: [PATCH v2 1/2] check-ref-format --print: Normalize refnames that start with slashes

[Jeff King]

On Mon, Aug 29, 2011 at 09:57:59PM +0200, Johannes Sixt wrote:

> Am 29.08.2011 20:50, schrieb Jeff King:
> >      Have you considered publishing the
> >      tips of topic branches you apply?
> 
> Try 'git remote add github git://github.com/gitster/git' and drop your
> jaws on the wealth of branches you get on every 'git fetch github'.

Hmph. I had no idea that existed. Searching the list, it was mentioned
in a What's Cooking in May 2010.

-Peff

Re: [PATCH v2 1/2] check-ref-format --print: Normalize refnames that start with slashes

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... I always assumed that you did one of:
>
>   1. Comment on the patch via email, just as any other reviewer, so it
>      can go into the re-roll.
>
>   2. Fix-up the patch or commit message during "am", with the assumption
>      that it is ready to be merged at least to "next", at which point
>      re-rolls are no longer OK, anyway.

Not really.

I usually amend patches queued to 'pu' (or queue fixup! separately) when I
am reasonably confident that my suggestion had enough merits.  Also I roll
in suggestions that are obviously (to me) good from the list, which may or
may not be the same to the list concensus. Maybe I should try to be less
aggressive.

> I mentioned "it takes effort" above. I don't mean "submitters shouldn't
> be expected to put in extra effort". But we should make sure the effort
> is well-spent, which means:
>
>   1. Giving them some indication that you tweaked things during
>      application. It doesn't have to be an inclusive list. Even saying
>      "Thanks, applied with some spelling fixes" instead of your usual
>      "Thanks" is enough (actually, I think you frequently do so
>      already).

Just started consciously doing so yesterday, after starthing this offtopic
therad ;-)

[PATCH v2 2/2] Forbid DEL characters in reference names

[Michael Haggerty]

DEL is an ASCII control character and therefore should not be
permitted in reference names.  Add tests for this and other unusual
characters.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---

Please check that the uses of "printf" in the test script are portable
and quoted correctly.

 refs.c                      |    2 +-
 t/t1402-check-ref-format.sh |    3 +++
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/refs.c b/refs.c
index 3a8789d..0fa8dcf 100644
--- a/refs.c
+++ b/refs.c
@@ -837,7 +837,7 @@ int for_each_rawref(each_ref_fn fn, void *cb_data)
 
 static inline int bad_ref_char(int ch)
 {
-	if (((unsigned) ch) <= ' ' ||
+	if (((unsigned) ch) <= ' ' || ch == 0x7f ||
 	    ch == '~' || ch == '^' || ch == ':' || ch == '\\')
 		return 1;
 	/* 2.13 Pattern Matching Notation */
diff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh
index 7563043..ed4275a 100755
--- a/t/t1402-check-ref-format.sh
+++ b/t/t1402-check-ref-format.sh
@@ -30,6 +30,9 @@ invalid_ref 'heads/foo.lock'
 valid_ref 'heads/foo@bar'
 invalid_ref 'heads/v@{ation'
 invalid_ref 'heads/foo\bar'
+invalid_ref "$(printf 'heads/foo\t')"
+invalid_ref "$(printf 'heads/foo\177')"
+valid_ref "$(printf 'heads/fu\303\237')"
 
 test_expect_success "check-ref-format --branch @{-1}" '
 	T=$(git write-tree) &&
-- 
1.7.6.8.gd2879

Re: [PATCH v2 2/2] Forbid DEL characters in reference names

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> Please check that the uses of "printf" in the test script are portable
> and quoted correctly.
> ...
> +invalid_ref "$(printf 'heads/foo\t')"
> +invalid_ref "$(printf 'heads/foo\177')"
> +valid_ref "$(printf 'heads/fu\303\237')"

I think these should be fine.