* Re: [PATCH 2/8] builtin-merge.c: call exclude_cmds() correctly.
From: Avery Pennarun @ 2009-11-26 22:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vpr75hmpq.fsf@alter.siamese.dyndns.org>
On Thu, Nov 26, 2009 at 12:36 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "Avery Pennarun" <apenwarr@gmail.com> writes:
>
>> We need to call exclude_cmds() after the loop, not during the loop, because
>> excluding a command from the array can change the indexes of objects in the
>> array. The result is that, depending on file ordering, some commands
>> weren't excluded as they should have been.
>
> As an independent bugfix, I would prefer this to be made against 'maint'
> and not as a part of this series.
>
> How did you notice it? Can you make a test case out of your experience of
> noticing this bug in the first place, by the way (I am suspecting that you
> saw some breakage and chased it in the debugger)?
The story behind this one is a bit silly, but since you asked: I
forgot to add recursive-ours and recursive-theirs to the list of known
merge strategies, but was surprised to find that my test for
recursive-theirs passed, while recursive-ours didn't. Investigating
further, I found that the printed list of "found" strategies included
recursive-theirs but not recursive-ours. Turns out that this is
because when recursive-ours was being (correctly) removed, that slot
in the array was being filled by recursive-theirs, and then
immediately i++, which meant that recursive-theirs was never checked
for exclusion as it should have been.
Of course, after fixing this bug *both* tests were broken, but the
correct thing to do was add both strategies to the list, which hides
the effect of this bugfix.
Since the bug is actually that *too many* strategies are listed
instead of too few, it's pretty minor and I doubt it needs to go into
maint. Also, I don't know of a way to test it that would be reliable.
And I doubt this particular bug will recur anyway. (If it were too
*few* strategies listed, I'm guessing it would be caught by any number
of other tests.)
Suggestions welcome.
Thanks,
Avery
^ permalink raw reply
* Re: [msysGit] [PATCH/RFC 08/11] daemon: use explicit file descriptor
From: Johannes Sixt @ 2009-11-26 22:03 UTC (permalink / raw)
To: msysgit; +Cc: Erik Faye-Lund, git, dotzenlabs, Erik Faye-Lund
In-Reply-To: <1259196260-3064-9-git-send-email-kusmabite@gmail.com>
On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:
> This patch adds support to specify an explicit file
> descriotor for communication with the client, instead
> of using stdin/stdout.
>
> This will be useful for the Windows port, because it
> will use threads instead of fork() to serve multiple
> clients, making it impossible to reuse stdin/stdout.
>
> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
> ---
> daemon.c | 34 ++++++++++++++++------------------
> 1 files changed, 16 insertions(+), 18 deletions(-)
>
> diff --git a/daemon.c b/daemon.c
> index 07d7356..a0aead5 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -263,7 +263,7 @@ static char *path_ok(char *directory)
> return NULL; /* Fallthrough. Deny by default */
> }
>
> -typedef int (*daemon_service_fn)(void);
> +typedef int (*daemon_service_fn)(int);
> struct daemon_service {
> const char *name;
> const char *config_name;
> @@ -287,7 +287,7 @@ static int git_daemon_config(const char *var, const
> char *value, void *cb) return 0;
> }
>
> -static int run_service(char *dir, struct daemon_service *service)
> +static int run_service(int fd, char *dir, struct daemon_service *service)
> {
> const char *path;
> int enabled = service->enabled;
> @@ -340,7 +340,7 @@ static int run_service(char *dir, struct daemon_service
> *service) */
> signal(SIGTERM, SIG_IGN);
>
> - return service->fn();
> + return service->fn(fd);
> }
>
> static void copy_to_log(int fd)
> @@ -364,7 +364,7 @@ static void copy_to_log(int fd)
> fclose(fp);
> }
>
> -static int run_service_command(const char **argv)
> +static int run_service_command(int fd, const char **argv)
> {
> struct child_process cld;
>
> @@ -372,37 +372,35 @@ static int run_service_command(const char **argv)
> cld.argv = argv;
> cld.git_cmd = 1;
> cld.err = -1;
> + cld.in = cld.out = fd;
You shouldn't do that. In fact, the next patch 9 has a hunk that correctly
calls dup() once.
> - close(0);
> - close(1);
Here, stdin and stdout were closed and start_command() used both. But these
two new calls
> + exit(execute(0, addr));
> ...
> + return execute(0, peer);
are the only places where a value is assigned to fd. Now it is always only
stdin. Where does the old code initialize stdout? Shouldn't this place need a
change, too?
-- Hannes
^ permalink raw reply
* Re: [PATCH 3/8] git-merge-recursive-{ours,theirs}
From: Avery Pennarun @ 2009-11-26 22:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vr5rlerqf.fsf@alter.siamese.dyndns.org>
On Thu, Nov 26, 2009 at 1:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
> - The original series was done over a few weeks in 'pu' and this
> intermediate step was done before a better alternative of not using
> these two extra merge strategies were discovered ("...may have been an
> easy way to experiment, but we should bite the bullet", in the next
> patch).
>
> As the second round to seriously polish the series for inclusion, it
> would make much more sense to squash this with the next patch to erase
> this failed approach that has already been shown as clearly inferiour.
ok.
> - I think we should avoid adding the extra argument to ll_merge_fn() by
> combining virtual_ancestor and favor into one "flags" parameter. If
> you do so, we do not have to change the callsites again next time we
> need to add new optional features that needs only a few bits.
>
> I vaguely recall that I did the counterpart of this patch that way
> exactly for the above reason, but it is more than a year ago, so maybe
> I didn't do it that way.
You did do that, in fact, but I had to redo a bunch of the flag stuff
anyway since a few other flags had been added in the meantime.
I actually tried it both ways (with and without an extra parameter),
but I observed that:
- There are more lines of code (and more confusion) if you use an
all-in-one flags vs. what I did.
- Several functions have the same signature with all-in-one flags vs.
their current boolean parameter, so the code would compile (and then
subtly not work) if I forgot to modify a particular function.
- When we go to add a third flag parameter, it wouldn't be any harder
to join them together at that time, and because it would *again*
modify the function signatures (from two flag params back down to
one), the compiler would *again* be able to catch any functions we
forgot to adjust.
If you think this logic doesn't work, I can redo it with all-in-one
flags as you request.
Avery
^ permalink raw reply
* Re: [PATCH 1/2] format-patch: fix dashdash usage
From: Felipe Contreras @ 2009-11-26 22:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vd4355aaw.fsf@alter.siamese.dyndns.org>
On Thu, Nov 26, 2009 at 9:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Otherwise 'git format-patch <committish> -- <non-existent-path>' doesn't
>> work.
>
> Instead of "doesn't work", I really wished you wrote something like:
>
> $ git format-patch <commit> -- <path>
>
> complains that <path> does not exist in the current work tree and the
> user needs to explicitly specify "--", even though the user _did_ give
> a "--". This is because it incorrectly removes "--" from the command
> line arguments that is later passed to setup_revisions().
Complaining is one thing... failing to do anything is another.
> Remember that you are trying to help somebody who has to write Release
> Notes out of "git log" output.
>
> I actually have a bigger question, though. Does it even make sense to
> allow pathspecs to format-patch? We sure are currently loose and take
> them, but I doubt it is by design.
Not everyone has clean branches only with pertinent patches.
I stumbled upon this trying to re-create (cleanly) a "branch" that was
constantly merged into another "master" branch that had a lot more
stuff. Maybe there was a smarter way to do that with 'git rebase', but
that doesn't mean format-patch -- <path> shouldn't work.
> The patch itself looks good and is a candidate 'maint' material, if the
> answer to the above question is a convincing "yes, because ...".
Yeah, I also think this should go into 'maint'.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH 1/2] format-patch: fix dashdash usage
From: Junio C Hamano @ 2009-11-26 23:11 UTC (permalink / raw)
To: Felipe Contreras; +Cc: Junio C Hamano, git
In-Reply-To: <94a0d4530911261414o533aa108l202d4c6926da361e@mail.gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
> On Thu, Nov 26, 2009 at 9:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> Otherwise 'git format-patch <committish> -- <non-existent-path>' doesn't
>>> work.
>>
>> Instead of "doesn't work", I really wished you wrote something like:
>>
>> $ git format-patch <commit> -- <path>
>>
>> complains that <path> does not exist in the current work tree and the
>> user needs to explicitly specify "--", even though the user _did_ give
>> a "--". This is because it incorrectly removes "--" from the command
>> line arguments that is later passed to setup_revisions().
>
> Complaining is one thing... failing to do anything is another.
Oh, I didn't mean to say "tone down from doesn't-work to complains".
Instead of "doesn't work" and saying nothing else useful to describe
the nature of the problem you are addressing, I really wished you
wrote something that has enough details like the sample explaination I
gave you has.
Is it clearer what I meant? More importantly, did I get the details
right?
>> I actually have a bigger question, though. Does it even make sense to
>> allow pathspecs to format-patch? We sure are currently loose and take
>> them, but I doubt it is by design.
>
> Not everyone has clean branches only with pertinent patches.
>
> I stumbled upon this trying to re-create (cleanly) a "branch" that was
> constantly merged into another "master" branch that had a lot more
> stuff. Maybe there was a smarter way to do that with 'git rebase', but
> that doesn't mean format-patch -- <path> shouldn't work.
>
>> The patch itself looks good and is a candidate 'maint' material, if the
>> answer to the above question is a convincing "yes, because ...".
>
> Yeah, I also think this should go into 'maint'.
Hmm, I have not seen a clear "yes, because..." yet.
For one thing, Documentation/git-format-patch.txt does not even hint that
you can give pathspecs. builtin_format_patch_usage[] doesn't, either. As
I wrote the initial version of format-patch I can say with some authority
that use with pathspecs were never meant to be supported---if it works, it
works by accident, giving long enough rope to users to potentially cause
themselves harm.
I am inclined to think that we shouldn't encourage use of pathspecs (just
like we never encouraged use of options like --name-only that never makes
sense in the context of the command) but I am undecided if we also should
forbid the use of pathspecs (just like we did for --name-only recently).
An appropriate patch that should go to 'maint' _might_ even be something
like this in addition to your patch, if we decide to be consistent.
---
builtin-log.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/builtin-log.c b/builtin-log.c
index 33fa6ea..3a9bc69 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -1036,6 +1036,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
argc = setup_revisions(argc, argv, &rev, "HEAD");
if (argc > 1)
die ("unrecognized argument: %s", argv[1]);
+ if (rev.prune_data)
+ die("unexpected pathspec");
if (rev.diffopt.output_format & DIFF_FORMAT_NAME)
die("--name-only does not make sense");
^ permalink raw reply related
* Re: [PATCH 1/2] format-patch: fix dashdash usage
From: Felipe Contreras @ 2009-11-26 23:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v7htc3mqo.fsf@alter.siamese.dyndns.org>
On Fri, Nov 27, 2009 at 1:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Is it clearer what I meant? More importantly, did I get the details
> right?
Yeah, I guess so.
> Hmm, I have not seen a clear "yes, because..." yet.
I'll repeat:
Not everyone has clean branches only with pertinent patches.
That's why revision filtering options make sense.
> For one thing, Documentation/git-format-patch.txt does not even hint that
> you can give pathspecs. builtin_format_patch_usage[] doesn't, either. As
> I wrote the initial version of format-patch I can say with some authority
> that use with pathspecs were never meant to be supported---if it works, it
> works by accident, giving long enough rope to users to potentially cause
> themselves harm.
>
> I am inclined to think that we shouldn't encourage use of pathspecs (just
> like we never encouraged use of options like --name-only that never makes
> sense in the context of the command) but I am undecided if we also should
> forbid the use of pathspecs (just like we did for --name-only recently).
How about 'git format-patch --full-diff'? Isn't that a valid way to
filter patches just like --author, --grep, and so on?
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH 1/2] format-patch: fix dashdash usage
From: Junio C Hamano @ 2009-11-26 23:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Felipe Contreras, git
In-Reply-To: <7v7htc3mqo.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> ...
>>> I actually have a bigger question, though. Does it even make sense to
>>> allow pathspecs to format-patch? We sure are currently loose and take
>>> them, but I doubt it is by design.
>>
>> Not everyone has clean branches only with pertinent patches.
>>
>> I stumbled upon this trying to re-create (cleanly) a "branch" that was
>> constantly merged into another "master" branch that had a lot more
>> stuff. Maybe there was a smarter way to do that with 'git rebase', but
>> that doesn't mean format-patch -- <path> shouldn't work.
>>
>>> The patch itself looks good and is a candidate 'maint' material, if the
>>> answer to the above question is a convincing "yes, because ...".
>>
>> Yeah, I also think this should go into 'maint'.
>
> Hmm, I have not seen a clear "yes, because..." yet.
Sorry, I guess I did it again of assuming the reader would read my mind.
Let's try to be a bit more explicit.
Your description defends why you think showing only part of a single
change in a patch form is jusitified. What I found unconvincing is that
it does not even try to justify why it is a good idea to give the full
description that explains the _whole change_, even the part to the set of
files that were omitted by the pathspecs, as an applicable format-patch
output. And that is why I suspect that it may be a long rope that harms
users.
> For one thing, Documentation/git-format-patch.txt does not even hint that
> you can give pathspecs. builtin_format_patch_usage[] doesn't, either. As
> I wrote the initial version of format-patch I can say with some authority
> that use with pathspecs were never meant to be supported---if it works, it
> works by accident, giving long enough rope to users to potentially cause
> themselves harm.
>
> I am inclined to think that we shouldn't encourage use of pathspecs (just
> like we never encouraged use of options like --name-only that never makes
> sense in the context of the command) but I am undecided if we also should
> forbid the use of pathspecs (just like we did for --name-only recently).
Compared to --name-only and friends that makes the output unapplicable by
"git am", a patch generated with pathspecs is worse. The output will
apply cleanly and the user can choose not to bother or forget cleaning up
the log message from the resulting commits.
On the other hand, if the pathspec affected only the choice of commits but
the command is changed in such a way that patches were always generated
with --full-diff option, I can understand its usefulness in the use case
you described. Instead of having to do "format-patch master..branch" and
then pick only the ones that are necessary by visual inspection, you would
run it to generate the ones that _might_ need to be applied by giving
pathspec to cover the files all relevant changes _must_ touch, only to cut
down the search space of your visual inspection of picking and choosing.
Then at least the ones that you choose to apply are expressed in full and
the patch text and the description should match, and I wouldn't find it
problematic nor a long rope that harms users.
But without such an explanation, I can only _guess_ what the intention
is. That is why I asked for justifications from you, as this was your
itch.
^ permalink raw reply
* Re: [PATCH/RFC 02/11] strbuf: add non-variadic function strbuf_vaddf()
From: Erik Faye-Lund @ 2009-11-26 23:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: msysgit, git, dotzenlabs, Alex Riesen
In-Reply-To: <7vbpip86q5.fsf@alter.siamese.dyndns.org>
On Thu, Nov 26, 2009 at 7:46 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Erik Faye-Lund <kusmabite@googlemail.com> writes:
>> In practice it seems that something like the following works
>> portably-enough for many applications, dunno if it's something we'll
>> be happy with:
>> #ifndef va_copy
>> #define va_copy(a,b) ((a) = (b))
>> #endif
>
> Since an obvious implementation of va_list would be to make it a pointer
> into the stack frame, doing the above would work on many systems. On
> esoteric systems that needs something different (e.g. where va_list is
> implemented as a size-1 array of pointers, va_copy(a,b) needs to be an
> assignment (*(a) = *(b))), people can add compatibility macro later.
>
> Historically some systems that do have a suitable implementation had it
> under the name __va_copy() instead, so it would have been better to define
> it as something like:
>
> #ifndef va_copy
> # ifdef __va_copy
> # define va_copy(a,b) __va_copy(a,b)
> # else
> # /* fallback for the most obvious implementation of va_list */
> # define va_copy(a,b) ((a) = (b))
> # endif
> #endif
>
> But I do not know it still matters in practice anymore.
Perhaps I can do one better: use memcpy instead of standard
assignment. The Autoconf manual[1] suggests that it's more portable.
Something like this:
#ifndef va_copy
# ifdef __va_copy
# define va_copy(a,b) __va_copy(a,b)
# else
# define va_copy(a,b) memcpy(&a, &b, sizeof (va_list))
# endif
#endif
I'll add this to git-compat-util.h this for the next round unless
someone yells really loud at me.
*[1] http://www.gnu.org/software/hello/manual/autoconf/Function-Portability.html#index-g_t_0040code_007bva_005fcopy_007d-357
--
Erik "kusma" Faye-Lund
^ permalink raw reply
* Re: [PATCH 1/2] format-patch: fix dashdash usage
From: Björn Steinbrink @ 2009-11-26 23:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Felipe Contreras, git
In-Reply-To: <7v7htc3mqo.fsf@alter.siamese.dyndns.org>
On 2009.11.26 15:11:27 -0800, Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> > On Thu, Nov 26, 2009 at 9:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >> I actually have a bigger question, though. Does it even make sense to
> >> allow pathspecs to format-patch? We sure are currently loose and take
> >> them, but I doubt it is by design.
> >
> > Not everyone has clean branches only with pertinent patches.
> >
> > I stumbled upon this trying to re-create (cleanly) a "branch" that was
> > constantly merged into another "master" branch that had a lot more
> > stuff. Maybe there was a smarter way to do that with 'git rebase', but
> > that doesn't mean format-patch -- <path> shouldn't work.
> >
> >> The patch itself looks good and is a candidate 'maint' material, if the
> >> answer to the above question is a convincing "yes, because ...".
> >
> > Yeah, I also think this should go into 'maint'.
>
> Hmm, I have not seen a clear "yes, because..." yet.
>
> For one thing, Documentation/git-format-patch.txt does not even hint that
> you can give pathspecs. builtin_format_patch_usage[] doesn't, either. As
> I wrote the initial version of format-patch I can say with some authority
> that use with pathspecs were never meant to be supported---if it works, it
> works by accident, giving long enough rope to users to potentially cause
> themselves harm.
>
> I am inclined to think that we shouldn't encourage use of pathspecs (just
> like we never encouraged use of options like --name-only that never makes
> sense in the context of the command) but I am undecided if we also should
> forbid the use of pathspecs (just like we did for --name-only recently).
A year ago, there was someone who had done a subtree merge and had
commits that changed the subtree in the "supertree" branch. He wanted to
generate patches to send them to upstream, and ended up using
format-patch with --relative and pathspecs.
http://thread.gmane.org/gmane.comp.version-control.git/101742
I guess this could be done by some "git rebase -s subtree ..."
invocation though, to first get commits that sit directly on the subtree
branch, and then you could turn them into patches as usual... Hmm..
Björn
^ permalink raw reply
* Re: Strange behavior of gitweb
From: Andreas Schwab @ 2009-11-26 23:58 UTC (permalink / raw)
To: Alan Stern; +Cc: git
In-Reply-To: <Pine.LNX.4.44L0.0911261225130.17259-100000@netrider.rowland.org>
Alan Stern <stern@rowland.harvard.edu> writes:
> I recently ran across this strange behavior in the gitweb server at
> git.kernel.org. The following URL:
>
> http://git.kernel.org/?p=linux/kernel/git/stable/linux-2.6.27.y.git;a=commit;h=2d93148ab6988cad872e65d694c95e8944e1b62
>
> brings up a page containing commit 2d93148[...]. But that commit isn't
> part of the 2.6.27.y tree! It belongs to Linus's main tree, and it was
> added long after 2.6.27.y was forked off. The actual commit applied to
> 2.6.27.y was 070bb0f3b6df167554f0ecdeb17a5bcdb1cd7b83.
>
> So what's going on here?
Nothing mysterious. Every tree on kernel.org borrows from Linus' main
tree via .git/objects/info/alternates, thus includes its whole object
database by reference.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply
* Re: [PATCH 1/2] format-patch: fix dashdash usage
From: Junio C Hamano @ 2009-11-27 0:03 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git
In-Reply-To: <94a0d4530911261523q25147f12h2e6c9e4fe4f6b12b@mail.gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
> How about 'git format-patch --full-diff'? Isn't that a valid way to
> filter patches just like --author, --grep, and so on?
Our messages obviously crossed and I think we are in agreement that
pathspec that only is used to pick which commit to show and not limits
which parts of the chosen commits are shown might have some uses.
In any case, your patch we are discussing, with a proper commit log
message (without discussing if it is a good idea to give pathspecs) would
be a good first step, regardless of which direction we end up going as the
next step.
As to the "next step", my current thinking, unless there are convincing
arguments why there should be a way to also limit the parts of the commits
are shown, is to
(0) take your patch with an updated message (eh, that is not "next step"
but the "first step");
(1) make --full-diff implicit and default of format-patch (--no-full-diff
could be supported _if_ somebody can argue successfully why limiting
the diff is also a useful thing to do); and
(2) document clearly that format-patch takes optional pathspecs, and in
what situation they are useful.
I think (0) is 'maint' material, and with a good documentation update (1)
and (2) could also be.
^ permalink raw reply
* Re: [PATCH] send-email: automatic envelope sender
From: Junio C Hamano @ 2009-11-27 2:34 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git
In-Reply-To: <1259262269-23937-1-git-send-email-felipe.contreras@gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 4f5da4e..da2e56e 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -862,7 +862,11 @@ X-Mailer: git-send-email $gitversion
>
> my @sendmail_parameters = ('-i', @recipients);
> my $raw_from = $sanitized_sender;
> - $raw_from = $envelope_sender if (defined $envelope_sender);
> + if (defined $envelope_sender) {
> + if (not $envelope_sender eq "auto") {
> + $raw_from = $envelope_sender;
> + }
> + }
Thanks.
I'd rewrite this to
if (defined $envelope_sender && $envelope_sender ne "auto") {
...
}
but otherwise the patch looks quite straightforward.
Tests?
^ permalink raw reply
* Re: [PATCH] Give the hunk comment its own color
From: Junio C Hamano @ 2009-11-27 2:38 UTC (permalink / raw)
To: Bert Wesarg; +Cc: Jeff King, git
In-Reply-To: <36ca99e90911260405y42a9a07cx419d2973ec673039@mail.gmail.com>
Bert Wesarg <bert.wesarg@googlemail.com> writes:
> may I kindly remind you of this patch.
Yes you may ;-) A more effective would have been a resend but it is
always appreciated.
> ... If it is only the nen-existing
> consensus of the default color, than please use the die.
If you are having me go find the mail and apply I would probably use
"plain" as I suggested.
^ permalink raw reply
* Re: [RFC/PATCH] gitweb: Make linking to actions requiring JavaScript a feature
From: Junio C Hamano @ 2009-11-27 2:39 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Junio C Hamano, Stephen Boyd, git
In-Reply-To: <200911262224.36371.jnareb@gmail.com>
Jakub Narebski <jnareb@gmail.com> writes:
> It might be however good *interim* solution, so people would be able
> to test 'blame_incremental' view without having to edit gitweb links.
Exactly. I thought you were responding to my earlier "ship it as a new
feature with known breakage so that people can choose to enable to help
debugging and fixing". If flipping on the new implementation makes an
alternative working implementation unavailable, that would be one reason
the site owners might consider _not_ enabling it. By making them both
available, the result will have one less reason not to try for site
owners.
^ permalink raw reply
* Re: [RFC/PATCH 1/2] status -s: respect the status.relativePaths option
From: Junio C Hamano @ 2009-11-27 3:15 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Jeff King
In-Reply-To: <62c5bb36940485deefbf73f4bdc3fd45bbea069e.1259248243.git.git@drmicha.warpmail.net>
Michael J Gruber <git@drmicha.warpmail.net> writes:
> so that 'status' and 'status -s' in a subdir produce the same file
> names.
This configuration is on by default which makes this change even more
important.
I'd squash this in and queue it at the tip of jk/1.7.0-status topic.
Thanks
diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 58d35fb..b3dfa42 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -114,8 +114,8 @@ compatibility) and `color.status.<slot>` configuration variables
to colorize its output.
If the config variable `status.relativePaths` is set to false, then all
-paths shown in the long format are relative to the repository root, not
-to the current directory.
+paths shown are relative to the repository root, not to the current
+directory.
If `status.submodulesummary` is set to a non zero number or true (identical
to -1 or an unlimited number), the submodule summary will be enabled for
^ permalink raw reply related
* Re: [PATCH] mergetool--lib: simplify guess_merge_tool()
From: David Aguilar @ 2009-11-27 3:22 UTC (permalink / raw)
To: René Scharfe
Cc: Junio C Hamano, Carlo Marcelo Arenas Belon, Bert Wesarg, git
In-Reply-To: <4B0B1ACD.6000008@lsrfire.ath.cx>
On Tue, Nov 24, 2009 at 12:29:17AM +0100, René Scharfe wrote:
> Use a case statement instead of calling grep to find out if the editor's
> name contains the string "vim". Remove the check for emacs, as this
> branch did the same as the default one anyway.
>
> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
> ---
> This removes all grep calls from this script.
Very nice.
Thanks,
--
David
^ permalink raw reply
* Re: [RFC/PATCH 2/2] status -s: obey color.status
From: Junio C Hamano @ 2009-11-27 5:15 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Junio C Hamano
In-Reply-To: <26d0a2022638ad7b75268ca291b8d02a22f1f66c.1259248243.git.git@drmicha.warpmail.net>
Michael J Gruber <git@drmicha.warpmail.net> writes:
> * Should I rename wt-status.c's color() into something more unique when
> I export it?
Is it an option to instead move short_unmerged(), short_status() and
friends to wt-status.c from builtin-commit.c? It's been quite a while
since I worked on the code, so I don't recall why it needs such cross
references at low level between two files.
> * Is there any policy regarding use of putchar/puts vs. printf?
J6t addressed it. You have mixture of putchar(' ') and printf(" ") which
looks somewhat funny ;-)
> * The way it is done now I "color" a space, otherwise one would need to
> break down the print statements even more. Since we always color the
> foreground only it is no problem, is it?
Some people do configure to use "reverse". For example, I have:
[diff.color]
old = red reverse
whitespace = blue reverse
[status.color]
updated = green
changed = red
untracked = blue reverse
The output should be consistent between long and short format (I do not
offhand recall what we do for the long format, though).
^ permalink raw reply
* Re: [PATCH] grep: --full-tree
From: Jeff King @ 2009-11-27 6:20 UTC (permalink / raw)
To: James Pickens; +Cc: Junio C Hamano, git
In-Reply-To: <885649360911260956p58c54a54rd887102c9adedcc9@mail.gmail.com>
On Thu, Nov 26, 2009 at 10:56:55AM -0700, James Pickens wrote:
> On Wed, Nov 25, 2009 at 3:20 PM, Jeff King <peff@peff.net> wrote:
> > Sure, there are all those downsides. But what is the other option?
> > Making me use the command line option (or pathspec magic) every single
> > time I invoke git grep?
>
> Yes, but only when you want non-default behavior, not every single time.
Did you miss the part of the thread where I explained that in certain
repos, I want it one way every single time, and in others, I want it the
other way?
So yes, in certain repos, it really is every single time.
> > That is a huge downside to me.
>
> Is it *really*? Does it also bother you that you have to tell standalone
> unix commands like diff and grep what you want them to diff or grep every
> single time you invoke them?
This is a strawman. I am not saying every command-line option should be
made into a configuration option. I am saying that some options,
including this one, would be useful as configuration options. I have
already explained several times in this thread exactly what
characteristics of this option make that so.
And please, questions like "Is it *really*?" don't add anything. Yes,
really, or I wouldn't be having this discussion. This behavior has
bitten me many times while using "git grep". I'm not making it up. Maybe
I am the only one in the world, but I don't see how it makes any sense
to argue that I am not actually annoyed by it.
> I really think that this config option wouldn't even help you, because
> you'll have to remember what that option is set to in each working repo,
> and type the right command based on the setting. That seems worse than
No, the _point_ is that I don't have to remember the right command in
each repo. I can set it up for the workflow that matches that repository
and then issue "git grep" without remembering which type I'm in.
> If you can get the behavior you want using an alias or a script, then I
> suggest you do that. I don't think this config option should be considered
> unless *many* people want it, and so far I count only 1.
Perhaps I am the only one who wants to use the config option per-repo.
But we have already seen support for both behaviors, which means there
are people who will be dissatisfied with either simply leaving the
default or changing the default. And I don't want to speak for Junio,
but he seemed to agree that what you most want would depend on the repo
organization (though I think he may disagree that it is important enough
to merit the hassle of a config option).
-Peff
^ permalink raw reply
* Re: [PATCH] grep: --full-tree
From: Jeff King @ 2009-11-27 6:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v8wdup2z2.fsf@alter.siamese.dyndns.org>
On Wed, Nov 25, 2009 at 04:02:57PM -0800, Junio C Hamano wrote:
> Yeah; what is your take on tr/reset-checkout-patch topic, by the way? I
> do not particularly like a configuration that changes the behaviour of a
> command in a drastic way---it will make helping others much harder, but I
> guess it should be Ok?
>
> This may sound like an OffTopic, but because we _are_ discussing
> consistency, it matters.
It is near the top of my to-review queue. Honestly, despite any
arguments I may have made when the original reset/checkout -p series was
posted, I have been pretty happy with the current behavior. I'll take a
look now and respond in more detail in that thread.
-Peff
^ permalink raw reply
* Re: [PATCH] Give the hunk comment its own color
From: Bert Wesarg @ 2009-11-27 6:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7v4oogzo74.fsf@alter.siamese.dyndns.org>
On Fri, Nov 27, 2009 at 03:38, Junio C Hamano <gitster@pobox.com> wrote:
> Bert Wesarg <bert.wesarg@googlemail.com> writes:
>
>> may I kindly remind you of this patch.
>
> Yes you may ;-) A more effective would have been a resend but it is
> always appreciated.
>
>> ... If it is only the nen-existing
>> consensus of the default color, than please use the die.
>
> If you are having me go find the mail and apply I would probably use
> "plain" as I suggested.
So I resend with plain as default. And therefore saved one resend ;-)
Bert
>
^ permalink raw reply
* Re: [RFC PATCH] {checkout,reset} -p: make patch direction configurable
From: Jeff King @ 2009-11-27 6:41 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Junio C Hamano
In-Reply-To: <527e9296b638eb4c9993b3fb0d1c6f51b64f4c2c.1258667920.git.trast@student.ethz.ch>
On Thu, Nov 19, 2009 at 11:03:57PM +0100, Thomas Rast wrote:
> When we implemented the -p mode for checkout, reset and stash, some
> discussion revolved around the involved patch direction.
>
> Make this configurable for reset and checkout with the following
> choices:
>
> index/HEAD other
> forward undo addition undo addition
> mixed undo addition apply removal
> reverse apply removal apply removal
> [...]
> ISTR that Peff wanted this, and maybe some others. I'm not too
> interested because I'm still convinced 'mixed' is the Right Option,
> but it was somewhere deep on my todo stack and maybe you like it ;-)
Actually, I am pretty happy with the current "discard this hunk" most of
the time. It is easy enough to see "you made this change, did you want
to get rid of it?". The one exception is during patch editing. Try
something simple like:
cat >file <<EOF
this
is
a
file
with
some
content
in
it
EOF
git add file
git commit -m base
cat >file <<EOF
this
is
a
file
with
some
other
content
EOF
git checkout -p
Now try to 'e'dit the patch to throw away the addition of "other", but
keep the deletion of the other two lines. Do you find it easy or hard?
Now try it with interactive.checkout.direction set to forward. I find
editing the forward direction _much_ simpler.
Assuming you agree, I'm not sure what that tells us, though. I probably
wouldn't personally set interactive.*.direction for the yes/no part. But
I would find it more convenient to do patch editing always in the
forward direction. I'm worried that it would be too jarring to the user,
though, to see the patch presented in one direction but edit it in the
opposite direction. Maybe it would be nice to have yet another
interactive option to swap between the two for this particular hunk, and
then you could edit the direction you prefer.
Junio raised the question of consistency in another thread. I don't see
a consistency problem here. This is by definition an interactive
procedure. I don't see a reason not to allow the user their preferred
style. But I think if I could swap when editing, I would personally not
care all that much about the other direction.
-Peff
^ permalink raw reply
* Re: [PATCH] Give the hunk comment its own color
From: Jeff King @ 2009-11-27 6:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Bert Wesarg, git
In-Reply-To: <7v4oogzo74.fsf@alter.siamese.dyndns.org>
On Thu, Nov 26, 2009 at 06:38:55PM -0800, Junio C Hamano wrote:
> > ... If it is only the nen-existing
> > consensus of the default color, than please use the die.
>
> If you are having me go find the mail and apply I would probably use
> "plain" as I suggested.
As the other person in the discussion, I'll just chime in that I also
think "plain" is the best of the suggested defaults.
-Peff
^ permalink raw reply
* [PATCH v3] Give the hunk comment its own color
From: Bert Wesarg @ 2009-11-27 6:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, Bert Wesarg
In-Reply-To: <7v4oogzo74.fsf@alter.siamese.dyndns.org>
Inspired by the coloring of quilt.
Introduce a separate color for the hunk comment part, i.e. the current function.
Whitespace between hunk header and hunk comment is now printed as plain.
The current default is plain.
Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
---
Documentation/config.txt | 8 +++---
combine-diff.c | 5 +++-
diff.c | 64 +++++++++++++++++++++++++++++++++++++++++++--
diff.h | 1 +
t/t4034-diff-words.sh | 4 ++-
5 files changed, 73 insertions(+), 9 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index a8e0876..a1e36d7 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -635,10 +635,10 @@ color.diff.<slot>::
Use customized color for diff colorization. `<slot>` specifies
which part of the patch to use the specified color, and is one
of `plain` (context text), `meta` (metainformation), `frag`
- (hunk header), `old` (removed lines), `new` (added lines),
- `commit` (commit headers), or `whitespace` (highlighting
- whitespace errors). The values of these variables may be specified as
- in color.branch.<slot>.
+ (hunk header), 'func' (function in hunk header), `old` (removed lines),
+ `new` (added lines), `commit` (commit headers), or `whitespace`
+ (highlighting whitespace errors). The values of these variables may be
+ specified as in color.branch.<slot>.
color.grep::
When set to `always`, always highlight matches. When `false` (or
diff --git a/combine-diff.c b/combine-diff.c
index 5b63af1..6162691 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -524,6 +524,7 @@ static void dump_sline(struct sline *sline, unsigned long cnt, int num_parent,
int i;
unsigned long lno = 0;
const char *c_frag = diff_get_color(use_color, DIFF_FRAGINFO);
+ const char *c_func = diff_get_color(use_color, DIFF_FUNCINFO);
const char *c_new = diff_get_color(use_color, DIFF_FILE_NEW);
const char *c_old = diff_get_color(use_color, DIFF_FILE_OLD);
const char *c_plain = diff_get_color(use_color, DIFF_PLAIN);
@@ -588,7 +589,9 @@ static void dump_sline(struct sline *sline, unsigned long cnt, int num_parent,
comment_end = i;
}
if (comment_end)
- putchar(' ');
+ printf("%s%s %s%s", c_reset,
+ c_plain, c_reset,
+ c_func);
for (i = 0; i < comment_end; i++)
putchar(hunk_comment[i]);
}
diff --git a/diff.c b/diff.c
index 0d7f5ea..fd999fb 100644
--- a/diff.c
+++ b/diff.c
@@ -39,6 +39,7 @@ static char diff_colors[][COLOR_MAXLEN] = {
GIT_COLOR_GREEN, /* NEW */
GIT_COLOR_YELLOW, /* COMMIT */
GIT_COLOR_BG_RED, /* WHITESPACE */
+ GIT_COLOR_NORMAL, /* FUNCINFO */
};
static void diff_filespec_load_driver(struct diff_filespec *one);
@@ -60,6 +61,8 @@ static int parse_diff_color_slot(const char *var, int ofs)
return DIFF_COMMIT;
if (!strcasecmp(var+ofs, "whitespace"))
return DIFF_WHITESPACE;
+ if (!strcasecmp(var+ofs, "func"))
+ return DIFF_FUNCINFO;
die("bad config variable '%s'", var);
}
@@ -344,6 +347,63 @@ static void emit_add_line(const char *reset,
}
}
+static void emit_hunk_line(struct emit_callback *ecbdata,
+ const char *line, int len)
+{
+ const char *plain = diff_get_color(ecbdata->color_diff, DIFF_PLAIN);
+ const char *frag = diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO);
+ const char *func = diff_get_color(ecbdata->color_diff, DIFF_FUNCINFO);
+ const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
+ const char *orig_line = line;
+ int orig_len = len;
+ const char *frag_start;
+ int frag_len;
+ const char *part_end = NULL;
+ int part_len = 0;
+
+ /* determine length of @ */
+ while (part_len < len && line[part_len] == '@')
+ part_len++;
+
+ /* find end of frag, (Ie. find second @@) */
+ part_end = memmem(line + part_len, len - part_len,
+ line, part_len);
+ if (!part_end)
+ return emit_line(ecbdata->file, frag, reset, line, len);
+ /* calculate total length of frag */
+ part_len = (part_end + part_len) - line;
+
+ /* remember frag part, we emit only if we find a space separator */
+ frag_start = line;
+ frag_len = part_len;
+
+ /* consume hunk header */
+ len -= part_len;
+ line += part_len;
+
+ /*
+ * for empty reminder or empty space sequence (exclusive any newlines
+ * or carriage returns) emit complete original line as FRAGINFO
+ */
+ if (!len || !(part_len = strspn(line, " \t")))
+ return emit_line(ecbdata->file, frag, reset,
+ orig_line, orig_len);
+
+ /* now emit the hunk header as FRAGINFO */
+ emit_line(ecbdata->file, frag, reset, frag_start, frag_len);
+
+ /* print whitespace sep as PLAIN */
+ emit_line(ecbdata->file, plain, reset, line, part_len);
+
+ /* consume whitespace sep */
+ len -= part_len;
+ line += part_len;
+
+ /* print reminder as FUNCINFO */
+ if (len)
+ emit_line(ecbdata->file, func, reset, line, len);
+}
+
static struct diff_tempfile *claim_diff_tempfile(void) {
int i;
for (i = 0; i < ARRAY_SIZE(diff_temp); i++)
@@ -781,9 +841,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
diff_words_flush(ecbdata);
len = sane_truncate_line(ecbdata, line, len);
find_lno(line, ecbdata);
- emit_line(ecbdata->file,
- diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO),
- reset, line, len);
+ emit_hunk_line(ecbdata, line, len);
if (line[len-1] != '\n')
putc('\n', ecbdata->file);
return;
diff --git a/diff.h b/diff.h
index 2740421..15fcecd 100644
--- a/diff.h
+++ b/diff.h
@@ -130,6 +130,7 @@ enum color_diff {
DIFF_FILE_NEW = 5,
DIFF_COMMIT = 6,
DIFF_WHITESPACE = 7,
+ DIFF_FUNCINFO = 8,
};
const char *diff_get_color(int diff_use_color, enum color_diff ix);
#define diff_get_color_opt(o, ix) \
diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
index 21db6e9..186eff8 100755
--- a/t/t4034-diff-words.sh
+++ b/t/t4034-diff-words.sh
@@ -8,6 +8,7 @@ test_expect_success setup '
git config diff.color.old red
git config diff.color.new green
+ git config diff.color.func magenta
'
@@ -16,6 +17,7 @@ decrypt_color () {
-e 's/.\[1m/<WHITE>/g' \
-e 's/.\[31m/<RED>/g' \
-e 's/.\[32m/<GREEN>/g' \
+ -e 's/.\[35m/<MAGENTA>/g' \
-e 's/.\[36m/<BROWN>/g' \
-e 's/.\[m/<RESET>/g'
}
@@ -70,7 +72,7 @@ cat > expect <<\EOF
<WHITE>+++ b/post<RESET>
<BROWN>@@ -1 +1 @@<RESET>
<RED>h(4)<RESET><GREEN>h(4),hh[44]<RESET>
-<BROWN>@@ -3,0 +4,4 @@ a = b + c<RESET>
+<BROWN>@@ -3,0 +4,4 @@<RESET> <RESET><MAGENTA>a = b + c<RESET>
<GREEN>aa = a<RESET>
--
tg: (ad7ace7..) bw/func-color (depends on: master)
^ permalink raw reply related
* Re: What's cooking in git.git (Nov 2009, #06; Wed, 25)
From: Jeff King @ 2009-11-27 6:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v8wduksgq.fsf@alter.siamese.dyndns.org>
On Wed, Nov 25, 2009 at 05:03:33PM -0800, Junio C Hamano wrote:
> * jc/grep-full-tree (2009-11-24) 1 commit.
> - grep: --full-tree
>
> We probably would want test, doc and a configuration variable to make it
> default (or non-default) before we can merge it to 'master'.
I can try to pick this up. But did we reach a decision on having a
configuration variable?
-Peff
^ permalink raw reply
* [PATCH] Add a notice that only certain functions can print color escape codes
From: Johannes Sixt @ 2009-11-27 7:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael J Gruber, git
In-Reply-To: <7vy6lt6rh3.fsf@alter.siamese.dyndns.org>
From: Johannes Sixt <j6t@kdbg.org>
We emulate color escape codes on Windows by overriding printf, fprintf,
and fputs. Warn users that these are the only functions that can be used
to print them.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
Junio C Hamano schrieb:
> Johannes Sixt <j.sixt@viscovery.net> writes:
>
>> Michael J Gruber schrieb:
>>> * Is there any policy regarding use of putchar/puts vs. printf?
>> If the printed string contains color escapes that should be obeyed, you
>> can use only fputs, printf, and fprintf. You should not use puts or putchar.
>
> This msysgit-imposed restriction is something even I do not remember
> offhand. Could you please document it somewhere in a file any developer
> would get by checking out the 'master' branch of git.git?
Like this?
color.h | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/color.h b/color.h
index 7d8da6f..edeaa3e 100644
--- a/color.h
+++ b/color.h
@@ -4,6 +4,11 @@
/* "\033[1;38;5;2xx;48;5;2xxm\0" is 23 bytes */
#define COLOR_MAXLEN 24
+/*
+ * IMPORTANT: Due to the way these color codes are emulated on Windows,
+ * write them only using printf, fprintf, and fputs. In particular,
+ * do not use puts.
+ */
#define GIT_COLOR_NORMAL ""
#define GIT_COLOR_RESET "\033[m"
#define GIT_COLOR_BOLD "\033[1m"
--
1.6.6.rc0.43.g50037
^ 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