* Re: [msysGit] [PATCH 06/17] mingw.c: Use the O_BINARY flag to open files
From: Alexey Borzenkov @ 2009-09-14 19:06 UTC (permalink / raw)
To: Marius Storm-Olsen; +Cc: Johannes.Schindelin, msysgit, git, lznuaa
In-Reply-To: <160ceafba5f58cfcadb13380e3d8ef724009b700.1252925290.git.mstormo@gmail.com>
On Mon, Sep 14, 2009 at 5:11 PM, Marius Storm-Olsen <mstormo@gmail.com> wrote:
> On Windows, binary files must be opened using the O_BINARY flag for
> untranslated mode. MinGW does this for us automatically, but Microsoft
> Visual C++ does not.
Wouldn't it be better to set explicitly set _fmode = _O_BINARY when
_MSC_VER is defined? _CRT_fmode is mingw specific, that in addition to
setting _fmode also sets mode on stdin/stdout/stderr (see
http://www.koders.com/c/fid2B507DE94598397757D615EDF52267578AD838A2.aspx).
Current patch will not set binary mode for files opened with fopen, as
well as stdin/stdout/stderr. One catch might be that declaring it like
"unsigned int _fmode = _O_BINARY" might not work, it seems that you
are supposed to set it somewhere in the code (see
http://msdn.microsoft.com/en-us/library/ee2849wt(VS.80).aspx). Perhaps
implementing a function like _mingw32_init_fmode in crc1.c in
mingw-runtime and calling it early in main() would be better?
Thanks,
Alexey.
^ permalink raw reply
* Re: [PATCH 15/17] Add MSVC to Makefile
From: Marius Storm-Olsen @ 2009-09-14 19:09 UTC (permalink / raw)
To: Alex Riesen; +Cc: Johannes.Schindelin, msysgit, git, lznuaa
In-Reply-To: <81b0412b0909140706s32e888ddwe4bbd668c3a7ccfd@mail.gmail.com>
Alex Riesen said the following on 14.09.2009 16:06:
> On Mon, Sep 14, 2009 at 15:11, Marius Storm-Olsen <mstormo@gmail.com> wrote:
>> @@ -1327,7 +1379,7 @@ strip: $(PROGRAMS) git$X
>> git.o: git.c common-cmds.h GIT-CFLAGS
>> $(QUIET_CC)$(CC) -DGIT_VERSION='"$(GIT_VERSION)"' \
>> '-DGIT_HTML_PATH="$(htmldir_SQ)"' \
>> - $(ALL_CFLAGS) -c $(filter %.c,$^)
>> + $(ALL_CFLAGS) -o git.o -c $(filter %.c,$^)
>
> Why do you use "-o git.o" instead of "-o $@"?
No reason really. I'll resend tomorrow.
--
.marius
^ permalink raw reply
* Re: Re: Tracking remote branches
From: Heiko Voigt @ 2009-09-14 19:19 UTC (permalink / raw)
To: Thiago Farina; +Cc: Nicolas Sebrecht, git
In-Reply-To: <a4c8a6d00909141130o628a069fu4c489b25f95f543d@mail.gmail.com>
On Mon, Sep 14, 2009 at 03:30:10PM -0300, Thiago Farina wrote:
> On Mon, Sep 14, 2009 at 2:01 AM, Nicolas Sebrecht <nicolas.s.dev@gmx.fr> wrote:
> > The 13/09/09, Thiago Farina wrote:
> > $ git checkout -t origin/maint
> >
> >> OK, now I switched to maint branch. And then I did:
> >> $ git status
> >>
> >> # On branch maint
> >> # Changes to be committed
> >> # (use "git reset HEAD <file>..." to unstage)
> >> #
> >> # new file: git-remote-curl
> >> # new file: git-replace
> >> #
> >>
> >> What happened here? What I have to do now?
>
> Why these files has this status? Is supposed to me to do something
> about that when this situation occurs?
I just checked and this is because you probably made a build on the
master branch which creates these files. They are untracked but ignored
through the .gitignore file in the master branch.
Now the situation is that the .gitignore on the current 'maint' branch
does not have these files ignored because they are quite new.
In this situation you can do a
git clean -f
which gets rid of all untracked but not ignored files (all files that
git status shows as new) or
make clean
while you are still on the master branch.
cheers Heiko
^ permalink raw reply
* Re: [PATCH 08/17] Test for WIN32 instead of __MINGW32_
From: Junio C Hamano @ 2009-09-14 19:31 UTC (permalink / raw)
To: Marius Storm-Olsen; +Cc: Johannes.Schindelin, msysgit, git, lznuaa
In-Reply-To: <8da5dcea21ce49b3a57fd224c088785a9f98e867.1252925290.git.mstormo@gmail.com>
Marius Storm-Olsen <mstormo@gmail.com> writes:
> From: Frank Li <lznuaa@gmail.com>
>
> The code which is conditional on MinGW32 is actually conditional on Windows.
> Use the WIN32 symbol, which is defined by the MINGW32 and MSVC environments,
> but not by Cygwin.
>
> Signed-off-by: Frank Li <lznuaa@gmail.com>
> Signed-off-by: Marius Storm-Olsen <mstormo@gmail.com>
The description sounds very sensible; I wish I read this one first, before
wasting time scratching my head while reading 03/17. Shouldn't that patch
be squashed into this one?
> ---
> help.c | 2 +-
> pager.c | 4 ++--
> run-command.c | 8 ++++----
> run-command.h | 2 +-
> setup.c | 2 +-
> 5 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/help.c b/help.c
> index fd51b8e..e8db31f 100644
> --- a/help.c
> +++ b/help.c
> @@ -126,7 +126,7 @@ static int is_executable(const char *name)
> !S_ISREG(st.st_mode))
> return 0;
>
> -#ifdef __MINGW32__
> +#ifdef WIN32
> { /* cannot trust the executable bit, peek into the file instead */
> char buf[3] = { 0 };
> int n;
> diff --git a/pager.c b/pager.c
> index f416d38..86facec 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -9,7 +9,7 @@
>
> static int spawned_pager;
>
> -#ifndef __MINGW32__
> +#ifndef WIN32
> static void pager_preexec(void)
> {
> /*
> @@ -72,7 +72,7 @@ void setup_pager(void)
> static const char *env[] = { "LESS=FRSX", NULL };
> pager_process.env = env;
> }
> -#ifndef __MINGW32__
> +#ifndef WIN32
> pager_process.preexec_cb = pager_preexec;
> #endif
> if (start_command(&pager_process))
> diff --git a/run-command.c b/run-command.c
> index 91f6d2e..cb006e7 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -75,7 +75,7 @@ fail_pipe:
>
> trace_argv_printf(cmd->argv, "trace: run_command:");
>
> -#ifndef __MINGW32__
> +#ifndef WIN32
> fflush(NULL);
> cmd->pid = fork();
> if (!cmd->pid) {
> @@ -315,7 +315,7 @@ int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const
> return run_command(&cmd);
> }
>
> -#ifdef __MINGW32__
> +#ifdef WIN32
> static unsigned WINAPI run_thread(void *data)
> {
> struct async *async = data;
> @@ -331,7 +331,7 @@ int start_async(struct async *async)
> return error("cannot create pipe: %s", strerror(errno));
> async->out = pipe_out[0];
>
> -#ifndef __MINGW32__
> +#ifndef WIN32
> /* Flush stdio before fork() to avoid cloning buffers */
> fflush(NULL);
>
> @@ -360,7 +360,7 @@ int start_async(struct async *async)
>
> int finish_async(struct async *async)
> {
> -#ifndef __MINGW32__
> +#ifndef WIN32
> int ret = wait_or_whine(async->pid, "child process", 0);
> #else
> DWORD ret = 0;
> diff --git a/run-command.h b/run-command.h
> index 0c00b25..fb34209 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -70,7 +70,7 @@ struct async {
> int (*proc)(int fd, void *data);
> void *data;
> int out; /* caller reads from here and closes it */
> -#ifndef __MINGW32__
> +#ifndef WIN32
> pid_t pid;
> #else
> HANDLE tid;
> diff --git a/setup.c b/setup.c
> index e3781b6..029371e 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -41,7 +41,7 @@ const char *prefix_path(const char *prefix, int len, const char *path)
> const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
> {
> static char path[PATH_MAX];
> -#ifndef __MINGW32__
> +#ifndef WIN32
> if (!pfx || !*pfx || is_absolute_path(arg))
> return arg;
> memcpy(path, pfx, pfx_len);
> --
> 1.6.2.1.418.g33d56.dirty
^ permalink raw reply
* Re: [PATCH 08/17] Test for WIN32 instead of __MINGW32_
From: Marius Storm-Olsen @ 2009-09-14 19:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes.Schindelin, msysgit, git, lznuaa
In-Reply-To: <7v1vm9cohd.fsf@alter.siamese.dyndns.org>
Junio C Hamano said the following on 14.09.2009 21:31:
> Marius Storm-Olsen <mstormo@gmail.com> writes:
>
>> From: Frank Li <lznuaa@gmail.com>
>>
>> The code which is conditional on MinGW32 is actually conditional on Windows.
>> Use the WIN32 symbol, which is defined by the MINGW32 and MSVC environments,
>> but not by Cygwin.
>>
>> Signed-off-by: Frank Li <lznuaa@gmail.com>
>> Signed-off-by: Marius Storm-Olsen <mstormo@gmail.com>
>
> The description sounds very sensible; I wish I read this one first, before
> wasting time scratching my head while reading 03/17. Shouldn't that patch
> be squashed into this one?
Heh, yeah, I agree. After patch got updated with suggestions from last
roll, it clearly makes more sense hear now. I'll update tomorrow. Thanks.
--
.marius
^ permalink raw reply
* Re: [PATCH 14/17] Add ftruncate implementation for MSVC
From: Junio C Hamano @ 2009-09-14 19:48 UTC (permalink / raw)
To: Marius Storm-Olsen; +Cc: Johannes.Schindelin, msysgit, git, lznuaa
In-Reply-To: <c95a367520435feec0833d2d0edc33d86cf939a8.1252925290.git.mstormo@gmail.com>
Marius Storm-Olsen <mstormo@gmail.com> writes:
> The MSVC Posix implementation doesn't contain ftruncate, so add our own
> which can handle large files (64bit offsets).
>
> Signed-off-by: Marius Storm-Olsen <mstormo@gmail.com>
> ---
> compat/msvc.c | 8 ++++++++
> compat/msvc.h | 2 ++
> 2 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/compat/msvc.c b/compat/msvc.c
> index ac04a4c..b96b045 100644
> --- a/compat/msvc.c
> +++ b/compat/msvc.c
> @@ -32,4 +32,12 @@ int closedir(DIR *dir)
> return 0;
> }
>
> +int ftruncate(int fd, __int64 length)
> +{
> + HANDLE fh = (HANDLE)_get_osfhandle(fd);
> + if (!fh || _lseeki64(fd, length, SEEK_SET))
> + return -1;
> + return SetEndOfFile(fh) ? 0 : -1;
> +}
> +
Wouldn't it break this toy program? ftruncate() should preserve the file
offset and use of the seek in the above feels iffy.
#include <stdlib.h>
#include <sys/types.h>
#include <fcntl.h>
#include <unistd.h>
int main(int ac, char **av)
{
int fd = open("testfile", O_CREAT|O_WRONLY, 0666);
if (fd < 0)
exit(1);
write(fd, "abcdefghijklmnopqrstuvwxyz\n", 27);
lseek(fd, 7, SEEK_SET);
write(fd, "H", 1);
ftruncate(fd, 24);
write(fd, "IJ", 2);
close(fd);
exit(0);
}
The only two in-tree users of ftruncate() do not care about this, though.
They both seek to the beginning and then truncate to zero length.
^ permalink raw reply
* Re: [PATCH 07/17] Fix __stdcall/WINAPI placement and function prototype
From: Johannes Sixt @ 2009-09-14 20:00 UTC (permalink / raw)
To: Marius Storm-Olsen; +Cc: msysgit, Johannes.Schindelin, git, lznuaa
In-Reply-To: <1d699897d4163224ce22db9400569fab20547e9d.1252925290.git.mstormo@gmail.com>
On Montag, 14. September 2009, Marius Storm-Olsen wrote:
> WINAPI is a macro which translates into the proper calling convention, so
> replace __stdcall with that.
I've already pointed out elsewhere that the documentation of _beginthreadex
explicitly says that the calling convention of the function pointer must be
__stdcall. It does not mention WINAPI. Therefore, I think that these two
changes are not correct:
> -static __stdcall unsigned ticktack(void *dummy)
> +static unsigned WINAPI ticktack(void *dummy)
> -static __stdcall unsigned run_thread(void *data)
> +static unsigned WINAPI run_thread(void *data)
You should s/WINAPI/__stdcall/.
-- Hannes
^ permalink raw reply
* Re: could not detach HEAD error didn't identify the cause of the issue
From: Ben Bradshaw @ 2009-09-14 21:01 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <4AAEAE15.7050607@catalyst.net.nz>
Hi Jeff,
I'm using git version 1.5.6.5 - I think I can elaborate on the issue
some more though. I'll step through the process that got me to the point
of failure. For the purposes of this example 'ben@carbon' is me at my
local machine, 'ben@staging' is our staging server. Our development
setup for this project has us all working on the staging server except
when we need to test something that we don't want on staging (could
break things) so I also have a local copy of staging. (This seems
backwards to me but it's working rather well for the short time line we
have).
In this case I did the following:
1. Add module site_map to carbon
2. Install and test on carbon
3. Add module site_map to staging
4. Install and test on staging
5. git commit && git push on staging
6. git pull --rebase on carbon
I think you will need two checkouts of your repo in order to trigger
this case?
Ben
>
> Jeff King wrote:
>> On Mon, Sep 14, 2009 at 12:14:24PM +1200, Ben Bradshaw wrote:
>>
>>
>>> I was asked to post this issue to the list, I've just had git tell me it
>>> can't detach HEAD which left me scratching mine as to the cause and fix.
>>> Turns out if I have local untracked files that the remote branch also
>>> has then this error will trigger. I have included my shell backlog
>>> (client data removed). In this example the site_map folder was present
>>> on master and I had a local, untracked copy.
>>>
>>> git can obviosly detect this issue and stop my local copy getting in a
>>> bad state, but an error message such as "sites/all/modules/site_map/ is
>>> present in HEAD but is untracked locally, unable to apply changes" (or
>>> something to point the finger) would be much appreciated.
>>>
>>
>> I couldn't reproduce it here; I get such a message.
>>
>> # set up forked repo, "another" exists only on master
>> $ mkdir repo && cd repo
>> $ git init
>> $ echo content >file && git add file && git commit -m base
>> $ echo content >another && git add another && git commit -m another
>> $ git checkout -b other HEAD^
>> $ echo changes >file && git commit -a -m changes
>>
>> # add untracked "another" here
>> $ echo untracked >another
>>
>> # try rebase
>> $ git rebase master
>> First, rewinding head to replay your work on top of it...
>> error: Untracked working tree file 'another' would be overwritten by merge.
>> could not detach HEAD
>>
>> # try git pull --rebase, in case it hides the message
>> $ git config branch.other.remote .
>> $ git config branch.other.merge refs/heads/master
>> $ git pull --rebase
>> From .
>> * branch master -> FETCH_HEAD
>> First, rewinding head to replay your work on top of it...
>> error: Untracked working tree file 'another' would be overwritten by merge.
>> could not detach HEAD
>>
>> I'll admit the "could not detach HEAD" message would probably be better as:
>>
>> rebase: unable to move HEAD to 'master', aborting rebase
>>
>> or something similar.
>>
>> What version of git are you using? What does my test case above produce
>> for you?
>>
>> -Peff
>>
>>
^ permalink raw reply
* Re: Patches for git-push --confirm and --show-subjects
From: Daniel Barkalow @ 2009-09-14 22:21 UTC (permalink / raw)
To: Owen Taylor; +Cc: Junio C Hamano, git
In-Reply-To: <1252895719.11581.53.camel@localhost.localdomain>
On Sun, 13 Sep 2009, Owen Taylor wrote:
> On Sun, 2009-09-13 at 17:47 -0700, Junio C Hamano wrote:
> > * I do not think it should use two separate push_refs call into transport
> > (first with dry-run and second with real).
> >
> > Immediately after match_refs() call in transport_push(), you know if
> > the push is a non-fast-forward (in which case you do not know what you
> > will be losing anyway because you haven't seen what you are missing
> > from the other end) or exactly what your fast-forward push will be
> > sending, so between that call and the actual transport->push_refs()
> > would be the ideal place to call the hook, with a list of "ref old
> > new", without running a dry-run.
>
> The reason I had to do two calls to transport->push_refs is not because
> it actually pushes the refs twice. It's because the logic for
> classifying the refs is in builtin-send-pack.c. When you pass in
> args.dry_run=1 you get the classification logic without the network
> traffic.
I think the classification logic should move to match_refs(), assuming you
mean the ref->nonfastforward and ref->deletion stuff. It would probably
also be worth having a bit for "already up to date". (Note that
cmd_send_pack() calls match_refs(), so there wouldn't have to be
duplication between the legacy cmd_send_pack() code path and the
transport_push() codepath if the code moved into match_refs()).
> (There's a little messiness about whether it sends the "flush" 0000 or
> not that I had to work around, but that's peripheral.)
>
> The way to clean it up is pretty obvious:
>
> - You add another vfunc to the transport - '->get_capabilities' or
> something - that encapsulates server_supports("delete-refs").
I think it would be better to have a vfunc that takes refs with the
classification bits set and sets the statuses based on the idea that we're
not going to lose any races and the remote won't reject our change for
some reason we don't know about. There's a potentially large and varied
set of restrictions on what the other side is willing to accept, and I
think it would be better to put that on the other side of the vfunc,
rather than having the main transport code know that "delete-refs" means
that you can delete refs, "nonfastforward" means you can force a
non-fast-forward, something means you can create files named "CVS", etc.
This is pretty similar to having a "dry run" call first, except that it
wouldn't end up rechecking the same things on the real run immediately
following the dry run, because the checking code has moved to a separate
method.
Of course, this step isn't entirely needed; without it, you just get asked
"Are you sure?" for changes the local side can tell won't be permitted, in
addition to for changes the local side can't tell won't be permitted.
(Like, you're allowed to delete refs in general, but not the one you're
trying to delete.)
> - You split the classification logic out into a helper function
> (maybe still in builtin-send-pack.c, maybe moved into some other
> file... don't know what's appropriate.)
>
> After all, if there was another push_refs backend, it shouldn't be
> duplicating the classification logic...
I think match_refs should do it.
> - You pass pre-classified ref updates to ->push_refs
>
> I don't know how that interacts with other planned changes to this code.
I think this is a good thing to clean up before further changes; any other
improvements would either duplicate the logic that's in builtin-send-pack
or be missing it.
> > for a few reasons.
> >
> > (1) When push.confirm is set, you do not want to interact with the user
> > when the standard input is not a terminal. But an automated script
> > that runs git-push can still use an appropriate pre-push hook to make
> > the decision to intervene without human presense.
> >
> > (2) As your --show-subjects patch shows, the likes and dislikes of the
> > output format for confirmation would be highly personal. A separate
> > hook that is fed list of <ref, old, new> would make it easier to
> > customize this to suite people's tastes.
>
> The --show-subjects idea is equally useful for --dry-run. And even when
> for successful/failed pushes when neither --confirm not --dry-run is
> passed.
>
> I'm not that convinced that there's that much scope for configurability
> in this area. Clearly there's some arbitrary decisions I made - that
> abbreviated hashes wouldn't be useful. That up to 8 commit subjects
> should be shown. Etc.
>
> But as yet, there's no data as to whether people would actually want to
> make *different* arbitrary decisions.
>
> Adding more configurability (formats, etc.) doesn't really bother me,
> though it does seem like coding in advance of need. But what would
> bother me is if the feature isn't useful without complex configuration
> or installing custom scripts.
I think a pre-push hook would be popular; I know I'd like to have a hook
that makes sure that I signed off anything I'm pushing (when the server
might check that *somebody* did, but wouldn't know that this push is
supposed to be me), and I'd like a hook that checks that I've referenced
an issue in an issue tracker for each commit that I'm pushing (but only
when I go to push it).
But I think a simple "ask an interactive user" check makes sense to have
in the same part of the code.
-Daniel
*This .sig left intentionally blank*
^ permalink raw reply
* Re: could not detach HEAD error didn't identify the cause of the issue
From: Jeff King @ 2009-09-14 22:39 UTC (permalink / raw)
To: Ben Bradshaw; +Cc: git
In-Reply-To: <4AAEAF28.2050604@catalyst.net.nz>
[Please don't top-post; the usual etiquette on this list is to reply
in-line].
On Tue, Sep 15, 2009 at 09:01:28AM +1200, Ben Bradshaw wrote:
> I'm using git version 1.5.6.5 - I think I can elaborate on the issue
> some more though. I'll step through the process that got me to the point
> of failure.
Thanks for following up. I was able to reproduce your problem. The more
complex setup you described is not necessary to reproduce (though thank
you for providing it, as it eliminated one possible source of
variation). It really is just about having an untracked file 'foo' in
your working tree, and the thing you are trying to rebase on top of also
has 'foo' as a tracked file.
The good news is that it has already been fixed. I bisected it down to:
commit 324c2c3177ca5b62b12029ce8821542fc29a2733
Author: Junio C Hamano <gitster@pobox.com>
Date: Mon Jul 14 14:05:35 2008 -0700
git-rebase: report checkout failure
When detaching the HEAD to the base commit, the "git checkout"
command could fail if, for example, upstream contains a file that
would overrwrite a local, untracked file. Unconditionally
discarding the standard error stream was done to squelch the
progress and notices back when checkout did not have -q option,
but there is no reason to keep doing it anymore.
Noticed by Robert Shearman.
which sounds about right. :)
It made it into git v1.6.0, but not into any of the v1.5.x maintenance
series. I would suggest upgrading if you can.
-Peff
^ permalink raw reply
* Re: Patches for git-push --confirm and --show-subjects
From: Owen Taylor @ 2009-09-14 23:18 UTC (permalink / raw)
To: Daniel Barkalow; +Cc: Junio C Hamano, git
In-Reply-To: <alpine.LNX.2.00.0909141745410.14907@iabervon.org>
On Mon, 2009-09-14 at 18:21 -0400, Daniel Barkalow wrote:
> On Sun, 13 Sep 2009, Owen Taylor wrote:
[...]
> I think the classification logic should move to match_refs(), assuming you
> mean the ref->nonfastforward and ref->deletion stuff. It would probably
> also be worth having a bit for "already up to date". (Note that
> cmd_send_pack() calls match_refs(), so there wouldn't have to be
> duplication between the legacy cmd_send_pack() code path and the
> transport_push() codepath if the code moved into match_refs()).
[...]
> > - You add another vfunc to the transport - '->get_capabilities' or
> > something - that encapsulates server_supports("delete-refs").
>
> I think it would be better to have a vfunc that takes refs with the
> classification bits set and sets the statuses based on the idea that we're
> not going to lose any races and the remote won't reject our change for
> some reason we don't know about. There's a potentially large and varied
> set of restrictions on what the other side is willing to accept, and I
> think it would be better to put that on the other side of the vfunc,
> rather than having the main transport code know that "delete-refs" means
> that you can delete refs, "nonfastforward" means you can force a
> non-fast-forward, something means you can create files named "CVS", etc.
match_refs seems like a reasonable place to put this logic, but I'm not
sure I completely follow what you are proposing in terms of
transport-specific customization.
match_refs() is called from a couple of places where there is no
'transport' (cmd_send_pack() and http-push.c) so it can't itself call
into the transport code.
Are you thinking of a virtual function that would be a "second pass"
after the main logic done is done by match_refs; so ->check_refs()
virtual function?
How would the 'bit for "already up to date"' differ from
REF_STATUS_UPTODATE. ?
> I think a pre-push hook would be popular; I know I'd like to have a hook
> that makes sure that I signed off anything I'm pushing (when the server
> might check that *somebody* did, but wouldn't know that this push is
> supposed to be me), and I'd like a hook that checks that I've referenced
> an issue in an issue tracker for each commit that I'm pushing (but only
> when I go to push it).
If I can figure out the rest of it, I'll look at adding a hook on top as
a sweetener :-)
- Owen
^ permalink raw reply
* Re: [msysGit] [PATCH 07/17] Fix __stdcall/WINAPI placement and function prototype
From: Johannes Schindelin @ 2009-09-15 0:24 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Marius Storm-Olsen, msysgit, git, lznuaa
In-Reply-To: <200909142200.52174.j6t@kdbg.org>
Hi,
On Mon, 14 Sep 2009, Johannes Sixt wrote:
> On Montag, 14. September 2009, Marius Storm-Olsen wrote:
> > WINAPI is a macro which translates into the proper calling convention, so
> > replace __stdcall with that.
>
> I've already pointed out elsewhere that the documentation of
> _beginthreadex explicitly says that the calling convention of the
> function pointer must be __stdcall. It does not mention WINAPI.
> Therefore, I think that these two changes are not correct:
>
> > -static __stdcall unsigned ticktack(void *dummy)
> > +static unsigned WINAPI ticktack(void *dummy)
>
> > -static __stdcall unsigned run_thread(void *data)
> > +static unsigned WINAPI run_thread(void *data)
>
> You should s/WINAPI/__stdcall/.
I don't think that comments by our most proficient MSys guy should be
disregarded as thee comments were.
Ciao,
Dscho
^ permalink raw reply
* Re: Patches for git-push --confirm and --show-subjects
From: Junio C Hamano @ 2009-09-15 0:46 UTC (permalink / raw)
To: Owen Taylor; +Cc: Daniel Barkalow, git
In-Reply-To: <1252970294.11581.71.camel@localhost.localdomain>
Owen Taylor <otaylor@redhat.com> writes:
> If I can figure out the rest of it, I'll look at adding a hook on top as
> a sweetener :-)
Please don't.
I seriously suggest you start from, and stick to, nothing but a hook.
The pre-push codepath is conceptually very simple --- something needs to
inspect a list of <ref, old, new> and say yes or no. But what the users
want needs great customizability (e.g. Daniel's sign-off validation
example). It's the prime example of codepath that should have a hook and
no built-in policy logic.
You have to enable the necessary hook in all your repositories, and if
that bothers you, then *that* can (and should) be solved as a separate
issue by devising a mechanism that can be extended to the other hooks to
solve the same issue once and for all.
E.g. perhaps in $HOME/.gitconfig, you may want to allow
[hook]
prePush = $HOME/.githooks/my-pre-push-hook
preCommit = $HOME/.githooks/my-pre-commit-hook
Lack of a general mechanism to allow users to say "I want this hook to
apply to all of my repositories" is not an excuse to add tons of complex
code in the codepath. Just give users the mechanism and leave the policy
logic to them.
^ permalink raw reply
* Re: Patches for git-push --confirm and --show-subjects
From: Daniel Barkalow @ 2009-09-15 0:55 UTC (permalink / raw)
To: Owen Taylor; +Cc: Junio C Hamano, git
In-Reply-To: <1252970294.11581.71.camel@localhost.localdomain>
On Mon, 14 Sep 2009, Owen Taylor wrote:
> On Mon, 2009-09-14 at 18:21 -0400, Daniel Barkalow wrote:
> > On Sun, 13 Sep 2009, Owen Taylor wrote:
>
> [...]
> > I think the classification logic should move to match_refs(), assuming you
> > mean the ref->nonfastforward and ref->deletion stuff. It would probably
> > also be worth having a bit for "already up to date". (Note that
> > cmd_send_pack() calls match_refs(), so there wouldn't have to be
> > duplication between the legacy cmd_send_pack() code path and the
> > transport_push() codepath if the code moved into match_refs()).
> [...]
> > > - You add another vfunc to the transport - '->get_capabilities' or
> > > something - that encapsulates server_supports("delete-refs").
> >
> > I think it would be better to have a vfunc that takes refs with the
> > classification bits set and sets the statuses based on the idea that we're
> > not going to lose any races and the remote won't reject our change for
> > some reason we don't know about. There's a potentially large and varied
> > set of restrictions on what the other side is willing to accept, and I
> > think it would be better to put that on the other side of the vfunc,
> > rather than having the main transport code know that "delete-refs" means
> > that you can delete refs, "nonfastforward" means you can force a
> > non-fast-forward, something means you can create files named "CVS", etc.
>
> match_refs seems like a reasonable place to put this logic, but I'm not
> sure I completely follow what you are proposing in terms of
> transport-specific customization.
>
> match_refs() is called from a couple of places where there is no
> 'transport' (cmd_send_pack() and http-push.c) so it can't itself call
> into the transport code.
>
> Are you thinking of a virtual function that would be a "second pass"
> after the main logic done is done by match_refs; so ->check_refs()
> virtual function?
Yes. match_refs() would answer the question of what the change to the ref
is, while ->check_refs() would determine whether, for this transport, that
change is possible. transport_push() would call match_refs(), then
->check_refs() for transport-specific limitations, then local "are you
sure" checks, then push_refs(). Other places that call match_refs() would
either call the appropriate implementation of check_refs()
(e.g., from cmd_send_pack) or just let the change get rejected when it is
actually attempted.
> How would the 'bit for "already up to date"' differ from
> REF_STATUS_UPTODATE. ?
It would put all of the things that match_refs() generated in the
collection of 1-bit flags, and leave ->status entirely for the
transport-specific code to set. Possibly transport_push() should set
status to REF_STATUS_UPTODATE if the bit is set, and similarly set status
to REF_STATUS_REJECT_NONFASTFORWARD if nonfastforward and not force.
> > I think a pre-push hook would be popular; I know I'd like to have a hook
> > that makes sure that I signed off anything I'm pushing (when the server
> > might check that *somebody* did, but wouldn't know that this push is
> > supposed to be me), and I'd like a hook that checks that I've referenced
> > an issue in an issue tracker for each commit that I'm pushing (but only
> > when I go to push it).
>
> If I can figure out the rest of it, I'll look at adding a hook on top as
> a sweetener :-)
Sounds like a good plan.
-Daniel
*This .sig left intentionally blank*
^ permalink raw reply
* Re: Patches for git-push --confirm and --show-subjects
From: Owen Taylor @ 2009-09-15 2:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Daniel Barkalow, git
In-Reply-To: <7v7hw19gr5.fsf@alter.siamese.dyndns.org>
On Mon, 2009-09-14 at 17:46 -0700, Junio C Hamano wrote:
> Owen Taylor <otaylor@redhat.com> writes:
>
> > If I can figure out the rest of it, I'll look at adding a hook on top as
> > a sweetener :-)
>
> Please don't.
>
> I seriously suggest you start from, and stick to, nothing but a hook.
>
> The pre-push codepath is conceptually very simple --- something needs to
> inspect a list of <ref, old, new> and say yes or no. But what the users
> want needs great customizability (e.g. Daniel's sign-off validation
> example). It's the prime example of codepath that should have a hook and
> no built-in policy logic.
Let me back up on this a little bit.
Is confirmation a general need?
In the context of the kernel or git personal repository workflows,
probably not. If you push something wrong, and discover it quickly, you
can just push over it and nobody is wiser. But a large fraction of the
projects listed on the front page of git-scm.com are using shared
repositories. And with a shared repository, a messed up push is more of
an issue: there may be notifications sent out over email or IRC, the
repository may be configured with denyFastForward true, people may
quickly pull your accidental push, etc.
It's also a sticky point for first using git. The push syntax and
behavior is a bit cryptic until you are used to it. Is it going to push
all branches or just the one I'm on? Is 'git push --tags' a superset of
'git push'? etc. If the first repository you are pushing to is public
and shared, heavy use of --dry-run at first is certainly advisable. But
repeating with --dry-run and without is pretty awkward.
How would the quality of use be as a hook?
Probably good enough. The broad outlines are achievable anyways. There
are some aspects of my patches that wouldn't be there. A few that come
to mind:
- The --show-subjects option applied to all displays of push
references, not just for --confirm.
- In the case of a successful push when the updates are exactly what
was confirmed, outputting them again after the push is suppressed.
How would ease of configuration be for a hook?
> E.g. perhaps in $HOME/.gitconfig, you may want to allow
>
> [hook]
> prePush = $HOME/.githooks/my-pre-push-hook
> preCommit = $HOME/.githooks/my-pre-commit-hook
This is certainly better than having to set it up per-repo, but if I
wanted to tell GNOME contributors how to turn it on, I'd have to provide
a gnome-contributor-git-setup.sh. Even if the hooks were shipped with
git, there's not going to be a cross-distro path to the where they are
installed.
Maybe if a there was a "hook path" that included ~/.githooks and a
system directory? Though:
git-config --global hook.prePush git-pre-push-confirm
could still overwrite something that they already have configured; it
wouldn't be an "orthogonal tip" that you could find on a web page and
apply blindly.
Providing a gnome-contributor-git-setup.sh is generally an approach of
last resort. I don't think there is anything unique or special about how
we do we do git on gnome.org that makes it different from other
shared-repository workflows. I'd like the knowledge that people get
using Git with GNOME to carry over to other work they do with Git and
vice-versa.
- Owen
^ permalink raw reply
* [Bug?] "diff -B --color" output doesn't show space errors
From: Nanako Shiraishi @ 2009-09-15 3:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Yuri D'Elia, git
In-Reply-To: <7viqfmsoej.fsf@alter.siamese.dyndns.org>
Quoting Junio C Hamano <gitster@pobox.com>
> By default, if the pathname that was present in the old version still
> appears in the new version, that path is not considered as a candiate
> for rename detection. Only "X used to be there but is gone" and "Y did
> not exist but appeared" are paired up and checked if they are similar.
>
> Give the command -B option, too, to break the filepair that does not
> disappear.
I wanted to try this -B option, and wrote a little test program.
While it shows correctly that the file was rewritten, it doesn't
point out various whitespace mistakes in the file anymore.
Is this a bug, or should I give some other options as well?
-- >8 -- cut here -- 8< --
git init
cat >file <<"EOF"
This is an article that will be
completely rewritten in a
later commit.
EOF
git add file
sed -e "s/T/\t/g" -e "s/_/ /g" >file <<"EOF"
An article was written but it was_
later rewritten to be_
a completely different text.
_____
An article was written but it was_
later rewritten to be_
a completely different text.
An article was written but it was_
later rewritten to be_
a completely different text.
Worse yet, the replacement text_
introduces a lot of
_Twhite space errors_
such as SP before HT and trailing
whitespaces, when the file was modified by the
later commit.
Also there are trailing empty lines at the end of the file.
EOF
git diff --color
git diff --color -B
# end of test program
--
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/
^ permalink raw reply
* Re: [PATCH 4/4] reset: add test cases for "--merge-dirty" option
From: Christian Couder @ 2009-09-15 4:32 UTC (permalink / raw)
To: Daniel Barkalow
Cc: Junio C Hamano, git, Johannes Schindelin, Stephan Beyer,
Jakub Narebski, Linus Torvalds
In-Reply-To: <alpine.LNX.2.00.0909110120520.28290@iabervon.org>
On Friday 11 September 2009, Daniel Barkalow wrote:
> On Fri, 11 Sep 2009, Christian Couder wrote:
> > On Friday 11 September 2009, Daniel Barkalow wrote:
> > > On Thu, 10 Sep 2009, Christian Couder wrote:
> > >
> > > This shows that with the "--merge-dirty" option,
> > >
> > > changes that are both in the work tree and the index are kept
> > >
> > > in the work tree after the reset (but discarded in the index). As
> > > with the "--merge" option,
> > >
> > > changes that are in both the work tree and the index are discarded
> > >
> > > after the reset.
> > >
> > > I'm lost here.
> > >
> > > If you have:
> > >
> > > working index HEAD target
> > > version B B A A
> > >
> > > You get:
> > >
> > > working index HEAD target
> > > --m-d B A A A
> > > --merge A A A A
> > >
> > > ?
> >
> > Yes, files that are not different between HEAD and target are changed
> > like that. Thanks for explaining it better than I could!
>
> I worked on the rules for merging way back when, so I've looked at tables
> of cases like that. If there are more cases to cover, it might work
> better to have a table like:
>
> working index HEAD target working index HEAD
> B B A A --m-d B A A
> --merge A A A
> B B A C --m-d (disallowed)
> --merge C C C
>
> Are there other differences?
Yes, I found that I messed up the last test in patch 4/4. I forgot to
replace some --merge with --merge-dirty :-(
In fact while "reset --merge" fails when there are changes in files that are
changed between HEAD and target, "reset --merge-dirty" will not fail and
discard these changes. So it is not really safe in this case and I am
working on trying to make it safer in this case.
> > > > ---
> > > > t/t7110-reset-merge.sh | 54
> > > > +++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 49
> > > > insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh
> > > > index 45714ae..1e6d634 100755
> > > > --- a/t/t7110-reset-merge.sh
> > > > +++ b/t/t7110-reset-merge.sh
> > > > @@ -19,7 +19,7 @@ test_expect_success 'creating initial files' '
> > > > git commit -m "Initial commit"
> > > > '
> > > >
> > > > -test_expect_success 'ok with changes in file not changed by reset'
> > > > ' +test_expect_success '--merge: ok if changes in file not touched
> > > > by reset' '
> > >
> > > Should probably have the "--merge: " from the beginning, since you're
> > > adding the test in this series anyway. That would make the diff come
> > > out clearer.
> >
> > Yeah, but I am not sure that patches 3/4 and 4/4 will get merged in the
> > end. If they are not merged it will be better if there is no "--merge:
> > ".
>
> Maybe write those lines to mention "reset --merge" naturally? Like:
>
> 'ok with changes in file not changed by reset --merge'
>
> 'reset --merge discards changes added to index 1'
Ok I will do that.
Thanks,
Christian.
^ permalink raw reply
* Re: [Bug?] "diff -B --color" output doesn't show space errors
From: Junio C Hamano @ 2009-09-15 4:44 UTC (permalink / raw)
To: Nanako Shiraishi; +Cc: Yuri D'Elia, git
In-Reply-To: <20090915123456.6117@nanako3.lavabit.com>
Nanako Shiraishi <nanako3@lavabit.com> writes:
> I wanted to try this -B option, and wrote a little test program.
>
> While it shows correctly that the file was rewritten, it doesn't
> point out various whitespace mistakes in the file anymore.
As you included the trailing blank lines, I assume you are running
'next'.
The code to emit complete rewrite patch hasn't changed much since it was
written, and I do not think it is aware of any whitespace error checking,
let alone the "trailing blank lines", which is pretty new for even the
regular diff codepath.
But a more interesting thing about your test program is that it exposes to
a bug in the new code in next. Let me cook up a patch to fix that issue
first, and then build probably a few more patches on it to add whitespace
error highlighting to the complete-rewrite codepath.
^ permalink raw reply
* [PATCH] diff --whitespace: fix blank lines at end
From: Junio C Hamano @ 2009-09-15 5:05 UTC (permalink / raw)
To: Nanako Shiraishi; +Cc: Yuri D'Elia, git
In-Reply-To: <7vvdjk95qi.fsf@alter.siamese.dyndns.org>
The earlier logic tried to colour any and all blank lines that were added
beyond the last blank line in the original, but this was very wrong. If
you added 96 blank lines, a non-blank line, and then 3 blank lines at the
end, only the last 3 lines should trigger the error, not the earlier 96
blank lines.
We need to also make sure that the lines are after the last non-blank line
in the postimage as well before deciding to paint them.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* This is _not_ about colouring whitespace errors in -B output;
that is a bit more involved change that needs a few more changes.
diff.c | 74 +++++++++++++++++++++++++++++++++-------------
t/t4019-diff-wserror.sh | 2 +-
2 files changed, 54 insertions(+), 22 deletions(-)
diff --git a/diff.c b/diff.c
index 2b285b8..63a3bfc 100644
--- a/diff.c
+++ b/diff.c
@@ -491,8 +491,10 @@ struct emit_callback {
struct xdiff_emit_state xm;
int color_diff;
unsigned ws_rule;
- int blank_at_eof;
+ int blank_at_eof_in_preimage;
+ int blank_at_eof_in_postimage;
int lno_in_preimage;
+ int lno_in_postimage;
sane_truncate_fn truncate;
const char **label_path;
struct diff_words_data *diff_words;
@@ -542,6 +544,17 @@ static void emit_line(FILE *file, const char *set, const char *reset, const char
fputc('\n', file);
}
+static int new_blank_line_at_eof(struct emit_callback *ecbdata, const char *line, int len)
+{
+ if (!((ecbdata->ws_rule & WS_BLANK_AT_EOF) &&
+ ecbdata->blank_at_eof_in_preimage &&
+ ecbdata->blank_at_eof_in_postimage &&
+ ecbdata->blank_at_eof_in_preimage <= ecbdata->lno_in_preimage &&
+ ecbdata->blank_at_eof_in_postimage <= ecbdata->lno_in_postimage))
+ return 0;
+ return ws_blank_line(line + 1, len - 1, ecbdata->ws_rule);
+}
+
static void emit_add_line(const char *reset, struct emit_callback *ecbdata, const char *line, int len)
{
const char *ws = diff_get_color(ecbdata->color_diff, DIFF_WHITESPACE);
@@ -549,11 +562,8 @@ static void emit_add_line(const char *reset, struct emit_callback *ecbdata, cons
if (!*ws)
emit_line(ecbdata->file, set, reset, line, len);
- else if ((ecbdata->ws_rule & WS_BLANK_AT_EOF) &&
- ecbdata->blank_at_eof &&
- (ecbdata->blank_at_eof <= ecbdata->lno_in_preimage) &&
- ws_blank_line(line + 1, len - 1, ecbdata->ws_rule))
- /* Blank line at EOF */
+ else if (new_blank_line_at_eof(ecbdata, line, len))
+ /* Blank line at EOF - paint '+' as well */
emit_line(ecbdata->file, ws, reset, line, len);
else {
/* Emit just the prefix, then the rest. */
@@ -581,12 +591,19 @@ static unsigned long sane_truncate_line(struct emit_callback *ecb, char *line, u
return allot - l;
}
-static int find_preimage_lno(const char *line)
+static void find_lno(const char *line, struct emit_callback *ecbdata)
{
- char *p = strchr(line, '-');
+ const char *p;
+ ecbdata->lno_in_preimage = 0;
+ ecbdata->lno_in_postimage = 0;
+ p = strchr(line, '-');
if (!p)
- return 0; /* should not happen */
- return strtol(p+1, NULL, 10);
+ return; /* cannot happen */
+ ecbdata->lno_in_preimage = strtol(p + 1, NULL, 10);
+ p = strchr(p, '+');
+ if (!p)
+ return; /* cannot happen */
+ ecbdata->lno_in_postimage = strtol(p + 1, NULL, 10);
}
static void fn_out_consume(void *priv, char *line, unsigned long len)
@@ -613,7 +630,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
if (line[0] == '@') {
len = sane_truncate_line(ecbdata, line, len);
- ecbdata->lno_in_preimage = find_preimage_lno(line);
+ find_lno(line, ecbdata);
emit_line(ecbdata->file,
diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO),
reset, line, len);
@@ -651,10 +668,13 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
diff_get_color(ecbdata->color_diff,
line[0] == '-' ? DIFF_FILE_OLD : DIFF_PLAIN);
ecbdata->lno_in_preimage++;
+ if (line[0] == ' ')
+ ecbdata->lno_in_postimage++;
emit_line(ecbdata->file, color, reset, line, len);
- return;
+ } else {
+ ecbdata->lno_in_postimage++;
+ emit_add_line(reset, ecbdata, line, len);
}
- emit_add_line(reset, ecbdata, line, len);
}
static char *pprint_rename(const char *a, const char *b)
@@ -1470,16 +1490,23 @@ static int count_trailing_blank(mmfile_t *mf, unsigned ws_rule)
return cnt;
}
-static int adds_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2, unsigned ws_rule)
+static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,
+ struct emit_callback *ecbdata)
{
int l1, l2, at;
+ unsigned ws_rule = ecbdata->ws_rule;
l1 = count_trailing_blank(mf1, ws_rule);
l2 = count_trailing_blank(mf2, ws_rule);
- if (l2 <= l1)
- return 0;
- /* starting where? */
+ if (l2 <= l1) {
+ ecbdata->blank_at_eof_in_preimage = 0;
+ ecbdata->blank_at_eof_in_postimage = 0;
+ return;
+ }
at = count_lines(mf1->ptr, mf1->size);
- return (at - l1) + 1; /* the line number counts from 1 */
+ ecbdata->blank_at_eof_in_preimage = (at - l1) + 1;
+
+ at = count_lines(mf2->ptr, mf2->size);
+ ecbdata->blank_at_eof_in_postimage = (at - l2) + 1;
}
static void builtin_diff(const char *name_a,
@@ -1572,8 +1599,7 @@ static void builtin_diff(const char *name_a,
ecbdata.found_changesp = &o->found_changes;
ecbdata.ws_rule = whitespace_rule(name_b ? name_b : name_a);
if (ecbdata.ws_rule & WS_BLANK_AT_EOF)
- ecbdata.blank_at_eof =
- adds_blank_at_eof(&mf1, &mf2, ecbdata.ws_rule);
+ check_blank_at_eof(&mf1, &mf2, &ecbdata);
ecbdata.file = o->file;
xpp.flags = XDF_NEED_MINIMAL | o->xdl_opts;
xecfg.ctxlen = o->context;
@@ -1699,7 +1725,13 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,
xdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);
if (data.ws_rule & WS_BLANK_AT_EOF) {
- int blank_at_eof = adds_blank_at_eof(&mf1, &mf2, data.ws_rule);
+ struct emit_callback ecbdata;
+ int blank_at_eof;
+
+ ecbdata.ws_rule = data.ws_rule;
+ check_blank_at_eof(&mf1, &mf2, &ecbdata);
+ blank_at_eof = ecbdata.blank_at_eof_in_preimage;
+
if (blank_at_eof) {
static char *err;
if (!err)
diff --git a/t/t4019-diff-wserror.sh b/t/t4019-diff-wserror.sh
index 1e75f1a..3a3663f 100755
--- a/t/t4019-diff-wserror.sh
+++ b/t/t4019-diff-wserror.sh
@@ -193,7 +193,7 @@ test_expect_success 'do not color trailing cr in context' '
test_expect_success 'color new trailing blank lines' '
{ echo a; echo b; echo; echo; } >x &&
git add x &&
- { echo a; echo; echo; echo; echo; } >x &&
+ { echo a; echo; echo; echo; echo c; echo; echo; echo; echo; } >x &&
git diff --color x >output &&
cnt=$(grep "${blue_grep}" output | wc -l) &&
test $cnt = 2
--
1.6.5.rc1.54.g4aad
^ permalink raw reply related
* Re: [PATCH] Improve --patch option documentation in git-add (updated patch)
From: Jari Aalto @ 2009-09-15 5:35 UTC (permalink / raw)
To: Sean Estabrooks; +Cc: Mikael Magnusson, git
In-Reply-To: <BLU0-SMTP18292B09CCFD873F4A6DF6AEE40@phx.gbl>
[-- Attachment #1: Type: text/plain, Size: 565 bytes --]
Sean Estabrooks <seanlkml@sympatico.ca> writes:
> ... To me though, it seems more difficult to parse this description
> than the one offered by Junio in an earlier thread ...perhaps you'd
> consider something closer to yours, such as:
>
> Interactively review the differences between the index and the
> work tree and choose which hunks to add into the index.
>
> This effectively runs ``add --interactive``, but bypasses the
> initial command menu and jumps directly to the `patch` subcommand.
> See ``Interactive mode'' for details.
Updated, thanks,
Jari
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Improve-patch-option-documentation-in-git-add.patch --]
[-- Type: text/x-diff, Size: 1129 bytes --]
>From be5eebc53c2e3dcf67edfb371d8aa8263e1a8d69 Mon Sep 17 00:00:00 2001
From: Jari Aalto <jari.aalto@cante.net>
Date: Tue, 15 Sep 2009 08:33:51 +0300
Subject: [PATCH] Improve --patch option documentation in git-add
Signed-off-by: Jari Aalto <jari.aalto@cante.net>
---
Documentation/git-add.txt | 9 ++++++---
1 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index e67b7e8..c57895a 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -72,9 +72,12 @@ OPTIONS
-p::
--patch::
- Similar to Interactive mode but the initial command loop is
- bypassed and the 'patch' subcommand is invoked using each of
- the specified filepatterns before exiting.
+ Interactively review the differences between the index and the
+ work tree and choose which hunks to add into the index.
+
+ This effectively runs ``add --interactive``, but bypasses the
+ initial command menu and jumps directly to the `patch` subcommand.
+ See ``Interactive mode'' for details.
-e, \--edit::
Open the diff vs. the index in an editor and let the user
--
1.6.3.3
^ permalink raw reply related
* Re: Patches for git-push --confirm and --show-subjects
From: Junio C Hamano @ 2009-09-15 5:50 UTC (permalink / raw)
To: Owen Taylor; +Cc: Daniel Barkalow, git
In-Reply-To: <1252982329.11581.111.camel@localhost.localdomain>
Owen Taylor <otaylor@redhat.com> writes:
> On Mon, 2009-09-14 at 17:46 -0700, Junio C Hamano wrote:
>> Owen Taylor <otaylor@redhat.com> writes:
>>
>> > If I can figure out the rest of it, I'll look at adding a hook on top as
>> > a sweetener :-)
>>
>> Please don't.
>>
>> I seriously suggest you start from, and stick to, nothing but a hook.
>>
>> The pre-push codepath is conceptually very simple --- something needs to
>> inspect a list of <ref, old, new> and say yes or no. But what the users
>> want needs great customizability (e.g. Daniel's sign-off validation
>> example). It's the prime example of codepath that should have a hook and
>> no built-in policy logic.
>
> Let me back up on this a little bit.
>
> Is confirmation a general need?
If you limit it to the confirmation alone, the answer is probably "not
necessarily". But a mechanism to allow validation logic to be plugged in
probably is.
You might not see a "policy" in your approach, but it makes some troubling
hardcoded policy decisions. Here are a few examples of what your patch
decides, and makes it harder for other people to build on (rather, "around):
- We support only interactive validation (confirmation). If you want to
have an unattended validation scheme, there is no way to enhance the
mechanism this patch adds to do so. You instead need to add yet
another command line option and hook into the same place as this patch
touches.
- We assume "git push" is run from terminal, and the only kind of
interactive validation we support is via typed confirmation from a line
terminal "[Y/n]?" If you want to run "git push" from a GUI frontend
and have the user interact with a dialog window popped up separately,
you are also out of luck.
- We assume it is good enough to have various built-in presentations of
supporting information while asking for confirmations; there is no way
for casual end users to customize and enhance it.
I honestly do not want to be a part of "We" in the above bullet points.
I do not object to having a good default presentation and default
interaction (assuming for a while that we limit ourselves only to
"interactive confirmation"). But that is a very different matter from
closing the door for other possibilities, which is essentially what the
approach to use built-in policy logic that is configurable with unbounded
number of future command line options to "git push" is.
> Providing a gnome-contributor-git-setup.sh is generally an approach of
> last resort.
No question about that. We do not have any complex built-in policy code
that is triggered at post-receive time at all, but many people use the
sample post-receive-email hook we ship unmodified in their repositories,
because the script is written in a highly configurable way. I do not see
why pre-push has to be any different.
In any case, this topic won't be part of 1.6.5, and we have plenty of time
to prototype and polish it before it goes to the end user.
^ permalink raw reply
* Re: [msysGit] [PATCH 07/17] Fix __stdcall/WINAPI placement and function prototype
From: Marius Storm-Olsen @ 2009-09-15 5:59 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Johannes Sixt, msysgit, git, lznuaa
In-Reply-To: <alpine.DEB.1.00.0909150224010.8306@pacific.mpi-cbg.de>
Johannes Schindelin said the following on 15.09.2009 02:24:
> Hi,
>
> On Mon, 14 Sep 2009, Johannes Sixt wrote:
>
>> On Montag, 14. September 2009, Marius Storm-Olsen wrote:
>>> WINAPI is a macro which translates into the proper calling convention, so
>>> replace __stdcall with that.
>> I've already pointed out elsewhere that the documentation of
>> _beginthreadex explicitly says that the calling convention of the
>> function pointer must be __stdcall. It does not mention WINAPI.
>> Therefore, I think that these two changes are not correct:
>>
>>> -static __stdcall unsigned ticktack(void *dummy)
>>> +static unsigned WINAPI ticktack(void *dummy)
>>> -static __stdcall unsigned run_thread(void *data)
>>> +static unsigned WINAPI run_thread(void *data)
>> You should s/WINAPI/__stdcall/.
>
> I don't think that comments by our most proficient MSys guy should be
> disregarded as thee comments were.
I also pointed out that WINAPI is just a define for __stdcall (since
_MSC_VER >= 800; VC 6.0 being _MSC_VER == 1200 and VC 2008 being
_MSC_VER == 1500), and that's probably never going to change now. So,
the change is not as bad as Hannes portrays it to be, and makes things
less convoluted by using the same convention all over.
However, I'll cave for the pressure, and reroll the commit :)
--
.marius
^ permalink raw reply
* [PATCH 0/4] Colouring whitespace errors in diff -B output
From: Junio C Hamano @ 2009-09-15 6:15 UTC (permalink / raw)
To: git; +Cc: Nanako Shiraishi
Here is a 4-patch miniseries to teach "diff -B" output routines to detect
and colour whitespace errors like we do for normal patches.
The first three patches are only about moving code around without changing
anything.
The last one hooks "diff -B" logic to the per-line output routines in a
way that mimicks how the normal patches are fed to them better, in order
to take advantage of all the existing whitespace error detection and
colouring logic.
Junio C Hamano (4):
diff.c: shuffling code around
diff.c: split emit_line() from the first char and the rest of the line
diff.c: emit_add_line() takes only the rest of the line
diff -B: colour whitespace errors
diff.c | 327 +++++++++++++++++++++++++++++++++++-----------------------------
1 files changed, 180 insertions(+), 147 deletions(-)
^ permalink raw reply
* [PATCH 1/4] diff.c: shuffling code around
From: Junio C Hamano @ 2009-09-15 6:15 UTC (permalink / raw)
To: git; +Cc: Nanako Shiraishi
In-Reply-To: <1252995306-32329-1-git-send-email-gitster@pobox.com>
Move function, type, and structure definitions for fill_mmfile(),
count_trailing_blank(), check_blank_at_eof(), emit_line(),
new_blank_line_at_eof(), emit_add_line(), sane_truncate_fn, and
emit_callback up in the file, so that they can be refactored into helper
functions and reused by codepath for emitting rewrite patches.
This only moves the lines around to make the next two patches easier to
read.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* This obviously comes on top of the earlier fix to the "blank lines at
eof" breakage.
diff.c | 250 ++++++++++++++++++++++++++++++++--------------------------------
1 files changed, 125 insertions(+), 125 deletions(-)
diff --git a/diff.c b/diff.c
index 63a3bfc..7548966 100644
--- a/diff.c
+++ b/diff.c
@@ -241,6 +241,23 @@ static struct diff_tempfile {
char tmp_path[PATH_MAX];
} diff_temp[2];
+typedef unsigned long (*sane_truncate_fn)(char *line, unsigned long len);
+
+struct emit_callback {
+ struct xdiff_emit_state xm;
+ int color_diff;
+ unsigned ws_rule;
+ int blank_at_eof_in_preimage;
+ int blank_at_eof_in_postimage;
+ int lno_in_preimage;
+ int lno_in_postimage;
+ sane_truncate_fn truncate;
+ const char **label_path;
+ struct diff_words_data *diff_words;
+ int *found_changesp;
+ FILE *file;
+};
+
static int count_lines(const char *data, int size)
{
int count, ch, completely_empty = 1, nl_just_seen = 0;
@@ -301,6 +318,114 @@ static void copy_file_with_prefix(FILE *file,
fprintf(file, "%s\n\\ No newline at end of file\n", reset);
}
+static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)
+{
+ if (!DIFF_FILE_VALID(one)) {
+ mf->ptr = (char *)""; /* does not matter */
+ mf->size = 0;
+ return 0;
+ }
+ else if (diff_populate_filespec(one, 0))
+ return -1;
+ mf->ptr = one->data;
+ mf->size = one->size;
+ return 0;
+}
+
+static int count_trailing_blank(mmfile_t *mf, unsigned ws_rule)
+{
+ char *ptr = mf->ptr;
+ long size = mf->size;
+ int cnt = 0;
+
+ if (!size)
+ return cnt;
+ ptr += size - 1; /* pointing at the very end */
+ if (*ptr != '\n')
+ ; /* incomplete line */
+ else
+ ptr--; /* skip the last LF */
+ while (mf->ptr < ptr) {
+ char *prev_eol;
+ for (prev_eol = ptr; mf->ptr <= prev_eol; prev_eol--)
+ if (*prev_eol == '\n')
+ break;
+ if (!ws_blank_line(prev_eol + 1, ptr - prev_eol, ws_rule))
+ break;
+ cnt++;
+ ptr = prev_eol - 1;
+ }
+ return cnt;
+}
+
+static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,
+ struct emit_callback *ecbdata)
+{
+ int l1, l2, at;
+ unsigned ws_rule = ecbdata->ws_rule;
+ l1 = count_trailing_blank(mf1, ws_rule);
+ l2 = count_trailing_blank(mf2, ws_rule);
+ if (l2 <= l1) {
+ ecbdata->blank_at_eof_in_preimage = 0;
+ ecbdata->blank_at_eof_in_postimage = 0;
+ return;
+ }
+ at = count_lines(mf1->ptr, mf1->size);
+ ecbdata->blank_at_eof_in_preimage = (at - l1) + 1;
+
+ at = count_lines(mf2->ptr, mf2->size);
+ ecbdata->blank_at_eof_in_postimage = (at - l2) + 1;
+}
+
+static void emit_line(FILE *file, const char *set, const char *reset, const char *line, int len)
+{
+ int has_trailing_newline, has_trailing_carriage_return;
+
+ has_trailing_newline = (len > 0 && line[len-1] == '\n');
+ if (has_trailing_newline)
+ len--;
+ has_trailing_carriage_return = (len > 0 && line[len-1] == '\r');
+ if (has_trailing_carriage_return)
+ len--;
+
+ fputs(set, file);
+ fwrite(line, len, 1, file);
+ fputs(reset, file);
+ if (has_trailing_carriage_return)
+ fputc('\r', file);
+ if (has_trailing_newline)
+ fputc('\n', file);
+}
+
+static int new_blank_line_at_eof(struct emit_callback *ecbdata, const char *line, int len)
+{
+ if (!((ecbdata->ws_rule & WS_BLANK_AT_EOF) &&
+ ecbdata->blank_at_eof_in_preimage &&
+ ecbdata->blank_at_eof_in_postimage &&
+ ecbdata->blank_at_eof_in_preimage <= ecbdata->lno_in_preimage &&
+ ecbdata->blank_at_eof_in_postimage <= ecbdata->lno_in_postimage))
+ return 0;
+ return ws_blank_line(line + 1, len - 1, ecbdata->ws_rule);
+}
+
+static void emit_add_line(const char *reset, struct emit_callback *ecbdata, const char *line, int len)
+{
+ const char *ws = diff_get_color(ecbdata->color_diff, DIFF_WHITESPACE);
+ const char *set = diff_get_color(ecbdata->color_diff, DIFF_FILE_NEW);
+
+ if (!*ws)
+ emit_line(ecbdata->file, set, reset, line, len);
+ else if (new_blank_line_at_eof(ecbdata, line, len))
+ /* Blank line at EOF - paint '+' as well */
+ emit_line(ecbdata->file, ws, reset, line, len);
+ else {
+ /* Emit just the prefix, then the rest. */
+ emit_line(ecbdata->file, set, reset, line, 1);
+ ws_check_emit(line + 1, len - 1, ecbdata->ws_rule,
+ ecbdata->file, set, reset, ws);
+ }
+}
+
static void emit_rewrite_diff(const char *name_a,
const char *name_b,
struct diff_filespec *one,
@@ -345,20 +470,6 @@ static void emit_rewrite_diff(const char *name_a,
copy_file_with_prefix(o->file, '+', two->data, two->size, new, reset);
}
-static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)
-{
- if (!DIFF_FILE_VALID(one)) {
- mf->ptr = (char *)""; /* does not matter */
- mf->size = 0;
- return 0;
- }
- else if (diff_populate_filespec(one, 0))
- return -1;
- mf->ptr = one->data;
- mf->size = one->size;
- return 0;
-}
-
struct diff_words_buffer {
mmfile_t text;
long alloc;
@@ -485,23 +596,6 @@ static void diff_words_show(struct diff_words_data *diff_words)
}
}
-typedef unsigned long (*sane_truncate_fn)(char *line, unsigned long len);
-
-struct emit_callback {
- struct xdiff_emit_state xm;
- int color_diff;
- unsigned ws_rule;
- int blank_at_eof_in_preimage;
- int blank_at_eof_in_postimage;
- int lno_in_preimage;
- int lno_in_postimage;
- sane_truncate_fn truncate;
- const char **label_path;
- struct diff_words_data *diff_words;
- int *found_changesp;
- FILE *file;
-};
-
static void free_diff_words_data(struct emit_callback *ecbdata)
{
if (ecbdata->diff_words) {
@@ -524,55 +618,6 @@ const char *diff_get_color(int diff_use_color, enum color_diff ix)
return "";
}
-static void emit_line(FILE *file, const char *set, const char *reset, const char *line, int len)
-{
- int has_trailing_newline, has_trailing_carriage_return;
-
- has_trailing_newline = (len > 0 && line[len-1] == '\n');
- if (has_trailing_newline)
- len--;
- has_trailing_carriage_return = (len > 0 && line[len-1] == '\r');
- if (has_trailing_carriage_return)
- len--;
-
- fputs(set, file);
- fwrite(line, len, 1, file);
- fputs(reset, file);
- if (has_trailing_carriage_return)
- fputc('\r', file);
- if (has_trailing_newline)
- fputc('\n', file);
-}
-
-static int new_blank_line_at_eof(struct emit_callback *ecbdata, const char *line, int len)
-{
- if (!((ecbdata->ws_rule & WS_BLANK_AT_EOF) &&
- ecbdata->blank_at_eof_in_preimage &&
- ecbdata->blank_at_eof_in_postimage &&
- ecbdata->blank_at_eof_in_preimage <= ecbdata->lno_in_preimage &&
- ecbdata->blank_at_eof_in_postimage <= ecbdata->lno_in_postimage))
- return 0;
- return ws_blank_line(line + 1, len - 1, ecbdata->ws_rule);
-}
-
-static void emit_add_line(const char *reset, struct emit_callback *ecbdata, const char *line, int len)
-{
- const char *ws = diff_get_color(ecbdata->color_diff, DIFF_WHITESPACE);
- const char *set = diff_get_color(ecbdata->color_diff, DIFF_FILE_NEW);
-
- if (!*ws)
- emit_line(ecbdata->file, set, reset, line, len);
- else if (new_blank_line_at_eof(ecbdata, line, len))
- /* Blank line at EOF - paint '+' as well */
- emit_line(ecbdata->file, ws, reset, line, len);
- else {
- /* Emit just the prefix, then the rest. */
- emit_line(ecbdata->file, set, reset, line, 1);
- ws_check_emit(line + 1, len - 1, ecbdata->ws_rule,
- ecbdata->file, set, reset, ws);
- }
-}
-
static unsigned long sane_truncate_line(struct emit_callback *ecb, char *line, unsigned long len)
{
const char *cp;
@@ -1464,51 +1509,6 @@ static const struct funcname_pattern_entry *diff_funcname_pattern(struct diff_fi
return NULL;
}
-static int count_trailing_blank(mmfile_t *mf, unsigned ws_rule)
-{
- char *ptr = mf->ptr;
- long size = mf->size;
- int cnt = 0;
-
- if (!size)
- return cnt;
- ptr += size - 1; /* pointing at the very end */
- if (*ptr != '\n')
- ; /* incomplete line */
- else
- ptr--; /* skip the last LF */
- while (mf->ptr < ptr) {
- char *prev_eol;
- for (prev_eol = ptr; mf->ptr <= prev_eol; prev_eol--)
- if (*prev_eol == '\n')
- break;
- if (!ws_blank_line(prev_eol + 1, ptr - prev_eol, ws_rule))
- break;
- cnt++;
- ptr = prev_eol - 1;
- }
- return cnt;
-}
-
-static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,
- struct emit_callback *ecbdata)
-{
- int l1, l2, at;
- unsigned ws_rule = ecbdata->ws_rule;
- l1 = count_trailing_blank(mf1, ws_rule);
- l2 = count_trailing_blank(mf2, ws_rule);
- if (l2 <= l1) {
- ecbdata->blank_at_eof_in_preimage = 0;
- ecbdata->blank_at_eof_in_postimage = 0;
- return;
- }
- at = count_lines(mf1->ptr, mf1->size);
- ecbdata->blank_at_eof_in_preimage = (at - l1) + 1;
-
- at = count_lines(mf2->ptr, mf2->size);
- ecbdata->blank_at_eof_in_postimage = (at - l2) + 1;
-}
-
static void builtin_diff(const char *name_a,
const char *name_b,
struct diff_filespec *one,
--
1.6.5.rc1.54.g4aad
^ permalink raw reply related
* [PATCH 2/4] diff.c: split emit_line() from the first char and the rest of the line
From: Junio C Hamano @ 2009-09-15 6:15 UTC (permalink / raw)
To: git; +Cc: Nanako Shiraishi
In-Reply-To: <1252995306-32329-1-git-send-email-gitster@pobox.com>
A new helper function emit_line_0() takes the first line of diff output
(typically "-", " ", or "+") separately from the remainder of the line.
No other functional changes.
This change will make it easier to reuse the logic when emitting the
rewrite diff, as we do not want to copy a line only to add "+"/"-"/" "
immediately before its first character when we produce rewrite diff
output.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
diff.c | 10 +++++++++-
1 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/diff.c b/diff.c
index 7548966..b5c2574 100644
--- a/diff.c
+++ b/diff.c
@@ -377,7 +377,8 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,
ecbdata->blank_at_eof_in_postimage = (at - l2) + 1;
}
-static void emit_line(FILE *file, const char *set, const char *reset, const char *line, int len)
+static void emit_line_0(FILE *file, const char *set, const char *reset,
+ int first, const char *line, int len)
{
int has_trailing_newline, has_trailing_carriage_return;
@@ -389,6 +390,7 @@ static void emit_line(FILE *file, const char *set, const char *reset, const char
len--;
fputs(set, file);
+ fputc(first, file);
fwrite(line, len, 1, file);
fputs(reset, file);
if (has_trailing_carriage_return)
@@ -397,6 +399,12 @@ static void emit_line(FILE *file, const char *set, const char *reset, const char
fputc('\n', file);
}
+static void emit_line(FILE *file, const char *set, const char *reset,
+ const char *line, int len)
+{
+ emit_line_0(file, set, reset, line[0], line+1, len-1);
+}
+
static int new_blank_line_at_eof(struct emit_callback *ecbdata, const char *line, int len)
{
if (!((ecbdata->ws_rule & WS_BLANK_AT_EOF) &&
--
1.6.5.rc1.54.g4aad
^ permalink raw reply related
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