{"thread":{"id":"7818","subject":"[PATCH] Improve request-pull to handle non-rebased branches","startedAt":"2007-04-24T16:40:19Z","lastAt":"2007-04-25T05:38:11Z","messageCount":2,"participants":["Shawn O. Pearce","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"40345","messageId":"20070424164019.GA5780@spearce.org","threadId":"7818","inReplyTo":null,"subject":"[PATCH] Improve request-pull to handle non-rebased branches","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-04-24T16:40:19Z","receivedAt":"2007-04-24T16:40:19Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"This is actually a few different changes to request-pull,\nmaking it slightly smarter:\n\n 1) Compute the merge-base between the two revisions upfront\n    and reuse that selected merge-base for all log and diff\n    operations.\n\n 2) Disable the PAGER when running shortlog and diff, as these\n    would otherwise activate the pager for each command when\n    git-request-pull is run on a tty.  Users can get the entire\n    output paged (if desired) using `git -p request-pull`.\n\n 3) Use shortlog rather than `git log | git shortlog` now that\n    recent shortlog versions are able to run the revision listing\n    internally.\n\n 4) Attempt to resolve the input URL using the user's configured\n    remotes.  This is useful if the URL you want the recipient to\n    see is also the one you used to push your changes.  If not a\n    config-file remote could easily be setup for the public URL\n    and request-pull can be passed that instead.\n\n 5) Minor cleanup of revision->base variable names, making it\n    follow the head/headrev naming convention that is already\n    in use.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n git-request-pull.sh |   18 ++++++++++--------\n 1 files changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/git-request-pull.sh b/git-request-pull.sh\nindex 4eacc3a..367b262 100755\n--- a/git-request-pull.sh\n+++ b/git-request-pull.sh\n@@ -9,25 +9,27 @@ LONG_USAGE='Summarizes the changes since <commit> to the standard output,\n and includes <url> in the message generated.'\n SUBDIRECTORY_OK='Yes'\n . git-sh-setup\n+. git-parse-remote\n \n-revision=$1\n+base=$1\n url=$2\n head=${3-HEAD}\n \n-[ \"$revision\" ] || usage\n+[ \"$base\" ] || usage\n [ \"$url\" ] || usage\n \n-baserev=`git-rev-parse --verify \"$revision\"^0` &&\n+baserev=`git-merge-base \"$base\" \"$head\"` &&\n headrev=`git-rev-parse --verify \"$head\"^0` || exit\n \n+PAGER=\n+export PAGER\n echo \"The following changes since commit $baserev:\"\n-git log --max-count=1 --pretty=short \"$baserev\" |\n-git-shortlog | sed -e 's/^\\(.\\)/  \\1/'\n+git shortlog --max-count=1 $baserev | sed -e 's/^\\(.\\)/  \\1/'\n \n echo \"are found in the git repository at:\" \n echo\n-echo \"  $url\"\n+echo \"  `get_remote_url \"$url\"`\"\n echo\n \n-git log  $baserev..$headrev | git-shortlog ;\n-git diff -M --stat --summary $baserev..$headrev\n+git shortlog ^$baserev $headrev\n+git diff -M --stat --summary $baserev $headrev\n-- \n1.5.1.1.135.gf948\n"},{"id":"40374","messageId":"7v1wi910ak.fsf@assigned-by-dhcp.cox.net","threadId":"7818","inReplyTo":"20070424164019.GA5780@spearce.org","subject":"Re: [PATCH] Improve request-pull to handle non-rebased branches","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-25T05:38:11Z","receivedAt":"2007-04-25T05:38:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> writes:\n\n> -revision=$1\n> +base=$1\n>  url=$2\n>  head=${3-HEAD}\n>  \n> -[ \"$revision\" ] || usage\n> +[ \"$base\" ] || usage\n>  [ \"$url\" ] || usage\n>  \n> -baserev=`git-rev-parse --verify \"$revision\"^0` &&\n> +baserev=`git-merge-base \"$base\" \"$head\"` &&\n>  headrev=`git-rev-parse --verify \"$head\"^0` || exit\n>  \n> +PAGER=\n> +export PAGER\n>  echo \"The following changes since commit $baserev:\"\n> -git log --max-count=1 --pretty=short \"$baserev\" |\n> -git-shortlog | sed -e 's/^\\(.\\)/  \\1/'\n> +git shortlog --max-count=1 $baserev | sed -e 's/^\\(.\\)/  \\1/'\n\nI know the message says \"since commit X\", so taking merge base\nfeels technically correct from that point of view.  But I am not\nconvinced this is a good change.\n\nIt is acceptable to send a pull request from somewhat stale side\nbranch.  git projects (not necessarily git.git itself, but the\nproject with distributed management style that git aims to\nsupport well) are inherently peer-to-peer, not hub-and-spoke, so\nsubsystem people should not be forced to rebase to get included.\n\nHowever, I think the common courtesy _should_ be for pull\nrequestor to be confident that what s/he is requesting to be\npulled would result in sensible merge results, before sending\nsuch a pull request.\n\nAnd one way to show that you did your side of due diligence is\nto say \"This would merge to your tip\".  Taking merge-base and\nsaying only \"I forked from you this long time ago\" makes the\noutput convey a wrong message.  I would certainly be suspicious\nof a message that says \"Here are fixes since v1.3.0\", even if\nthe changes are isolated to the files that never changed since\nv1.3.0.\n\n>  echo \"are found in the git repository at:\" \n>  echo\n> -echo \"  $url\"\n> +echo \"  `get_remote_url \"$url\"`\"\n>  echo\n\nIf you can verify headrev is at the tip of a branch (and that is\nwhat you are asking to be pulled), I think we would want to see\nthe branch name at the end.  A recent example from the kernel\nlist:\n\n   On Thu, 19 Apr 2007, Avi Kivity wrote:\n   > \n   > Please pull from the 'linus' branch of\n   > \n   >   git://git.kernel.org/pub/scm/linux/kernel/git/avi/kvm.git\n\n   *please* put the branch-name after the git repo, so that I can \n   cut-and-paste without noticing only afterwards that the diffstat doesn't \n   match what it was supposed to, and I got the wrong commits, and have to \n   undo and re-do the pull..\n\n   Yeah, I didn't do that mistake this time, and in fact, I seldom do, but \n   every time the branch is mentioned somewhere else than with the actual \n   repo to pull, I get nervous. \n\n   So I've said this a million times before, but I'll say it one more \n   time: if it's not the main branch, the message should be\n\n      Please pull from the 'linus' branch of\n\n        git://git.kernel.org/pub/scm/linux/kernel/git/avi/kvm.git linus\n\n      to get the following changes: ...\n\n   so that when I cut-and-paste the URL (you didn't think I'd try to type it \n   in by hand, did you?) I get the branch automatically.\n\n> -git log  $baserev..$headrev | git-shortlog ;\n> -git diff -M --stat --summary $baserev..$headrev\n> +git shortlog ^$baserev $headrev\n> +git diff -M --stat --summary $baserev $headrev\n\nUse of the merge base revision for the last diff makes sense.\nThere should not be any difference for the shortlog if there is\nonly one merge base (and if there is more than one, then using\nonly one of them is probably buggy), so I think you would want\nto say \"shortlog $base..$headrev\" instead (iow keep the original\nfor this shortlog).\n"}]}