* Re: [PATCH 1/1] MSVC: port pthread code to native Windows threads
From: Johannes Sixt @ 2009-11-04 8:24 UTC (permalink / raw)
To: Andrzej K. Haczewski; +Cc: git
In-Reply-To: <16cee31f0911032344m3263730l607c02eb4e9adef5@mail.gmail.com>
[please don't cull Cc list on this ML]
Andrzej K. Haczewski schrieb:
>> Pardon my ignorance, but is there a reason to not use Pthreads for Win32?
>> http://sourceware.org/pthreads-win32/
>>
>
> Not using pthreads on Windows makes Git:
> 1. faster on that platform
I believe this only if you present hard numbers. My guess is that (for
example) packing objects with two threads is still faster with a slow
pthreads emulation than without threading at all.
> 2. not depend on Pthreads for Win32
Why is this an advantage?
> IMHO that makes Git one step closer to become native on Windows, and
> is a sensible step.
Emulating pthreads on Windows with all its facets is an extremely
difficult task. If exact POSIX conformance is needed, I would choose an
existing package over doing it myself at any time.
Granted, we don't need the esoteric parts (cancelation points), which
would simplify the emulation a lot. But, as I pointed out in my other
mail, even a pthread_cond_wait() is not that trivial to implement with the
Windows API.
-- Hannes
^ permalink raw reply
* Re: [PATCH 1/1] MSVC: port pthread code to native Windows threads
From: Andrzej K. Haczewski @ 2009-11-04 8:17 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <alpine.DEB.1.00.0911040031210.4985@pacific.mpi-cbg.de>
2009/11/4 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> Could you please add the reasoning from the cover letter to this commit
> message? And add a sign-off?
Sure, will do so for next submission of that patch.
> It is unlikely that an #ifdef "contamination" of this extent will go
> through easily, but I have a suggestion that may make your patch both
> easier to read and more likely to be accepted into git.git: Try to wrap
> the win32 calls into pthread-compatible function signatures. Then you can
> add a compat/win32/pthread.h and not even touch core files of git.git at
> all.
First of all I didn't want to use wrappers because (if not inlined)
they introduce one additional call, that can be avoided with #defines
(as you can see even pthread_init can be done with macro). Second
reason is that I didn't want to create wrapping structures that would
need to be initialized / allocated / tracked. That patch translates
pthread calls to purely Win32 calls without anything in between.
Here are my reasoning for some of these #ifdefs and what can be done
and what can't (without using wrappers):
1. Thread routine has very different signature:
void *__cdecl func(void *); /* pthreads */
uint32_t __stdcall func(void *); /* Windows API */
First I thought it might be a problem to do (especially return value,
which is different size for 64-bit architectures), but since Git
doesn't use return value, it can be done.
2. Initialization of CRITICAL_SECTION and SEMAPHORE (used by condition
variables implementation). These need explicit initialization on
Windows and can't be done statically with PTHREAD_MUTEX_INITIALIZER
and PTHREAD_COND_INITIALIZER. There's no easy way around that (read:
it needs wrappers).
> Oh, and you definitely do not want to copy-paste err_win_to_posix(). You
> definitely want to reuse the existing instance.
Yeah, that was lazy, mea culpa.
I'll resubmit the patch with some fixes shortly,
Andrew
^ permalink raw reply
* Re: [PATCH 1/1] MSVC: port pthread code to native Windows threads
From: Johannes Sixt @ 2009-11-04 8:15 UTC (permalink / raw)
To: Andrzej K. Haczewski; +Cc: git
In-Reply-To: <1257283802-29726-2-git-send-email-ahaczewski@gmail.com>
Andrzej K. Haczewski schrieb:
> ---
You should sign-off your patches.
> #ifdef THREADED_DELTA_SEARCH
> -#include "thread-utils.h"
> -#include <pthread.h>
> +# include "thread-utils.h"
> +# ifndef _WIN32
> +# include <pthread.h>
> +# else
> +# include <winthread.h>
> +# endif
> #endif
Can't you just use the pthread package that is included in msysgit?
> +#ifndef _WIN32
> static void *threaded_find_deltas(void *arg)
> +#else
> +static DWORD WINAPI threaded_find_deltas(LPVOID arg)
> +#endif
> ...
> +#ifndef _WIN32
> return NULL;
> +#else
> + return 0;
> +#endif
> etc ...
You have far too many #ifdef in the generic code. There must be a better
way to hide the implementation details of this emulation.
> +#ifdef _WIN32
> + /*
> + * Windows require initialization of mutex (CRITICAL_SECTION)
> + * and conditional variable.
> + */
> + pthread_mutex_init(&read_mutex);
> + pthread_mutex_init(&cache_mutex);
> + pthread_mutex_init(&progress_mutex);
> + win32_cond_init(&progress_cond);
> +#endif
*If* we are going to use this minimal pthreads implementation, then I
think it will be OK to call pthread_*_init even on non-Windows.
> +static __inline int win32_cond_init(win32_cond_t *cond)
> +{
> + cond->waiters = 0;
> +
> + InitializeCriticalSection(&cond->waiters_lock);
> +
> + cond->sema = CreateSemaphore(NULL, 0, LONG_MAX, NULL);
Wouldn't an Event object be lighter-weight? (I'm only guessing.)
> + if (NULL == cond->sema)
> + return -1;
> + return 0;
> +}
> +
> +static __inline int win32_cond_destroy(win32_cond_t *cond)
> +{
> + CloseHandle(cond->sema);
> + cond->sema = NULL;
> +
> + DeleteCriticalSection(&cond->waiters_lock);
> +
> + return 0;
> +}
> +
> +static __inline int win32_cond_wait(win32_cond_t *cond, CRITICAL_SECTION *mutex)
And the reason that this is not pthread_cond_wait, is...?
> +{
> + DWORD result;
> + int ret = 0;
> +
> + /* we're waiting... */
> + EnterCriticalSection(&cond->waiters_lock);
> + ++cond->waiters;
> + LeaveCriticalSection(&cond->waiters_lock);
> +
> + /* unlock external mutex and wait for signal */
> + LeaveCriticalSection(mutex);
> + result = WaitForSingleObject(cond->sema, INFINITE);
Releasing the mutex and entering the wait state as well as leaving the
wait state and reacquiring the mutex should be atomic. Neither are in this
implementation. You are not mentioning why you are implementing things
like this and why this would be acceptable.
> +
> + if (0 != result)
> + ret = -1;
> +
> + /* one waiter less */
> + EnterCriticalSection(&cond->waiters_lock);
> + --cond->waiters;
> + LeaveCriticalSection(&cond->waiters_lock);
> +
> + /* lock external mutex again */
> + EnterCriticalSection(mutex);
> +/* almost copy-paste code of mingw.c */
> +static int err_win_to_posix()
> +{
There must be a better way than to just copy & paste this huge piece of code.
-- Hannes
^ permalink raw reply
* Re: [PATCH bg/format-patch-p-noop] log-tree: always add --- marker when options are patch and a stat
From: Björn Gustavsson @ 2009-11-04 7:49 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Stephen Boyd, git
In-Reply-To: <20091104071053.GB24263@coredump.intra.peff.net>
On Wed, Nov 4, 2009 at 8:10 AM, Jeff King <peff@peff.net> wrote:
> This patch goes on top of master, and terribly conflicts with Björn's
> changes in the area. But I had the impression you wanted to revert those
> changes for now anyway, so probably this should go in as a bug fix and
> everything else should be built on top. It actually would be an even
> smaller change on top of his "always show patch, even when other formats
> are given" change, but I didn't want to depend on it.
No problem. I can re-implement my patch series on top of your patch.
/Björn
--
Björn Gustavsson, Erlang/OTP, Ericsson AB
^ permalink raw reply
* Re: [PATCH 1/1] MSVC: port pthread code to native Windows threads
From: Andrzej K. Haczewski @ 2009-11-04 7:44 UTC (permalink / raw)
To: git
In-Reply-To: <4AF0E842.2010201@workspacewhiz.com>
>
> Pardon my ignorance, but is there a reason to not use Pthreads for Win32?
> http://sourceware.org/pthreads-win32/
>
Not using pthreads on Windows makes Git:
1. faster on that platform
2. not depend on Pthreads for Win32
IMHO that makes Git one step closer to become native on Windows, and
is a sensible step.
Andrew
^ permalink raw reply
* Re: [PATCH bg/format-patch-p-noop] log-tree: always add --- marker when options are patch and a stat
From: Jeff King @ 2009-11-04 7:37 UTC (permalink / raw)
To: Stephen Boyd; +Cc: Junio C Hamano, git, Björn Gustavsson
In-Reply-To: <4AF12EC5.4030407@gmail.com>
On Tue, Nov 03, 2009 at 11:35:33PM -0800, Stephen Boyd wrote:
> >@@ -940,6 +941,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> > { OPTION_CALLBACK, 0, "thread", &thread, "style",
> > "enable message threading, styles: shallow, deep",
> > PARSE_OPT_OPTARG, thread_callback },
> >+ OPT_BOOLEAN('p', NULL, &use_patch_format,
> >+ "show patch format instead of default (patch + stat)"),
> > OPT_END()
> > };
>
> I don't imagine we want this option in the messaging group though.
> Can you move it up?
Thanks, good catch. I just tacked it onto the end, forgetting that we
were using grouping. Junio, can you tweak it, or do you want a resend?
-Peff
^ permalink raw reply
* Re: [PATCH bg/format-patch-p-noop] log-tree: always add --- marker when options are patch and a stat
From: Stephen Boyd @ 2009-11-04 7:35 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Björn Gustavsson
In-Reply-To: <20091104071940.GA15011@coredump.intra.peff.net>
Jeff King wrote:
>
> This patch unbreaks what 68daa64 did (while still preserving
> what 68daa64 was trying to do), reinstating "-p" to suppress
> the default behavior. We do this by parsing "-p" ourselves
> in format-patch, and noting whether it was used explicitly.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>
This looks good to me; covering 2 and 3 of Junio's TODO list.
> @@ -940,6 +941,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> { OPTION_CALLBACK, 0, "thread", &thread, "style",
> "enable message threading, styles: shallow, deep",
> PARSE_OPT_OPTARG, thread_callback },
> + OPT_BOOLEAN('p', NULL, &use_patch_format,
> + "show patch format instead of default (patch + stat)"),
> OPT_END()
> };
I don't imagine we want this option in the messaging group though. Can
you move it up?
^ permalink raw reply
* Re: [PATCH bg/format-patch-p-noop] log-tree: always add --- marker when options are patch and a stat
From: Junio C Hamano @ 2009-11-04 7:31 UTC (permalink / raw)
To: Björn Gustavsson; +Cc: Stephen Boyd, git
In-Reply-To: <6672d0160911032300w1a2dfdbck5b1db98f2059639b@mail.gmail.com>
Björn Gustavsson <bgustavsson@gmail.com> writes:
> Since -p has been broken for 14 months, is really necessary to reinstate
> it? (Or has the breakage not been reported because the people who care
> still use a git version older than 14 months?)
Oh, 1.6.0 has the old behaviour and we see many people still on 1.5.X
series. Hopefully nobody uses 1.4.X series anymore but I wouldn't be
overly surprised if somebody raised hand in the next 6 hours after seeing
this message ;-)
> Why not just add a new --no-stat option?
I am all for teaching _new_ people to say "format-patch --no-stat", but it
won't help people who have been happy with 1.6.0 when they update, if they
have to change their "format-patch -p" to "format-patch --no-stat".
^ permalink raw reply
* Re: [PATCH] Require a struct remote in transport_get()
From: Daniel Barkalow @ 2009-11-04 7:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vbpjin8v0.fsf@alter.siamese.dyndns.org>
On Tue, 3 Nov 2009, Junio C Hamano wrote:
> Daniel Barkalow <barkalow@iabervon.org> writes:
>
> > cmd_ls_remote() was calling transport_get() with a NULL remote and a
> > non-NULL url in the case where it was run outside a git
> > repository. This involved a bunch of ill-tested special
> > cases. Instead, simply get the struct remote for the URL with
> > remote_get(), which works fine outside a git repository, and can also
> > take global options into account.
> >
> > This fixes a tiny and obscure bug where "git ls-remote" without a repo
> > didn't support global url.*.insteadOf, even though "git clone" and
> > "git ls-remote" in any repo did.
> >
> > Also, enforce that all callers provide a struct remote to transport_get().
> >
> > Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
> > ---
> > This is sufficient to stop the segfault when tring "git ls-remote
> > http://..." outside of a repo, but not to make it work, which requires
> > either something simple but not ideal or something complex.
>
> Thanks; I think this and your other patch are important fixes, and should
> go directly on 'maint'. Do you prefer to queue them on 'next' to cook for
> a week instead?
I don't think a week on 'next' is likely to turn up any new information;
these are all uncommon code paths. It might be worth seeing if the
original reporter is happy with how it's behaving now, though.
-Daniel
*This .sig left intentionally blank*
^ permalink raw reply
* Re: [PATCH] gitk: disable checkout of remote branch
From: Jeff King @ 2009-11-04 7:27 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: Tim Mazid, git
In-Reply-To: <fabb9a1e0911032241u3735fa30heaa195d959879f5a@mail.gmail.com>
On Wed, Nov 04, 2009 at 07:41:28AM +0100, Sverre Rabbelier wrote:
> On Wed, Nov 4, 2009 at 07:17, Tim Mazid <timmazid@hotmail.com> wrote:
> > So instead of invoking 'git checkout REMOTE/BRANCH', do 'git checkout -b
> > BRANCH REMOTE/BRANCH'.
>
> Automagically doing 'git checkout -t remote/branch' when asked to do
> 'git checkout remote/branch' was suggested earlier on the list and I
> think there was even a patch that implemented it, not sure what the
> outcome of the series was. I do remember that Peff was annoyed by it
> at the GitTogether though so it might be a bad idea.
It's in 'next' now. And for the record, my complaint about its behavior
turned out to be partially because I was an idiot. I am still not
convinced that we won't later regret leaving the stale local branch
sitting around, or that users won't find it confusing to see:
$ git checkout foo
Branch foo set up to track remote branch foo from origin.
Switched to a new branch 'foo'
... time passes ...
$ git checkout foo
Switched to branch 'foo'
Your branch is behind 'origin/foo' by 1 commit, and can be fast-forwarded.
(i.e., you do the same thing, but get two very different results, and
you have to know how to do the fast-forward. Trivial if you are used to
working with branches, but perhaps not if you are just sightseeing).
But I am no longer planning on writing a long-winded rant about the
feature. ;)
-Peff
^ permalink raw reply
* Re: [PATCH bg/format-patch-p-noop] log-tree: always add --- marker when options are patch and a stat
From: Junio C Hamano @ 2009-11-04 7:26 UTC (permalink / raw)
To: Jeff King; +Cc: Stephen Boyd, git, Björn Gustavsson
In-Reply-To: <20091104063612.GA24263@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Nov 03, 2009 at 09:49:46PM -0800, Junio C Hamano wrote:
>
>> Even though I personally find the stat information very useful, I would be
>> happier if somebody reverts the bg/format-patch-p-noop series and instead
>> fixes the regression caused by 68daa64, and does so without touching any
>> output from the low-level plumbing like diff-tree that may be used by
>> scripts.
>
> I agree that 68daa64 is a hack (and I even noted in the commit log that
> "-p" is now a no-op).
Correct, and thanks---it was not your fault and I didn't mean to blame
you. If anything it was mine.
> The problem is that we don't have the one critical
> bit of information in cmd_format_patch that we do in diff_opt_parse: was
> the format set explicitly, or was it a side-effect of -U (or --binary,
> or maybe others).
The appoarch your "how about this" takes feels right. We did discuss "set
of hardwired defaults specific to the individual commands, that are masked
by set of defaults read from the config, that are in turn masked by set of
command line options", but I do not think that level of complexity is worth
for this "is it -U<n> or -p" issue.
> My test case checks the current output (i.e., missing dashes). I think
> it should probably have dashes, but that should be fixed in a separate
> patch.
Agreed.
^ permalink raw reply
* Re: [PATCH] Allow curl helper to work without a local repository
From: Daniel Barkalow @ 2009-11-04 7:21 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: Junio C Hamano, git
In-Reply-To: <fabb9a1e0911032132v5e76e4b6n559169ad43d9f7c0@mail.gmail.com>
On Wed, 4 Nov 2009, Sverre Rabbelier wrote:
> Heya,
>
> On Wed, Nov 4, 2009 at 03:52, Daniel Barkalow <barkalow@iabervon.org> wrote:
> > This is the simple change to let remote-curl work without a local
> > repository for git ls-remote; it leave the transport-helper code assuming
> > that all helpers can list without a local repo, which happens to be true
> > of this helper, the only one in current git.
>
> Add a capability for it? :P
That's the longer-term patch, yes. But doing anything meaningful with that
requires communicating down to transport-helper that we're not in a local
repo, which shades into reworking the whole setup/environment code. That's
why I went with the easy patch for now, since it does fix the bug.
-Daniel
*This .sig left intentionally blank*
^ permalink raw reply
* Re: [PATCH bg/format-patch-p-noop] log-tree: always add --- marker when options are patch and a stat
From: Jeff King @ 2009-11-04 7:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stephen Boyd, git, Björn Gustavsson
In-Reply-To: <20091104071053.GB24263@coredump.intra.peff.net>
On Wed, Nov 04, 2009 at 02:10:54AM -0500, Jeff King wrote:
> Subject: [PATCH] format-patch: make "-p" suppress diffstat
Ugh. And here is one that actually compiles. Yes, I actually did test
it, but the one-line typo was sitting uncommitted in my workdir.
-- >8 --
Subject: [PATCH] format-patch: make "-p" suppress diffstat
Once upon a time, format-patch would use its default stat
plus patch format only when no diff format was given on the
command line. This meant that "format-patch -p" would
suppress the stat and show just the patch.
Commit 68daa64 changed this to keep the stat format when we
had an "implicit" patch format, like "-U5". As a side
effect, this meant that an explicit patch format was now
ignored (because cmd_format_patch didn't know the reason
that the format was set way down in diff_opt_parse).
This patch unbreaks what 68daa64 did (while still preserving
what 68daa64 was trying to do), reinstating "-p" to suppress
the default behavior. We do this by parsing "-p" ourselves
in format-patch, and noting whether it was used explicitly.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin-log.c | 9 +++++++--
t/t4014-format-patch.sh | 21 +++++++++++++++++++++
2 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/builtin-log.c b/builtin-log.c
index 207a361..0ff032b 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -891,6 +891,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
struct patch_ids ids;
char *add_signoff = NULL;
struct strbuf buf = STRBUF_INIT;
+ int use_patch_format = 0;
const struct option builtin_format_patch_options[] = {
{ OPTION_CALLBACK, 'n', "numbered", &numbered, NULL,
"use [PATCH n/m] even with a single patch",
@@ -940,6 +941,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
{ OPTION_CALLBACK, 0, "thread", &thread, "style",
"enable message threading, styles: shallow, deep",
PARSE_OPT_OPTARG, thread_callback },
+ OPT_BOOLEAN('p', NULL, &use_patch_format,
+ "show patch format instead of default (patch + stat)"),
OPT_END()
};
@@ -1027,8 +1030,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
if (argc > 1)
die ("unrecognized argument: %s", argv[1]);
- if (!rev.diffopt.output_format
- || rev.diffopt.output_format == DIFF_FORMAT_PATCH)
+ if (use_patch_format)
+ rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
+ else if (!rev.diffopt.output_format ||
+ rev.diffopt.output_format == DIFF_FORMAT_PATCH)
rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY | DIFF_FORMAT_PATCH;
if (!DIFF_OPT_TST(&rev.diffopt, TEXT) && !no_binary_diff)
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 531f5b7..cab6ce2 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -455,6 +455,27 @@ test_expect_success 'format-patch respects -U' '
'
+cat > expect << EOF
+
+diff --git a/file b/file
+index 40f36c6..2dc5c23 100644
+--- a/file
++++ b/file
+@@ -14,3 +14,19 @@ C
+ D
+ E
+ F
++5
+EOF
+
+test_expect_success 'format-patch -p suppresses stat' '
+
+ git format-patch -p -2 &&
+ sed -e "1,/^$/d" -e "/^+5/q" < 0001-This-is-an-excessively-long-subject-line-for-a-messa.patch > output &&
+ test_cmp expect output
+
+'
+
test_expect_success 'format-patch from a subdirectory (1)' '
filename=$(
rm -rf sub &&
--
1.6.5.2.308.g2bb5
^ permalink raw reply related
* Re: [PATCH v2] commit -c/-C/--amend: reset timestamp and authorship to committer with --reset-author
From: Junio C Hamano @ 2009-11-04 7:14 UTC (permalink / raw)
To: Erick Mattos; +Cc: git
In-Reply-To: <1257304811-26812-1-git-send-email-erick.mattos@gmail.com>
Erick Mattos <erick.mattos@gmail.com> writes:
> Cutting --author away would make impossible for someone to force a new author
> with a new timestamp in case he is templating. As an example he can be using
> the --author because he is doing a change in a computer not his own or
> something alike.
Sorry, but I cannot help feeling a bit frustrated and mildly irritated.
I had an impression that we have already established that setting the
author with --author="Somebody Else <s@b.e>" and committing with the
current time does not make much sense from the workflow point of view long
time ago in this thread.
The mail transport might have mangled the name, and when using --amend (or
read-tree followed by commit -c), it is handy to fix the mangled name by
using --author, but in such a case you would actively want to keep the
timestamp obtained from the e-mail via either --amend or -c.
But allowing this combination, even though it might not make much sense,
is just giving extra length to the rope, so it may not be such a big deal.
I didn't feel motivated enough to read the whole thing while other patches
are in my inbox, so I instead ran diff between the previous one (without
my suggestion today) and this round.
I see that you fixed a lot of grammar in the log message of my earlier
suggestion, all of which looked very good. Also you added a check in the
program to make sure that --renew is given only when -C/-c/--amend is
given, which is also good. Neither of our set of tests checks this
condition, though. IOW, we would need to add something like this at the
end of my version (adjust to --reset-author for your version):
test_expect_success '--mine should be rejected without -c/-C/--amend' '
git checkout Initial &&
echo "Test 7" >>foo &&
test_tick &&
test_must_fail git commit -a --mine -m done
'
I am not sure why you insist to use your version of test script and keep
changing it, though. It looks a lot worse even only after reviewing its
early part.
- author_id runs an extra grep that is unnecessary. The separation of
_id and _timestamp are unnecessary if you checked against an expected
author ident and timestamp as a single string, i.e.
FRIGATE='Frigate <flying@over.world>' ;# do this only once at the beginning
...
git commit -C HEAD --reset-author --author="$FRIGATE" &&
echo "author $FRIGATE $GIT_AUTHOR_TIME" >expect &&
author_header HEAD >actual &&
test_cmp expect actual
This becomes irrelevant if we don't support mixing --renew and
--author, of course.
- message_body() now has a backslash whose sole purpose is to be an
eyesore.
- initiate_test() does not string the commands together with &&
I might change my mind after I take a break, review others' patches, and
spend some time on my own hacking on other topics before revisiting this
patch, but at this point I find that reviewing newer rounds of this series
has rather quickly diminishing value, and more time is being spent on
teaching shell scripting to you rather than on polishing the end result.
Sorry, but I cannot help feeling a bit frustrated and mildly irritated.
Time to take a break and attend other topics for a change.
^ permalink raw reply
* Re: [PATCH bg/format-patch-p-noop] log-tree: always add --- marker when options are patch and a stat
From: Jeff King @ 2009-11-04 7:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stephen Boyd, git, Björn Gustavsson
In-Reply-To: <20091104063612.GA24263@coredump.intra.peff.net>
On Wed, Nov 04, 2009 at 01:36:13AM -0500, Jeff King wrote:
> Here's a patch which fixes the regression, but it feels awfully hack-ish
> to me, as it treats "-p" specially in diff_opt (but in a way that no
> other existing code should care about). It would be "cleaner" to me to
> have some infrastructure for keeping an implicit and an explicit format,
> and then merging them at the end. But I don't think we ever care about
> this explicitness for any other formats, so this is at least simple.
>
> Another option might be for format-patch to simply parse "-p" itself,
> setting the format and marking an "explicit" flag. I'll look into that
> as an alternative.
OK, doing that turned out to be much nicer. Less code, localized to
format-patch, and less confusing to read.
This patch goes on top of master, and terribly conflicts with Björn's
changes in the area. But I had the impression you wanted to revert those
changes for now anyway, so probably this should go in as a bug fix and
everything else should be built on top. It actually would be an even
smaller change on top of his "always show patch, even when other formats
are given" change, but I didn't want to depend on it.
-- >8 --
Subject: [PATCH] format-patch: make "-p" suppress diffstat
Once upon a time, format-patch would use its default stat
plus patch format only when no diff format was given on the
command line. This meant that "format-patch -p" would
suppress the stat and show just the patch.
Commit 68daa64 changed this to keep the stat format when we
had an "implicit" patch format, like "-U5". As a side
effect, this meant that an explicit patch format was now
ignored (because cmd_format_patch didn't know the reason
that the format was set way down in diff_opt_parse).
This patch unbreaks what 68daa64 did (while still preserving
what 68daa64 was trying to do), reinstating "-p" to suppress
the default behavior. We do this by parsing "-p" ourselves
in format-patch, and noting whether it was used explicitly.
Signed-off-by: Jeff King <peff@peff.net>
---
The help text for "-p" is up for debate. It can also of course be used
as "show patch in addition to other things you already specified on the
command line". But I wanted to point out that its main use is the side
effect that it suppresses the default output. I am open to suggestions.
builtin-log.c | 9 +++++++--
t/t4014-format-patch.sh | 21 +++++++++++++++++++++
2 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/builtin-log.c b/builtin-log.c
index 207a361..da79047 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -891,6 +891,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
struct patch_ids ids;
char *add_signoff = NULL;
struct strbuf buf = STRBUF_INIT;
+ int use_patch_format = 0;
const struct option builtin_format_patch_options[] = {
{ OPTION_CALLBACK, 'n', "numbered", &numbered, NULL,
"use [PATCH n/m] even with a single patch",
@@ -940,6 +941,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
{ OPTION_CALLBACK, 0, "thread", &thread, "style",
"enable message threading, styles: shallow, deep",
PARSE_OPT_OPTARG, thread_callback },
+ OPT_BOOLEAN('p', NULL, &use_patch_format,
+ "show patch format instead of default (patch + stat)"),
OPT_END()
};
@@ -1027,8 +1030,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
if (argc > 1)
die ("unrecognized argument: %s", argv[1]);
- if (!rev.diffopt.output_format
- || rev.diffopt.output_format == DIFF_FORMAT_PATCH)
+ if (patch_format)
+ rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
+ else if (!rev.diffopt.output_format ||
+ rev.diffopt.output_format == DIFF_FORMAT_PATCH)
rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY | DIFF_FORMAT_PATCH;
if (!DIFF_OPT_TST(&rev.diffopt, TEXT) && !no_binary_diff)
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 531f5b7..cab6ce2 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -455,6 +455,27 @@ test_expect_success 'format-patch respects -U' '
'
+cat > expect << EOF
+
+diff --git a/file b/file
+index 40f36c6..2dc5c23 100644
+--- a/file
++++ b/file
+@@ -14,3 +14,19 @@ C
+ D
+ E
+ F
++5
+EOF
+
+test_expect_success 'format-patch -p suppresses stat' '
+
+ git format-patch -p -2 &&
+ sed -e "1,/^$/d" -e "/^+5/q" < 0001-This-is-an-excessively-long-subject-line-for-a-messa.patch > output &&
+ test_cmp expect output
+
+'
+
test_expect_success 'format-patch from a subdirectory (1)' '
filename=$(
rm -rf sub &&
--
1.6.5.2.308.g2bb5
^ permalink raw reply related
* Re: [PATCH bg/format-patch-p-noop] log-tree: always add --- marker when options are patch and a stat
From: Björn Gustavsson @ 2009-11-04 7:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stephen Boyd, git
In-Reply-To: <7vy6mmltz9.fsf@alter.siamese.dyndns.org>
2009/11/4 Junio C Hamano <gitster@pobox.com>:
> In an ideal world, I would probably say:
>
> * format-patch should have three-dash after the commit message, no matter
> what format the patch is asked for, and it always will give patch text.
I agree.
> * format-patch -p should be reinstated as a way to ask for "just patch
> text, no diffstat". Introducing a new option --no-stat _in addition_
> to improve the UI is Ok.
Since -p has been broken for 14 months, is really necessary to reinstate
it? (Or has the breakage not been reported because the people who care
still use a git version older than 14 months?)
Why not just add a new --no-stat option?
> * format-patch -U<n> should not be mistaken as a request to suppress
> diffstat; what 68daa64 _tried_ to do was worthy.
I agree.
/Björn
--
Björn Gustavsson, Erlang/OTP, Ericsson AB
^ permalink raw reply
* Re: [PATCH] t1200: cleanup and modernize test style
From: Stephen Boyd @ 2009-11-04 6:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vr5seltyi.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Thanks.
>
> The sequence of commands are suppopsed to match what the user manual
> teaches, and I suspect we have had quite a many updates to the manual
> since this test script was last touched. Do you happen to know if they
> still match the manual?
I just read through it and it's different. I'm ignoring the cloning,
fetching, pushing, and gitk parts too. There is a whole section on
merging which isn't really covered. I'll resend sometime soon (the last
test is broken too, sorry).
^ permalink raw reply
* Re: [PATCH] gitk: disable checkout of remote branch
From: Sverre Rabbelier @ 2009-11-04 6:41 UTC (permalink / raw)
To: Tim Mazid; +Cc: git
In-Reply-To: <1257315478920-3943388.post@n2.nabble.com>
Heya,
On Wed, Nov 4, 2009 at 07:17, Tim Mazid <timmazid@hotmail.com> wrote:
> So instead of invoking 'git checkout REMOTE/BRANCH', do 'git checkout -b
> BRANCH REMOTE/BRANCH'.
Automagically doing 'git checkout -t remote/branch' when asked to do
'git checkout remote/branch' was suggested earlier on the list and I
think there was even a patch that implemented it, not sure what the
outcome of the series was. I do remember that Peff was annoyed by it
at the GitTogether though so it might be a bad idea.
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [PATCH] Update packfile transfer protocol documentation
From: Sverre Rabbelier @ 2009-11-04 6:36 UTC (permalink / raw)
To: Scott Chacon; +Cc: git list, Junio C Hamano, Shawn O. Pearce
In-Reply-To: <d411cc4a0911032158j2a4664e5w2601c4af59ba0837@mail.gmail.com>
Heya,
On Wed, Nov 4, 2009 at 06:58, Scott Chacon <schacon@gmail.com> wrote:
> The technical documentation for the packfile protocol is both sparse and incorrect
Each time I read this line I read:
"This technical documentation for the packfile protocol is both sparse
and incorrect"
Perhaps if there is another resend after this you can change it to:
"The current technical documentation for the packfile protocol is both sparse"
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [PATCH bg/format-patch-p-noop] log-tree: always add --- marker when options are patch and a stat
From: Jeff King @ 2009-11-04 6:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stephen Boyd, git, Björn Gustavsson
In-Reply-To: <7vy6mmltz9.fsf@alter.siamese.dyndns.org>
On Tue, Nov 03, 2009 at 09:49:46PM -0800, Junio C Hamano wrote:
> I actually am more worried about 68daa64 from 14 months ago, as I vaguely
> recall seeing an explicit user request that in some community the diffstat
> information is unwanted on their mailing list, and I am reasonably sure
> that "-p suppresses --stat" was done deliberately to satisfy them (even
> though it may have been a suboptimal UI and --no-stat might have been a
> lot more straightforward).
>
> Even though I personally find the stat information very useful, I would be
> happier if somebody reverts the bg/format-patch-p-noop series and instead
> fixes the regression caused by 68daa64, and does so without touching any
> output from the low-level plumbing like diff-tree that may be used by
> scripts.
I agree that 68daa64 is a hack (and I even noted in the commit log that
"-p" is now a no-op). The problem is that we don't have the one critical
bit of information in cmd_format_patch that we do in diff_opt_parse: was
the format set explicitly, or was it a side-effect of -U (or --binary,
or maybe others).
Here's a patch which fixes the regression, but it feels awfully hack-ish
to me, as it treats "-p" specially in diff_opt (but in a way that no
other existing code should care about). It would be "cleaner" to me to
have some infrastructure for keeping an implicit and an explicit format,
and then merging them at the end. But I don't think we ever care about
this explicitness for any other formats, so this is at least simple.
Another option might be for format-patch to simply parse "-p" itself,
setting the format and marking an "explicit" flag. I'll look into that
as an alternative.
---
diff --git a/builtin-log.c b/builtin-log.c
index a359679..3b5819e 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -1034,8 +1034,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
if (rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF)
die("--check does not make sense");
- if (!rev.diffopt.output_format
- || rev.diffopt.output_format == DIFF_FORMAT_PATCH)
+ if (!rev.diffopt.output_format ||
+ (rev.diffopt.output_format == DIFF_FORMAT_PATCH &&
+ !rev.diffopt.explicit_patch_format))
rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;
/* Always generate a patch */
diff --git a/diff.c b/diff.c
index 9cd9693..9ce5520 100644
--- a/diff.c
+++ b/diff.c
@@ -2643,8 +2643,10 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
const char *arg = av[0];
/* Output format options */
- if (!strcmp(arg, "-p") || !strcmp(arg, "-u"))
+ if (!strcmp(arg, "-p") || !strcmp(arg, "-u")) {
options->output_format |= DIFF_FORMAT_PATCH;
+ options->explicit_patch_format = 1;
+ }
else if (opt_arg(arg, 'U', "unified", &options->context))
options->output_format |= DIFF_FORMAT_PATCH;
else if (!strcmp(arg, "--raw"))
diff --git a/diff.h b/diff.h
index 55f3203..406c0c6 100644
--- a/diff.h
+++ b/diff.h
@@ -90,6 +90,7 @@ struct diff_options {
int skip_stat_unmatch;
int line_termination;
int output_format;
+ int explicit_patch_format;
int pickaxe_opts;
int rename_score;
int rename_limit;
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index f826348..5689d59 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -455,6 +455,27 @@ test_expect_success 'format-patch respects -U' '
'
+cat > expect << EOF
+
+diff --git a/file b/file
+index 40f36c6..2dc5c23 100644
+--- a/file
++++ b/file
+@@ -14,3 +14,19 @@ C
+ D
+ E
+ F
++5
+EOF
+
+test_expect_success 'format-patch -p suppresses stat' '
+
+ git format-patch -p -2 &&
+ sed -e "1,/^$/d" -e "/^+5/q" < 0001-This-is-an-excessively-long-subject-line-for-a-messa.patch > output &&
+ test_cmp expect output
+
+'
+
test_expect_success 'format-patch from a subdirectory (1)' '
filename=$(
rm -rf sub &&
> With older (say 1.6.0) git, format-patch with the -p option does not give
> these three-dash lines, and it does look funny. Even though the same
> funniness appears only when you use --raw or --numstat with the current
> code, if we fix "-p" to suppress the default "--stat", this will become an
> issue again.
My test case checks the current output (i.e., missing dashes). I think
it should probably have dashes, but that should be fixed in a separate
patch.
-Peff
^ permalink raw reply related
* Re: [PATCH] gitk: disable checkout of remote branch
From: Tim Mazid @ 2009-11-04 6:17 UTC (permalink / raw)
To: git
In-Reply-To: <2e24e5b90911031758t651735f9xe9d078079112cfa6@mail.gmail.com>
Sitaram Chamarty wrote:
>
> On Wed, Nov 4, 2009 at 6:18 AM, Tim Mazid <timmazid@hotmail.com> wrote:
>
>> Might be better to include a configuration option to allow this, for
>> those
>> that know what they're doing. Most of the people that know what they're
>> doing will use the command line, anyway, but it may irritate some people.
>
> I considered that but found my tcl fu was seriously lacking. These
> are literally the first 3 lines of tcl I ever wrote in my life, and
> this program is one huge 11,000+ line monolith, so I'm naturally
> scared to make more than very, very, small changes :)
>
> In any case, as you said, most people who know what they're doing can
> use the CLI to get there anyway...
>
Hm, now that I think about it, what might be better is to just do what
should be done for them. :P
So instead of invoking 'git checkout REMOTE/BRANCH', do 'git checkout -b
BRANCH REMOTE/BRANCH'.
I know you said, you don't know, tcl, but just throwing it out there for
anyone else.
Cheers,
Tim.
--
View this message in context: http://n2.nabble.com/PATCH-gitk-disable-checkout-of-remote-branch-tp3939363p3943388.html
Sent from the git mailing list archive at Nabble.com.
^ permalink raw reply
* Re: [PATCH] commit: fix too generous RFC-2822 footer handling
From: Junio C Hamano @ 2009-11-04 6:11 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: David Brown, git
In-Reply-To: <1257304146-15543-1-git-send-email-szeder@ira.uka.de>
SZEDER Gábor <szeder@ira.uka.de> writes:
> builtin-commit.c | 8 ++++++++
> t/t7501-commit.sh | 4 ++--
> 2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/builtin-commit.c b/builtin-commit.c
> index beddf01..4971156 100644
> --- a/builtin-commit.c
> +++ b/builtin-commit.c
> @@ -429,6 +429,14 @@ static int ends_rfc2822_footer(struct strbuf *sb)
> hit = (buf[i] == '\n');
> }
>
> + for (j = i-1; j > 0; j--)
> + if (buf[j] == '\n') {
> + hit = 1;
> + break;
> + }
> + if (!hit) /* one-line message */
> + return 0;
> +
That looks overly convoluted. Why isn't the attached patch enough?
- We inspected the last line of the message buffer, and 'i' is at the
beginning of that last line;
- At the line that begins at 'i', we found something that does not match
the sob we are going to add;
- We want a newline if it is a single liner (i.e. i == 0), or if that
last one is not sob/acked-by and friends.
If you are anal and want to allow an author with a funny name "is allowed
as the first word", we _could_ encounter a single-liner commit like this:
From: is allowed as the first word <author@example.xz>
Subject: Signed-off-by: is allowed as the first word <author@example.xz>
Signed-off-by: is allowed as the first word <author@example.xz>
and you may want to add "!i ||" in front of prefixcmp(), but I do not
think that is worth it.
builtin-commit.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/builtin-commit.c b/builtin-commit.c
index c395cbf..cfa6b06 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -530,7 +530,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
for (i = sb.len - 1; i > 0 && sb.buf[i - 1] != '\n'; i--)
; /* do nothing */
if (prefixcmp(sb.buf + i, sob.buf)) {
- if (!ends_rfc2822_footer(&sb))
+ if (!i || !ends_rfc2822_footer(&sb))
strbuf_addch(&sb, '\n');
strbuf_addbuf(&sb, &sob);
}
^ permalink raw reply related
* [PATCH] Update packfile transfer protocol documentation
From: Scott Chacon @ 2009-11-04 5:58 UTC (permalink / raw)
To: git list; +Cc: Junio C Hamano, Shawn O. Pearce
The technical documentation for the packfile protocol is both sparse and
incorrect. This documents the fetch-pack/upload-pack and send-pack/
receive-pack protocols much more fully.
Add documentation from Shawn's upcoming http-protocol docs that is shared
by the packfile protocol. protocol-common.txt describes ABNF notation
amendments, refname rules and the packet line format.
Add documentation on the various capabilities supported by the
upload-pack and receive-pack protocols. protocol-capabilities.txt describes
multi-ack, thin-pack, side-band[-64k], shallow, no-progress, include-tag,
ofs-delta, delete-refs and report-status.
Signed-off-by: Scott Chacon <schacon@gmail.com>
---
OK, I think I've incorporated all the comments from you and Shawn
here. Let me know if that's cool or you need me to update anything
else.
Documentation/technical/pack-protocol.txt | 535 +++++++++++++++++++--
Documentation/technical/protocol-capabilities.txt | 186 +++++++
Documentation/technical/protocol-common.txt | 96 ++++
3 files changed, 776 insertions(+), 41 deletions(-)
create mode 100644 Documentation/technical/protocol-capabilities.txt
create mode 100644 Documentation/technical/protocol-common.txt
diff --git a/Documentation/technical/pack-protocol.txt
b/Documentation/technical/pack-protocol.txt
index 9cd48b4..4b476da 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -1,41 +1,494 @@
-Pack transfer protocols
-=======================
-
-There are two Pack push-pull protocols.
-
-upload-pack (S) | fetch/clone-pack (C) protocol:
-
- # Tell the puller what commits we have and what their names are
- S: SHA1 name
- S: ...
- S: SHA1 name
- S: # flush -- it's your turn
- # Tell the pusher what commits we want, and what we have
- C: want name
- C: ..
- C: want name
- C: have SHA1
- C: have SHA1
- C: ...
- C: # flush -- occasionally ask "had enough?"
- S: NAK
- C: have SHA1
- C: ...
- C: have SHA1
- S: ACK
- C: done
- S: XXXXXXX -- packfile contents.
-
-send-pack | receive-pack protocol.
-
- # Tell the pusher what commits we have and what their names are
- C: SHA1 name
- C: ...
- C: SHA1 name
- C: # flush -- it's your turn
- # Tell the puller what the pusher has
- S: old-SHA1 new-SHA1 name
- S: old-SHA1 new-SHA1 name
- S: ...
- S: # flush -- done with the list
- S: XXXXXXX --- packfile contents.
+Packfile transfer protocols
+===========================
+
+Git supports transferring data in packfiles over the ssh://, git:// and
+file:// transports. There exist two sets of protocols, one for pushing
+data from a client to a server and another for fetching data from a
+server to a client. All three transports (ssh, git, file) use the same
+protocol to transfer data.
+
+The processes invoked in the canonical Git implementation are 'upload-pack'
+on the server side and 'fetch-pack' on the client side for fetching data;
+then 'receive-pack' on the server and 'send-pack' on the client for pushing
+data. The protocol functions to have a server tell a client what is
+currently on the server, then for the two to negotiate the smallest amount
+of data to send in order to fully update one or the other.
+
+Transports
+----------
+There are three transports over which the packfile protocol is
+initiated. The Git transport is a simple, unauthenticated server that
+takes the command (almost always 'upload-pack', though Git
+servers can be configured to be globally writable, in which 'receive-
+pack' initiation is also allowed) with which the client wishes to
+communicate and executes it and connects it to the requesting
+process.
+
+In the SSH transport, the client just runs the 'upload-pack'
+or 'receive-pack' process on the server over the SSH protocol and then
+communicates with that invoked process over the SSH connection.
+
+The file:// transport runs the 'upload-pack' or 'receive-pack'
+process locally and communicates with it over a pipe.
+
+Git Transport
+-------------
+
+The Git protocol starts off by sending the command and repository
+on the wire using the pkt-line format, followed by a null byte and a
+hostname paramater, terminated by a null byte.
+
+ 0032git-upload-pack /project.git\0host=myserver.com\0
+
+--
+ git-proto-request = request-command SP pathname NUL [ host-parameter NUL ]
+ request-command = "git-upload-pack" / "git-receive-pack" /
+ "git-upload-archive" ; case sensitive
+ pathname = *( %x01-ff ) ; exclude NUL
+ host-parameter = "host=" hostname [ ":" port ]
+--
+
+Only host-parameter is allowed in the git-proto-request. Clients
+MUST NOT attempt to send additional parameters. It is used for the
+git-daemon name based virtual hosting. See --interpolated-path
+option to git daemon, with the %H/%CH format characters.
+
+Basically what the Git client is doing to connect to an 'upload-pack'
+process on the server side over the Git protocol is this:
+
+ $ echo -e -n \
+ "0039git-upload-pack /schacon/gitbook.git\0host=example.com\0" |
+ nc -v example.com 9418
+
+
+SSH Transport
+-------------
+
+Initiating the upload-pack or receive-pack processes over SSH is
+executing the binary on the server via SSH remote execution.
+It is basically equivalent to running this:
+
+ $ ssh git.example.com "git-upload-pack '/project.git'"
+
+For a server to support Git pushing and pulling for a given user over
+SSH, that user needs to be able to execute one or both of those
+commands via the SSH shell that they are provided on login. On some
+systems, that shell access is limited to only being able to run those
+two commands, or even just one of them.
+
+In an ssh:// format URI, it's absolute in the URI, so the '/' after
+the host name (or port number) is sent as an argument, which is then
+read by the remote git-upload-pack exactly as is, so it's effectively
+an absolute path in the remote filesystem.
+
+ git clone ssh://user@example.com/project.git
+ |
+ v
+ ssh user@example.com "git-upload-pack '/project.git'"
+
+In a "user@host:path" format URI, its relative to the user's home
+directory, because the Git client will run:
+
+ git clone user@example.com:project.git
+ |
+ v
+ ssh user@example.com "git-upload-pack 'project.git'"
+
+The exception is if a '~' is used, in which case
+we execute it without the leading '/'.
+
+ ssh://user@example.com/~alice/project.git,
+ |
+ v
+ ssh user@example.com "git-upload-pack '~alice/project.git'"
+
+A few things to remember here:
+
+- The "command name" is spelled with dash (e.g. git-upload-pack), but
+ this can be overridden by the client;
+
+- The repository path is always quoted with single quotes.
+
+Fetching Data From a Server
+===========================
+
+When one Git repository wants to get data that a second repository
+has, the first can 'fetch' from the second. This operation determines
+what data the server has that the client does not then streams that
+data down to the client in packfile format.
+
+
+Reference Discovery
+-------------------
+
+When the client initially connects the server will immediately respond
+with a listing of each reference it has (all branches and tags) along
+with the object name that each reference currently points to.
+
+ $ echo -e -n "0039git-upload-pack
/schacon/gitbook.git\0host=example.com\0" |
+ nc -v example.com 9418
+ 00887217a7c7e582c46cec22a130adf4b9d7d950fba0 HEAD\0multi_ack
thin-pack side-band side-band-64k ofs-delta shallow no-progress
include-tag
+ 00441d3fcd5ced445d1abc402225c0b8a1299641f497 refs/heads/integration
+ 003f7217a7c7e582c46cec22a130adf4b9d7d950fba0 refs/heads/master
+ 003cb88d2441cac0977faf98efc80305012112238d9d refs/tags/v0.9
+ 003c525128480b96c89e6418b1e40909bf6c5b2d580f refs/tags/v1.0
+ 003fe92df48743b7bc7d26bcaabfddde0a1e20cae47c refs/tags/v1.0^{}
+ 0000
+
+Server SHOULD terminate each non-flush line using LF ("\n") terminator;
+client MUST NOT complain if there is no terminator.
+
+The returned response is a pkt-line stream describing each ref and
+its known value. The stream MUST be sorted by name according to
+the C locale ordering.
+
+If HEAD is a valid ref, HEAD MUST appear as the first advertised
+ref. If HEAD is not a valid ref, HEAD MUST NOT appear in the
+advertisement list at all, but other refs may still appear.
+
+The stream MUST include capability declarations behind a NUL on the
+first ref. The peeled value of a ref (that is "ref^{}") MUST be
+immediately after the ref itself, if presented. A conforming server
+MUST peel the ref if its an annotated tag.
+
+----
+ advertised-refs = (no-refs / list-of-refs)
+ flush-pkt
+
+ no-refs = PKT-LINE(zero-id SP "capabilities^{}"
+ NUL capability-list LF)
+
+ list-of-refs = first-ref *other-ref
+ first-ref = PKT-LINE(obj-id SP refname
+ NUL capability-list LF)
+
+ other-ref = PKT-LINE(other-tip / other-peeled)
+ other-tip = obj-id SP refname LF
+ other-peeled = obj-id SP refname "^{}" LF
+
+ capability-list = capability *(SP capability)
+ capability = 1*(LC_ALPHA / DIGIT / "-" / "_")
+ LC_ALPHA = %x61-7A
+----
+
+Server and client SHOULD use lowercase for SHA1, both MUST treat SHA1
+as case-insensitive.
+
+See protocol-capabilities.txt for a list of allowed server capabilities
+and descriptions.
+
+Packfile Negotiation
+--------------------
+After reference and capabilities discovery, the client can decide
+to terminate the connection by sending a flush-pkt, telling the
+server it can now gracefully terminate (as happens with the ls-remote
+command) or it can enter the negotiation phase, where the client and
+server determine what the minimal packfile necessary for transport is.
+
+Once the client has the initial list of references that the server
+has, as well as the list of capabilities, it will begin telling the
+server what objects it wants and what objects it has, so the server
+can make a packfile that only has the objects that the client needs.
+The client will also send a list of the capabilities it supports out
+of what the server said it could do with the first 'want' line.
+
+----
+ upload-request = want-list
+ have-list
+ compute-end
+
+ want-list = first-want
+ *additional-want
+ flush-pkt
+
+ first-want = PKT-LINE("want" SP obj-id SP capability-list LF)
+ additional-want = PKT-LINE("want" SP obj-id LF)
+
+ have-list = *have-line
+ have-line = PKT-LINE("have" SP obj-id LF)
+ compute-end = flush-pkt / PKT-LINE("done")
+----
+
+Clients MUST send all the obj-ids it wants from the reference
+discovery phase as 'want' lines. Clients MUST send at least one
+'want' command in the request body. Clients MUST NOT mention an
+obj-id in a 'want' command which did not appear in the response
+obtained through ref discovery.
+
+If client is requesting a shallow clone, it will now send a 'deepen'
+line with the depth it is requesting.
+
+Once all the "want"s (and optional 'deepen') are transferred,
+clients MUST send a flush-pkt. If the client has all the references
+on the server, client flushes and disconnects.
+
+TODO: shallow/unshallow response and document the deepen command in the ABNF.
+
+Now the client will send a list of the obj-ids it has using 'have'
+lines. In multi_ack mode, the canonical implementation will send up
+to 32 of these at a time, then will send a flush-pkt. The canonical
+implementation will skip ahead and send the next 32 immediately,
+so that there is always a block of 32 "in-flight on the wire" at a
+time.
+
+If the server reads 'have' lines, it then will respond by ACKing any
+of the obj-ids the client said it had that the server also has. The
+server will ACK obj-ids differently depending on which ack mode is
+signaled by the client.
+
+In multi_ack mode:
+
+ * the server will respond with 'ACK obj-id continue' for any common
+ commits.
+
+ * once the server has found an acceptable common base commit and is
+ ready to make a packfile, it will blindly ACK all 'have' obj-ids
+ back to the client.
+
+ * the server will then send a 'NACK' and then wait for another response
+ from the client - either a 'done' or another list of 'have' lines.
+
+In multi_ack_detailed mode:
+
+ * the server will differentiate the ACKs where it is signaling
+ that it is ready to send data with 'ACK obj-id ready' lines, and
+ signals the identified common commits with 'ACK obj-id common' lines.
+
+Without multi_ack:
+
+ * upload-pack sends "ACK obj-id" on the first common object it finds.
+ After that it says nothing until the client gives it a "done".
+
+ * upload-pack sends "NAK" on a flush-pkt if no common object
+ has been found yet. If one has been found, and thus an ACK
+ was already sent, its silent on the flush-pkt.
+
+After the client has gotten enough ACK responses that it can determine
+that the server has enough information to send an efficient packfile
+(in the canonical implementation, this is determined when it has received
+enough ACKs that it can color everything left in the --date-order queue
+as common with the server, or the --date-order queue is empty), or the
+client determines that it wants to give up (in the canonical implementation,
+this is determined when the client sends 256 'have' lines without getting
+any of them ACKed by the server - meaning there is nothing in common and
+the server should just send all it's objects), then the client will send
+a 'done' command. The 'done' command signals to the server that the client
+is ready to receive it's packfile data.
+
+However, the 256 limit *only* turns on in the canonical client
+implementation if we have received at least one "ACK %s continue"
+during a prior round. This helps to ensure that at least one common
+ancestor is found before we give up entirely.
+
+Once the 'done' line is read from the client, the server will either
+send a final 'ACK obj-id' or it will send a 'NAK'. The server only sends
+ACK after 'done' if there is at least one common base and multi_ack or
+multi_ack_detailed is enabled. The server always sends NAK after 'done'
+if there is no common base found.
+
+Then the server will start sending it's packfile data.
+
+----
+ server-response = *ack_multi ack / nak
+ ack_multi = PKT-LINE("ACK" SP obj-id ack_status LF)
+ ack_status = "continue" / "common" / "ready"
+ ack = PKT-LINE("ACK SP obj-id LF)
+ nak = PKT-LINE("NAK" LF)
+----
+
+A simple clone may look like this (with no 'have' lines):
+
+----
+ C: 0054want 74730d410fcb6603ace96f1dc55ea6196122532d\0multi_ack \
+ side-band-64k ofs-delta\n
+ C: 0032want 7d1665144a3a975c05f1f43902ddaf084e784dbe\n
+ C: 0032want 5a3f6be755bbb7deae50065988cbfa1ffa9ab68a\n
+ C: 0032want 7e47fe2bd8d01d481f44d7af0531bd93d3b21c01\n
+ C: 0032want 74730d410fcb6603ace96f1dc55ea6196122532d\n
+ C: 0000
+ C: 0009done\n
+
+ S: 0008NAK\n
+ S: [PACKFILE]
+----
+
+An incremental update (fetch) response might look like this:
+
+----
+ C: 0054want 74730d410fcb6603ace96f1dc55ea6196122532d\0multi_ack \
+ side-band-64k ofs-delta\n
+ C: 0032want 7d1665144a3a975c05f1f43902ddaf084e784dbe\n
+ C: 0032want 5a3f6be755bbb7deae50065988cbfa1ffa9ab68a\n
+ C: 0000
+ C: 0032have 7e47fe2bd8d01d481f44d7af0531bd93d3b21c01\n
+ C: [30 more have lines]
+ C: 0032have 74730d410fcb6603ace96f1dc55ea6196122532d\n
+ C: 0000
+
+ S: 003aACK 7e47fe2bd8d01d481f44d7af0531bd93d3b21c01 continue\n
+ S: 003aACK 74730d410fcb6603ace96f1dc55ea6196122532d continue\n
+ S: 0008NAK\n
+
+ C: 0009done\n
+
+ S: 003aACK 74730d410fcb6603ace96f1dc55ea6196122532d\n
+ S: [PACKFILE]
+----
+
+
+Packfile Data
+-------------
+
+Now that the client and server have done some negotiation about what
+the minimal amount of data that can be sent to the client is, the server
+will construct and send the required data in packfile format.
+
+See pack-format.txt for what the packfile itself actually looks like.
+
+If 'side-band' or 'side-band-64k' capabilities have been specified by
+the client, the server will send the packfile data multiplexed.
+
+Each packet starting with the packet-line length of the amount of data
+that follows, followed by a single byte specifying the sideband the
+following data is coming in on.
+
+In 'side-band' mode, it will send up to 999 data bytes plus 1 control
+code, for a total of up to 1000 bytes in a pkt-line. In 'side-band-64k'
+mode it will send up to 65519 data bytes plus 1 control code, for a
+total of up to 65520 bytes in a pkt-line.
+
+The sideband byte will be a '1', '2' or a '3'. Sideband '1' will contain
+packfile data, sideband '2' will be used for progress information that the
+client will generally print to stderr and sideband '3' is used for error
+information.
+
+If no 'side-band' capability was specified, the server will stream the
+entire packfile without multiplexing.
+
+
+Pushing Data To a Server
+========================
+
+Pushing data to a server will invoke the 'receive-pack' process on the
+server, which will allow the client to tell it which references it should
+update and then send all the data the server will need for those new
+references to be complete. Once all the data is received and validated,
+the server will then update it's references to what the client specified.
+
+Authentication
+--------------
+
+The protocol itself contains no authentication mechanisms. That is to be
+handled by the transport, such as SSH, before the 'receive-pack' process is
+invoked. If 'receive-pack' is configured over the Git transport, those
+repositories will be writable by anyone who can access that port (9418) as
+that transport is unauthenticated.
+
+Reference Discovery
+-------------------
+
+The reference discovery phase is done nearly the same way as it is in the
+fetching protocol. Each reference obj-id and name on the server is sent
+in packet-line format to the client, followed by a flush packet. The only
+real difference is that the capability listing is different - the only
+possible values are 'report-status', 'delete-refs' and 'ofs-delta'.
+
+Reference Update Request and Packfile Transfer
+----------------------------------------------
+
+Once the client knows what references the server is at, it can send a
+list of reference update requests. For each reference on the server
+that it wants to update, it sends a line listing the obj-id currently on
+the server, the obj-id the client would like to update it to and the name
+of the reference.
+
+This list is followed by a flush packet and then the packfile that should
+contain all the objects that the server will need to complete the new
+references.
+
+----
+ update-request = command-list [pack-file]
+
+ command-list = PKT-LINE(command NUL capability-list LF)
+ *PKT-LINE(command LF)
+ flush-pkt
+
+ command = create / delete / update
+ create = zero-id SP new-id SP name
+ delete = old-id SP zero-id SP name
+ update = old-id SP new-id SP name
+
+ old-id = obj-id
+ new-id = obj-id
+
+ pack-file = "PACK" 28*(OCTET)
+----
+
+If the receiving end does not support delete-refs, the sending end MUST
+NOT ask for delete command.
+
+The pack-file MUST NOT be sent if the only command used is 'delete'.
+
+A pack-file MUST be sent if either create or update command is used,
+even if the server already has all the necessary objects. In this
+case the client MUST send an empty pack-file. The only time this
+is likely to happen is if the client is doing something like creating
+a new branch that points to an existing obj-id.
+
+The server will receive the packfile, unpack it, then validate each
+reference that is being updated that it hasn't changed while the request
+was being processed (the obj-id is still the same as the old-id), and
+it will run any update hooks to make sure that the update is acceptable.
+If all of that is fine, the server will then update the references.
+
+Report Status
+-------------
+
+After receiving the pack data from the sender, the client sends a
+report if 'report-status' capability was sent to the server.
+It is a short listing of what happened in that update. It will first
+list the status of the packfile unpacking as either 'unpack ok' or
+'unpack [error]'. Then it will list the status for each of the references
+that it tried to update. Each line be either 'ok [refname]' if the
+update was successful, or 'ng [refname] [error]' if the update was not.
+
+----
+ report-status = unpack-status
+ 1*(command-status)
+ flush-pkt
+
+ unpack-status = PKT-LINE("unpack" SP unpack-result LF)
+ unpack-result = "ok" / error-msg
+
+ command-status = command-ok / command-fail
+ command-ok = PKT-LINE("ok" SP refname LF)
+ command-fail = PKT-LINE("ng" SP refname SP error-msg LF)
+
+ error-msg = 1*(OCTECT) ; where not "ok"
+----
+
+Updates can be unsuccessful for a number of reasons. The reference can have
+changed since the reference discovery phase was originally sent, meaning
+someone pushed in the meantime. The reference being pushed could be a
+non-fast-forward reference and the update hooks or configuration could be
+set to not allow that, etc. Also, some references can be updated while others
+can be rejected.
+
+An example client/server communication might look like this:
+
+----
+ S: 007c74730d410fcb6603ace96f1dc55ea6196122532d
refs/heads/local\0report-status delete-refs ofs-delta\n
+ S: 003e7d1665144a3a975c05f1f43902ddaf084e784dbe refs/heads/debug\n
+ S: 003f74730d410fcb6603ace96f1dc55ea6196122532d refs/heads/master\n
+ S: 003f74730d410fcb6603ace96f1dc55ea6196122532d refs/heads/team\n
+ S: 0000
+
+ C: 003e7d1665144a3a975c05f1f43902ddaf084e784dbe
74730d410fcb6603ace96f1dc55ea6196122532d refs/heads/debug\n
+ C: 003e74730d410fcb6603ace96f1dc55ea6196122532d
5a3f6be755bbb7deae50065988cbfa1ffa9ab68a refs/heads/master\n
+ C: 0000
+ C: [PACKDATA]
+
+ S: 000aunpack ok\n
+ S: 0014ok refs/heads/debug\n
+ S: 0026ng refs/heads/master non-fast-forward\n
+----
diff --git a/Documentation/technical/protocol-capabilities.txt
b/Documentation/technical/protocol-capabilities.txt
new file mode 100644
index 0000000..f4bf986
--- /dev/null
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -0,0 +1,186 @@
+Git Protocol Capabilities
+=========================
+
+Servers SHOULD support all capabilities defined in this document.
+
+On the very first line of the initial server response of either
+receive-pack and upload-pack the first reference is followed by
+a null byte and then a list of space delimited server capabilities.
+These allow the server to declare what it can and cannot support
+to the client.
+
+Client will then send a space separated list of capabilities it
+can support. The client SHOULD NOT ask for capabilities the server
+did not say it supports.
+
+Server MUST ignore capabilities it does not understand. Server MUST
+NOT ignore capabilities that client requested and server advertised.
+
+The 'report-status' and 'delete-refs' capabilities are sent and
+recognized by the receive-pack (push to server) process.
+
+The 'ofs-delta' capability is sent and recognized by both upload-pack
+and receive-pack protocols.
+
+All other capabilities are only recognized by the upload-pack (fetch
+from server) process.
+
+multi_ack
+---------
+
+The 'multi_ack' capability allows the server to return "ACK obj-id
+continue" as soon as it finds a commit that it can use as a common
+base, between the client's wants and the client's have set.
+
+By sending this early, the server can potentially head off the client
+from walking any further down that particular branch of the client's
+repository history. The client may still need to walk down other
+branches, sending have lines for those, until the server has a
+complete cut across the DAG, or the client has said "done".
+
+Without multi_ack, a client sends have lines in --date-order until
+the server has found a common base. That means the client will send
+have lines that are already known by the server to be common, because
+they overlap in time with another branch that the server hasn't found
+a common base on yet.
+
+For example suppose the client has commits in caps that the server
+doesn't and the server has commits in lower case that the client
+doesn't, as in the following diagram:
+
+ +---- u ---------------------- x
+ / +----- y
+ / /
+ a -- b -- c -- d -- E -- F
+ \
+ +--- Q -- R -- S
+
+If the client wants x,y and starts out by saying have F,S, the server
+doesn't know what F,S is. Eventually the client says "have d" and
+the server sends "ACK d continue" to let the client know to stop
+walking down that line (so don't send c-b-a), but its not done yet,
+it needs a base for x. The client keeps going with S-R-Q, until a
+gets reached, at which point the server has a clear base and it all
+ends.
+
+Without multi_ack the client would have sent that c-b-a chain anyway,
+interleaved with S-R-Q.
+
+thin-pack
+---------
+
+This capability implies that the server can send 'thin' packs, packs
+which do not contain base objects; if those base objects are available
+on client side. Client requests 'thin-pack' capability when it
+understands how to "thicken" them adding required delta bases making
+them self contained.
+
+Client MUST NOT request 'thin-pack' capability if it cannot turn thin
+packs into proper independent packs.
+
+
+side-band, side-band-64k
+------------------------
+
+This means that server can send, and client understand multiplexed
+progress reports and error info interleaved with the packfile itself.
+
+These two options are mutually exclusive. A modern client always
+favors 'side-band-64k'.
+
+Either mode indicates that the packfile data will be streamed broken
+up into packets of up to either 1000 bytes in the case of 'side_band',
+or 65520 bytes in the case of 'side_band_64k'. Each packet is made up
+of a leading 4-byte pkt-line length of how much data is in the packet,
+followed by a 1-byte stream code, followed by the actual data.
+
+The stream code can be one of:
+
+ 1 - pack data
+ 2 - progress messages
+ 3 - fatal error message just before stream aborts
+
+The "side-band-64k" capability came about as a way for newer clients
+that can handle much larger packets to request packets that are
+actually crammed nearly full, while maintaining backward compatibility
+for the older clients.
+
+Further, with side-band and its up to 1000-byte messages, it's actually
+999 bytes of payload and 1 byte for the stream code. With side-band-64k,
+same deal, you have up to 65519 bytes of data and 1 byte for the stream
+code.
+
+The client MUST send only maximum of one of "side-band" and "side-
+band-64k". Server MUST diagnose it as an error if client requests
+both.
+
+ofs-delta
+---------
+
+Server can send, and client understand PACKv2 with delta refering to
+its base by position in pack rather than by SHA-1. That is, they can
+send/read OBJ_OFS_DELTA (aka type 6) in a packfile.
+
+shallow
+-------
+
+This capability adds "deepen", "shallow" and "unshallow" commands to
+the fetch-pack/upload-pack protocol so clients can request shallow
+clones.
+
+no-progress
+-----------
+
+The client was started with "git clone -q" or something, and doesn't
+want that side band 2. Basically the client just says "I do not
+wish to receive stream 2 on sideband, so do not send it to me, and if
+you did, I will drop it on the floor anyway". However, the sideband
+channel 3 is still used for error responses.
+
+include-tag
+-----------
+
+The 'include-tag' capability is about sending annotated tags if we are
+sending objects they point to. If we pack an object to the client, and
+a tag object points exactly at that object, we pack the tag object too.
+In general this allows a client to get all new annotated tags when it
+fetches a branch, in a single network connection.
+
+Clients MAY always send include-tag, hardcoding it into a request when
+the server advertises this capability. The decision for a client to
+request include-tag only has to do with the client's desires for tag
+data, whether or not a server had advertised objects in the
+refs/tags/* namespace.
+
+Servers MUST pack the tags if their referrant is packed and the client
+has requested include-tags.
+
+Clients MUST be prepared for the case where a server has ignored
+include-tag and has not actually sent tags in the pack. In such
+cases the client SHOULD issue a subsequent fetch to acquire the tags
+that include-tag would have otherwise given the client.
+
+The server SHOULD send include-tag, if it supports it, regardless
+of whether or not there are tags available.
+
+report-status
+-------------
+
+The upload-pack process can receive a 'report-status' capability,
+which tells it that the client wants a report of what happened after
+a packfile upload and reference update. If the pushing client requests
+this capability, after unpacking and updating references the server
+will respond with whether the packfile unpacked successfully and if
+each reference was updated successfully. If any of those were not
+successful, it will send back an error message. See pack-protocol.txt
+for example messages.
+
+delete-refs
+-----------
+
+If the server sends back the 'delete-refs' capability, it means that
+it is capable of accepting an all-zeroed SHA-1 value as the target
+value of a reference update. It is not sent back by the client, it
+simply informs the client that it can be sent zeroed SHA-1 values
+to delete references.
+
diff --git a/Documentation/technical/protocol-common.txt
b/Documentation/technical/protocol-common.txt
new file mode 100644
index 0000000..2dca642
--- /dev/null
+++ b/Documentation/technical/protocol-common.txt
@@ -0,0 +1,96 @@
+Documentation Common to Pack and Http Protocols
+===============================================
+
+ABNF Notation
+-------------
+
+ABNF notation as described by RFC 5234 is used within the protocol documents,
+except the following replacement core rules are used:
+----
+ HEXDIG = DIGIT / "a" / "b" / "c" / "d" / "e" / "f"
+----
+
+We also define the following common rules:
+----
+ NUL = %x00
+ zero-id = 40*"0"
+ obj-id = 40*(HEXDIGIT)
+
+ refname = "HEAD"
+ refname /= "refs/" <see discussion below>
+----
+
+A refname is a hierarichal octet string beginning with "refs/" and
+not violating the 'git-check-ref-format' command's validation rules.
+More specifically, they:
+
+. They can include slash `/` for hierarchical (directory)
+ grouping, but no slash-separated component can begin with a
+ dot `.`.
+
+. They must contain at least one `/`. This enforces the presence of a
+ category like `heads/`, `tags/` etc. but the actual names are not
+ restricted.
+
+. They cannot have two consecutive dots `..` anywhere.
+
+. They cannot have ASCII control characters (i.e. bytes whose
+ values are lower than \040, or \177 `DEL`), space, tilde `~`,
+ caret `{caret}`, colon `:`, question-mark `?`, asterisk `*`,
+ or open bracket `[` anywhere.
+
+. They cannot end with a slash `/` nor a dot `.`.
+
+. They cannot end with the sequence `.lock`.
+
+. They cannot contain a sequence `@{`.
+
+. They cannot contain a `\\`.
+
+
+pkt-line Format
+---------------
+
+Much (but not all) of the payload is described around pkt-lines.
+
+A pkt-line is a variable length binary string. The first four bytes
+of the line, the pkt-len, indicates the total length of the line,
+in hexadecimal. The pkt-len includes the 4 bytes used to contain
+the length's hexadecimal representation.
+
+A pkt-line MAY contain binary data, so implementors MUST ensure
+pkt-line parsing/formatting routines are 8-bit clean.
+
+A non-binary line SHOULD BE terminated by an LF, which if present
+MUST be included in the total length.
+
+The maximum length of a pkt-line's data component is 65520 bytes.
+Implementations MUST NOT send pkt-line whose length exceeds 65524
+(65520 bytes of payload + 4 bytes of length data).
+
+Implementations SHOULD NOT send an empty pkt-line ("0004").
+
+A pkt-line with a length field of 0 ("0000"), called a flush-pkt,
+is a special case and MUST be handled differently than an empty
+pkt-line ("0004").
+
+----
+ pkt-line = data-pkt / flush-pkt
+
+ data-pkt = pkt-len pkt-payload
+ pkt-len = 4*(HEXDIG)
+ pkt-payload = (pkt-len - 4)*(OCTET)
+
+ flush-pkt = "0000"
+----
+
+Examples (as C-style strings):
+
+----
+ pkt-line actual value
+ ---------------------------------
+ "0006a\n" "a\n"
+ "0005a" "a"
+ "000bfoobar\n" "foobar\n"
+ "0004" ""
+----
--
1.6.5.2.340.g000a3e
^ permalink raw reply related
* Re: [PATCH] t1200: cleanup and modernize test style
From: Junio C Hamano @ 2009-11-04 5:50 UTC (permalink / raw)
To: Stephen Boyd; +Cc: git
In-Reply-To: <1257282328-6491-1-git-send-email-bebarino@gmail.com>
Stephen Boyd <bebarino@gmail.com> writes:
> Many parts of the tests in t1200 are run outside the test harness,
> circumventing the usefulness of -v and spewing messages to stdout when
> -v isn't used. Fix these problems by modernizing the test a bit.
>
> An extra test_done has existed since commit 6a74642 (git-commit --amend:
> two fixes., 2006-04-20) leading to the last 6 tests never being run.
> Remove it and teach the resolve merge test about fast-forward merges.
>
> Finally, we remove the TODO notes, because fetch, push, and clone have
> their own tests since t1200 was introduced and we're not going to add
> them here 4 years later.
>
> Signed-off-by: Stephen Boyd <bebarino@gmail.com>
> ---
>
> I saw some output when running this test and thought it could be modernized a
> bit.
Thanks.
The sequence of commands are suppopsed to match what the user manual
teaches, and I suspect we have had quite a many updates to the manual
since this test script was last touched. Do you happen to know if they
still match the manual?
> t/t1200-tutorial.sh | 131 +++++++++++++++++++++++++++++----------------------
> 1 files changed, 74 insertions(+), 57 deletions(-)
>
> diff --git a/t/t1200-tutorial.sh b/t/t1200-tutorial.sh
> index 67e637b..16c5b6d 100755
> --- a/t/t1200-tutorial.sh
> +++ b/t/t1200-tutorial.sh
> @@ -7,14 +7,18 @@ test_description='A simple turial in the form of a test case'
>
> . ./test-lib.sh
>
> -echo "Hello World" > hello
> -echo "Silly example" > example
> +test_expect_success 'blob' '
> + echo "Hello World" > hello &&
> + echo "Silly example" > example &&
>
> -git update-index --add hello example
> + git update-index --add hello example &&
>
> -test_expect_success 'blob' "test blob = \"$(git cat-file -t 557db03)\""
> + test blob = "$(git cat-file -t 557db03)"
> +'
>
> -test_expect_success 'blob 557db03' "test \"Hello World\" = \"$(git cat-file blob 557db03)\""
> +test_expect_success 'blob 557db03' '
> + test "Hello World" = "$(git cat-file blob 557db03)"
> +'
>
> echo "It's a new day for git" >>hello
> cat > diff.expect << EOF
> @@ -26,25 +30,33 @@ index 557db03..263414f 100644
> Hello World
> +It's a new day for git
> EOF
> -git diff-files -p > diff.output
> -test_expect_success 'git diff-files -p' 'cmp diff.expect diff.output'
> -git diff > diff.output
> -test_expect_success 'git diff' 'cmp diff.expect diff.output'
> -
> -tree=$(git write-tree 2>/dev/null)
>
> -test_expect_success 'tree' "test 8988da15d077d4829fc51d8544c097def6644dbb = $tree"
> +test_expect_success 'git diff-files -p' '
> + git diff-files -p > diff.output &&
> + cmp diff.expect diff.output
> +'
>
> -output="$(echo "Initial commit" | git commit-tree $(git write-tree) 2>&1 > .git/refs/heads/master)"
> +test_expect_success 'git diff' '
> + git diff > diff.output &&
> + cmp diff.expect diff.output
> +'
>
> -git diff-index -p HEAD > diff.output
> -test_expect_success 'git diff-index -p HEAD' 'cmp diff.expect diff.output'
> +test_expect_success 'tree' '
> + tree=$(git write-tree 2>/dev/null)
> + test 8988da15d077d4829fc51d8544c097def6644dbb = $tree
> +'
>
> -git diff HEAD > diff.output
> -test_expect_success 'git diff HEAD' 'cmp diff.expect diff.output'
> +test_expect_success 'git diff-index -p HEAD' '
> + echo "Initial commit" | \
> + git commit-tree $(git write-tree) 2>&1 > .git/refs/heads/master &&
> + git diff-index -p HEAD > diff.output &&
> + cmp diff.expect diff.output
> +'
>
> -#rm hello
> -#test_expect_success 'git read-tree --reset HEAD' "git read-tree --reset HEAD ; test \"hello: needs update\" = \"$(git update-index --refresh)\""
> +test_expect_success 'git diff HEAD' '
> + git diff HEAD > diff.output &&
> + cmp diff.expect diff.output
> +'
>
> cat > whatchanged.expect << EOF
> commit VARIABLE
> @@ -69,39 +81,45 @@ index 0000000..557db03
> +Hello World
> EOF
>
> -git whatchanged -p --root | \
> - sed -e "1s/^\(.\{7\}\).\{40\}/\1VARIABLE/" \
> +test_expect_success 'git whatchanged -p --root' '
> + git whatchanged -p --root | \
> + sed -e "1s/^\(.\{7\}\).\{40\}/\1VARIABLE/" \
> -e "2,3s/^\(.\{8\}\).*$/\1VARIABLE/" \
> -> whatchanged.output
> -test_expect_success 'git whatchanged -p --root' 'cmp whatchanged.expect whatchanged.output'
> -
> -git tag my-first-tag
> -test_expect_success 'git tag my-first-tag' 'cmp .git/refs/heads/master .git/refs/tags/my-first-tag'
> + > whatchanged.output &&
> + cmp whatchanged.expect whatchanged.output
> +'
>
> -# TODO: test git clone
> +test_expect_success 'git tag my-first-tag' '
> + git tag my-first-tag &&
> + cmp .git/refs/heads/master .git/refs/tags/my-first-tag
> +'
>
> -git checkout -b mybranch
> -test_expect_success 'git checkout -b mybranch' 'cmp .git/refs/heads/master .git/refs/heads/mybranch'
> +test_expect_success 'git checkout -b mybranch' '
> + git checkout -b mybranch &&
> + cmp .git/refs/heads/master .git/refs/heads/mybranch
> +'
>
> cat > branch.expect <<EOF
> master
> * mybranch
> EOF
>
> -git branch > branch.output
> -test_expect_success 'git branch' 'cmp branch.expect branch.output'
> +test_expect_success 'git branch' '
> + git branch > branch.output &&
> + cmp branch.expect branch.output
> +'
>
> -git checkout mybranch
> -echo "Work, work, work" >>hello
> -git commit -m 'Some work.' -i hello
> +test_expect_success 'git resolve now fails' '
> + git checkout mybranch &&
> + echo "Work, work, work" >>hello &&
> + git commit -m "Some work." -i hello &&
>
> -git checkout master
> + git checkout master &&
>
> -echo "Play, play, play" >>hello
> -echo "Lots of fun" >>example
> -git commit -m 'Some fun.' -i hello example
> + echo "Play, play, play" >>hello &&
> + echo "Lots of fun" >>example &&
> + git commit -m "Some fun." -i hello example &&
>
> -test_expect_success 'git resolve now fails' '
> test_must_fail git merge -m "Merge work in mybranch" mybranch
> '
>
> @@ -112,10 +130,6 @@ Play, play, play
> Work, work, work
> EOF
>
> -git commit -m 'Merged "mybranch" changes.' -i hello
> -
> -test_done
> -
> cat > show-branch.expect << EOF
> * [master] Merged "mybranch" changes.
> ! [mybranch] Some work.
> @@ -124,21 +138,26 @@ cat > show-branch.expect << EOF
> *+ [mybranch] Some work.
> EOF
>
> -git show-branch --topo-order master mybranch > show-branch.output
> -test_expect_success 'git show-branch' 'cmp show-branch.expect show-branch.output'
> -
> -git checkout mybranch
> +test_expect_success 'git show-branch' '
> + git commit -m "Merged \"mybranch\" changes." -i hello &&
> + git show-branch --topo-order master mybranch > show-branch.output &&
> + cmp show-branch.expect show-branch.output
> +'
>
> cat > resolve.expect << EOF
> -Updating from VARIABLE to VARIABLE
> +Updating VARIABLE..VARIABLE
> +Fast forward
> example | 1 +
> hello | 1 +
> 2 files changed, 2 insertions(+), 0 deletions(-)
> EOF
>
> -git merge -s "Merge upstream changes." master | \
> - sed -e "1s/[0-9a-f]\{40\}/VARIABLE/g" >resolve.output
> -test_expect_success 'git resolve' 'cmp resolve.expect resolve.output'
> +test_expect_success 'git resolve' '
> + git checkout mybranch &&
> + git merge -s resolve master | \
> + sed -e "1s/[0-9a-f]\{7\}/VARIABLE/g" >resolve.output &&
> + cmp resolve.expect resolve.output
> +'
>
> cat > show-branch2.expect << EOF
> ! [master] Merged "mybranch" changes.
> @@ -147,12 +166,10 @@ cat > show-branch2.expect << EOF
> -- [master] Merged "mybranch" changes.
> EOF
>
> -git show-branch --topo-order master mybranch > show-branch2.output
> -test_expect_success 'git show-branch' 'cmp show-branch2.expect show-branch2.output'
> -
> -# TODO: test git fetch
> -
> -# TODO: test git push
> +test_expect_success 'git show-branch' '
> + git show-branch --topo-order master mybranch > show-branch2.output &&
> + cmp show-branch2.expect show-branch2.output
> +'
>
> test_expect_success 'git repack' 'git repack'
> test_expect_success 'git prune-packed' 'git prune-packed'
> --
> 1.6.5.2.181.gd6f41
^ permalink raw reply
* Re: [PATCH bg/format-patch-p-noop] log-tree: always add --- marker when options are patch and a stat
From: Junio C Hamano @ 2009-11-04 5:49 UTC (permalink / raw)
To: Stephen Boyd; +Cc: git, Björn Gustavsson
In-Reply-To: <1257283456-7007-1-git-send-email-bebarino@gmail.com>
Stephen Boyd <bebarino@gmail.com> writes:
> Previously, the three dash marker (---) would only be added if the diff
> output format was a patch and diffstat (usually -p and --stat). Now that
> patches are always generated by format-patch regardless of the stat
> format being used (--stat, --raw, --numstat, etc.), always add the three
> dash marker when a patch is being generated and a stat option is used.
>
> This allows users to choose the stat format they want and unifies the
> format of patches with stats. It also make patches easier to apply when
> generated by format-patch with non-standard stat options as the stat is
> no longer considered part of the commit message.
>
> Signed-off-by: Stephen Boyd <bebarino@gmail.com>
> ---
I actually am more worried about 68daa64 from 14 months ago, as I vaguely
recall seeing an explicit user request that in some community the diffstat
information is unwanted on their mailing list, and I am reasonably sure
that "-p suppresses --stat" was done deliberately to satisfy them (even
though it may have been a suboptimal UI and --no-stat might have been a
lot more straightforward).
Even though I personally find the stat information very useful, I would be
happier if somebody reverts the bg/format-patch-p-noop series and instead
fixes the regression caused by 68daa64, and does so without touching any
output from the low-level plumbing like diff-tree that may be used by
scripts.
With older (say 1.6.0) git, format-patch with the -p option does not give
these three-dash lines, and it does look funny. Even though the same
funniness appears only when you use --raw or --numstat with the current
code, if we fix "-p" to suppress the default "--stat", this will become an
issue again.
> I'm not sure this is wanted though and I guess this could break people's
> scripts. Are people actually using --numstat or --raw to put the stat into
> the commit message?
I am not worried so much about "format-patch --any-option" output; I think
it is sane to have three-dash line in it and that is what people expect to
see.
I however think people used "diff-tree --pretty --raw" as a mechanism to
obtain statistics. A script can easily see where the header is and where
messages are (they are four-space indented), and what remains after
stripping them give you the list of paths each commit touches. --numstat
was invented to help this kind of application gather line-level statistics
more easily, and I am a bit reluctant to suddenly start giving three-dash
in their output. It will upset what is reading from the pipe downstream.
In an ideal world, I would probably say:
* format-patch should have three-dash after the commit message, no matter
what format the patch is asked for, and it always will give patch text.
* format-patch -p should be reinstated as a way to ask for "just patch
text, no diffstat". Introducing a new option --no-stat _in addition_
to improve the UI is Ok.
* format-patch -U<n> should not be mistaken as a request to suppress
diffstat; what 68daa64 _tried_ to do was worthy.
* Other commands of "log" family that understand -p/-U<n> to produce
patch text should also give three-dash after the log message, and no
three-dash when they don't produce patch text.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox