* Re: [RFC/PATCH] gitweb: Make linking to actions requiring JavaScript a feature
From: Junio C Hamano @ 2009-11-26 20:34 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Junio C Hamano, Stephen Boyd, git
In-Reply-To: <200911262112.16280.jnareb@gmail.com>
Jakub Narebski <jnareb@gmail.com> writes:
> Let gitweb turn some links (like 'blame' links) into linking to
> actions which require JavaScript (like 'blame_incremental' action)
> only if 'javascript-actions' feature is enabled.
Hmm, instead of "fixLinks" that actually munges existing links to working
action with some other action, can that be "addLinks" that _adds_ new
links to features available only when JS can be used?
^ permalink raw reply
* Re: [msysGit] [PATCH/RFC 03/11] mingw: implement syslog
From: Johannes Sixt @ 2009-11-26 21:23 UTC (permalink / raw)
To: msysgit; +Cc: Erik Faye-Lund, git, dotzenlabs, Erik Faye-Lund
In-Reply-To: <1259196260-3064-4-git-send-email-kusmabite@gmail.com>
On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:
> +void syslog(int priority, const char *fmt, ...) {
Style: Brace goes to next line.
> + struct strbuf msg;
> + va_list va;
> + WORD logtype;
> +
> + strbuf_init(&msg, 0);
> + va_start(va, fmt);
> + strbuf_vaddf(&msg, fmt, va);
> + va_end(va);
I would
const char* arg;
if (strcmp(fmt, "%s"))
die("format string of syslog() not implemented")
va_start(va, fmt);
arg = va_arg(va, char*);
va_end(va);
because we have exactly one invocation of syslog(), which passes "%s" as
format string. Then strbuf_vaddf is not needed. Or even simpler: declare the
function as
void syslog(int priority, const char *fmt, const char*arg);
> + ReportEventA(ms_eventlog,
> + logtype,
> + (WORD)NULL,
> + (DWORD)NULL,
> + NULL,
> + 1,
> + 0,
> + (const char **)&msg.buf,
> + NULL);
Why do you pass NULL and apply casts? The first one gives a warning.
Did you read this paragraph in the documentation?
http://msdn.microsoft.com/en-us/library/aa363679(VS.85).aspx
"Note that the string that you log cannot contain %n, where n is an integer
value (for example, %1) because the event viewer treats it as an insertion
string. ..."
How are the chances that this condition applies to our use of the function?
-- Hannes
^ permalink raw reply
* Re: [RFC/PATCH] gitweb: Make linking to actions requiring JavaScript a feature
From: Jakub Narebski @ 2009-11-26 21:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stephen Boyd, git
In-Reply-To: <7vws1d3tzk.fsf@alter.siamese.dyndns.org>
Dnia czwartek 26. listopada 2009 21:34, Junio C Hamano napisał:
> Jakub Narebski <jnareb@gmail.com> writes:
>
> > Let gitweb turn some links (like 'blame' links) into linking to
> > actions which require JavaScript (like 'blame_incremental' action)
> > only if 'javascript-actions' feature is enabled.
>
> Hmm, instead of "fixLinks" that actually munges existing links to working
> action with some other action, can that be "addLinks" that _adds_ new
> links to features available only when JS can be used?
I am not sure if this would make sense.
Both 'blame' (running "git blame --porcelain") and 'blame_incremental'
(running "git blame --incremental") actions generate *the same* view,
modulo some very minor differences. The idea behind 'blame_incremental'
is that it provides incremental updates to long-running action, and
hopefully also reduce server load, at least a bit.
It might be however good *interim* solution, so people would be able
to test 'blame_incremental' view without having to edit gitweb links.
Hmmm....
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [msysGit] [PATCH/RFC 06/11] run-command: add kill_async() and is_async_alive()
From: Johannes Sixt @ 2009-11-26 21:46 UTC (permalink / raw)
To: msysgit; +Cc: Erik Faye-Lund, git, dotzenlabs, Erik Faye-Lund
In-Reply-To: <1259196260-3064-7-git-send-email-kusmabite@gmail.com>
On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:
> +int kill_async(struct async *async)
> +{
> +#ifndef WIN32
> + return kill(async->pid, SIGTERM);
> +#else
> + DWORD ret = 0;
> + if (!TerminateThread(async->tid, 0))
> + ret = error("killing thread failed: %lu", GetLastError());
Ugh! Did you read the documentation of TerminateThread()?
We need to kill processes/threads when we detect that there are too many
connections. But TerminateThread() is such a dangerous function that we
cannot pretend that everything is good, and we continue to accept
connections.
Unless we find a different solution, I would prefer to punt and die instead.
> + else if (!GetExitCodeThread(async->tid, &ret))
> + ret = error("cannot get thread exit code: %lu", GetLastError());
What should the exit code be good for? The return value of this function can
only be -1 (failure, could not kill) or 0 (success, process killed).
-- Hannes
^ permalink raw reply
* [PATCH] gitworkflows: Consistently back-quote git commands
From: Björn Gustavsson @ 2009-11-26 21:49 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
Signed-off-by: Björn Gustavsson <bgustavsson@gmail.com>
---
Documentation/gitworkflows.txt | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/Documentation/gitworkflows.txt b/Documentation/gitworkflows.txt
index 91c0eea..065441d 100644
--- a/Documentation/gitworkflows.txt
+++ b/Documentation/gitworkflows.txt
@@ -229,7 +229,7 @@ To verify that 'master' is indeed a superset of 'maint', use git log:
.Verify 'master' is a superset of 'maint'
[caption="Recipe: "]
=====================================
-git log master..maint
+`git log master..maint`
=====================================
This command should not list any commits. Otherwise, check out
--
1.6.6.rc0.15.g4fa80
^ permalink raw reply related
* Re: [msysGit] [PATCH/RFC 07/11] run-command: support input-fd
From: Johannes Sixt @ 2009-11-26 21:53 UTC (permalink / raw)
To: msysgit; +Cc: Erik Faye-Lund, git, dotzenlabs, Erik Faye-Lund
In-Reply-To: <1259196260-3064-8-git-send-email-kusmabite@gmail.com>
On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:
> This patch adds the possibility to supply a non-0
> file descriptor for communucation, instead of the
> default-created pipe. The pipe gets duplicated, so
> the caller can free it's handles.
>
> This is usefull for async communication over sockets.
>
> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
> ---
> run-command.c | 5 ++++-
> 1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index e5a0e06..98771ef 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -327,7 +327,10 @@ int start_async(struct async *async)
> {
> int pipe_out[2];
>
> - if (pipe(pipe_out) < 0)
> + if (async->out) {
> + pipe_out[0] = dup(async->out);
> + pipe_out[1] = dup(async->out);
> + } else if (pipe(pipe_out) < 0)
> return error("cannot create pipe: %s", strerror(errno));
> async->out = pipe_out[0];
Hm. If async->out != 0:
pipe_out[0] = dup(async->out);
async->out = pipe_out[0];
This is confusing.
Moreover, you are assigning (a dup of) the same fd to the writable end. This
assumes a bi-directional channel. I don't yet know what I should think about
this (haven't studied the later patches, yet).
It would be great if you could add a few words to
Documentation/technical/api-runcommand.txt.
-- Hannes
^ permalink raw reply
* Re: [PATCH 1/8] git-merge-file --ours, --theirs
From: Avery Pennarun @ 2009-11-26 21:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Nanako Shiraishi
In-Reply-To: <7vy6ltdd2l.fsf@alter.siamese.dyndns.org>
On Thu, Nov 26, 2009 at 1:17 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Except for parse-optification, this one is more or less a verbatim copy of
> my patch, and I think I probably deserve an in-body "From: " line for this
> [PATCH 1/8], [PATCH 6/8] and [PATCH 8/8] to take the full authorship of
> them.
[...]
> Imagine that Avery were an area expert (the subsystem maintainer) on "git
> merge" and downwards, and somebody who did not know that "merge" has
> already been rewritten in C, nor some parts of the system have been
> rewritten to use parse-options, submitted a patch series for review and
> Avery is helping to polish it up [*1*].
I'm quite open to doing this however you want; I definitely consider
it your patch series. My main measurable contribution is just the
unit tests that I wrote.
However, when thinking about this, I wasn't worried so much about the
correct placement of credit as of discredit. The merge code has
changed sufficiently since you wrote this patch series that every one
of them required quite a lot of conflict resolution. Most of the
conflicts were pretty obvious how to resolve, but it was tedious and
error prone, and there's a reasonably high probability that I screwed
up something while doing so.
I imagined what people would expect to see when they do 'git blame' to
explain the source of a problem. If they see your name, you might be
blamed for my errors; if they see my name with a "based on a patch by
Junio" in the changelog, then I would be (probably correctly) blamed
for the errors, while you can retain credit for the success.
Mostly, however, I didn't want to be sending out patches in your name
that weren't actually done by you. If you'd like me to do so,
however, then I will :)
>> +/* merge favor modes */
>> +#define XDL_MERGE_FAVOR_OURS 0x0010
>> +#define XDL_MERGE_FAVOR_THEIRS 0x0020
>> +#define XDL_MERGE_FAVOR(flags) (((flags)>>4) & 3)
>
> This is a bad change. It forces the high-level layer of the resulting
> code to be aware that the favor bits are shifted by 4 and it is different
> from what the low-level layer expects. If I were porting it to
> parse-options, I would have kept OURS = 1 and THEIRS = 2 as the original
> patch, [...]
Ouch, yes, that wasn't very clear thinking on my part. I meant for
XDL_MERGE_FAVOR(flags) to return either XDL_MERGE_FAVOR_OURS or
XDL_MERGE_FAVOR_THEIRS, but clearly it doesn't. Will fix.
Avery
^ permalink raw reply
* qgit question: tagged commits not on a branch
From: Chris.Cheney @ 2009-11-26 21:54 UTC (permalink / raw)
To: git
My commit graph has a number of forks (I can't use the word "branches"
here) that are referenced only by a tag. Whereas gitk --all displays this
graph including those forks, qgit does not display those forks - I don't
see a way to make it do so, other than by adding branches to those tagged
commits.
Have I overlooked something?
TIA
Chris
^ permalink raw reply
* 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
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