* Re: [RFC/PATCH 0/2] place cherry pick line below commit title
From: Jonathan Tan @ 2016-10-05 19:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder
In-Reply-To: <xmqq37kcufya.fsf@gitster.mtv.corp.google.com>
On 10/04/2016 11:28 AM, Junio C Hamano wrote:
> An addendum. We may also want to be prepared to accept an input
> that has some garbage lines _after_ the trailer block, if we can
> clearly identify them as such. For example, we could change the
> definition of "the last paragraph" as "the block of lines that
> do not have any empty (or blank) line, that appear either at the end
> of the input, or immediately before three-dash lines", to allow
>
> commit title
>
> explanation of the change
>
> Signed-off-by: Some Body <some@body.xz>
> [some other things]
> Acked-by: Some Other Person <some@other.xz>
>
> ---
> additional comment
>
> which (unfortunately) is a rather common pattern for people who plan
> to send the commit over e-mail.
>
> If we add a new field "const char *beginning_of_tail_garbage" next
> to "end_of_message_proper" that points at the blank line before the
> three-dash line in the above example, the parser should be able to
> break such an input into a parsed form, allow the trailer[] array to
> be manipulated and reproduce a commit log message.
How important is this feature? It doesn't seem too difficult to add,
although it does break compatibility (in particular, "--signoff" must
now be documented as "after the last trailer" instead of "at the end of
the commit message").
^ permalink raw reply
* Re: [PATCH 5/6] Documentation/git-merge.txt: improve short description in DESCRIPTION
From: Junio C Hamano @ 2016-10-05 20:01 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Sergey Organov, git
In-Reply-To: <e03a84ad-9aa6-8ada-5828-7b28f76baaaf@gmail.com>
Jakub Narębski <jnareb@gmail.com> writes:
>> +
>> +This command is used by 'git pull' to incorporate changes from another
>> +repository, and can be used by hand to merge changes from one branch
>> +into another.
>
> Rather "can be used by 'git pull'", or "is used by 'git pull' (unless
> configured otherwise)"...
I think you are misreading the original and the update (see my
comments in the other message).
>> @@ -31,11 +36,11 @@ Assume the following history exists and the current branch is
>> D---E---F---G master
>> ------------
>>
>> -Then "`git merge topic`" will replay the changes made on the
>> -`topic` branch since it diverged from `master` (i.e., `E`) until
>> -its current commit (`C`) on top of `master`, and record the result
>> -in a new commit along with the names of the two parent commits and
>> -a log message from the user describing the changes.
>> +Then "`git merge topic`" will replay the changes made on the `topic`
>> +branch since it diverged from `master` (i.e., `E`) until its current
>> +commit (`C`) on top of `master`, and record the result in a new commit
>> +along with references to the two parent commits and a log message from
>> +the user describing the changes.
>
> What the happened here!?! Please do not rewrap documentation, especially
> not without changes!
Yes, reflowing is bad but you can spot the change from "along with
the names of the parent commits" to "along with references to the
parent commits" if you stare at it long enough ;-)
^ permalink raw reply
* [PATCH] run-command: fix build on cygwin (stdin is a macro)
From: Ramsay Jones @ 2016-10-05 20:19 UTC (permalink / raw)
To: Lars Schneider; +Cc: Junio C Hamano, GIT Mailing-list
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
Hi Lars,
Commit 6007c69e ("run-command: add wait_on_exit", 04-10-2016), which
is part of your 'ls/filter-process' branch, causes the build to fail
on cygwin, since 'stdin' is defined as a macro thus:
#define stdin (_REENT->_stdin)
(you can probably guess what stdout and stderr look like!) where _REENT
in turn expands to a function call (__getreent()) which returns a pointer
to a 'struct _reent', etc., ...
I am not suggesting that you apply this exact patch (stdin_ is not a good
choice), but I wanted to show the exact patch I used to get the build to
complete on cygwin.
ATB,
Ramsay Jones
run-command.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/run-command.c b/run-command.c
index 96c54fe..a9dd91a 100644
--- a/run-command.c
+++ b/run-command.c
@@ -22,7 +22,7 @@ void child_process_clear(struct child_process *child)
struct child_to_clean {
pid_t pid;
char *name;
- int stdin;
+ int stdin_;
int wait;
struct child_to_clean *next;
};
@@ -37,8 +37,8 @@ static void cleanup_children(int sig, int in_signal)
/* Close the the child's stdin as indicator that Git will exit soon */
while (p) {
if (p->wait)
- if (p->stdin > 0)
- close(p->stdin);
+ if (p->stdin_ > 0)
+ close(p->stdin_);
p = p->next;
}
@@ -73,12 +73,12 @@ static void cleanup_children_on_exit(void)
cleanup_children(SIGTERM, 0);
}
-static void mark_child_for_cleanup(pid_t pid, const char *name, int stdin, int wait)
+static void mark_child_for_cleanup(pid_t pid, const char *name, int stdin_, int wait)
{
struct child_to_clean *p = xmalloc(sizeof(*p));
p->pid = pid;
p->wait = wait;
- p->stdin = stdin;
+ p->stdin_ = stdin_;
if (name)
p->name = xstrdup(name);
else
@@ -94,7 +94,7 @@ static void mark_child_for_cleanup(pid_t pid, const char *name, int stdin, int w
}
#ifdef NO_PTHREADS
-static void mark_child_for_cleanup_no_wait(pid_t pid, const char *name, int timeout, int stdin)
+static void mark_child_for_cleanup_no_wait(pid_t pid, const char *name, int timeout, int stdin_)
{
mark_child_for_cleanup(pid, NULL, 0, 0);
}
--
2.10.0
^ permalink raw reply related
* Re: [RFC/PATCH 0/2] place cherry pick line below commit title
From: Junio C Hamano @ 2016-10-05 20:33 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, Christian Couder
In-Reply-To: <a7304731-fa05-92c2-6139-ecd1e6adcefd@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> Sounds reasonable to me. Would the "[" be a bit of overspecification,
> though, since Git doesn't produce it? Also, identifying it as a
> garbage line probably wouldn't change any behavior - in the Linux
> kernel examples, it is used to show what happened in between
> sign-offs, so there will always be one "Signed-off-by:" at the top.
Good thinking. As "interpret trailers" cannot locate such a line to
manipulate (as it lacks <token>), we can simply treat it as a
garbage line.
>> struct {
>> const char *whole;
>> const char *end_of_message_proper;
>> struct {
>> const char *token;
>> const char *contents;
>> } *trailer;
>> int alloc_trailers, nr_trailers;
>> };
>>
>> where
>>
>> - whole points at the first byte of the input, i.e. the beginning
>> of the commit message buffer.
>>
>> - end-of-message-proper points at the first byte of the trailer
>> block into the buffer at "whole".
>>
>> - token is a canonical header name for easy comparison for
>> interpret-trailers (you can use NULL for garbage lines, and made
>> up token like "[bracket]" and "(cherrypick)" that would not clash
>> with real tokens like "Signed-off-by").
>>
>> - contents is the bytes on the logical line, including the header
>> part
>>
>> E.g. an element in trailer[] array may say
>>
>> {
>> .token = "Signed-off-by",
>> .contents = "Signed-Off-By: Some Body <some@body.xz>\n",
>> }
>
> I get the impression from the rest of your e-mail that no strings are
> meant to be copied - is that true? (That sounds like a good idea to
> me.)
I was envisioning that "whole", "end-of-message" can point into the
input buffer, while trailer[].contents may have to be copied, if
only to make it to a NUL-terminated string. But I am fine with
<ptr,len> or <begin,end> pair to avoid copying .contents if that is
desired. You'd need to worry about differentiating .contents that
need to be freed after "interpret trailers" inserted a new entry or
replaced the contents, though.
^ permalink raw reply
* Re: [PATCH 1/6] git-merge: clarify "usage" by adding "-m <msg>"
From: Sergey Organov @ 2016-10-05 20:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqint6pu3b.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> sorganov@gmail.com writes:
>
>> From: Sergey Organov <sorganov@gmail.com>
>>
>> "-m <msg>" is one of essential distinctions between obsolete
>> invocation form and the recent one. Add it to the "usage" returned by
>> 'git merge -h' for more clarity.
>>
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>> builtin/merge.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/builtin/merge.c b/builtin/merge.c
>> index a8b57c7..0e367ba 100644
>> --- a/builtin/merge.c
>> +++ b/builtin/merge.c
>> @@ -43,7 +43,7 @@ struct strategy {
>> };
>>
>> static const char * const builtin_merge_usage[] = {
>> - N_("git merge [<options>] [<commit>...]"),
>> + N_("git merge [<options>] [-m <msg>] [<commit>...]"),
>> N_("git merge [<options>] <msg> HEAD <commit>"),
>> N_("git merge --abort"),
>> NULL
>
> While this is not wrong per-se, as the deprecated form will go away
> soon, I hope you do not mind if I had to drop this one from the
> series to avoid merge conflicts to 'pu' (I do not know how bad the
> conflict would be yet; I am just reviewing in my MUA).
Yeah, sure. I was not aware obsolete form description is to go away
soon.
^ permalink raw reply
* Re: [PATCH 4/6] Documentation/git-merge.txt: improve short description in NAME
From: Sergey Organov @ 2016-10-05 20:44 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster
In-Reply-To: <20161005175512.aruzndaow3g2jmt7@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Oct 05, 2016 at 05:46:22PM +0300, sorganov@gmail.com wrote:
>
>> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
>> index 216d2f4..cc0329d 100644
>> --- a/Documentation/git-merge.txt
>> +++ b/Documentation/git-merge.txt
>> @@ -3,7 +3,8 @@ git-merge(1)
>>
>> NAME
>> ----
>> -git-merge - Join two or more development histories together
>> +
>> +git-merge - Merge one or more branches to the current branch
>
> I wonder if we should be more clear that you don't have to merge a
> branch; you can merge any commit. I do agree that the original was
> unnecessarily general. And I think "the current branch" is accurate
> (technically it can be to a detached HEAD, but that is pedantry that
> doesn't need to make it into the synopsis).
>
> So maybe "Merge one or more commits into the current branch". I guess
> that is a bit vague, too. It is really "commit tips" or "lines of
> development" that we are merging. Bringing them in of course brings in
> many commits, but the "or more" there is meant to hint at multi-parent
> merges.
>
> So perhaps "one or more branches", while not completely accurate, is the
> best we can do. I dunno.
You've basically repeated my entire line of thinking that lead to the
patch.
-- Sergey.
^ permalink raw reply
* Re: [PATCH v9 04/14] run-command: add wait_on_exit
From: Lars Schneider @ 2016-10-05 20:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, ramsay, jnareb, j6t, tboegi, peff, mlbright
In-Reply-To: <xmqqh98rud20.fsf@gitster.mtv.corp.google.com>
> On 04 Oct 2016, at 21:30, Junio C Hamano <gitster@pobox.com> wrote:
>
> larsxschneider@gmail.com writes:
>
>> From: Lars Schneider <larsxschneider@gmail.com>
>>
>> The flag 'clean_on_exit' kills child processes spawned by Git on exit.
>> A hard kill like this might not be desired in all cases.
>>
>> Add 'wait_on_exit' which closes the child's stdin on Git exit and waits
>> until the child process has terminated.
>>
>> The flag is used in a subsequent patch.
>>
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>> ...
>> static void cleanup_children(int sig, int in_signal)
>> {
>> + int status;
>> + struct child_to_clean *p = children_to_clean;
>> +
>> + /* Close the the child's stdin as indicator that Git will exit soon */
>> + while (p) {
>> + if (p->wait)
>> + if (p->stdin > 0)
>> + close(p->stdin);
>> + p = p->next;
>> + }
>
> This part and the "stdin" field feels a bit too specific to the
> caller you are adding. Allowing the user of the API to specify what
> clean-up cation needs to be taken in the form of a callback function
> may not be that much more effort and would be more flexible and
> useful, I would imagine?
OK. Something like the patch below would work nicely.
Does this look acceptable?
Thanks,
Lars
diff --git a/run-command.c b/run-command.c
index 3269362..a0256a6 100644
--- a/run-command.c
+++ b/run-command.c
@@ -21,6 +21,7 @@ void child_process_clear(struct child_process *child)
struct child_to_clean {
pid_t pid;
+ void (*clean_on_exit_handler)(pid_t, int);
struct child_to_clean *next;
};
static struct child_to_clean *children_to_clean;
@@ -31,6 +32,11 @@ static void cleanup_children(int sig, int in_signal)
while (children_to_clean) {
struct child_to_clean *p = children_to_clean;
children_to_clean = p->next;
+
+ if (p->clean_on_exit_handler) {
+ p->clean_on_exit_handler(p->pid, in_signal);
+ }
+
kill(p->pid, sig);
if (!in_signal)
free(p);
@@ -49,10 +55,11 @@ static void cleanup_children_on_exit(void)
cleanup_children(SIGTERM, 0);
}
-static void mark_child_for_cleanup(pid_t pid)
+static void mark_child_for_cleanup(pid_t pid, void (*clean_on_exit_handler)(pid_t, int))
{
struct child_to_clean *p = xmalloc(sizeof(*p));
p->pid = pid;
+ p->clean_on_exit_handler = clean_on_exit_handler;
p->next = children_to_clean;
children_to_clean = p;
@@ -421,8 +428,8 @@ int start_command(struct child_process *cmd)
}
if (cmd->pid < 0)
error_errno("cannot fork() for %s", cmd->argv[0]);
- else if (cmd->clean_on_exit)
- mark_child_for_cleanup(cmd->pid);
+ else if (cmd->clean_on_exit || cmd->clean_on_exit_handler)
+ mark_child_for_cleanup(cmd->pid, cmd->clean_on_exit_handler);
/*
* Wait for child's execvp. If the execvp succeeds (or if fork()
@@ -482,8 +489,8 @@ int start_command(struct child_process *cmd)
failed_errno = errno;
if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))
error_errno("cannot spawn %s", cmd->argv[0]);
- if (cmd->clean_on_exit && cmd->pid >= 0)
- mark_child_for_cleanup(cmd->pid);
+ if ((cmd->clean_on_exit || cmd->clean_on_exit_handler) && cmd->pid >= 0)
+ mark_child_for_cleanup(cmd->pid, cmd->clean_on_exit_handler);
argv_array_clear(&nargv);
cmd->argv = sargv;
@@ -765,7 +772,7 @@ int start_async(struct async *async)
exit(!!async->proc(proc_in, proc_out, async->data));
}
- mark_child_for_cleanup(async->pid);
+ mark_child_for_cleanup(async->pid, NULL);
if (need_in)
close(fdin[0]);
diff --git a/run-command.h b/run-command.h
index cf29a31..3630733 100644
--- a/run-command.h
+++ b/run-command.h
@@ -43,6 +43,7 @@ struct child_process {
unsigned stdout_to_stderr:1;
unsigned use_shell:1;
unsigned clean_on_exit:1;
+ void (*clean_on_exit_handler)(pid_t, int);
};
#define CHILD_PROCESS_INIT { NULL, ARGV_ARRAY_INIT, ARGV_ARRAY_INIT }
^ permalink raw reply related
* Re: [PATCH 4/6] Documentation/git-merge.txt: improve short description in NAME
From: Sergey Organov @ 2016-10-05 21:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqa8eiptt2.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> sorganov@gmail.com writes:
>
>> From: Sergey Organov <sorganov@gmail.com>
>>
>> Old description not only raised the question of why the tool is called
>> git-merge rather than git-join, but "join histories" also sounds like
>> very simple operation, something like what "git-merge -s ours" does.
>>
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>> Documentation/git-merge.txt | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
>> index 216d2f4..cc0329d 100644
>> --- a/Documentation/git-merge.txt
>> +++ b/Documentation/git-merge.txt
>> @@ -3,7 +3,8 @@ git-merge(1)
>>
>> NAME
>> ----
>> -git-merge - Join two or more development histories together
>> +
>> +git-merge - Merge one or more branches to the current branch
>
> This patch, evaluated by itself, looks like a regression in that it
> tries to explain "merge" by using verb "merge", making it fuzzier to
> those who do not yet know what a "merge" is. That was why it tried
> to explain "merge" as an operation to join histories.
My thought was that "merge", the operation, is so well-known term that
it could well go into the NAME section without explanation.
Besides:
$ man merge
NAME
merge - three-way file merge
[...]
Uses the same pattern.
>
> However, the next one, 5/6, resurrects the "join history" in the
> description part to help them, so the damage is not so severe when
> we take them together.
Damage? In SCM world we can track the issue back to:
$ man -k rcsmerge
rcsmerge (1) - merge RCS revisions
-- Sergey
^ permalink raw reply
* Re: [PATCH 2/6] Documentation/git-merge.txt: remove list of options from SYNOPSIS
From: Sergey Organov @ 2016-10-05 21:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqeg3upu0m.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> sorganov@gmail.com writes:
>
>> From: Sergey Organov <sorganov@gmail.com>
>>
>> This partial list of option is confusing as it lacks a lot of
>> available options. It also clutters the SYNOPSIS making differences
>> between forms of invocation less clear.
>>
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>> Documentation/git-merge.txt | 5 +----
>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
>> index b758d55..90342eb 100644
>> --- a/Documentation/git-merge.txt
>> +++ b/Documentation/git-merge.txt
>> @@ -9,10 +9,7 @@ git-merge - Join two or more development histories together
>> SYNOPSIS
>> --------
>> [verse]
>> -'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]
>> - [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]]
>> - [--[no-]allow-unrelated-histories]
>> - [--[no-]rerere-autoupdate] [-m <msg>] [<commit>...]
>> +'git merge' [options] [-m <msg>] [<commit>...]
>> 'git merge' <msg> HEAD <commit>...
>> 'git merge' --abort
>
> Same comment as 1/6; as we'd hopefully be removing the deprecated
> form soonish, it would probably make sense to leave only two, i.e.
>
> git merge [options] [<commit>...]
> git merge --abort
>
> in synposis.
Same "yes" as in 1/6, obviously.
-- Sergey
^ permalink raw reply
* Re: Feature Request: user defined suffix for temp files created by git-mergetool
From: Johannes Sixt @ 2016-10-05 21:04 UTC (permalink / raw)
To: Josef Ridky; +Cc: git
In-Reply-To: <1499287628.1324571.1475653631366.JavaMail.zimbra@redhat.com>
Am 05.10.2016 um 09:47 schrieb Josef Ridky:
> Add support for user defined suffix part of name of temporary files
> created by git mergetool
Do I understand correctly that your users have problems to identify
which of the "_BASE_", "_LOCAL_", "_REMOTE_" and "_BACKUP_" files they
must edit? I agree that there is some room for improvement.
The goal is that you want the user to see the label, e.g., "_EDIT_THIS_"
instead of "_LOCAL_". Now you have to teach your users that they have to
pass --local=_EDIT_THIS_. Why don't you just teach your users to edit
the file labeled "_LOCAL_"?
Therefore, I think that your patch as written does not help to reduce
the confusion. It may be a building block for further improvement, but
if you stop here, it is pointless.
> SYNOPSIS
> --------
> [verse]
> -'git mergetool' [--tool=<tool>] [-y | --[no-]prompt] [<file>...]
> +'git mergetool' [--tool=<tool>] [-y | --[no-]prompt] [--local=<name>] [--remote=<name>] [--backup=<name>] [--base=<name>] [<file>...]
>
> DESCRIPTION
> -----------
> @@ -79,6 +79,30 @@ success of the resolution after the custom tool has exited.
> Prompt before each invocation of the merge resolution program
> to give the user a chance to skip the path.
>
> +--local=<name>::
> + Use string from <name> as part of suffix of name of temporary
> + file (local) for merging. If not used or is equal with any
> + other (remote|backup|base), default value is used.
> + Default suffix is LOCAL.
> +
> +--remote=<name>::
> + Use string from <name> as part of suffix of name of temporary
> + file (remote) for merging. If not used or is equal with any
> + other (local|backup|base), default value is used.
> + Default suffix is REMOTE.
> +
> +--backup=<name>::
> + Use string from <name> as part of suffix of name of temporary
> + file (backup) for merging. If not used or is equal with any
> + other (local|remote|base), default value is used.
> + Default suffix is BACKUP.
> +
> +--base=<name>::
> + Use string from <name> as part of suffix of name of temporary
> + file (base) for merging. If not used or is equal with any
> + other (local|remote|backup), default value is used.
> + Default suffix is BASE.
^ permalink raw reply
* Re: [PATCH v9 04/14] run-command: add wait_on_exit
From: Junio C Hamano @ 2016-10-05 21:12 UTC (permalink / raw)
To: Lars Schneider; +Cc: git, ramsay, jnareb, j6t, tboegi, peff, mlbright
In-Reply-To: <1FD7FB64-0F40-47F0-A047-25B91B170E66@gmail.com>
Lars Schneider <larsxschneider@gmail.com> writes:
> OK. Something like the patch below would work nicely.
Yeah, something along that line; it would eliminate the need to
worry about a field named "stdin" ;-)
But ...
> while (children_to_clean) {
> struct child_to_clean *p = children_to_clean;
> children_to_clean = p->next;
> +
> + if (p->clean_on_exit_handler) {
> + p->clean_on_exit_handler(p->pid, in_signal);
> + }
... the application that used run_command() API would want to be
able to pass extra piece of data that is appliation-specific for the
child being killed, so it may make sense to extend the function
signature to take a pointer to "struct child_process" for the child
process being killed, together with a new field added to "struct
child_process" that is "void *exit_handler_cbdata;", perhaps?
^ permalink raw reply
* Re: [PATCH 5/6] Documentation/git-merge.txt: improve short description in DESCRIPTION
From: Sergey Organov @ 2016-10-05 21:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq60p6pt4k.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> sorganov@gmail.com writes:
>
>> From: Sergey Organov <sorganov@gmail.com>
>>
>> Old description had a few problems:
>>
>> - sounded as if commits have changes
>>
>> - stated that changes are taken since some "divergence point"
>> that was not defined.
>>
>> New description rather uses "common ancestor" and "merge base",
>> definitions of which are easily discoverable in the rest of GIT
>> documentation.
>>
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>> Documentation/git-merge.txt | 25 +++++++++++++++----------
>> 1 file changed, 15 insertions(+), 10 deletions(-)
>>
>> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
>> index cc0329d..351b8fc 100644
>> --- a/Documentation/git-merge.txt
>> +++ b/Documentation/git-merge.txt
>> @@ -16,11 +16,16 @@ SYNOPSIS
>>
>> DESCRIPTION
>> -----------
>> -Incorporates changes from the named commits (since the time their
>> -histories diverged from the current branch) into the current
>> -branch. This command is used by 'git pull' to incorporate changes
>> -from another repository and can be used by hand to merge changes
>> -from one branch into another.
>> +
>> +Incorporates changes that lead to the named commits into the current
>> +branch, and joins corresponding histories. The best common ancestor of
>> +named commits and the current branch, called "merge base", is
>> +calculated, and then net changes taken from the merge base to
>> +the named commits are applied.
>> +
>> +This command is used by 'git pull' to incorporate changes from another
>> +repository, and can be used by hand to merge changes from one branch
>> +into another.
>
> Content change together with re-flowing the text makes it more
> costly than necessary to review a change like this. Please avoid
> doing so in your future patches.
OK, I see. So, what is the best way to handle this? Immediately follow
content change patch with another patch that only re-flows?
> I like what the updated description says very much. I however
> wonder if "and can be used by hand..." is still appropriate, or
> needs a bit of modernizing. It feels a bit awkward by making it
> sound as if 'git merge' is primarily an implementation detail of
> 'git pull' but it can also be used as the first-class command, which
> used to be the case in the old days back when "git pull . other" was
> also perfectly good way to merge the 'other' branch from your own
> repository, but I think your update is meant to clarify that we no
> longer live in that old world ;-)
Yes, exactly, but 6/6 removes most of the mentions of git-pull from the
manual anyway, so I felt it better belongs there.
>
>> @@ -31,11 +36,11 @@ Assume the following history exists and the current branch is
>> D---E---F---G master
>> ------------
>>
>> -Then "`git merge topic`" will replay the changes made on the
>> -`topic` branch since it diverged from `master` (i.e., `E`) until
>> -its current commit (`C`) on top of `master`, and record the result
>> -in a new commit along with the names of the two parent commits and
>> -a log message from the user describing the changes.
>
>> -Then "`git merge topic`" will replay the changes made on the `topic`
>> -branch since it diverged from `master` (i.e., `E`) until its current
>> -commit (`C`) on top of `master`, and record the result in a new commit
>> -along with the names of the two parent commits and a log message from
>> -the user describing the changes.
>
>> +Then "`git merge topic`" will replay the changes made on the `topic`
>> +branch since it diverged from `master` (i.e., `E`) until its current
>> +commit (`C`) on top of `master`, and record the result in a new commit
>> +along with references to the two parent commits and a log message from
>> +the user describing the changes.
>
> Content change together with re-flowing the text makes it more
> costly than necessary to review a change like this. Please avoid
> doing so in your future patches.
Yeah, got it.
> I had to re-flow the original you removed to match how you flowed in
> the updated one and stare at it for a while to spot that the only
> change was to rephrase "the names of the parents" to "references to
> the parents". I do not know if the updated phrasing is better. The
> "name" in the original was meant to be a short-hand for "object name",
> and I would support a change to spell it out to clarify; "reference"
> can be a vague word that can mean different things in Git, and when
> the word is given without context, most Git people would think that
> the word refers to "refs", but that is definitely not what the new
> commit records, so...
I won't insist on the change, but "name" sounded wrong to me, and
"reference" was most general term I was able to come up with in this
context.
First, "name" somehow suggested that it could be the case that
$ git branch
* master
$ git merge topic
will store strings "master" and "topic" in the resulting commit.
Next, should one really be aware of "10.2 Git Internals - Git Objects"
to grok this part of the manual if it actually meant to be "object
name"?
Last, if "reference" is not good enough and we get to internals anyway,
why not say SHA1 then?
-- Sergey
^ permalink raw reply
* Re: [PATCH 5/5] versioncmp: cope with common leading parts in versionsort.prereleaseSuffix
From: SZEDER Gábor @ 2016-10-05 21:26 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Leho Kraav, Nguyễn Thái Ngọc Duy, git
In-Reply-To: <xmqq4m4qrapv.fsf@gitster.mtv.corp.google.com>
Quoting Junio C Hamano <gitster@pobox.com>:
> SZEDER Gábor <szeder@ira.uka.de> writes:
>
>> And a final sidenote: sorting based on the longest matching suffix
>> also allows us to (ab)use version sort with prerelease suffixes to
>> sort postrelease tags as we please, too:
>>
>> $ ~/src/git/git -c versionsort.prereleasesuffix=-alpha \
>> -c versionsort.prereleasesuffix=-beta \
>> -c versionsort.prereleasesuffix= \
>> -c versionsort.prereleasesuffix=-gamma \
>> -c versionsort.prereleasesuffix=-delta \
>> tag -l --sort=version:refname 'v3.0*'
>> v3.0-alpha1
>> v3.0-beta1
>> v3.0
>> v3.0-gamma1
>> v3.0-delta1
>
> Assuming that gamma and delta are meant to indicate "these are
> post-release updates",
Indeed they were meant as post-release suffixes. Naturally following
alpha and beta, they were the first to spring to mind that should be
sorted in non-lexicographical order, so I could show of postrelease
reordering. It's just that we don't have a config like
'versionsort.postreleasesuffix', which is half the abuse. The other
half of the abuse is that I had to explicitly indicate the position
of suffixless versions with an empty suffix between pre- and
postrelease suffixes. The empty suffix matches on every tag, but
then it's overridden by all configured suffixes, so such a version
just stays in the middle.
> I think a mechanism that can yield the above
> result is fantastic ;-)
Heh.
Gut feeling tells me that I should take this as a subtle
encouragement to look into adding 'versionsort.postreleasesuffix',
shouldn't I ;)
^ permalink raw reply
* Re: [PATCH 5/6] Documentation/git-merge.txt: improve short description in DESCRIPTION
From: Sergey Organov @ 2016-10-05 21:27 UTC (permalink / raw)
To: Jakub Narębski; +Cc: git, Junio C Hamano
In-Reply-To: <e03a84ad-9aa6-8ada-5828-7b28f76baaaf@gmail.com>
Jakub Narębski <jnareb@gmail.com> writes:
> W dniu 05.10.2016 o 16:46, sorganov@gmail.com pisze:
>> From: Sergey Organov <sorganov@gmail.com>
>>
>> Old description had a few problems:
>>
>> - sounded as if commits have changes
>>
>> - stated that changes are taken since some "divergence point"
>> that was not defined.
>>
>> New description rather uses "common ancestor" and "merge base",
>> definitions of which are easily discoverable in the rest of GIT
>> documentation.
>
> This is a step in a good direction, but it has a few issues.
Thanks a lot for reviewing! I'll need time to read your reply carefully.
-- Sergey
^ permalink raw reply
* Re: [PATCH 6/6] Documentation/git-merge.txt: get rid of irrelevant references to git-pull
From: Sergey Organov @ 2016-10-05 21:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqzimioc7s.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> sorganov@gmail.com writes:
[...]
>> @@ -138,14 +133,15 @@ will exit early with the message "Already up-to-date."
>> FAST-FORWARD MERGE
>> ------------------
>>
>> -Often the current branch head is an ancestor of the named commit.
>> +Often the current branch head is an ancestor of the named commit. In
>> +this case, a new commit is not needed to store the combined history;
>> +instead, the `HEAD` (along with the index) is updated to point at the
>> +named commit, without creating an extra merge commit.
>> +
>> This is the most common case especially when invoked from 'git
>> pull': you are tracking an upstream repository, you have committed
>> no local changes, and now you want to update to a newer upstream
>> -revision. In this case, a new commit is not needed to store the
>> -combined history; instead, the `HEAD` (along with the index) is
>> -updated to point at the named commit, without creating an extra
>> -merge commit.
>> +revision.
>
> I am not sure if the post-image of this hunk is better than the
> original.
That's what I've tried to explain in the description of the patch:
"No awareness of git-pull is required to understand git-merge operation,
so leave reference to git-pull only where it actually makes sense, in
the description of fast-forward merges, and only as clarification of
when this merging behaviour is mostly useful."
So I believe this change is inline with the rest of the patch. The
reference to git-pull (if it remains) should be a side-note, not part of
explanation of operation.
-- Sergey
^ permalink raw reply
* Re: [PATCH 5/6] Documentation/git-merge.txt: improve short description in DESCRIPTION
From: Junio C Hamano @ 2016-10-05 21:41 UTC (permalink / raw)
To: Sergey Organov; +Cc: git
In-Reply-To: <871szuqyjo.fsf@javad.com>
Sergey Organov <sorganov@gmail.com> writes:
> OK, I see. So, what is the best way to handle this? Immediately follow
> content change patch with another patch that only re-flows?
Or no reflowing at all.
>> the parents". I do not know if the updated phrasing is better. The
>> "name" in the original was meant to be a short-hand for "object name",
>> and I would support a change to spell it out to clarify; "reference"
>> can be a vague word that can mean different things in Git, and when
>> the word is given without context, most Git people would think that
>> the word refers to "refs", but that is definitely not what the new
>> commit records, so...
>
> I won't insist on the change, but "name" sounded wrong to me, and
> "reference" was most general term I was able to come up with in this
> context.
> ...
> Last, if "reference" is not good enough and we get to internals anyway,
> why not say SHA1 then?
Because that is still colloquial? I think s/name/object name/ is
a sensible change, but not s/name/reference/.
^ permalink raw reply
* Re: [PATCH 6/6] Documentation/git-merge.txt: get rid of irrelevant references to git-pull
From: Junio C Hamano @ 2016-10-05 21:43 UTC (permalink / raw)
To: Sergey Organov; +Cc: git
In-Reply-To: <87shsapjiz.fsf@javad.com>
Sergey Organov <sorganov@gmail.com> writes:
> So I believe this change is inline with the rest of the patch. The
> reference to git-pull (if it remains) should be a side-note, not part of
> explanation of operation.
Not really. The thing is, "This is the most common" needs to be
close to "Often...". "git merge" directly invoked by the end user
is much less likely to encounter a fast forward situation; getting
invoked indirectly by "git pull" makes it common.
^ permalink raw reply
* Re: [PATCH v3 00/14] Mark strings in Perl scripts for translation
From: Jakub Narębski @ 2016-10-05 21:50 UTC (permalink / raw)
To: Vasco Almeida, git
Cc: Jiang Xin, Ævar Arnfjörð Bjarmason,
Jean-Noël AVILA, David Aguilar, Junio C Hamano
In-Reply-To: <20161005172110.30801-1-vascomalmeida@sapo.pt>
W dniu 05.10.2016 o 19:20, Vasco Almeida pisze:
> Mark messages in some perl scripts for translation.
>
> Thanks for the reviews of Junio Hamano and Jakub Narębski. Although I think
> Jakub Narębski's suggestions are overall good, I am not willing to go that path
> because I cannot see huge benefits from them given what we already have and
> also I lack Perl skills.
All right. While I think that Locale::TextDomain-like interpolation
in translated strings (__x, __xn / __nx, etc.), which is labelled as
'perl-brace-format' by gettext, is more Perl-ish and better for unbiased
translators, the printf based interpolation, labelled as ‘perl-format’
(and identical to 'c-format', I think), may be preferred in this case.
The 'perl-brace-format' doesn't need TRANSLATOR comments to explain
what placeholders are, and placeholders are easier to reorder. On
the other hand translator needs to know to not translate contents
of placeholders.
"This is the {color} {thing}.\n"
With 'perl-format' / 'c-format' the translator might need to know
how to change order of placeholders, but he or she should know
how to do it translating strings from C code.
"This is the %s %s.\n"
Also, if Perl code shares translation strings with C code, as in
most cases here, then printf format is needed to do translation
only once.
tldr; I am reversing my opinion, and agree with your solution.
>
> Interdiff bellow.
One thing I have noticed in the interdiff is using *translated*
strings in hash content (translation takes time, and might not
be necessary), that is:
$hashref = {
KEY => __('value'),
}
... $hashref->{KEY} ...
instead of marking value for translation, and doing translation
only on print, when it is necessary
$hashref = {
KEY => N__('value'),
}
... __($hashref->{KEY}) ...
>
> Vasco Almeida (14):
[...]
I'll try to review those later.
Thank you for your work,
--
Jakub Narębski
^ permalink raw reply
* Re: [RFC/PATCH] attr: Document a new possible thread safe API
From: Junio C Hamano @ 2016-10-05 22:09 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Brandon Williams
In-Reply-To: <CAGZ79kZ7zNkXBVUFq5aLSup9xsdCbZpy_x8z5=uAQqftmOq26A@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> I think so, instead of resending the documentation, maybe the header
> file shows that we're on the same page, I converted everything except
> attr.c to follow this header attr.h:
OK. The function signature of git_check_attr() looks suspect (it
does not match the "typical case" illustration in the message you
are responding to), but other than that I think this matches my
expectation.
Thanks for taking this over.
^ permalink raw reply
* Re: [PATCH 5/5] versioncmp: cope with common leading parts in versionsort.prereleaseSuffix
From: Junio C Hamano @ 2016-10-05 22:15 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Jeff King, Leho Kraav, Nguyễn Thái Ngọc Duy, git
In-Reply-To: <20161005232609.Horde.VetzEIKHDJUdcaOod9sHxuK@webmail.informatik.kit.edu>
SZEDER Gábor <szeder@ira.uka.de> writes:
> Gut feeling tells me that I should take this as a subtle
> encouragement to look into adding 'versionsort.postreleasesuffix',
> shouldn't I ;)
It is more like "this made me realize that these are merely 'suffix'
after the real release name, no pre- or post- about them", also
known as "I think PREreleasesuffix was a mistake and we weren't
thinking clearly enough when we added it."
To me, this looks like a list of possible suffixes that can include
an empty suffix to denote "the real thing", e.g.
versionsort.suffix = "-alpha" "-beta" "" "-gamma" "-delta"
and that position in the list determines the order of things inside
the same family of versions that share the same "non-suffix" part.
^ permalink raw reply
* Re: [PATCH v3 03/14] i18n: add--interactive: mark strings with interpolation for translation
From: Junio C Hamano @ 2016-10-05 22:28 UTC (permalink / raw)
To: Vasco Almeida
Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
Jean-Noël AVILA, Jakub Narębski, David Aguilar
In-Reply-To: <20161005172110.30801-4-vascomalmeida@sapo.pt>
Vasco Almeida <vascomalmeida@sapo.pt> writes:
> if (!defined $bottom) {
> - error_msg "Huh ($choice)?\n";
> + error_msg sprintf(__("Huh (%s)?\n"), $choice);
> next TOPLOOP;
> }
> }
> if ($opts->{SINGLETON} && $bottom != $top) {
> - error_msg "Huh ($choice)?\n";
> + error_msg sprintf(__("Huh (%s)?"), $choice);
> next TOPLOOP;
Doesn't this want "\n" just like the other one in this hunk?
^ permalink raw reply
* Re: [PATCH v9 00/14] Git filter protocol
From: Jakub Narębski @ 2016-10-05 22:31 UTC (permalink / raw)
To: larsxschneider, git; +Cc: ramsay, gitster, j6t, tboegi, peff, mlbright
In-Reply-To: <20161004125947.67104-1-larsxschneider@gmail.com>
W dniu 04.10.2016 o 14:59, larsxschneider@gmail.com pisze:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> The goal of this series is to avoid launching a new clean/smudge filter
> process for each file that is filtered.
>
> A short summary about v1 to v5 can be found here:
> https://git.github.io/rev_news/2016/08/17/edition-18/
>
> This series is also published on web:
> https://github.com/larsxschneider/git/pull/13
>
> Patches 1 and 2 are cleanups and not strictly necessary for the series.
> Patches 3 to 12 are required preparation. Patch 13 is the main patch.
> Patch 14 adds an example how to use the Git filter protocol in contrib.
>
> Thanks a lot to
> Ramsay, Jakub, Junio, Johannes, Torsten, and Peff
> for very helpful reviews,
> Lars
I'll try to review it before the end of the week.
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH v3 05/14] i18n: add--interactive: mark plural strings
From: Junio C Hamano @ 2016-10-05 22:41 UTC (permalink / raw)
To: Vasco Almeida
Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
Jean-Noël AVILA, Jakub Narębski, David Aguilar
In-Reply-To: <20161005172110.30801-6-vascomalmeida@sapo.pt>
Vasco Almeida <vascomalmeida@sapo.pt> writes:
> @@ -70,6 +72,8 @@ Git::I18N - Perl interface to Git's Gettext localizations
>
> printf __("The following error occurred: %s\n"), $error;
>
> + printf __n("commited %d file", "commited %d files", $files), $files;
> +
A micronit: the existing example above prints a whole line,
i.e. terminated with a LF. The new one probably should match.
^ permalink raw reply
* Re: [PATCH 5/5] versioncmp: cope with common leading parts in versionsort.prereleaseSuffix
From: Jacob Keller @ 2016-10-06 0:40 UTC (permalink / raw)
To: Junio C Hamano
Cc: SZEDER Gábor, Jeff King, Leho Kraav,
Nguyễn Thái Ngọc Duy, Git mailing list
In-Reply-To: <xmqqwphmmoi6.fsf@gitster.mtv.corp.google.com>
On Wed, Oct 5, 2016 at 3:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> SZEDER Gábor <szeder@ira.uka.de> writes:
>
>> Gut feeling tells me that I should take this as a subtle
>> encouragement to look into adding 'versionsort.postreleasesuffix',
>> shouldn't I ;)
>
> It is more like "this made me realize that these are merely 'suffix'
> after the real release name, no pre- or post- about them", also
> known as "I think PREreleasesuffix was a mistake and we weren't
> thinking clearly enough when we added it."
>
> To me, this looks like a list of possible suffixes that can include
> an empty suffix to denote "the real thing", e.g.
>
> versionsort.suffix = "-alpha" "-beta" "" "-gamma" "-delta"
>
> and that position in the list determines the order of things inside
> the same family of versions that share the same "non-suffix" part.
This is what makes sense to me. Perhaps migrade to "releasesuffix" and
deprecate "prereleasesuffix"? I don't think that's too painful
especially if git accepts the old value and warns about it's
deprecation? That or we can just stick with calling it pre-release and
they sort based on order with '' being the empty value?
Thanks,
Jake
^ permalink raw reply
* Re: Setting pager.add=true breaks add --patch
From: Tom Hale @ 2016-10-06 3:55 UTC (permalink / raw)
To: Anatoly Borodin; +Cc: Git mailing list
In-Reply-To: <CACNzp2kd6wdE6pGsb5d5+cvkJa9M-gzG+5=oLhZr9dLn4o8gOQ@mail.gmail.com>
On 2016-10-03 00:00, Anatoly Borodin wrote:
> I've reported this one bug recently:
>
> https://public-inbox.org/git/nrmbrl$hsk$1@blaine.gmane.org/
>
> The developers know about it, but it will require some deeper refactoring.
Thanks Anatoly for reporting this.
[Meta] All: For updates, is there an issue I can watch, or a way to
subscribe/monitor only this one thread?
--
Cheers,
Tom
^ 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