* [PATCH 15/15] rev-list: expose and document --single-worktree
From: Nguyễn Thái Ngọc Duy @ 2017-02-17 14:19 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Michael Haggerty, Stefan Beller,
Johannes Schindelin, Nguyễn Thái Ngọc Duy
In-Reply-To: <20170217141908.18012-1-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/rev-list-options.txt | 8 ++++++++
revision.c | 2 ++
2 files changed, 10 insertions(+)
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 5da7cf5a8..dd773f97c 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -179,6 +179,14 @@ explicitly.
Pretend as if all objects mentioned by reflogs are listed on the
command line as `<commit>`.
+--single-worktree::
+ By default, all working trees will be examined by the
+ following options when there are more than one (see
+ linkgit:git-worktree[1]): `--all`, `--reflog` and
+ `--indexed-objects`.
+ This option forces them to examine the current working tree
+ only.
+
--ignore-missing::
Upon seeing an invalid object name in the input, pretend as if
the bad input was not given.
diff --git a/revision.c b/revision.c
index ecfd6fea6..2a27e55fe 100644
--- a/revision.c
+++ b/revision.c
@@ -2222,6 +2222,8 @@ static int handle_revision_pseudo_opt(const char *submodule,
return error("invalid argument to --no-walk");
} else if (!strcmp(arg, "--do-walk")) {
revs->no_walk = 0;
+ } else if (!strcmp(arg, "--single-worktree")) {
+ revs->single_worktree = 1;
} else {
return 0;
}
--
2.11.0.157.gd943d85
^ permalink raw reply related
* Re: new git-diff switch to eliminate leading "+" and "-" characters
From: Duy Nguyen @ 2017-02-17 14:53 UTC (permalink / raw)
To: Vanderhoof, Tzadik; +Cc: git@vger.kernel.org
In-Reply-To: <2C8817BDA27E034F8E9A669458E375EF11886C5A@APSWP0428.ms.ds.uhc.com>
On Tue, Feb 14, 2017 at 6:01 AM, Vanderhoof, Tzadik
<tzadik.vanderhoof@optum360.com> wrote:
> The output of git-diff includes lines beginning with "+" and "-" to indicate added and deleted lines. A somewhat common task (at least for me) is to want to copy output from a "diff" (usually the deleted lines) and paste it back into my code.
>
> This is quite inconvenient because of the leading "+" and "-" characters. I know there are shell and IDE / editor workarounds but it would be nice if there was a switch to git-diff to make it leave out those characters, especially since "--color" kind of makes those leading characters obsolete.
Color wouldn't show you new/old empty lines though (unless you use
different background color, but I doubt that looks readable).
> Would it make sense to develop such a switch or has there been work on that already?
I face this "problem" every day, but most editors nowadays have
block-based editing that would allow you to remove a column of "+/-"
easily. At least it has not bothered me to think of improving it.
--
Duy
^ permalink raw reply
* libgit2::git_describe_workdir() fails with depth-cloned + detached head
From: Lukas Lueg @ 2017-02-17 15:39 UTC (permalink / raw)
To: git
Hi,
I have a build on Travis failing with a weird error that happens while
trying to get a description from a repo. One of the integration tests
that are executed on Travis executes libgit2::git_describe_workdir()
on it's own repo and fails with code: -3, klass: 9, message: "object
not found - no match for id
(77f95f14776deb7e120a2a26f7b56abf2903bc62)".
The id 77f95f1 from the error message refers to a commit that is of no
particular interest and actually weeks behind; I never asked for it.
I was finally able to reproduce the problem by very carefully
following how Travis clones the repo:
git clone --depth=50 https://... repo_root
cd repo_root
git fetch origin +refs/pull/414/merge:
git checkout -qf FETCH_HEAD
If and only if the repo is cloned recursively, the call to
libgit2::git_describe_workdir() results in this error, always
referring to 77f95f1. If the repo is cloned without '--depth 50', no
error occurs. The repo does not even have submodules.
My local git client does the right thing and get's the latest commit
oid without problems.
Any idea what's going on here?
Best regards
Lukas
^ permalink raw reply
* Re: [PATCH] mingw: make stderr unbuffered again
From: Johannes Schindelin @ 2017-02-17 16:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Johannes Sixt, Jeff Hostetler
In-Reply-To: <xmqqlgt7110q.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Wed, 15 Feb 2017, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >
> >> FWIW I wish it were different, that git.git's `master` reflected more
> >> closely what the current Git for Windows version has.
> >
> > Well, we two wishing the same thing together without doing anything
> > else would make it happen.
>
> ehh, would *not* make it happen, of course.
That is the reason why set aside a substantial part of my maintainer time
to upstream those patches. You may not see how much time it costs me, say,
to get the putty-w-args stuff upstream, but rest assured that I would not
be able to do this were it not for a company paying me to do exactly that.
> > As an experiment to see if our process can be improved, I've been
> > meaning to suggest (which is what was behind my "question at a bit
> > higher level" to Hannes [*1*]) asking you to throw me occasional
> > pull requests for changes that are only about Windows specific
> > issues, bypassing "patches on the list" for things like a hotfix to
> > js/mingw-isatty [*2*] and use of OpenSSL SHA-1 on Windows [*3*],
> > essentially treating Windows specific changes as "a sub-maintainer
> > makes pull requests" we already do with Paul, Eric and Pat.
The problem here is that Git for Windows is not a subsystem. It touches
pretty much all of Git. That is very different from the Tcl/Tk code, or
from git-svn (whose code is not shared by anything else in Git's source
code, despite the fact that it is written in Perl).
And even if you were to accept the occasional Pull Request from me, it
would not solve the even bigger problem that we essentially reject so much
expertise out there, so many potential contributions by very competent
developers who just have no desire to fight the contribution process.
> While this may ease the flow of upstreaming windows specific
> changes, we need a separate thing to address the on-going issue you
> raised in your message. A Windows-less person would not know his
... or her...
> change to a generic code that is innocuous-looking has fallouts on
> Windows (read this sentence with "Windows" replaced with any
> specific platform name). When somebody writes c == '/' that should
> have been written as is_dir_sep(c), you or Hannes often finds it
> during the review here, and after repeatedly seeing such reviews,
> that (slowly) rubs off on other Window-less folks. A new code may
> still hit 'next' and 'master' with such an issue if it goes
> unnoticed during the review.
Apart from the fact that we have no prayer at coming up with a system that
keeps track of open issues (because we do not use any tracker, but instead
rely on people to remember that some thing still may not have been
addressed), there is a different problem here: you stated very explicitly
that it is okay for `pu` to be broken [*1*]. If it were different, if any
breakage would imply that a fix is really, really required lest the patch
series be evicted from `pu`, we could easily modify my Continuous Testing
setup to report more visibly what is broken. But since it is okay for `pu`
to be broken, that would just annoy everybody and people would learn to
ignore/procmail those reports.
> The CI you are setting up [*1*]
which *1*?
And... I already set it up. I just did not bother to make it more public
because the builds were broken more often than not. IIRC the entire month
of October was a solid red.
> may certainly be a step in the good direction. Having more people like
> Hannes working off of upstream may also be a great way to help the
> "forget 'next' and upstream in general" issue. Any other ideas?
The Continuous Integration is actually not so much a Continuous
Integration as it is a Continuous Testing. If it became more of a CI, that
would certainly reduce the impression that Git's bleeding edge only ever
works on Linux.
Ciao,
Johannes
Footnote *1*: You probably read between the lines that this is
unfortunate, in my opinion. It sets the mood that lets experimental (if
useful) features such as worktrees be broken for the better part of a
year.
^ permalink raw reply
* RE: new git-diff switch to eliminate leading "+" and "-" characters
From: Vanderhoof, Tzadik @ 2017-02-17 16:04 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git@vger.kernel.org
In-Reply-To: <CACsJy8CTL1GUreqNVBYyu2EqqdiUxj-dOOE9=Rr0ivK6-7yKjw@mail.gmail.com>
> From: Duy Nguyen [mailto:pclouds@gmail.com]
> Sent: Friday, February 17, 2017 6:54 AM
> To: Vanderhoof, Tzadik
> Cc: git@vger.kernel.org
>
> On Tue, Feb 14, 2017 at 6:01 AM, Vanderhoof, Tzadik
> <tzadik.vanderhoof@optum360.com> wrote:
> > The output of git-diff includes lines beginning with "+" and "-" to indicate
> added and deleted lines. A somewhat common task (at least for me) is to
> want to copy output from a "diff" (usually the deleted lines) and paste it back
> into my code.
> >
> > This is quite inconvenient because of the leading "+" and "-" characters. I
> know there are shell and IDE / editor workarounds but it would be nice if
> there was a switch to git-diff to make it leave out those characters, especially
> since "--color" kind of makes those leading characters obsolete.
>
> Color wouldn't show you new/old empty lines though (unless you use
> different background color, but I doubt that looks readable).
>
> > Would it make sense to develop such a switch or has there been work on
> that already?
>
> I face this "problem" every day, but most editors nowadays have block-
> based editing that would allow you to remove a column of "+/-"
> easily. At least it has not bothered me to think of improving it.
Would a patch be welcome?
Tzadik
> --
> Duy
This e-mail, including attachments, may include confidential and/or
proprietary information, and may be used only by the person or entity
to which it is addressed. If the reader of this e-mail is not the intended
recipient or his or her authorized agent, the reader is hereby notified
that any dissemination, distribution or copying of this e-mail is
prohibited. If you have received this e-mail in error, please notify the
sender by replying to this message and delete this e-mail immediately.
^ permalink raw reply
* RE: new git-diff switch to eliminate leading "+" and "-" characters
From: Vanderhoof, Tzadik @ 2017-02-17 16:06 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git@vger.kernel.org
In-Reply-To: <CACsJy8CTL1GUreqNVBYyu2EqqdiUxj-dOOE9=Rr0ivK6-7yKjw@mail.gmail.com>
> From: Duy Nguyen [mailto:pclouds@gmail.com]
> Sent: Friday, February 17, 2017 6:54 AM
> To: Vanderhoof, Tzadik
> Cc: git@vger.kernel.org
>
> > Would it make sense to develop such a switch or has there been work on
> that already?
>
> I face this "problem" every day, but most editors nowadays have block-
> based editing that would allow you to remove a column of "+/-"
> easily. At least it has not bothered me to think of improving it.
Would a patch be welcome?
Tzadik
> --
> Duy
This e-mail, including attachments, may include confidential and/or
proprietary information, and may be used only by the person or entity
to which it is addressed. If the reader of this e-mail is not the intended
recipient or his or her authorized agent, the reader is hereby notified
that any dissemination, distribution or copying of this e-mail is
prohibited. If you have received this e-mail in error, please notify the
sender by replying to this message and delete this e-mail immediately.
^ permalink raw reply
* Re: body-CC-comment regression
From: Johan Hovold @ 2017-02-17 16:42 UTC (permalink / raw)
To: Matthieu Moy
Cc: Johan Hovold, git, Jeff King, Kevin Daudt, Junio C Hamano,
Larry Finger
In-Reply-To: <vpq7f4pdkjp.fsf@anie.imag.fr>
On Fri, Feb 17, 2017 at 02:16:42PM +0100, Matthieu Moy wrote:
> Johan Hovold <johan@kernel.org> writes:
>
> > There is another option, namely to only accept a single address for tags
> > in the body. I understand that being able to copy a CC-header to either
> > the header section or to the command line could be useful, but I don't
> > really see the point in allowing this in the tags in the body (a SoB
> > always has one address, and so should a CC-tag).
>
> I mostly agree for the SoB, but why should a Cc tag have only one email?
For symmetry (with SoB) and readability reasons (one tag per line).
These are body tags, not mail headers, after all.
> The "multiple emails per Cc: field" has been there for a while already
> (b1c8a11c8024 released in 2.6.0, sept 2015), some users may have got
> used to it. What you are proposing breaks their flow.
Note that that commit never mentions multiple addresses in either
headers or body-tags -- it's all about being able to specify multiple
entries on the command line.
There does not seem to be single commit in the kernel where multiple
address are specified in a CC tag since after git-send-email started
allowing it, but there are ten commits before (to my surprise), and that
should be contrasted with at least 4178 commits with trailing comments
including a # sign.
> > And since this is a regression for something that has been working for
> > years that was introduced by a new feature, I also think it's reasonable
> > to (partially) revert the feature.
>
> I'd find it rather ironic to fix your case by breaking a feature that
> has been working for more than a year :-(. What would you answer to a
> contributor comming one year from now and proposing to revert your
> reversion because it breaks his flow?
Such conflicts are not uncommon when dealing with regressions introduced
by new features, and need to be dealt with on a case-by-case basis. But
the fact that trailing comments have been properly supported for more
than four years should carry some weight.
> All that said, I think another fix would be both satisfactory for
> everyone and rather simple:
>
> 1) Stop calling Mail::Address even if available. It used to make sense
> to do that when our in-house parser was really poor, but we now have
> something essentially as good as Mail::Address. We test our parser
> against Mail::Address and we do have a few known differences (see
> t9000), but they are really corner-cases and shouldn't matter.
>
> A good consequence of this is that we stop depending on the way Perl
> is installed to parse emails. Regardless of the current issue, I
> think it is a good thing.
Right, that sounds like the right thing to do regardless.
> 2) Modify our in-house parser to discard garbage after the >. The patch
> should look like (untested):
>
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -903,11 +903,11 @@ sub parse_mailboxes {
> my (@addr_list, @phrase, @address, @comment, @buffer) = ();
> foreach my $token (@tokens) {
> if ($token =~ /^[,;]$/) {
> - # if buffer still contains undeterminated strings
> - # append it at the end of @address or @phrase
> - if ($end_of_addr_seen) {
> - push @phrase, @buffer;
> - } else {
> + # if buffer still contains undeterminated
> + # strings append it at the end of @address,
> + # unless we already saw the closing >, in
> + # which case we discard it.
> + if (!$end_of_addr_seen) {
> push @address, @buffer;
> }
>
> What do you think?
Sounds perfectly fine to me, and seems to work too after quick test.
Note however that there's another minor issue with using multiple
addresses in a Cc-tag in that it breaks --suppress-cc=self, but I guess
that can be fixed separately.
Thanks,
Johan
^ permalink raw reply
* Re: [PATCH v2 2/2] rev-parse: fix several options when running in a subdirectory
From: Johannes Schindelin @ 2017-02-17 16:53 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Nguyễn Thái Ngọc Duy, Michael Rappazzo
In-Reply-To: <xmqqo9y9lvqk.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Fri, 10 Feb 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> > index ff13e59e1db..84af2802f6f 100644
> > --- a/builtin/rev-parse.c
> > +++ b/builtin/rev-parse.c
> > @@ -545,6 +545,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> > unsigned int flags = 0;
> > const char *name = NULL;
> > struct object_context unused;
> > + struct strbuf buf = STRBUF_INIT;
> >
> > if (argc > 1 && !strcmp("--parseopt", argv[1]))
> > return cmd_parseopt(argc - 1, argv + 1, prefix);
> > @@ -599,7 +600,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> > if (!strcmp(arg, "--git-path")) {
> > if (!argv[i + 1])
> > die("--git-path requires an argument");
> > - puts(git_path("%s", argv[i + 1]));
> > + strbuf_reset(&buf);
> > + puts(relative_path(git_path("%s", argv[i + 1]),
> > + prefix, &buf));
> > i++;
> > continue;
> > }
> > @@ -821,8 +824,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> > continue;
> > }
> > if (!strcmp(arg, "--git-common-dir")) {
> > - const char *pfx = prefix ? prefix : "";
> > - puts(prefix_filename(pfx, strlen(pfx), get_git_common_dir()));
> > + strbuf_reset(&buf);
> > + puts(relative_path(get_git_common_dir(),
> > + prefix, &buf));
> > continue;
> > }
> > if (!strcmp(arg, "--is-inside-git-dir")) {
> > @@ -845,7 +849,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> > die(_("Could not read the index"));
> > if (the_index.split_index) {
> > const unsigned char *sha1 = the_index.split_index->base_sha1;
> > - puts(git_path("sharedindex.%s", sha1_to_hex(sha1)));
> > + const char *path = git_path("sharedindex.%s", sha1_to_hex(sha1));
> > + strbuf_reset(&buf);
> > + puts(relative_path(path, prefix, &buf));
> > }
> > continue;
> > }
> > @@ -906,5 +912,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> > die_no_single_rev(quiet);
> > } else
> > show_default();
> > + strbuf_release(&buf);
>
> This uses "reset then use" pattern for repeated use of strbuf, and
> causes the string last held in the strbuf to leak on early return,
... which cannot happen due to the lack of an early return...
> which can be mitigated by using "use then reset" pattern. I.e.
>
> if (!strcmp(arg, "--git-common-dir")) {
> puts(relative_path(get_git_common_dir(),
> prefix, &buf));
> strbuf_reset(&buf);
> continue;
> }
>
> I'd think.
This would not release the memory, though:
#define strbuf_reset(sb) strbuf_setlen(sb, 0)
and
static inline void strbuf_setlen(struct strbuf *sb, size_t len)
{
if (len > (sb->alloc ? sb->alloc - 1 : 0))
die("BUG: strbuf_setlen() beyond buffer");
sb->len = len;
sb->buf[len] = '\0';
}
There is not a single free() statement there.
So the "use then reset" scheme would leak *just the same*.
> You'd still want to release it at the end anyway for good code hygiene,
> though.
Which I do.
Technically, this is not even necessary because all of the cmd_*()
functions are immediately followed by a call to exit(). Wasn't that the
genius idea in the early Git days, that we could simply get away with
sloppy memory management because the program exit()s shortly afterwards,
anyway? ;-)
In any case, I adjusted the commit message to clarify why the "reset then
use" scheme is correct here.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v2 1/2] rev-parse tests: add tests executed from a subdirectory
From: Johannes Schindelin @ 2017-02-17 16:55 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Michael Rappazzo, Nguyễn Thái Ngọc Duy
In-Reply-To: <xmqqshnllw2f.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Fri, 10 Feb 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
> > index 292a0720fcc..1d6e27a09d8 100755
> > --- a/t/t1700-split-index.sh
> > +++ b/t/t1700-split-index.sh
> > @@ -200,4 +200,21 @@ EOF
> > test_cmp expect actual
> > '
> >
> > +test_expect_failure 'rev-parse --shared-index-path' '
> > + rm -rf .git &&
> > + test_create_repo . &&
> > + git update-index --split-index &&
> > + ls -t .git/sharedindex* | tail -n 1 >expect &&
> > + git rev-parse --shared-index-path >actual &&
> > + test_cmp expect actual &&
> > + mkdir work &&
> > + test_when_finished "rm -rf work" &&
> > + (
> > + cd work &&
> > + ls -t ../.git/sharedindex* | tail -n 1 >expect &&
> > + git rev-parse --shared-index-path >actual &&
> > + test_cmp expect actual
> > + )
>
> This looks iffy.
Indeed.
> If we expect multiple sharedindex* files, the first output from "ls -t"
> may or may not match the real one in use (multiple things do happen
> within a single second or whatever your filesystem's time granularity
> is). Two "ls -t" run in this test would (hopefully) give stable
> results, but I suspect that the chance the first line in the output
> matches what --shared-index-path reports is 50% if we expect to have two
> sharedindex* files.
>
> On the other hand, if we do not expect multiple sharedindex* files,
> use of "ls" piped to "tail" is simply misleading.
>
> If this test can be written in such a way that there is only one
> such file that match the pattern, it would be the cleanest to
> understand and explain. As there is only a single invocation of
> "update-index --split-index" immediately after a new repository is
> created, I suspect that the expectation to see only one sharedindex*
> file already holds (because its name is unpredictable, we still need
> to catch it with wildcard), and if that is the case, we can just
> lose "-t" and pipe to tail, i.e. "ls .git/sharedindex* >expect".
Indeed. We can expect only one sharedindex file to be present, and we do
not even have to call out to `ls` but can get away with calling `echo`
(which is a builtin in Bash).
Ciao,
Johannes
^ permalink raw reply
* Re: body-CC-comment regression
From: Matthieu Moy @ 2017-02-17 16:58 UTC (permalink / raw)
To: Johan Hovold; +Cc: git, Jeff King, Kevin Daudt, Junio C Hamano, Larry Finger
In-Reply-To: <20170217164241.GE2625@localhost>
> On Fri, Feb 17, 2017 at 02:16:42PM +0100, Matthieu Moy wrote:
>> Johan Hovold <johan@kernel.org> writes:
>
>> The "multiple emails per Cc: field" has been there for a while already
>> (b1c8a11c8024 released in 2.6.0, sept 2015), some users may have got
>> used to it. What you are proposing breaks their flow.
>
> Note that that commit never mentions multiple addresses in either
> headers or body-tags -- it's all about being able to specify multiple
> entries on the command line.
Indeed. I'm not the author of the patch, but I was supervising the
students who wrote it and "multiple addresses in Cc:" was not the goal,
but a (IMHO positive) side effect we discovered after the fact.
If I had a time machine, I'd probably go back then and forbid multiple
addresses there, but ...
> There does not seem to be single commit in the kernel where multiple
> address are specified in a CC tag since after git-send-email started
> allowing it, but there are ten commits before (to my surprise), and that
> should be contrasted with at least 4178 commits with trailing comments
> including a # sign.
Hey, there's a life outside the kernel ;-).
>> 1) Stop calling Mail::Address even if available.[...]
>
> Right, that sounds like the right thing to do regardless.
>
>> 2) Modify our in-house parser to discard garbage after the >. [...]
>
> Sounds perfectly fine to me, and seems to work too after quick test.
OK, sounds like the way to go.
Do you want to work on a patch? If not, I should be able to do that
myself. The code changes are straightforward, but we probably want a
proper test for that.
> addresses in a Cc-tag in that it breaks --suppress-cc=self, but I guess
> that can be fixed separately.
OK. If it's unrelated enough, please start a separate thread to explain
the problem (and/or write a patch ;-) ).
Thanks,
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH v2 1/2] rev-parse tests: add tests executed from a subdirectory
From: Johannes Schindelin @ 2017-02-17 16:57 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Michael Rappazzo, Nguyễn Thái Ngọc Duy
In-Reply-To: <xmqqpoipkd3l.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Fri, 10 Feb 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
> > index 292a0720fcc..1d6e27a09d8 100755
> > --- a/t/t1700-split-index.sh
> > +++ b/t/t1700-split-index.sh
> > @@ -200,4 +200,21 @@ EOF
> > test_cmp expect actual
> > '
> >
> > +test_expect_failure 'rev-parse --shared-index-path' '
> > + rm -rf .git &&
> > + test_create_repo . &&
>
> Another thing that I notice only after merging this and other topics
> to 'pu' was that this piece needs to always come at the end of the
> script because of this removal. It would make the test more robust
> to create a test repository for this test and work inside it.
Yes, this is indeed unnecessary and even undesirable.
I waited a couple of days to give the original author a chance to fix
this. As I want a fix really badly (because I am really embarrassed of
this bug), I adjusted t1700 appropriately and will send out v3 in a
second.
Ciao,
Johannes
^ permalink raw reply
* [PATCH v3 0/2] Fix bugs in rev-parse's output when run in a subdirectory
From: Johannes Schindelin @ 2017-02-17 16:58 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy,
Michael Rappazzo
In-Reply-To: <cover.1486740772.git.johannes.schindelin@gmx.de>
The bug that bit me (hard!) and that triggered not only a long series of
curses but also my writing a patch and sending it to the list was that
`git rev-parse --git-path HEAD` would give *incorrect* output when run
in a subdirectory of a regular checkout, but *correct* output when run
in a subdirectory of an associated *worktree*.
I had tested the script in question quite a bit, but in a worktree. And
in production, it quietly did exactly the wrong thing.
Changes relative to v2:
- the "iffy" test in t1700 was made "uniffy"
- clarified in the commit message of 2/2 why we can get away with the
"reset then use" pattern
Johannes Schindelin (1):
rev-parse: fix several options when running in a subdirectory
Michael Rappazzo (1):
rev-parse tests: add tests executed from a subdirectory
builtin/rev-parse.c | 15 +++++++++++----
t/t1500-rev-parse.sh | 28 ++++++++++++++++++++++++++++
t/t1700-split-index.sh | 16 ++++++++++++++++
t/t2027-worktree-list.sh | 10 +++++++++-
4 files changed, 64 insertions(+), 5 deletions(-)
base-commit: 076c05393a047247ea723896289b48d6549ed7d0
Published-As: https://github.com/dscho/git/releases/tag/git-path-in-subdir-v3
Fetch-It-Via: git fetch https://github.com/dscho/git git-path-in-subdir-v3
Interdiff vs v2:
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 84af2802f6f..2cfd8d2aae4 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -903,6 +903,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
continue;
verify_filename(prefix, arg, 1);
}
+ strbuf_release(&buf);
if (verify) {
if (revs_count == 1) {
show_rev(type, sha1, name);
@@ -912,6 +913,5 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
die_no_single_rev(quiet);
} else
show_default();
- strbuf_release(&buf);
return 0;
}
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 446ff34f966..6096f2c6309 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -201,17 +201,16 @@ EOF
'
test_expect_success 'rev-parse --shared-index-path' '
- rm -rf .git &&
- test_create_repo . &&
- git update-index --split-index &&
- ls -t .git/sharedindex* | tail -n 1 >expect &&
- git rev-parse --shared-index-path >actual &&
- test_cmp expect actual &&
- mkdir work &&
- test_when_finished "rm -rf work" &&
+ test_create_repo split-index &&
(
- cd work &&
- ls -t ../.git/sharedindex* | tail -n 1 >expect &&
+ cd split-index &&
+ git update-index --split-index &&
+ echo .git/sharedindex* >expect &&
+ git rev-parse --shared-index-path >actual &&
+ test_cmp expect actual &&
+ mkdir subdirectory &&
+ cd subdirectory &&
+ echo ../.git/sharedindex* >expect &&
git rev-parse --shared-index-path >actual &&
test_cmp expect actual
)
--
2.11.1.windows.1.2.g87ad093.dirty
^ permalink raw reply
* [PATCH v3 1/2] rev-parse tests: add tests executed from a subdirectory
From: Johannes Schindelin @ 2017-02-17 16:59 UTC (permalink / raw)
To: git; +Cc: Michael Rappazzo, Junio C Hamano,
Nguyễn Thái Ngọc Duy
In-Reply-To: <cover.1487350582.git.johannes.schindelin@gmx.de>
From: Michael Rappazzo <rappazzo@gmail.com>
t2027-worktree-list has an incorrect expectation for --git-common-dir
which has been adjusted and marked to expect failure.
Some of the tests added have been marked to expect failure. These
demonstrate a problem with the way that some options to git rev-parse
behave when executed from a subdirectory of the main worktree.
[jes: fixed incorrect assumption that objects/ lives in the
worktree-specific git-dir (it lives in the common dir instead). Also
adjusted t1700 so that the test case does not *need* to be the last
one in that script.]
Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t1500-rev-parse.sh | 28 ++++++++++++++++++++++++++++
t/t1700-split-index.sh | 16 ++++++++++++++++
t/t2027-worktree-list.sh | 12 ++++++++++--
3 files changed, 54 insertions(+), 2 deletions(-)
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 038e24c4014..f39f783f2db 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -87,4 +87,32 @@ test_rev_parse -C work -g ../repo.git -b t 'GIT_DIR=../repo.git, core.bare = tru
test_rev_parse -C work -g ../repo.git -b u 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
+test_expect_success 'git-common-dir from worktree root' '
+ echo .git >expect &&
+ git rev-parse --git-common-dir >actual &&
+ test_cmp expect actual
+'
+
+test_expect_failure 'git-common-dir inside sub-dir' '
+ mkdir -p path/to/child &&
+ test_when_finished "rm -rf path" &&
+ echo "$(git -C path/to/child rev-parse --show-cdup).git" >expect &&
+ git -C path/to/child rev-parse --git-common-dir >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'git-path from worktree root' '
+ echo .git/objects >expect &&
+ git rev-parse --git-path objects >actual &&
+ test_cmp expect actual
+'
+
+test_expect_failure 'git-path inside sub-dir' '
+ mkdir -p path/to/child &&
+ test_when_finished "rm -rf path" &&
+ echo "$(git -C path/to/child rev-parse --show-cdup).git/objects" >expect &&
+ git -C path/to/child rev-parse --git-path objects >actual &&
+ test_cmp expect actual
+'
+
test_done
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 292a0720fcc..b754865a618 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -200,4 +200,20 @@ EOF
test_cmp expect actual
'
+test_expect_failure 'rev-parse --shared-index-path' '
+ test_create_repo split-index &&
+ (
+ cd split-index &&
+ git update-index --split-index &&
+ echo .git/sharedindex* >expect &&
+ git rev-parse --shared-index-path >actual &&
+ test_cmp expect actual &&
+ mkdir subdirectory &&
+ cd subdirectory &&
+ echo ../.git/sharedindex* >expect &&
+ git rev-parse --shared-index-path >actual &&
+ test_cmp expect actual
+ )
+'
+
test_done
diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
index 465eeeacd3d..c1a072348e7 100755
--- a/t/t2027-worktree-list.sh
+++ b/t/t2027-worktree-list.sh
@@ -8,16 +8,24 @@ test_expect_success 'setup' '
test_commit init
'
-test_expect_success 'rev-parse --git-common-dir on main worktree' '
+test_expect_failure 'rev-parse --git-common-dir on main worktree' '
git rev-parse --git-common-dir >actual &&
echo .git >expected &&
test_cmp expected actual &&
mkdir sub &&
git -C sub rev-parse --git-common-dir >actual2 &&
- echo sub/.git >expected2 &&
+ echo ../.git >expected2 &&
test_cmp expected2 actual2
'
+test_expect_failure 'rev-parse --git-path objects linked worktree' '
+ echo "$(git rev-parse --show-toplevel)/.git/objects" >expect &&
+ test_when_finished "rm -rf linked-tree && git worktree prune" &&
+ git worktree add --detach linked-tree master &&
+ git -C linked-tree rev-parse --git-path objects >actual &&
+ test_cmp expect actual
+'
+
test_expect_success '"list" all worktrees from main' '
echo "$(git rev-parse --show-toplevel) $(git rev-parse --short HEAD) [$(git symbolic-ref --short HEAD)]" >expect &&
test_when_finished "rm -rf here && git worktree prune" &&
--
2.11.1.windows.1.2.g87ad093.dirty
^ permalink raw reply related
* [PATCH v3 2/2] rev-parse: fix several options when running in a subdirectory
From: Johannes Schindelin @ 2017-02-17 16:59 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy,
Michael Rappazzo
In-Reply-To: <cover.1487350582.git.johannes.schindelin@gmx.de>
In addition to making git_path() aware of certain file names that need
to be handled differently e.g. when running in worktrees, the commit
557bd833bb (git_path(): be aware of file relocation in $GIT_DIR,
2014-11-30) also snuck in a new option for `git rev-parse`:
`--git-path`.
On the face of it, there is no obvious bug in that commit's diff: it
faithfully calls git_path() on the argument and prints it out, i.e. `git
rev-parse --git-path <filename>` has the same precise behavior as
calling `git_path("<filename>")` in C.
The problem lies deeper, much deeper. In hindsight (which is always
unfair), implementing the .git/ directory discovery in
`setup_git_directory()` by changing the working directory may have
allowed us to avoid passing around a struct that contains information
about the current repository, but it bought us many, many problems.
In this case, when being called in a subdirectory, `git rev-parse`
changes the working directory to the top-level directory before calling
`git_path()`. In the new working directory, the result is correct. But
in the working directory of the calling script, it is incorrect.
Example: when calling `git rev-parse --git-path HEAD` in, say, the
Documentation/ subdirectory of Git's own source code, the string
`.git/HEAD` is printed.
Side note: that bug is hidden when running in a subdirectory of a
worktree that was added by the `git worktree` command: in that case, the
(correct) absolute path of the `HEAD` file is printed.
In the interest of time, this patch does not go the "correct" route to
introduce a struct with repository information (and removing global
state in the process), instead this patch chooses to detect when the
command was called in a subdirectory and forces the result to be an
absolute path.
While at it, we are also fixing the output of --git-common-dir and
--shared-index-path.
Lastly, please note that we reuse the same strbuf for all of the
relative_path() calls; this avoids frequent allocation (and duplicated
code), and it does not risk memory leaks, for two reasons: 1) the
cmd_rev_parse() function does not return anywhere between the use of
the new strbuf instance and its final release, and 2) git-rev-parse is
one of these "one-shot" programs in Git, i.e. it exits after running
for a very short time, meaning that all allocated memory is released
with the exit() call anyway.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/rev-parse.c | 15 +++++++++++----
t/t1500-rev-parse.sh | 4 ++--
t/t1700-split-index.sh | 2 +-
t/t2027-worktree-list.sh | 4 ++--
4 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index ff13e59e1db..2cfd8d2aae4 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -545,6 +545,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
unsigned int flags = 0;
const char *name = NULL;
struct object_context unused;
+ struct strbuf buf = STRBUF_INIT;
if (argc > 1 && !strcmp("--parseopt", argv[1]))
return cmd_parseopt(argc - 1, argv + 1, prefix);
@@ -599,7 +600,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
if (!strcmp(arg, "--git-path")) {
if (!argv[i + 1])
die("--git-path requires an argument");
- puts(git_path("%s", argv[i + 1]));
+ strbuf_reset(&buf);
+ puts(relative_path(git_path("%s", argv[i + 1]),
+ prefix, &buf));
i++;
continue;
}
@@ -821,8 +824,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
continue;
}
if (!strcmp(arg, "--git-common-dir")) {
- const char *pfx = prefix ? prefix : "";
- puts(prefix_filename(pfx, strlen(pfx), get_git_common_dir()));
+ strbuf_reset(&buf);
+ puts(relative_path(get_git_common_dir(),
+ prefix, &buf));
continue;
}
if (!strcmp(arg, "--is-inside-git-dir")) {
@@ -845,7 +849,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
die(_("Could not read the index"));
if (the_index.split_index) {
const unsigned char *sha1 = the_index.split_index->base_sha1;
- puts(git_path("sharedindex.%s", sha1_to_hex(sha1)));
+ const char *path = git_path("sharedindex.%s", sha1_to_hex(sha1));
+ strbuf_reset(&buf);
+ puts(relative_path(path, prefix, &buf));
}
continue;
}
@@ -897,6 +903,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
continue;
verify_filename(prefix, arg, 1);
}
+ strbuf_release(&buf);
if (verify) {
if (revs_count == 1) {
show_rev(type, sha1, name);
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index f39f783f2db..d74f09ad93e 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -93,7 +93,7 @@ test_expect_success 'git-common-dir from worktree root' '
test_cmp expect actual
'
-test_expect_failure 'git-common-dir inside sub-dir' '
+test_expect_success 'git-common-dir inside sub-dir' '
mkdir -p path/to/child &&
test_when_finished "rm -rf path" &&
echo "$(git -C path/to/child rev-parse --show-cdup).git" >expect &&
@@ -107,7 +107,7 @@ test_expect_success 'git-path from worktree root' '
test_cmp expect actual
'
-test_expect_failure 'git-path inside sub-dir' '
+test_expect_success 'git-path inside sub-dir' '
mkdir -p path/to/child &&
test_when_finished "rm -rf path" &&
echo "$(git -C path/to/child rev-parse --show-cdup).git/objects" >expect &&
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index b754865a618..6096f2c6309 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -200,7 +200,7 @@ EOF
test_cmp expect actual
'
-test_expect_failure 'rev-parse --shared-index-path' '
+test_expect_success 'rev-parse --shared-index-path' '
test_create_repo split-index &&
(
cd split-index &&
diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
index c1a072348e7..848da5f3684 100755
--- a/t/t2027-worktree-list.sh
+++ b/t/t2027-worktree-list.sh
@@ -8,7 +8,7 @@ test_expect_success 'setup' '
test_commit init
'
-test_expect_failure 'rev-parse --git-common-dir on main worktree' '
+test_expect_success 'rev-parse --git-common-dir on main worktree' '
git rev-parse --git-common-dir >actual &&
echo .git >expected &&
test_cmp expected actual &&
@@ -18,7 +18,7 @@ test_expect_failure 'rev-parse --git-common-dir on main worktree' '
test_cmp expected2 actual2
'
-test_expect_failure 'rev-parse --git-path objects linked worktree' '
+test_expect_success 'rev-parse --git-path objects linked worktree' '
echo "$(git rev-parse --show-toplevel)/.git/objects" >expect &&
test_when_finished "rm -rf linked-tree && git worktree prune" &&
git worktree add --detach linked-tree master &&
--
2.11.1.windows.1.2.g87ad093.dirty
^ permalink raw reply related
* Re: [PATCH 1/3] delete_refs(): accept a reflog message argument
From: Junio C Hamano @ 2017-02-17 17:05 UTC (permalink / raw)
To: Jeff King; +Cc: Kyle Meyer, git
In-Reply-To: <20170217081205.zn7j6d5cffgdvfbn@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> diff --git a/refs.h b/refs.h
>> index 9fbff90e7..81627a63d 100644
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -277,7 +277,7 @@ int reflog_exists(const char *refname);
>> * be NULL_SHA1. flags is passed through to ref_transaction_delete().
>> */
>> int delete_ref(const char *refname, const unsigned char *old_sha1,
>> - unsigned int flags);
>> + unsigned int flags, const char *msg);
>
> Should the "msg" argument go at the beginning, to match update_ref()?
Probably. rename/create have the message at the end but their
parameters are very different from update/delete. The parameters
update and delete take are not identical, but we can view them as a
lot more similar than the other two. So I think it makes sense for
delete to try matching update, even though trying to make all four
the same may proabably be pointless.
^ permalink raw reply
* Re: [PATCH/RFC 00/15] Fix git-gc losing objects in multi worktree
From: Johannes Schindelin @ 2017-02-17 17:09 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Junio C Hamano, Michael Haggerty, Stefan Beller
In-Reply-To: <20170217141908.18012-1-pclouds@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 484 bytes --]
Hi Duy,
On Fri, 17 Feb 2017, Nguyễn Thái Ngọc Duy wrote:
> So here is my latest attempt on fixing this issue. For people who are
> not aware of it, git-gc does not take per-worktree refs, reflogs and
> indexes into account. An odb prune may leave HEAD and references in
> other worktrees pointing to nowhere.
Thank you so much for working on this. The bug really affects my daily
work very, very negatively.
Will try to review as soon as possible.
Ciao,
Dscho
^ permalink raw reply
* [L10N] Kickoff for Git 2.12.0 l10n round 2
From: Jiang Xin @ 2017-02-17 17:11 UTC (permalink / raw)
To: Alexander Shopov, Jordi Mas, Ralf Thielow, Jean-Noël Avila,
Marco Paolone, Changwoo Ryu, Vasco Almeida, Dimitriy Ryazantcev,
Peter Krefting, Trần Ngọc Quân, Jiang Xin
Cc: Git List
Hi guys,
Git v2.12.0-rc1 introduced another two new messages need to be translated,
so let's start l10n for Git 2.12.0 round 2:
l10n: git.pot: v2.12.0 round 2 (2 new)
Generate po/git.pot from v2.12.0-rc1 for git v2.12.0 l10n round 2.
Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
You can get it from the usual place:
https://github.com/git-l10n/git-po/
As how to update your XX.po and help to translate Git, please see
"Updating a XX.po file" and other sections in “po/README" file.
--
Jiang Xin
^ permalink raw reply
* Re: [PATCH 2/3] update-ref: pass reflog message argument to delete_refs
From: Junio C Hamano @ 2017-02-17 17:11 UTC (permalink / raw)
To: Jeff King; +Cc: Kyle Meyer, git
In-Reply-To: <20170217082253.kxjezkxfqkfxjhzr@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> ...
All good review comments.
I briefly wondered if recording the deletion of the current branch
in HEAD's reflog has much practical values (Porcelain "git branch"
would not even allow deletion in the first place), but because it is
a rare and unusual event, it probably makes more sense to have a
record of it.
^ permalink raw reply
* Re: body-CC-comment regression
From: Johan Hovold @ 2017-02-17 17:30 UTC (permalink / raw)
To: Matthieu Moy
Cc: Johan Hovold, git, Jeff King, Kevin Daudt, Junio C Hamano,
Larry Finger
In-Reply-To: <vpq4lzs7o0s.fsf@anie.imag.fr>
On Fri, Feb 17, 2017 at 05:58:11PM +0100, Matthieu Moy wrote:
> > On Fri, Feb 17, 2017 at 02:16:42PM +0100, Matthieu Moy wrote:
> >> Johan Hovold <johan@kernel.org> writes:
> >
> >> The "multiple emails per Cc: field" has been there for a while already
> >> (b1c8a11c8024 released in 2.6.0, sept 2015), some users may have got
> >> used to it. What you are proposing breaks their flow.
> >
> > Note that that commit never mentions multiple addresses in either
> > headers or body-tags -- it's all about being able to specify multiple
> > entries on the command line.
>
> Indeed. I'm not the author of the patch, but I was supervising the
> students who wrote it and "multiple addresses in Cc:" was not the goal,
> but a (IMHO positive) side effect we discovered after the fact.
Yeah, and the broken --suppress-cc=self I mention below is indicative
of that too.
> If I had a time machine, I'd probably go back then and forbid multiple
> addresses there, but ...
>
> > There does not seem to be single commit in the kernel where multiple
> > address are specified in a CC tag since after git-send-email started
> > allowing it, but there are ten commits before (to my surprise), and that
> > should be contrasted with at least 4178 commits with trailing comments
> > including a # sign.
>
> Hey, there's a life outside the kernel ;-).
Sure, but it's the origin of git as well as the tags we're discussing (I
believe).
My point of bringing it up was that the multiple addresses in a CC-tag
was indeed an unintended (and undocumented) side-effect and I doubt many
people have started using it given that it's sort of counter-intuitive
(again, compare with SoB).
If either the trailing comments or multiple addresses in a CC-tag has to
go, I think dropping the latter is clearly the best choice.
> >> 1) Stop calling Mail::Address even if available.[...]
> >
> > Right, that sounds like the right thing to do regardless.
> >
> >> 2) Modify our in-house parser to discard garbage after the >. [...]
> >
> > Sounds perfectly fine to me, and seems to work too after quick test.
>
> OK, sounds like the way to go.
>
> Do you want to work on a patch? If not, I should be able to do that
> myself. The code changes are straightforward, but we probably want a
> proper test for that.
Feel free to implement it this way if that's what people prefer. As long
as trailing comments are supported and discarded, I don't really have a
preference.
> > addresses in a Cc-tag in that it breaks --suppress-cc=self, but I guess
> > that can be fixed separately.
>
> OK. If it's unrelated enough, please start a separate thread to explain
> the problem (and/or write a patch ;-) ).
Well, it's related to the "offending" patch that added support for
multiple addresses in tags. By disallowing that, as my fix does, the
problem goes away.
# Now parse the message body
while(<$fh>) {
$message .= $_;
if (/^(Signed-off-by|Cc): (.*)$/i) {
chomp;
my ($what, $c) = ($1, $2);
chomp $c;
my $sc = sanitize_address($c);
if ($sc eq $sender) {
next if ($suppress_cc{'self'});
The problem here is that $sc will never match $sender when there are more
than one address in a tag. For example:
From: Johan Hovold <johan@kernel.org>
...
Cc: alpha <test1@a.com>, Johan Hovold <johan@kernel.org>
results in
sc = alpha <test1@a.com>, Johan Hovold <johan@kernel.org>
sender = Johan Hovold <johan@kernel.org>
so that --suppress-cc=self is not honoured.
Thanks,
Johan
^ permalink raw reply
* Re: body-CC-comment regression
From: Linus Torvalds @ 2017-02-17 17:38 UTC (permalink / raw)
To: Matthieu Moy
Cc: Johan Hovold, Git Mailing List, Jeff King, Kevin Daudt,
Junio C Hamano, Larry Finger
In-Reply-To: <vpq7f4pdkjp.fsf@anie.imag.fr>
On Fri, Feb 17, 2017 at 5:16 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
>
> I mostly agree for the SoB, but why should a Cc tag have only one email?
Because changing that clearly broke real and useful behavior.
The "multiple email addresses" thing is bogus and wrong. Just don't do it.
How would you even parse it sanely? Are the Cc: lines now
SMTP-compliant with the whole escaping and all the usual "next line"
rules?
For example, in email, the rule for "next line" is that if you're in a
header block, and it starts with whitespace, then it's a continuation
of the last line.
That's *not* how Cc: lines work in commit messages. They are all
individual lines, and we have lots of tools (mainly just scripts with
grepping) that simply depend on it.
So this notion that the bottom of the commit message is some email
header crap is WRONG.
Stop it. It caused bugs. It's wrong. Don't do it.
Linus
^ permalink raw reply
* Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log
From: Junio C Hamano @ 2017-02-17 17:50 UTC (permalink / raw)
To: Jeff King; +Cc: Kyle Meyer, git
In-Reply-To: <20170217083112.vn7m4udsopmlvnn5@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Thu, Feb 16, 2017 at 10:58:00PM -0500, Kyle Meyer wrote:
>
>> When the current branch is renamed, the deletion of the old ref is
>> recorded in HEAD's log with an empty message. Now that delete_refs()
>> accepts a reflog message, provide a more descriptive message. This
>> message will not be included in the reflog of the renamed branch, but
>> its log already covers the renaming event with a message of "Branch:
>> renamed ...".
>
> Right, makes sense. The code overall looks fine, though I have one
> nit:
>
>> @@ -2616,10 +2617,15 @@ static int files_rename_ref(struct ref_store *ref_store,
>> return error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s",
>> oldrefname, strerror(errno));
>>
>> - if (delete_ref(oldrefname, orig_sha1, REF_NODEREF, NULL)) {
>> + strbuf_addf(&logmsg_del, "Deleted %s", oldrefname);
>
> We've been anything but consistent with our reflog messages in the past,
> but I think the general guideline for new ones is to use "command:
> action". Of course we don't _know_ the action here, because we're deep
> in rename_ref().
>
> Should we have the caller pass it in for us, as we do with delete_ref()
> and update_ref()?
>
> I see we actually already have a "logmsg" parameter. It already says
> "Branch: renamed %s to %s". Could we just reuse that? I know that this
> step is technically just the deletion, but I think it more accurately
> describes the whole operation that the deletion is part of.
True, but stepping back a bit,...
Do we even want these "internal" delete_ref() invocations to be
logged in HEAD's reflog? I understand that this is inside the
implementation of renaming an old ref to a new ref, and reflog
message given to delete_ref() would matter only if the HEAD happens
to be pointing at old ref---but then HEAD will be repointed to the
new ref by somebody else [*1*] that called this function to rename
old to new and it _will_ log it. So I am not sure if it is a good
thing to describe the deletion more readably with a message (which
is what this patch does) in the first place. If we can just say
"don't log this deletion event in HEAD's reflog", wouldn't that be
more desirable?
[Footnote]
*1* Is the reason why the code in files_rename_ref() we are looking
at does not adjust HEAD to point at the new ref is because it is
just handing one ref-store and obviouvious to symrefs in other
backends?
^ permalink raw reply
* Re: body-CC-comment regression
From: Junio C Hamano @ 2017-02-17 18:18 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Johan Hovold, git, Jeff King, Kevin Daudt, Larry Finger
In-Reply-To: <vpq4lzs7o0s.fsf@anie.imag.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>> On Fri, Feb 17, 2017 at 02:16:42PM +0100, Matthieu Moy wrote:
> ...
> If I had a time machine, I'd probably go back then and forbid multiple
> addresses there, but ...
>
>> There does not seem to be single commit in the kernel where multiple
>> address are specified in a CC tag since after git-send-email started
>> allowing it, but there are ten commits before (to my surprise), and that
>> should be contrasted with at least 4178 commits with trailing comments
>> including a # sign.
>
> Hey, there's a life outside the kernel ;-).
> ...
>>> 1) Stop calling Mail::Address even if available.[...]
>>
>> Right, that sounds like the right thing to do regardless.
>>
>>> 2) Modify our in-house parser to discard garbage after the >. [...]
>>
>> Sounds perfectly fine to me, and seems to work too after quick test.
>
> OK, sounds like the way to go.
>
> Do you want to work on a patch? If not, I should be able to do that
> myself. The code changes are straightforward, but we probably want a
> proper test for that.
The true headers and the things at the bottom seem to be handled in
a separate loop in send-email, so treating Cc: found in the former
and in the latter differently should be doable. I think it is OK to
explicitly treat the latter as "these are not e-mail addresses, but
just a single e-mail address possibly with non-address cruft",
without losing the ability to have more than one addresses on a
single CC: e-mail header.
^ permalink raw reply
* Re: body-CC-comment regression
From: Johan Hovold @ 2017-02-17 18:23 UTC (permalink / raw)
To: Junio C Hamano
Cc: Matthieu Moy, Johan Hovold, git, Jeff King, Kevin Daudt,
Larry Finger
In-Reply-To: <xmqqd1egu1dl.fsf@gitster.mtv.corp.google.com>
On Fri, Feb 17, 2017 at 10:18:46AM -0800, Junio C Hamano wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
> >> On Fri, Feb 17, 2017 at 02:16:42PM +0100, Matthieu Moy wrote:
> > ...
> > If I had a time machine, I'd probably go back then and forbid multiple
> > addresses there, but ...
> >
> >> There does not seem to be single commit in the kernel where multiple
> >> address are specified in a CC tag since after git-send-email started
> >> allowing it, but there are ten commits before (to my surprise), and that
> >> should be contrasted with at least 4178 commits with trailing comments
> >> including a # sign.
> >
> > Hey, there's a life outside the kernel ;-).
> > ...
> >>> 1) Stop calling Mail::Address even if available.[...]
> >>
> >> Right, that sounds like the right thing to do regardless.
> >>
> >>> 2) Modify our in-house parser to discard garbage after the >. [...]
> >>
> >> Sounds perfectly fine to me, and seems to work too after quick test.
> >
> > OK, sounds like the way to go.
> >
> > Do you want to work on a patch? If not, I should be able to do that
> > myself. The code changes are straightforward, but we probably want a
> > proper test for that.
>
> The true headers and the things at the bottom seem to be handled in
> a separate loop in send-email, so treating Cc: found in the former
> and in the latter differently should be doable. I think it is OK to
> explicitly treat the latter as "these are not e-mail addresses, but
> just a single e-mail address possibly with non-address cruft",
> without losing the ability to have more than one addresses on a
> single CC: e-mail header.
That's precisely what the patch I posted earlier in the thread did.
Thanks,
Johan
^ permalink raw reply
* Re: [PATCH 06/15] update submodules: add submodule config parsing
From: Jacob Keller @ 2017-02-17 18:24 UTC (permalink / raw)
To: Stefan Beller
Cc: Git mailing list, brian m. carlson, Jonathan Nieder,
Brandon Williams, Junio C Hamano
In-Reply-To: <20170216003811.18273-7-sbeller@google.com>
On Wed, Feb 15, 2017 at 4:38 PM, Stefan Beller <sbeller@google.com> wrote:
> Similar to b33a15b08 (push: add recurseSubmodules config option,
> 2015-11-17) and 027771fcb1 (submodule: allow erroneous values for the
> fetchRecurseSubmodules option, 2015-08-17), we add submodule-config code
> that is later used to parse whether we are interested in updating
> submodules.
>
> We need the `die_on_error` parameter to be able to call this parsing
> function for the config file as well, which if incorrect lets Git die.
>
> As we're just touching the header file, also mark all functions extern.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> submodule-config.c | 22 ++++++++++++++++++++++
> submodule-config.h | 17 +++++++++--------
> 2 files changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/submodule-config.c b/submodule-config.c
> index 93453909cf..93f01c4378 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -234,6 +234,28 @@ int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg)
> return parse_fetch_recurse(opt, arg, 1);
> }
>
> +static int parse_update_recurse(const char *opt, const char *arg,
> + int die_on_error)
> +{
> + switch (git_config_maybe_bool(opt, arg)) {
> + case 1:
> + return RECURSE_SUBMODULES_ON;
> + case 0:
> + return RECURSE_SUBMODULES_OFF;
> + default:
> + if (!strcmp(arg, "checkout"))
> + return RECURSE_SUBMODULES_ON;
> + if (die_on_error)
> + die("bad %s argument: %s", opt, arg);
> + return RECURSE_SUBMODULES_ERROR;
> + }
> +}
Ok so this function here reads a recurse submodules parameter which is
a boolean or it can be set to the word "checkout"? Why does checkout
need its own value separate from true? Just so that we have a synonym?
or so that we can expand on it in the future?
^ permalink raw reply
* Re: [PATCH v3 0/2] Fix bugs in rev-parse's output when run in a subdirectory
From: Junio C Hamano @ 2017-02-17 18:25 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Nguyễn Thái Ngọc Duy, Michael Rappazzo
In-Reply-To: <cover.1487350582.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> The bug that bit me (hard!) and that triggered not only a long series of
> curses but also my writing a patch and sending it to the list was that
> `git rev-parse --git-path HEAD` would give *incorrect* output when run
> in a subdirectory of a regular checkout, but *correct* output when run
> in a subdirectory of an associated *worktree*.
>
> I had tested the script in question quite a bit, but in a worktree. And
> in production, it quietly did exactly the wrong thing.
>
> Changes relative to v2:
>
> - the "iffy" test in t1700 was made "uniffy"
>
> - clarified in the commit message of 2/2 why we can get away with the
> "reset then use" pattern
It is no longer relevant between "reset then use" and "use then
reset", I think, because you did something much better, which is to
move strbuf_release() up so that it comes before the possible early
returns.
Both patches look good. Let's queue this and move it to 'next'
shortly. Personally, I think it is OK to fast-track this to
'master' before the final, but just like any other bugs, we've lived
with the bug for some time, and it is not a big deal if we have to
live with it for a bit longer.
Thanks.
^ 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