* Re: [PATCH v3 01/38] sequencer: avoid unnecessary curly braces
From: Jeff King @ 2017-01-14 18:05 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Junio C Hamano, git, Kevin Daudt, Dennis Kaarsemaker,
Stephan Beyer
In-Reply-To: <alpine.DEB.2.20.1701141856240.3469@virtualbox>
On Sat, Jan 14, 2017 at 06:57:13PM +0100, Johannes Schindelin wrote:
> On Thu, 12 Jan 2017, Junio C Hamano wrote:
>
> > Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> >
> > >
> > > - if (!commit->parents) {
> > > + if (!commit->parents)
> > > parent = NULL;
> > > - }
> > > else if (commit->parents->next) {
> > > /* Reverting or cherry-picking a merge commit */
> > > int cnt;
> >
> > The result becomes
> >
> > if (...)
> > single statement;
> > else if (...) {
> > multiple;
> > statements;
> > }
> >
> > which is not quite an improvement.
>
> Yet, this used to be the coding style of Git, and your statement comes
> quite as a surprise to me.
Yeah, I thought we were OK with:
if (cond)
single statement;
else {
multiple;
statements;
}
but not the other way around:
if (cond) {
multiple;
statements;
} else
single statement;
I don't know if the "else if" changes that or not, but I certainly have
written things like your patch does.
-Peff
^ permalink raw reply
* Re: [PATCH v3 00/38] Teach the sequencer to act as rebase -i's backend
From: Johannes Schindelin @ 2017-01-14 18:04 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Kevin Daudt, Dennis Kaarsemaker, Stephan Beyer, Jeff King
In-Reply-To: <xmqqinpnuaxl.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 9 Jan 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > Changes since v2:
> >
> > - fixed a TRANSLATORS: comment
> > ...
> > - replaced a spawned `diff-tree` command by code using the diff functions
> > directly
>
> I just finished skimming the interdiff (the difference between the
> result of merging the v2 into 'master' and the result of applying
> this series on 'master').
I wish you would not have skimmed it, but provided a thorough review.
There was a rather serious bug in this (not the first problem introduced
into one of my patch series *because of* code review, unhidden-git and
mmap-regexec are also very recent examples, I really should learn to
resist the prodding to replace well-tested code with code of unknown
correctness).
The problem in this instance was that the authorship is no longer retained
when continuing after resolving a conflict. Let me stress again that this
has not been a problem with v1 of sequencer-i, nor with v2. The regression
was caused by changes required by the code review.
In case you wonder: Yes, I am upset by this.
The required fixup patch is:
-- snipsnap --
Subject: [PATCH] fixup! sequencer: make reading author-script more elegant
An unfortunate regression of formerly battle-tested code sadly crept
into Git for Windows v2.11.0(2): authorship was not retained in case of
conflicts during picks.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 2 +-
t/t3404-rebase-interactive.sh | 16 ++++++++++++++++
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index 73b2ec6894..8ecab02291 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -612,7 +612,7 @@ static int read_env_script(struct argv_array *env)
count++;
}
- for (i = 0; i < count; i++) {
+ for (i = 0, p = script.buf; i < count; i++) {
argv_array_push(env, p);
p += strlen(p) + 1;
}
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 71b9c8ef8b..61113be08a 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -237,6 +237,22 @@ test_expect_success 'retain authorship' '
git show HEAD | grep "^Author: Twerp Snog"
'
+test_expect_success 'retain authorship w/ conflicts' '
+ git reset --hard twerp &&
+ test_commit a conflict a conflict-a &&
+ git reset --hard twerp &&
+ GIT_AUTHOR_NAME=AttributeMe \
+ test_commit b conflict b conflict-b &&
+ set_fake_editor &&
+ test_must_fail git rebase -i conflict-a &&
+ echo resolved >conflict &&
+ git add conflict &&
+ git rebase --continue &&
+ test $(git rev-parse conflict-a^0) = $(git rev-parse HEAD^) &&
+ git show >out &&
+ grep AttributeMe out
+'
+
test_expect_success 'squash' '
git reset --hard twerp &&
echo B > file7 &&
--
2.11.0.windows.3
^ permalink raw reply related
* Re: [PATCH v3 01/38] sequencer: avoid unnecessary curly braces
From: Johannes Schindelin @ 2017-01-14 17:57 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Kevin Daudt, Dennis Kaarsemaker, Stephan Beyer, Jeff King
In-Reply-To: <xmqqk2a0ktxr.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Thu, 12 Jan 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> >
> > - if (!commit->parents) {
> > + if (!commit->parents)
> > parent = NULL;
> > - }
> > else if (commit->parents->next) {
> > /* Reverting or cherry-picking a merge commit */
> > int cnt;
>
> The result becomes
>
> if (...)
> single statement;
> else if (...) {
> multiple;
> statements;
> }
>
> which is not quite an improvement.
Yet, this used to be the coding style of Git, and your statement comes
quite as a surprise to me.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v3 06/38] sequencer (rebase -i): implement the 'edit' command
From: Johannes Schindelin @ 2017-01-14 17:56 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Kevin Daudt, Dennis Kaarsemaker, Stephan Beyer, Jeff King
In-Reply-To: <xmqq7f60kssh.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Thu, 12 Jan 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > +static int make_patch(struct commit *commit, struct replay_opts *opts)
> > +{
> > + struct strbuf buf = STRBUF_INIT;
> > + struct rev_info log_tree_opt;
> > + const char *subject, *p;
> > + int res = 0;
> > +
> > + p = short_commit_name(commit);
> > + if (write_message(p, strlen(p), rebase_path_stopped_sha(), 1) < 0)
> > + return -1;
> > +
> > + strbuf_addf(&buf, "%s/patch", get_dir(opts));
> > + memset(&log_tree_opt, 0, sizeof(log_tree_opt));
> > + init_revisions(&log_tree_opt, NULL);
> > + log_tree_opt.abbrev = 0;
> > + log_tree_opt.diff = 1;
> > + log_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH;
> > + log_tree_opt.disable_stdin = 1;
> > + log_tree_opt.no_commit_id = 1;
> > + log_tree_opt.diffopt.file = fopen(buf.buf, "w");
> > + log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;
> > + if (!log_tree_opt.diffopt.file)
> > + res |= error_errno(_("could not open '%s'"), buf.buf);
> > + else {
> > + res |= log_tree_commit(&log_tree_opt, commit);
> > + fclose(log_tree_opt.diffopt.file);
> > + }
> > + strbuf_reset(&buf);
> > +
> > + strbuf_addf(&buf, "%s/message", get_dir(opts));
> > + if (!file_exists(buf.buf)) {
> > + const char *commit_buffer = get_commit_buffer(commit, NULL);
> > + find_commit_subject(commit_buffer, &subject);
> > + res |= write_message(subject, strlen(subject), buf.buf, 1);
> > + unuse_commit_buffer(commit, commit_buffer);
> > + }
> > + strbuf_release(&buf);
> > +
> > + return res;
> > +}
>
> Unlike the scripted version, where a merge is shown with "diff --cc"
> and a root commit is shown as "Root commit", this only deals with a
> single-parent commit.
Indeed. The reason is that we never encounter a merge commit (as we
explicitly do not handle --preserve-merges) nor root commits (as we
explicitly do not handle --root)
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCHv3 2/2] builtin/commit.c: switch to xstrfmt(), instead of snprintf,
From: René Scharfe @ 2017-01-14 16:31 UTC (permalink / raw)
To: Elia Pinto, git
In-Reply-To: <20170113175801.39468-2-gitter.spiros@gmail.com>
Am 13.01.2017 um 18:58 schrieb Elia Pinto:
> In this patch, instead of using xnprintf instead of snprintf, which asserts
> that we don't truncate, we are switching to dynamic allocation with xstrfmt(),
> , so we can avoid dealing with magic numbers in the code and reduce the cognitive burden from
> the programmers, because they no longer have to count bytes needed for static allocation.
> As a side effect of this patch we have also reduced the snprintf() calls, that may silently truncate
> results if the programmer is not careful.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
> This is the third version of the patch.
>
> Changes from the first version: I have split the original commit in two, as discussed here
> http://public-inbox.org/git/20161213132717.42965-1-gitter.spiros@gmail.com/.
>
> Changes from the second version:
> - Changed the commit message to clarify the purpose of the patch (
> as suggested by Junio)
> https://public-inbox.org/git/xmqqtw95mfo3.fsf@gitster.mtv.corp.google.com/T/#m2e6405a8a78a8ca1ed770614c91398290574c4a1
>
>
>
> builtin/commit.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 09bcc0f13..37228330c 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1526,12 +1526,10 @@ static int git_commit_config(const char *k, const char *v, void *cb)
> static int run_rewrite_hook(const unsigned char *oldsha1,
> const unsigned char *newsha1)
> {
> - /* oldsha1 SP newsha1 LF NUL */
> - static char buf[2*40 + 3];
> + char *buf;
> struct child_process proc = CHILD_PROCESS_INIT;
> const char *argv[3];
> int code;
> - size_t n;
>
> argv[0] = find_hook("post-rewrite");
> if (!argv[0])
> @@ -1547,11 +1545,11 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
> code = start_command(&proc);
> if (code)
> return code;
> - n = snprintf(buf, sizeof(buf), "%s %s\n",
> - sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
> + buf = xstrfmt("%s %s\n", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
> sigchain_push(SIGPIPE, SIG_IGN);
> - write_in_full(proc.in, buf, n);
> + write_in_full(proc.in, buf, strlen(buf));
> close(proc.in);
> + free(buf);
> sigchain_pop(SIGPIPE);
> return finish_command(&proc);
> }
>
Perhaps I missed it from the discussion, but why not use strbuf? It
would avoid counting the generated string's length. That's probably
not going to make a measurable difference performance-wise, but it's
easy to avoid and doesn't even take up more lines:
---
builtin/commit.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 711f96cc43..73bb72016f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1525,12 +1525,10 @@ static int git_commit_config(const char *k, const char *v, void *cb)
static int run_rewrite_hook(const unsigned char *oldsha1,
const unsigned char *newsha1)
{
- /* oldsha1 SP newsha1 LF NUL */
- static char buf[2*40 + 3];
+ struct strbuf sb = STRBUF_INIT;
struct child_process proc = CHILD_PROCESS_INIT;
const char *argv[3];
int code;
- size_t n;
argv[0] = find_hook("post-rewrite");
if (!argv[0])
@@ -1546,11 +1544,11 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
code = start_command(&proc);
if (code)
return code;
- n = snprintf(buf, sizeof(buf), "%s %s\n",
- sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
+ strbuf_addf(&sb, "%s %s\n", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
sigchain_push(SIGPIPE, SIG_IGN);
- write_in_full(proc.in, buf, n);
+ write_in_full(proc.in, sb.buf, sb.len);
close(proc.in);
+ strbuf_release(&sb);
sigchain_pop(SIGPIPE);
return finish_command(&proc);
}
--
2.11.0
^ permalink raw reply related
* Re: [PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context
From: René Scharfe @ 2017-01-14 14:58 UTC (permalink / raw)
To: Junio C Hamano, Vegard Nossum; +Cc: git
In-Reply-To: <xmqqbmvaecpl.fsf@gitster.mtv.corp.google.com>
Am 14.01.2017 um 00:56 schrieb Junio C Hamano:
> Vegard Nossum <vegard.nossum@oracle.com> writes:
>
>> The patch will work as intended and as expected for 95% of the users out
>> there (javadoc, Doxygen, kerneldoc, etc. all have the comment
>> immediately preceding the function) and fixes a very real problem for me
>> (and I expect many others) _today_; for the remaining 5% (who put a
>> blank line between their comment and the start of the function) it will
>> revert back to the current behaviour, so there should be no regression
>> for them.
>
> I notice your 95% are all programming languages, but I am more
> worried about the contents written in non programming languages
> (René gave HTML an an example--there may be other types of contents
> that we programmer types do not deal with every day, but Git users
> depend on).
>
> I am also more focused on keeping the codebase maintainable in good
> health by making sure that we made an effort to find a solution that
> is general-enough before solving a single specific problem you have
> today. We may end up deciding that a blank-line heuristics gives us
> good enough tradeoff, but I do not want us to make a decision before
> thinking.
How about extending the context upward only up to and excluding a line
that is either empty *or* a function line? That would limit the extra
context to a single function in the worst case.
Reducing context at the bottom with the aim to remove comments for the
next section is more tricky as it could remove part of the function that
we'd like to show if we get the boundary wrong. How bad would it be to
keep the southern border unchanged?
René
^ permalink raw reply
* Re: [PATCHv3 2/2] builtin/commit.c: switch to xstrfmt(), instead of snprintf,
From: Elia Pinto @ 2017-01-14 12:42 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Junio C Hamano
In-Reply-To: <20170113183309.GA28002@google.com>
Ok. I agree. But is it strictly necessary to resend for this ?
Thanks
2017-01-13 19:33 GMT+01:00 Brandon Williams <bmwill@google.com>:
> On 01/13, Elia Pinto wrote:
>> In this patch, instead of using xnprintf instead of snprintf, which asserts
>> that we don't truncate, we are switching to dynamic allocation with xstrfmt(),
>> , so we can avoid dealing with magic numbers in the code and reduce the cognitive burden from
>> the programmers, because they no longer have to count bytes needed for static allocation.
>> As a side effect of this patch we have also reduced the snprintf() calls, that may silently truncate
>> results if the programmer is not careful.
>>
>> Helped-by: Junio C Hamano <gitster@pobox.com>
>> Helped-by: Jeff King <peff@peff.net>
>> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
>
> Small nit's with the commit message:
> * Stray comma ',' of on its own
> * lines are longer than 80 characters
>
>> ---
>> This is the third version of the patch.
>>
>> Changes from the first version: I have split the original commit in two, as discussed here
>> http://public-inbox.org/git/20161213132717.42965-1-gitter.spiros@gmail.com/.
>>
>> Changes from the second version:
>> - Changed the commit message to clarify the purpose of the patch (
>> as suggested by Junio)
>> https://public-inbox.org/git/xmqqtw95mfo3.fsf@gitster.mtv.corp.google.com/T/#m2e6405a8a78a8ca1ed770614c91398290574c4a1
>>
>>
>>
>> builtin/commit.c | 10 ++++------
>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index 09bcc0f13..37228330c 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -1526,12 +1526,10 @@ static int git_commit_config(const char *k, const char *v, void *cb)
>> static int run_rewrite_hook(const unsigned char *oldsha1,
>> const unsigned char *newsha1)
>> {
>> - /* oldsha1 SP newsha1 LF NUL */
>> - static char buf[2*40 + 3];
>> + char *buf;
>> struct child_process proc = CHILD_PROCESS_INIT;
>> const char *argv[3];
>> int code;
>> - size_t n;
>>
>> argv[0] = find_hook("post-rewrite");
>> if (!argv[0])
>> @@ -1547,11 +1545,11 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
>> code = start_command(&proc);
>> if (code)
>> return code;
>> - n = snprintf(buf, sizeof(buf), "%s %s\n",
>> - sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
>> + buf = xstrfmt("%s %s\n", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
>> sigchain_push(SIGPIPE, SIG_IGN);
>> - write_in_full(proc.in, buf, n);
>> + write_in_full(proc.in, buf, strlen(buf));
>> close(proc.in);
>> + free(buf);
>> sigchain_pop(SIGPIPE);
>> return finish_command(&proc);
>> }
>> --
>> 2.11.0.154.g5f5f154
>>
>
> --
> Brandon Williams
^ permalink raw reply
* [ANNOUNCE] Git for Windows 2.11.0(3)
From: Johannes Schindelin @ 2017-01-14 12:09 UTC (permalink / raw)
To: git-for-windows, git; +Cc: Johannes Schindelin
MIME-Version: 1.0
Fcc: Sent
Dear Git users,
It is my pleasure to announce that Git for Windows 2.11.0(3) is available from:
https://git-for-windows.github.io/
Changes since Git for Windows v2.11.0(2) (January 13th 2017)
Bug Fixes
• Fixed an off-by-two bug in the POSIX emulation layer that possibly
affected third-party Perl scripts that load native libraries
dynamically.
• A regression in rebase -i, introduced into v2.11.0(2), which caused
commit attribution to be mishandled after resolving conflicts, was
fixed.
Filename | SHA-256
-------- | -------
Git-2.11.0.3-64-bit.exe | c3897e078cd7f7f496b0e4ab736ce144c64696d3dbee1e5db417ae047ca3e27f
Git-2.11.0.3-32-bit.exe | dff9bec9c4e21eaba5556fe4a7b1071d1f18e3a8b9645bffb48fda9eaee37e62
PortableGit-2.11.0.3-64-bit.7z.exe | 41a4ab3a1f0c88a3254b5a30d49c0c6e4ef06c204ddce53e23fc15d6e56f8d24
PortableGit-2.11.0.3-32-bit.7z.exe | 8bf3769c37945e991903dd1b988c6b1d97bbf0f3afc9851508974f38bf94dc01
MinGit-2.11.0.3-64-bit.zip | bf3714e04bcbafb464353235a27c328c43d40568d6b2e9064f1a63444b8236c5
MinGit-2.11.0.3-32-bit.zip | db05d5e98ef1017dd07f27fa4641dc8c0d66ba09da5a196ef656e7f1d7c078e2
Git-2.11.0.3-64-bit.tar.bz2 | 65296c54f0b8294374547cc6a169d6ea95178e12dee04cd2d4632f39d8fe7852
Git-2.11.0.3-32-bit.tar.bz2 | 0f0e2f78fc9b91d6c860eb7de742f3601b0ccd13c5c61444c7cf55b00bcb4ed4
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH 2/2] Use 'env' to find perl instead of fixed path
From: Eric Wong @ 2017-01-14 10:31 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Pat Pannuto, Johannes Schindelin, Johannes Sixt,
git
In-Reply-To: <20170114075408.hyidkb4rzxzmm2je@sigill.intra.peff.net>
Jeff King <peff@peff.net> wrote:
> Just as a devil's advocate, why do we care about warnings in third-party
> modules? Or more specifically, why do _users_ who are running Git care
> about them? We cannot fix them in Git. A user may report the error to
> the module author, but the module author may not be responsive, or even
> may not be inclined to fix the problem (because they have a particular
> opinion on that warning).
Every user is a potential developer(*). And I do feel
we (git developers) should be at least somewhat responsible
for helping maintain and fix the projects we depend on;
or moving to alternatives if we can't fix them.
There is a chance a newly-introduced warning in a 3rd-party
module points to a real problem with the way git uses it, too.
Having that warning would help us fix or workaround the bug
(either in git or the module).
I doubt any module author would be unresponsive to having a
localized "no warnings" for special cases. AFAIK, "-w" is
widespread amongst Perl users (unlike Ruby in my experience).
(*) I feel that more strongly in the git case, and even more so
for the Perl bits since the source is already on the user's
machine.
> In the meantime, the user is stuck with an annoying warning message
> until Git is updated as you showed above. Why not just start there
> preemptively, and let module authors worry about their own warnings?
I'm not saying we blindly start using '-w' everywhere today.
But we may at least try it and see if it introduces new
warnings, first, and only enable '-w' when it it looks quiet
(and perhaps start working with module authors to fix warnings
if not).
As a user, I'd rather have some indication of where something
might be wrong, than no warning at all when something does go
wrong.
^ permalink raw reply
* Re: [PATCH v10 00/20] port branch.c to use ref-filter's printing options
From: Karthik Nayak @ 2017-01-14 10:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jacob Keller
In-Reply-To: <xmqqbmver63s.fsf@gitster.mtv.corp.google.com>
On Wed, Jan 11, 2017 at 2:21 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> index 81db67d74..08be8462c 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -95,13 +95,17 @@ refname::
>> The name of the ref (the part after $GIT_DIR/).
>> For a non-ambiguous short name of the ref append `:short`.
>> The option core.warnAmbiguousRefs is used to select the strict
>> + abbreviation mode. If `lstrip=<N>` is appended, strips `<N>`
>> + slash-separated path components from the front of the refname
>> + (e.g., `%(refname:lstrip=2)` turns `refs/tags/foo` into `foo` and
>> + `%(refname:rstrip=2)` turns `refs/tags/foo` into `refs`).
>
> I hiccupped while reading this, as the (e.g.) example talks about rstrip
> that is not mentioned in the main text that is enhanced by the
> example.
>
> If `lstrip=<N>` (`rstrip=<N>`) is appended, strips `<N>`
> slash-separated path components from the front (tail) of the
> refname (e.g. `%(refname:lstrip=2)` ...
>
> perhaps?
>
>> + if `<N>` is a negative number, then only `<N>` path components
>> + are left behind.
>
> Begin the sentence with cap? I'd rephrase it a bit while at it if I
> were doing this:
>
> If `<N>` is a negative number, strip as many path components
> as necessary from the specified end to leave `-<N>` path
> components.
>
> Other than the above, looks very good to me.
>
> Thanks.
I like how you rephrased it.
So apart form this and the other change suggested by you and Jacob,
nothing to be added.
Should I re-roll?
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: [PATCH v10 03/20] ref-filter: implement %(if:equals=<string>) and %(if:notequals=<string>)
From: Karthik Nayak @ 2017-01-14 10:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jacob Keller
In-Reply-To: <xmqqfukqr6ep.fsf@gitster.mtv.corp.google.com>
Hello,
On Wed, Jan 11, 2017 at 2:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> + if_then_else->condition_satisfied = 1;
>> + } else if (if_then_else->cmp_status == COMPARE_UNEQUAL) {
>
> Please, no space before tabs (locally fixed--no need to resend).
Thanks.
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: [PATCH v10 19/20] branch: use ref-filter printing APIs
From: Karthik Nayak @ 2017-01-14 10:01 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list, Junio C Hamano
In-Reply-To: <CA+P7+xqV+CJwP-0_27V26UZbkDzBqbdstGw_Rq=8c3SkjAq2bA@mail.gmail.com>
Hello,
On Thu, Jan 12, 2017 at 5:17 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Tue, Jan 10, 2017 at 12:49 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> diff --git a/builtin/branch.c b/builtin/branch.c
>> index 34cd61cd9..f293ee5b0 100644
>> --- a/builtin/branch.c
>> +++ b/builtin/branch.c
>> @@ -37,11 +37,11 @@ static unsigned char head_sha1[20];
>> static int branch_use_color = -1;
>> static char branch_colors[][COLOR_MAXLEN] = {
>> GIT_COLOR_RESET,
>> - GIT_COLOR_NORMAL, /* PLAIN */
>> - GIT_COLOR_RED, /* REMOTE */
>> - GIT_COLOR_NORMAL, /* LOCAL */
>> - GIT_COLOR_GREEN, /* CURRENT */
>> - GIT_COLOR_BLUE, /* UPSTREAM */
>> + GIT_COLOR_NORMAL, /* PLAIN */
>> + GIT_COLOR_RED, /* REMOTE */
>> + GIT_COLOR_NORMAL, /* LOCAL */
>> + GIT_COLOR_GREEN, /* CURRENT */
>> + GIT_COLOR_BLUE, /* UPSTREAM */
>> };
>
>
> What's... actually changing here? It looks like just white space? Is
> there a strong reason for why this is changing?
>
> Thanks,
> Jake
None, I'm not sure how this ended up being added too.
--
Regards,
Karthik Nayak
^ permalink raw reply
* gitk pull request // was: Re: gitk: "lime" color incompatible with older Tk versions
From: David Aguilar @ 2017-01-14 8:48 UTC (permalink / raw)
To: Junio C Hamano
Cc: Stefan Beller, Andrew Janke, Paul Mackerras, git@vger.kernel.org
In-Reply-To: <20170113112043.j7nowdilolswyk2k@gmail.com>
On Fri, Jan 13, 2017 at 03:20:43AM -0800, David Aguilar wrote:
>
> Ping.. it would be nice to get this patch applied.
Sorry for the noise, and thank you Paul for the fix.
This was already fixed by Paul in gitk@22a713c72df.
I'm sure Junio will merge gitk.git into git.git soon enough so I
can sit tight until then, but while I'm here I might as well
send out a pull request:
The following changes since commit 22a713c72df8b6799c59287c50cee44c4a6db51e:
gitk: Follow themed bgcolor in help dialogs (2016-03-19 14:12:21 +1100)
are available in the git repository at:
git://ozlabs.org/~paulus/gitk.git
for you to fetch changes up to fbf426478e540f4737860dae622603cc0daba3d2:
gitk: Update copyright notice to 2016 (2016-12-12 20:46:42 +1100)
----------------------------------------------------------------
Markus Hitter (3):
gitk: Turn off undo manager in the text widget
gitk: Remove closed file descriptors from $blobdifffd
gitk: Clear array 'commitinfo' on reload
Paul Mackerras (2):
gitk: Use explicit RGB green instead of "lime"
gitk: Update copyright notice to 2016
Rogier Goossens (3):
gitk: Add a 'rename' option to the branch context menu
gitk: Allow checking out a remote branch
gitk: Include commit title in branch dialog
Satoshi Yasushima (1):
gitk: Fix Japanese translation for "marked commit"
Stefan Dotterweich (1):
gitk: Fix missing commits when using -S or -G
Vasco Almeida (2):
gitk: Makefile: create install bin directory
gitk: Add Portuguese translation
Makefile | 1 +
gitk | 166 +++++--
po/bg.po | 4 +-
po/ca.po | 6 +-
po/de.po | 4 +-
po/es.po | 4 +-
po/fr.po | 4 +-
po/hu.po | 4 +-
po/it.po | 4 +-
po/ja.po | 13 +-
po/pt_br.po | 4 +-
po/pt_pt.po | 1376 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
po/ru.po | 4 +-
po/sv.po | 8 +-
po/vi.po | 4 +-
15 files changed, 1549 insertions(+), 57 deletions(-)
create mode 100644 po/pt_pt.po
Thanks,
--
David
^ permalink raw reply
* Re: [PATCH 2/2] Use 'env' to find perl instead of fixed path
From: Jeff King @ 2017-01-14 7:54 UTC (permalink / raw)
To: Eric Wong
Cc: Junio C Hamano, Pat Pannuto, Johannes Schindelin, Johannes Sixt,
git
In-Reply-To: <20170113185246.GA17441@starla>
On Fri, Jan 13, 2017 at 06:52:46PM +0000, Eric Wong wrote:
> > If something we _use_ from a third-party is not warnings-clean,
> > there is no easy way to squelch them if we use "-w", which is a
> > potential downside, isn't it? I do not know how serious a problem
> > it is in practice. I suspect that the core package we use from perl
> > distribution are supposed to be warnings-clean, but we use a handful
> > of things from outside the core and I do not know what state they
> > are in.
>
> Yes, "-w" will trigger warnings in third party packages.
> Existing uses we have should be fine, and I think most Perl
> modules we use or would use are vigilant about being
> warnings-clean. If we have to leave off a "-w", there should
> probably be a comment at the top stating the reason:
>
> #!/usr/bin/perl
> # Not using "perl -w" since Foo::Bar <= X.Y.Y is not warnings-clean
> use strict;
> use warnings;
> use Foo::Bar;
> ...
Just as a devil's advocate, why do we care about warnings in third-party
modules? Or more specifically, why do _users_ who are running Git care
about them? We cannot fix them in Git. A user may report the error to
the module author, but the module author may not be responsive, or even
may not be inclined to fix the problem (because they have a particular
opinion on that warning).
In the meantime, the user is stuck with an annoying warning message
until Git is updated as you showed above. Why not just start there
preemptively, and let module authors worry about their own warnings?
-Peff
^ permalink raw reply
* merge maintaining history
From: David J. Bakeman @ 2017-01-14 2:01 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 854 bytes --]
History
git cloned a remote repository and made many changes pushing them all to
said repository over many months.
The powers that be then required me to move project to new repository
server did so by pushing local version to new remote saving all history!
Now have to merge back to original repository(which has undergone many
changes since I split off) but how do I do that without loosing the
history of all the commits since the original move? Note I need to push
changes to files that are already in existence. I found on the web a
bunch of ways to insert a whole new directory structure into an existing
repository but as I said I need to do it on top of existing files. Of
course I can copy all the files from my local working repository to the
cloned remote repository and commit any changes but I loose all the
history that way.
Thanks.
[-- Attachment #2: nakuru.vcf --]
[-- Type: text/x-vcard, Size: 248 bytes --]
begin:vcard
fn:David J. Bakeman
n:Bakeman;David J.
org:Nakuru Software Inc.
adr:;;1504 North 57th Street;Seattle;WA;98103;USA
email;internet:nakuru@comcast.net
tel;work:(206)545-0609
tel;fax:(206)600-6957
x-mozilla-html:TRUE
version:2.1
end:vcard
^ permalink raw reply
* Re: [PATCH] submodule update: run custom update script for initial populating as well
From: Stefan Beller @ 2017-01-14 0:00 UTC (permalink / raw)
To: Junio C Hamano
Cc: Brandon Williams, Chris Packham, Spencer Olson,
git@vger.kernel.org
In-Reply-To: <xmqq7f5yeclw.fsf@gitster.mtv.corp.google.com>
On Fri, Jan 13, 2017 at 3:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> + if test "$update_module" = "merge" ||
>> + test "$update_module" = "rebase" ||
>> + test "$update_module" = "none"
>> + then
>> + update_module=checkout
>> + fi
>
> case "$update_module" in
> merge | rebase | none)
> update_module=checkout ;;
> esac
>
> Shorter and probably easier to update.
agreed, want me to reroll or squash locally?
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH] submodule update: run custom update script for initial populating as well
From: Junio C Hamano @ 2017-01-13 23:58 UTC (permalink / raw)
To: Stefan Beller; +Cc: bmwill, judge.packham, olsonse, git
In-Reply-To: <20170113194326.13950-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> + if test "$update_module" = "merge" ||
> + test "$update_module" = "rebase" ||
> + test "$update_module" = "none"
> + then
> + update_module=checkout
> + fi
case "$update_module" in
merge | rebase | none)
update_module=checkout ;;
esac
Shorter and probably easier to update.
^ permalink raw reply
* Re: [PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context
From: Junio C Hamano @ 2017-01-13 23:56 UTC (permalink / raw)
To: Vegard Nossum; +Cc: René Scharfe, git
In-Reply-To: <c74c260d-1a4d-39f6-a644-4f90a67d6d82@oracle.com>
Vegard Nossum <vegard.nossum@oracle.com> writes:
> The patch will work as intended and as expected for 95% of the users out
> there (javadoc, Doxygen, kerneldoc, etc. all have the comment
> immediately preceding the function) and fixes a very real problem for me
> (and I expect many others) _today_; for the remaining 5% (who put a
> blank line between their comment and the start of the function) it will
> revert back to the current behaviour, so there should be no regression
> for them.
I notice your 95% are all programming languages, but I am more
worried about the contents written in non programming languages
(René gave HTML an an example--there may be other types of contents
that we programmer types do not deal with every day, but Git users
depend on).
I am also more focused on keeping the codebase maintainable in good
health by making sure that we made an effort to find a solution that
is general-enough before solving a single specific problem you have
today. We may end up deciding that a blank-line heuristics gives us
good enough tradeoff, but I do not want us to make a decision before
thinking.
>> The way "diff -W" codepath used it as if it were always the very
>> first line of a function was bound to invite a patch like this, and
>> if we want to be extra elaborate, I agree that an extra mechanism to
>> say "the line the funcline regexp matches is not the beginning of a
>> function, but the beginning is a line that matches this other regexp
>> before that line" may help.
>>
>> Do we really want to be that elaborate, though? I dunno.
>
> Adding a regex instead of the simple "blank line" test doesn't seem very
> difficult to do, but I am doubtful that it will make any difference in
> practice. But that can be done incrementally as well by the people who
> would actually need it (who I strongly suspect do not exist in the first
> place).
At least, the damage can be limited to the cases we know would work
well if we go that way.
^ permalink raw reply
* [PATCH] transport submodules: correct error message
From: Stefan Beller @ 2017-01-13 23:54 UTC (permalink / raw)
To: gitster; +Cc: git, hvoigt, dborowitz, Stefan Beller
When push.recurseSubmodules is set to "check" or "on-demand", the transport
layer tries to determine if a submodule needs pushing. This check is done
by walking all remote refs that are known.
For remotes we only store the refs/heads/* (and tags), which doesn't
include all commits. In e.g. Gerrit commits often end up at refs/changes/*
(that we do not store) when pushing to refs/for/master (which we also do
not store). So a workflow such as the following still fails:
$ git -C <submodule> push origin HEAD:refs/for/master
$ git push origin HEAD:refs/for/master
The following submodule paths contain changes that can
not be found on any remote:
submodule
Please try
git push --recurse-submodules=on-demand
or cd to the path and use
git push
to push them to a remote.
Trying to push with --recurse-submodules=on-demand would run into
the same problem. To fix this issue
1) specifically mention that we looked for branches on the remote.
2) advertise pushing without recursing into submodules. ("Use this
command to make the error message go away")
While at it, remove some empty lines, as they blow up the error message.
Reported-by: Dave Borowitz <dborowitz@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
transport.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/transport.c b/transport.c
index 3e8799a611..2445bf0dca 100644
--- a/transport.c
+++ b/transport.c
@@ -883,14 +883,14 @@ static void die_with_unpushed_submodules(struct string_list *needs_pushing)
int i;
fprintf(stderr, _("The following submodule paths contain changes that can\n"
- "not be found on any remote:\n"));
+ "not be found on any remote branch:\n"));
for (i = 0; i < needs_pushing->nr; i++)
fprintf(stderr, " %s\n", needs_pushing->items[i].string);
- fprintf(stderr, _("\nPlease try\n\n"
- " git push --recurse-submodules=on-demand\n\n"
- "or cd to the path and use\n\n"
- " git push\n\n"
- "to push them to a remote.\n\n"));
+ fprintf(stderr, _("\nSuppress submodule checks via\n"
+ " git push --no-recurse-submodules\n"
+ "or cd to the path and use\n"
+ " git push\n"
+ "to push them to a remote.\n"));
string_list_clear(needs_pushing, 0);
--
2.11.0.297.g298debce27
^ permalink raw reply related
* Re: [PATCH] submodule update: run custom update script for initial populating as well
From: Stefan Beller @ 2017-01-13 23:52 UTC (permalink / raw)
To: Junio C Hamano
Cc: Brandon Williams, Chris Packham, Spencer Olson,
git@vger.kernel.org
In-Reply-To: <xmqqfukmedca.fsf@gitster.mtv.corp.google.com>
On Fri, Jan 13, 2017 at 3:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> In 1b4735d9f3 (submodule: no [--merge|--rebase] when newly cloned,
>> 2011-02-17), all actions were defaulted to checkout for populating
>> a submodule initially, because merging or rebasing makes no sense
>> in that situation.
>>
>> Other commands however do make sense, such as the custom command
>> that was added later (6cb5728c43, submodule update: allow custom
>> command to update submodule working tree, 2013-07-03).
>
> Makes sense.
>
>> I am unsure about the "none" command, as I can see an initial
>> checkout there as a useful thing. On the other hand going strictly
>> by our own documentation, we should do nothing in case of "none"
>> as well, because the user asked for it.
>
> I think "none" is "I'll decide which revision of the submodule
> should be there---do not decide it for me". If the user is
> explicitly saying with "git submodule init" to have "some" version,
> and if the user did not have any (because the user didn't show
> interest in any checkout of the submodule before), then I think it
> probably makes more sense to checkout the version bound to the
> superproject, than leaving the directory empty.
>
>> Reported-by: Han-Wen Nienhuys <hanwen@google.com>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>> git-submodule.sh | 7 ++++++-
>> t/t7406-submodule-update.sh | 15 +++++++++++++++
>> 2 files changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index 554bd1c494..aeb721ab7e 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -606,7 +606,12 @@ cmd_update()
>> if test $just_cloned -eq 1
>> then
>> subsha1=
>> - update_module=checkout
>> + if test "$update_module" = "merge" ||
>> + test "$update_module" = "rebase" ||
>> + test "$update_module" = "none"
>> + then
>> + update_module=checkout
>> + fi
>
> ... which seems to be what you did. Do we need a documentation
> update, or does this just make the behaviour of this corner case
> consistent with what is already documented?
I think we do not need to update the documentation, because the
documentation doesn't call out the first/initial call to update to be special.
So for a non existing submodule we can do:
git submodule update --init --[rebase|merge]
and that falls back to checkout, which *looks* like it was a rebase/merge.
The original bug report was that
$ git config submodule.<name>.update !echo-script.sh
$ git submodule update <submodule>
Submodule path '<submodule>': 'echo-script.sh'
$ rm -rf <submodule>
$ git submodule update <submodule>
.. checked out ..
So while I usually think more verbose documentation is a good idea,
this time it's different, as it merely aligns current documented
behavior with reality.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH] submodule update: run custom update script for initial populating as well
From: Junio C Hamano @ 2017-01-13 23:42 UTC (permalink / raw)
To: Stefan Beller; +Cc: bmwill, judge.packham, olsonse, git
In-Reply-To: <20170113194326.13950-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> In 1b4735d9f3 (submodule: no [--merge|--rebase] when newly cloned,
> 2011-02-17), all actions were defaulted to checkout for populating
> a submodule initially, because merging or rebasing makes no sense
> in that situation.
>
> Other commands however do make sense, such as the custom command
> that was added later (6cb5728c43, submodule update: allow custom
> command to update submodule working tree, 2013-07-03).
Makes sense.
> I am unsure about the "none" command, as I can see an initial
> checkout there as a useful thing. On the other hand going strictly
> by our own documentation, we should do nothing in case of "none"
> as well, because the user asked for it.
I think "none" is "I'll decide which revision of the submodule
should be there---do not decide it for me". If the user is
explicitly saying with "git submodule init" to have "some" version,
and if the user did not have any (because the user didn't show
interest in any checkout of the submodule before), then I think it
probably makes more sense to checkout the version bound to the
superproject, than leaving the directory empty.
> Reported-by: Han-Wen Nienhuys <hanwen@google.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> git-submodule.sh | 7 ++++++-
> t/t7406-submodule-update.sh | 15 +++++++++++++++
> 2 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 554bd1c494..aeb721ab7e 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -606,7 +606,12 @@ cmd_update()
> if test $just_cloned -eq 1
> then
> subsha1=
> - update_module=checkout
> + if test "$update_module" = "merge" ||
> + test "$update_module" = "rebase" ||
> + test "$update_module" = "none"
> + then
> + update_module=checkout
> + fi
... which seems to be what you did. Do we need a documentation
update, or does this just make the behaviour of this corner case
consistent with what is already documented?
Thanks.
^ permalink raw reply
* Re: [PATCH v2 1/2] diff --no-index: follow symlinks
From: Junio C Hamano @ 2017-01-13 23:37 UTC (permalink / raw)
To: Dennis Kaarsemaker; +Cc: git
In-Reply-To: <20170113102021.6054-2-dennis@kaarsemaker.net>
Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> Git's diff machinery does not follow symlinks, which makes sense as git
> itself also does not, but stores the symlink destination.
>
> In --no-index mode however, it is useful for diff to to follow symlinks,
> matching the behaviour of ordinary diff. A new --no-dereference (name
> copied from diff) option has been added to disable this behaviour.
If you add a --no-dereference option, --dereference option should
also be there, so that "--no-dereference" earlier on the command
line (perhaps coming from a configured alias) can be countermanded.
While I am not opposed to giving an optional feature to treat a
symlink as if it is a regular file with the contents of its link
target, I am not enthused that this patch tries to make that the
default behaviour. We are not matching the behaviour of ordinary
diff anyway [*1*].
It probably makes more sense for our first step to introduce this
feature that is only enabled when "--dereference" option is given.
Making it the default for "--no-index" case should be discussed as
a separate step.
[Footnote]
*1* E.g. "git diff --no-index dirA/ dirB/" does not say "Only in
dirA: file". It also recurses into subdirectories of dirA/ and
dirB/ without the --recursive option.
^ permalink raw reply
* Re: [PATCH v2 2/2] diff --no-index: support reading from pipes
From: Junio C Hamano @ 2017-01-13 23:24 UTC (permalink / raw)
To: Dennis Kaarsemaker; +Cc: git
In-Reply-To: <20170113102021.6054-3-dennis@kaarsemaker.net>
Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> + /*
> + * In --no-index mode, we support reading from pipes. canon_mode, called by
> + * fill_filespec, gets confused by this and thinks we now have subprojects.
> + * Detect this and tell the rest of the diff machinery to treat pipes as
> + * normal files.
> + */
> + if (S_ISGITLINK(s->mode))
> + s->mode = S_IFREG | ce_permissions(mode);
Hmph. Pipes on your system may satisfy S_ISGITLINK() and confuse
later code, and this hack may work it around. But a proper gitlink
that was thrown at this codepath (probably by mistake) will also be
caught and pretend as if it were a regular file. Do we know for
certain that pipes everywhere will be munged to appear as
S_ISGITLINK()? Is it possible to do the "are we looking at an end
of a pipe?" check _before_ canon_mode() munges and stores the result
in s->mode in diff-no-index.c somewhere, perhaps inside get_mode()?
> diff --git a/diff.c b/diff.c
> index 2fc0226338..bb04eab331 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -2839,9 +2839,18 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
> fd = open(s->path, O_RDONLY);
> if (fd < 0)
> goto err_empty;
> - s->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);
> + if (!S_ISREG(st.st_mode)) {
> + struct strbuf sb = STRBUF_INIT;
> + strbuf_read(&sb, fd, 0);
> + s->size = sb.len;
> + s->data = strbuf_detach(&sb, NULL);
> + s->should_free = 1;
> + }
> + else {
> + s->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);
> + s->should_munmap = 1;
> + }
> close(fd);
> - s->should_munmap = 1;
I like the fact that, by extending the !S_ISREG() check this patch
introduces, we can later use the new "do not mmap but allocate to
read" codepath for small files, which may be more efficient. We may
want to have a small helper
static int should_mmap_file_contents(struct stat *st)
{
return S_ISREG(st->st_mode);
}
so that we can do such an enhancement later more easily.
So, I am skeptical with the "do we have pipe" check in the earlier
hunk, but otherwise I think what this patch wanted to solve is a
reasonable problem to tackle.
Thanks.
^ permalink raw reply
* Re: [PATCH 2/2] Use 'env' to find perl instead of fixed path
From: Eric Wong @ 2017-01-13 21:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Pat Pannuto, Johannes Schindelin, Johannes Sixt, git
In-Reply-To: <xmqq37gmhgpn.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> wrote:
> Eric Wong <e@80x24.org> writes:
> > Junio C Hamano <gitster@pobox.com> wrote:
> >> Eric Wong <e@80x24.org> writes:
> >> > Pat Pannuto <pat.pannuto@gmail.com> wrote:
> >> >> You may still want the 1/2 patch in this series, just to make things
> >> >> internally consistent with "-w" vs "use warnings;" inside git's perl
> >> >> scripts.
> >> >
> >> > No, that is a step back. "-w" affects the entire process, so it
> >> > spots more potential problems. The "warnings" pragma is scoped
> >> > to the enclosing block, so it won't span across files.
> >>
> >> OK, so with "-w", we do not have to write "use warnings" in each of
> >> our files to get them checked. It is handy when we ship our own
> >> libs (e.g. Git.pm) that are used by our programs.
> >
> > Yes. "use warnings" should be in our own libs in case other
> > people run without "-w"
>
> Would it mean that we need both anyway? That is, add missing "use
> warnings" without removing "-w" from she-bang line?
Yes, we keep "use warnings" other people may use, at least.
No harm in keeping that in top-level scripts, I guess.
> Speaking of Perl, I recall that somebody complained that we ship
> with and do use a stale copy of Error.pm that has been deprecated.
> I am not asking you to do so, but we may want to see somebody look
> into it (i.e. assessing the current situation, and if it indeed is
> desirable for us to wean ourselves away from Error.pm, update our
> codepaths that use it).
Agreed, I'd definitely prefer to move towards the basic eval/die
construct without relying on a bundled 3rd-party mechanism.
But we might need a migration path for out-of-tree users of
Git.pm (if any)...
I'm sure I've agreed this was a path we should be taking in the
past, but did something about it myself. So yeah, maybe Pat or
somebody else interested can take care of this :)
Thanks.
^ permalink raw reply
* Re: [PATCH 5/5] describe: teach describe negative pattern matches
From: Johannes Sixt @ 2017-01-13 21:31 UTC (permalink / raw)
To: Jacob Keller; +Cc: Jacob Keller, Git mailing list
In-Reply-To: <CA+P7+xq1LMkRG_aSyamrsPUQE+rDv4A9Qd19tDMgx-_a5OHsqQ@mail.gmail.com>
Am 13.01.2017 um 07:57 schrieb Jacob Keller:
> On Thu, Jan 12, 2017 at 10:43 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> When you write
>>
>> git log --branches --exclude=origin/* --remotes
>>
>> --exclude=origin/* applies only to --remotes, but not to --branches.
>
> Well for describe I don't think the order matters.
That is certainly true today. But I would value consistency more. We
would lose it if some time in the future 'describe' accepts --branches
and --remotes in addition to --tags and --all.
-- Hannes
^ 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