git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Michael Haggerty <mhagger@alum.mit.edu>,
	git@vger.kernel.org, cmn@elego.de
Subject: Re: [PATCH v2 1/2] check-ref-format --print: Normalize refnames that start with slashes
Date: Mon, 29 Aug 2011 14:50:12 -0400	[thread overview]
Message-ID: <20110829185011.GC756@sigill.intra.peff.net> (raw)
In-Reply-To: <7vty92adv0.fsf@alter.siamese.dyndns.org>

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

  parent reply	other threads:[~2011-08-29 18:50 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-08-27  4:12 [PATCH v2 0/2] Fix reference name format check and normalization Michael Haggerty
2011-08-27  4:12 ` [PATCH v2 1/2] check-ref-format --print: Normalize refnames that start with slashes Michael Haggerty
2011-08-27  4:22   ` Michael Haggerty
2011-08-27 18:30     ` Junio C Hamano
2011-08-28 15:50       ` git project patch workflow Michael Haggerty
2011-08-29 18:50       ` Jeff King [this message]
2011-08-29 19:01         ` [PATCH v2 1/2] check-ref-format --print: Normalize refnames that start with slashes Jeff King
2011-08-29 20:56           ` Junio C Hamano
2011-08-29 21:27             ` Jeff King
2011-08-29 19:57         ` Johannes Sixt
2011-08-29 20:02           ` Jeff King
2011-08-29 22:22         ` Junio C Hamano
2011-08-27  4:12 ` [PATCH v2 2/2] Forbid DEL characters in reference names Michael Haggerty
2011-08-27 18:40   ` Junio C Hamano

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=20110829185011.GC756@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=cmn@elego.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=mhagger@alum.mit.edu \
    /path/to/YOUR_REPLY

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

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).