* Re: [PATCH 1/3] rebase -i: identify problems with core.commentchar
From: Johannes Schindelin @ 2016-11-22 16:09 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, git, Ralf Thielow,
Nguyễn Thái Ngọc Duy, Taufiq Hoven
In-Reply-To: <xmqqbmx8k8c0.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 21 Nov 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
> > index 29e91d8..202ac07 100755
> > --- a/t/t0030-stripspace.sh
> > +++ b/t/t0030-stripspace.sh
> > @@ -432,6 +432,13 @@ test_expect_success '-c with changed comment char' '
> > test_cmp expect actual
> > '
> >
> > +test_expect_failure '-c with comment char defined in .git/config' '
> > + test_config core.commentchar = &&
> > + printf "= foo\n" >expect &&
> > + printf "foo" | git stripspace -c >actual &&
>
> We'd want "\n" on this printf to match the one before as well,
True.
> The analysis of the log message in [2/3] is wrong and needs
> updating, though.
Thanks for following up on that, and for fixing the commit message.
> > +test_expect_failure 'rebase -i respects core.commentchar=auto' '
> > + test_config core.commentchar auto &&
> > + write_script copy-edit-script.sh <<-\EOF &&
> > + cp "$1" edit-script
> > + EOF
> > + test_set_editor "$(pwd)/copy-edit-script.sh" &&
> > + test_when_finished "git rebase --abort || :" &&
> > + git rebase -i HEAD^ &&
> > + grep "^#" edit-script &&
>
> This was added for debugging that was forgotten?
No, this was me trying to ensure that the comment character '#' *is* used.
As opposed to somehow testing any edit script that contains no commented
lines whatsoever.
But if you don't care, I won't, either.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto
From: Johannes Schindelin @ 2016-11-22 16:04 UTC (permalink / raw)
To: Johannes Sixt
Cc: git, Junio C Hamano, Ralf Thielow,
Nguyễn Thái Ngọc Duy, Taufiq Hoven
In-Reply-To: <975c0002-8afe-efa6-d967-72f2a0d21169@kdbg.org>
Hi Hannes,
On Mon, 21 Nov 2016, Johannes Sixt wrote:
> Am 21.11.2016 um 15:18 schrieb Johannes Schindelin:
> > -comment_char=$(git config --get core.commentchar 2>/dev/null | cut -c1)
> > -: ${comment_char:=#}
> > +comment_char=$(git config --get core.commentchar 2>/dev/null)
> > +case "$comment_char" in
> > +''|auto)
> > + comment_char=#
> > + ;;
> > +?)
> > + ;;
> > +*)
> > + comment_char=$(comment_char | cut -c1)
>
> comment_char is a command? Did you mean
>
> comment_char=$(echo "$comment_char" | cut -c1)
D'oh.
> It could be written without forking a process:
>
> comment_char=${comment_char%${comment_char#?}}
>
> (aka "remove from the end what remains after removing the first character")
I was considering this, actually, but it is rather unreadable. Better
rewrite it in C, to begin with.
Thanks,
Dscho
^ permalink raw reply
* Re: [GIT PULL] l10n updates for 2.11.0 round 2
From: Jiang Xin @ 2016-11-22 15:11 UTC (permalink / raw)
To: Junio C Hamano
Cc: Alexander Shopov, Alex Henrie, Ralf Thielow, Jean-Noël Avila,
Marco Paolone, Changwoo Ryu, Vasco Almeida, Dimitriy Ryazantcev,
Peter Krefting, Trần Ngọc Quân, Jiang Xin,
Git List
In-Reply-To: <CANYiYbGtfZKshmfYxHM0sL2Z67=3KTF0kixP1OjumRw87e9XkA@mail.gmail.com>
Git 2.11.0-rc2 introduced a very small l10n update. Because the
update windows will be
closed tomorrow, I made a batch updates. See commit:
* https://github.com/git-l10n/git-po/commit/275588f93
I have a reduced diff for this commit using a custom diff driver, see:
* https://gist.github.com/jiangxin/6384b1e865249228e00385fab84ef3f3
2016-11-22 22:59 GMT+08:00 Jiang Xin <worldhello.net@gmail.com>:
> Hi Junio,
>
> The following changes since commit 1310affe024fba407bff55dbe65cd6d670c8a32d:
>
> Git 2.11-rc2 (2016-11-17 13:47:36 -0800)
>
> are available in the git repository at:
>
> git://github.com/git-l10n/git-po tags/l10n-2.11.0-rnd2
--
Jiang Xin
^ permalink raw reply
* [GIT PULL] l10n updates for 2.11.0 round 2
From: Jiang Xin @ 2016-11-22 14:59 UTC (permalink / raw)
To: Junio C Hamano
Cc: Alexander Shopov, Alex Henrie, Ralf Thielow, Jean-Noël Avila,
Marco Paolone, Changwoo Ryu, Vasco Almeida, Dimitriy Ryazantcev,
Peter Krefting, Trần Ngọc Quân, Jiang Xin,
Git List
Hi Junio,
The following changes since commit 1310affe024fba407bff55dbe65cd6d670c8a32d:
Git 2.11-rc2 (2016-11-17 13:47:36 -0800)
are available in the git repository at:
git://github.com/git-l10n/git-po tags/l10n-2.11.0-rnd2
for you to fetch changes up to 275588f93eedd8d7a556c38b75944b858e704dce:
l10n: Fixed typo of git fetch-pack command (2016-11-22 22:24:59 +0800)
----------------------------------------------------------------
l10n-2.11.0-rnd2
----------------------------------------------------------------
Changwoo Ryu (1):
l10n: ko.po: Update Korean translation
Dimitriy Ryazantcev (1):
l10n: ru.po: update Russian translation
Jean-Noel Avila (1):
l10n: fr.po v2.11.0_rnd1
Jiang Xin (8):
l10n: git.pot: v2.11.0 round 1 (209 new, 53 removed)
Merge branch 'russian-l10n' of https://github.com/DJm00n/git-po-ru
Merge branch 'ko/merge-l10n' of https://github.com/changwoo/git-l10n-ko
Merge branch 'fr_v2.11.0_rnd1' of git://github.com/jnavila/git
l10n: zh_CN: for git v2.11.0 l10n round 1
Merge branch 'master' of git://github.com/git-l10n/git-po
l10n: git.pot: v2.11.0 round 2 (1 new, 1 removed)
l10n: Fixed typo of git fetch-pack command
Peter Krefting (1):
l10n: sv.po: Update Swedish translation (2913t0f0u)
Trần Ngọc Quân (1):
l10n: vi.po: Updated translation to v2.11.0 (2913t)
Vasco Almeida (1):
l10n: pt_PT: update Portuguese translation
jfbu (1):
l10n: fr.po fix grammar mistakes
po/fr.po | 8916 +++++++++++++++++++++++++++++++++--------------------------
po/git.pot | 6664 ++++++++++++++++++++++++--------------------
po/ko.po | 8729 +++++++++++++++++++++++++++++++--------------------------
po/pt_PT.po | 8794 ++++++++++++++++++++++++++++++++--------------------------
po/ru.po | 4429 +++++++++++++++++------------
po/sv.po | 8709 +++++++++++++++++++++++++++++++--------------------------
po/vi.po | 8697 +++++++++++++++++++++++++++++++--------------------------
po/zh_CN.po | 8689 +++++++++++++++++++++++++++++++--------------------------
8 files changed, 35318 insertions(+), 28309 deletions(-)
--
Jiang Xin
^ permalink raw reply
* v2.11 new diff heuristic?
From: Robert Dailey @ 2016-11-22 14:42 UTC (permalink / raw)
To: Git
The release notes mention a new heuristic for diff:
* Output from "git diff" can be made easier to read by selecting
which lines are common and which lines are added/deleted
intelligently when the lines before and after the changed section
are the same. A command line option is added to help with the
experiment to find a good heuristics.
However, it lacks information on exactly how to use this new feature.
I dug into the git diff documentation here:
https://git-scm.com/docs/git-diff
It mentions a "--compaction-heuristic" option. Is this the new
heuristic outlined by the release notes? If not, which is it? Is the
compaction heuristic compatible with the histogram diff algorithm? Is
there a config option to turn this on all the time? For that matter,
is this something I can keep on all the time or is it only useful in
certain situations?
There's still so much more about this feature I would like to know.
^ permalink raw reply
* Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines
From: Vasco Almeida @ 2016-11-22 14:10 UTC (permalink / raw)
To: git
Cc: Jiang Xin, Ævar Arnfjörð Bjarmason,
Jean-Noël AVILA, Jakub Narębski, David Aguilar,
Junio C Hamano
In-Reply-To: <20161111124541.8216-2-vascomalmeida@sapo.pt>
A Sex, 11-11-2016 às 11:45 -0100, Vasco Almeida escreveu:
> +=item comment_lines ( STRING [, STRING... ])
> +
> +Comments lines following core.commentchar configuration.
> +
> +=cut
> +
> +sub comment_lines {
> + my $comment_line_char = config("core.commentchar") || '#';
> + return prefix_lines("$comment_line_char ", @_);
> +}
> +
In light of the recent "Fix problems with rebase -i when
core.commentchar is defined" [1], I realized that this patch does not
handle the 'auto' value of core.commentchat configuration variable.
I propose to do the patch below in the next re-roll.
[1] http://www.mail-archive.com/git@vger.kernel.org/msg107818.html
-- >8 --
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 3a6d846..8d33634 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1073,6 +1073,7 @@ sub edit_hunk_manually {
my $is_reverse = $patch_mode_flavour{IS_REVERSE};
my ($remove_plus, $remove_minus) = $is_reverse ? ('-', '+') : ('+', '-');
my $comment_line_char = Git::config("core.commentchar") || '#';
+ $comment_line_char = '#' if ($comment_line_char eq 'auto');
print $fh Git::comment_lines sprintf(__ <<EOF, $remove_minus, $remove_plus, $comment_line_char),
---
To remove '%s' lines, make them ' ' lines (context).
diff --git a/perl/Git.pm b/perl/Git.pm
index 69cd1dd..47b5899 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -1459,6 +1459,7 @@ Comments lines following core.commentchar configuration.
sub comment_lines {
my $comment_line_char = config("core.commentchar") || '#';
+ $comment_line_char = '#' if ($comment_line_char eq 'auto');
return prefix_lines("$comment_line_char ", @_);
}
^ permalink raw reply related
* Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange
From: Duy Nguyen @ 2016-11-22 13:20 UTC (permalink / raw)
To: Christian Couder
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason, git,
Christian Couder
In-Reply-To: <CAP8UFD2VFQs2X14omWEy-iJzEeTEeEOa+fxMEq3HTQK3rFyuCg@mail.gmail.com>
On Tue, Nov 22, 2016 at 8:13 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> So if we now mix things up just to avoid one more configuration
> option, we could very well make things harder to develop, to
> configure, to parse and to understand later, so it is not a trade off
> worth making.
OK since we're still in the early phase of determining how to
re-split, I guess going with elaborate, precise configuration is
better, even if it's more config options. Once we know better, maybe
we can decide a good default cover-all "auto" then (or maybe not
because we find out no shoe fits all).
--
Duy
^ permalink raw reply
* Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange
From: Christian Couder @ 2016-11-22 13:13 UTC (permalink / raw)
To: Duy Nguyen
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason, git,
Christian Couder
In-Reply-To: <CACsJy8BazeR=4tz3q2f35x=fCfp-Ld9LJz0mQh_CZoR_iXKEEQ@mail.gmail.com>
On Tue, Nov 22, 2016 at 11:35 AM, Duy Nguyen <pclouds@gmail.com> wrote:
[...]
>>> In my opinion, "true" _is_ auto, which is a way to say "I trust you to
>>> do the right thing, just re-split the index when it makes sense", "no"
>>> is disabled of course. If the user wants to be specific, just write
>>> "10" or some other percentage.(and either 0 or 100 would mean enable
>>> split-index but do not re-split automatically, let _me_ do it when I
>>> want it)
>>
>> The meaning of a future "auto" option for "core.splitIndex" could be
>> "use the split-index feature only if the number of entries in whole
>> index is greater than 10000 (by default)".
>
> Well.. with the "just re-split the index when it makes sense" part,
> the user entrusts git to do something sensible in all cases,
That's an interpretation of what "core.splitIndex=true" could mean,
but there could be users who trust Git to re-split when it makes
sense, but who do want to use the split-index on all theirs repos even
the small ones or who just don't trust Git to choose when it might be
better to use it or not.
Yeah, a typical git user would most of the time just trust Git for all
those things, but on the other hand there are companies out there that
are willing to tweak many configuration options to get the better
possible behavior for them.
In fact I am working on this for Booking.com, and if we find out later
that we would gain something significant, like performance
improvements or configuration simplification, by adding "auto" and/or
other configuration variables to tweak more split-index related
things, we might very well post patch series to do that.
So if we now mix things up just to avoid one more configuration
option, we could very well make things harder to develop, to
configure, to parse and to understand later, so it is not a trade off
worth making.
> and going
> with absolute numbers might not be the best way, I think. It's big
> responsibility :)
About going with absolute number, yeah I am not sure at all it is the
best way, but this was just part of an example to try to explain what
I am saying above.
^ permalink raw reply
* [PATCH] merge-recursive.c: use QSORT macro
From: Nguyễn Thái Ngọc Duy @ 2016-11-22 12:30 UTC (permalink / raw)
To: git; +Cc: René Scharfe, Nguyễn Thái Ngọc Duy
This is the follow up of rs/qsort series, merged in b8688ad (Merge
branch 'rs/qsort' - 2016-10-10), where coccinelle was used to do
automatic transformation.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
coccinelle missed this place, understandably, because it can't know
that
sizeof(*entries->items)
is the same as
sizeof(*df_name_compare.items)
without some semantic analysis.
merge-recursive.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 9041c2f..2d4dca9 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -451,7 +451,7 @@ static void record_df_conflict_files(struct merge_options *o,
string_list_append(&df_sorted_entries, next->string)->util =
next->util;
}
- qsort(df_sorted_entries.items, entries->nr, sizeof(*entries->items),
+ QSORT(df_sorted_entries.items, entries->nr,
string_list_df_name_compare);
string_list_clear(&o->df_conflict_file_set, 1);
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* Re: [PATCH 31/35] pathspec: allow querying for attributes
From: Duy Nguyen @ 2016-11-22 10:41 UTC (permalink / raw)
To: Stefan Beller; +Cc: Junio C Hamano, Brandon Williams, Git Mailing List
In-Reply-To: <20161110203428.30512-32-sbeller@google.com>
On Fri, Nov 11, 2016 at 3:34 AM, Stefan Beller <sbeller@google.com> wrote:
> @@ -139,7 +140,8 @@ static size_t common_prefix_len(const struct pathspec *pathspec)
> PATHSPEC_LITERAL |
> PATHSPEC_GLOB |
> PATHSPEC_ICASE |
> - PATHSPEC_EXCLUDE);
> + PATHSPEC_EXCLUDE |
> + PATHSPEC_ATTR);
Hmm.. common_prefix_len() has always been a bit relaxing and can cover
more than needed. It's for early pruning. Exact pathspec matching
_will_ be done later anyway.
Is that obvious? I'm wondering if we need to add a line or two in the
big comment code before this statement. I'm thinking it is and we
probably don't need more comments...
--
Duy
^ permalink raw reply
* Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange
From: Duy Nguyen @ 2016-11-22 10:35 UTC (permalink / raw)
To: Christian Couder
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason, git,
Christian Couder
In-Reply-To: <CAP8UFD1mun7wz2WqV8GCj6MODVjP2mPEBRGbJTf_ypiGCgtb9Q@mail.gmail.com>
On Fri, Nov 18, 2016 at 9:34 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Mon, Nov 7, 2016 at 10:38 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> (sorry I got sick in the last few weeks and could not respond to this earlier)
>
> (Yeah, I have also been sick during the last few weeks.)
>
>> On Mon, Nov 7, 2016 at 4:44 AM, Christian Couder
>> <christian.couder@gmail.com> wrote:
>>> Le 6 nov. 2016 09:16, "Junio C Hamano" <gitster@pobox.com> a écrit :
>>>>
>>>> Christian Couder <christian.couder@gmail.com> writes:
>>>>
>>>> > I think it is easier for user to be able to just set core.splitIndex
>>>> > to true to enable split-index.
>>>>
>>>> You can have that exact benefit by making core.splitIndex to
>>>> bool-or-more. If your default is 20%, take 'true' as if the user
>>>> specified 20% and take 'false' as if the user specified 100% (or is
>>>> it 0%? I do not care about the details but you get the point).
>>
>>> Then if we ever add 'auto' and the user wants for example 10% instead of the
>>> default 20%, we will have to make it accept things like "auto,10".
>
> (Sorry for writing the above on my phone which added HTML, so that it
> didn't reach the list.)
>
>> In my opinion, "true" _is_ auto, which is a way to say "I trust you to
>> do the right thing, just re-split the index when it makes sense", "no"
>> is disabled of course. If the user wants to be specific, just write
>> "10" or some other percentage.(and either 0 or 100 would mean enable
>> split-index but do not re-split automatically, let _me_ do it when I
>> want it)
>
> The meaning of a future "auto" option for "core.splitIndex" could be
> "use the split-index feature only if the number of entries in whole
> index is greater than 10000 (by default)".
Well.. with the "just re-split the index when it makes sense" part,
the user entrusts git to do something sensible in all cases, and going
with absolute numbers might not be the best way, I think. It's big
responsibility :)
> If there is no difference between "true" and "auto" then, when users
> who have "core.splitIndex=true" will migrate to the git version that
> adds the "auto" feature, their repos with under 10000 entires will not
> use the split-index feature anymore. These users may then be annoyed
> that the behavior has been switched under them, and that the
> split-index feature is not always used despite having
> "core.splitIndex=true" in their config.
--
Duy
^ permalink raw reply
* Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto
From: Duy Nguyen @ 2016-11-22 10:31 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Git Mailing List, Junio C Hamano, Ralf Thielow, Taufiq Hoven
In-Reply-To: <9ef529a4fbb60990a91d7bbfdd49c6d20d49e442.1479737858.git.johannes.schindelin@gmx.de>
On Mon, Nov 21, 2016 at 9:18 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> When 84c9dc2 (commit: allow core.commentChar=auto for character auto
> selection, 2014-05-17) extended the core.commentChar functionality to
> allow for the value 'auto', it forgot that rebase -i was already taught to
> handle core.commentChar,
I think this is 180bad3 (rebase -i: respect core.commentchar -
2013-02-11) and a couple followup fixes. My bad.
> and in turn forgot to let rebase -i handle that
> new value gracefully.
>
> Reported by Taufiq Hoven.
Another report in the same area: a merge conflict always writes the
"Conflicts:" section in the commit message with '#' as comment char
when core.commentChar is auto. I've known this for a while, but it has
not bitten me hard enough to do something about it,
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> git-rebase--interactive.sh | 13 +++++++++++--
> t/t3404-rebase-interactive.sh | 2 +-
> 2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index ca994c5..6bb740c 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -93,8 +93,17 @@ eval '
> GIT_CHERRY_PICK_HELP="$resolvemsg"
> export GIT_CHERRY_PICK_HELP
>
> -comment_char=$(git config --get core.commentchar 2>/dev/null | cut -c1)
> -: ${comment_char:=#}
> +comment_char=$(git config --get core.commentchar 2>/dev/null)
> +case "$comment_char" in
> +''|auto)
> + comment_char=#
My first thought was "this kinda defeats the purpose of auto, doesn't
it". But 'auto' here is irrelevant because we control the leading
character of all lines in the todo file. 'auto' makes little sense in
this context, falling back to any comment char would do.
So, ack (after you fix the $(comment_char other people have mentioned,
of course).
--
Duy
^ permalink raw reply
* Re: [PATCH 2/3] stripspace: respect repository config
From: Duy Nguyen @ 2016-11-22 10:10 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Git Mailing List, Junio C Hamano, Ralf Thielow, Taufiq Hoven
In-Reply-To: <5567548295222401fab10d3f2901c1787afbfd07.1479737858.git.johannes.schindelin@gmx.de>
On Mon, Nov 21, 2016 at 9:18 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> When eff80a9 (Allow custom "comment char", 2013-01-16) taught the
> `stripspace` command to respect the config setting `core.commentChar`,
> it forgot that this variable may be defined in .git/config.
>
> So when rebasing interactively with a commentChar defined in the current
> repository's config, the help text at the bottom of the edit script
> potentially used an incorrect comment character. This was not only
> funny-looking, but also resulted in tons of warnings like this one:
>
> Warning: the command isn't recognized in the following line
> - #
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> builtin/stripspace.c | 4 +++-
> t/t0030-stripspace.sh | 2 +-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/stripspace.c b/builtin/stripspace.c
> index 15e716e..1e62a00 100644
> --- a/builtin/stripspace.c
> +++ b/builtin/stripspace.c
> @@ -44,8 +44,10 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
> if (argc)
> usage_with_options(stripspace_usage, options);
>
> - if (mode == STRIP_COMMENTS || mode == COMMENT_LINES)
> + if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
> + setup_git_directory_gently(NULL);
> git_config(git_default_config, NULL);
> + }
This conditional config file reading is a trap for similar bugs to
happen again. Is there any reason we should not just mark the command
RUN_SETUP_GENTLY in git.c and call git_config() here unconditionally?
>
> if (strbuf_read(&buf, 0, 1024) < 0)
> die_errno("could not read the input");
--
Duy
^ permalink raw reply
* [PATCH 3/3] worktree list: keep the list sorted
From: Nguyễn Thái Ngọc Duy @ 2016-11-22 10:00 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, rappazzo, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161122100046.8341-1-pclouds@gmail.com>
It makes it easier to write tests for. But it should also be good for
the user since locating a worktree by eye would be easier once they
notice this.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/worktree.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index b835b91..80ccc51 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -434,6 +434,13 @@ static void measure_widths(struct worktree **wt, int *abbrev, int *maxlen)
}
}
+static int compare_worktree(const void *a_, const void *b_)
+{
+ const struct worktree *const *a = a_;
+ const struct worktree *const *b = b_;
+ return fspathcmp((*a)->path, (*b)->path);
+}
+
static int list(int ac, const char **av, const char *prefix)
{
int porcelain = 0;
@@ -448,11 +455,20 @@ static int list(int ac, const char **av, const char *prefix)
usage_with_options(worktree_usage, options);
else {
struct worktree **worktrees = get_worktrees();
- int path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;
+ int path_maxlen = 0, abbrev = DEFAULT_ABBREV, i, nr;
if (!porcelain)
measure_widths(worktrees, &abbrev, &path_maxlen);
+ for (i = nr = 0; worktrees[i]; i++)
+ nr++;
+
+ /*
+ * don't sort the first item (main worktree), which will
+ * always be the first
+ */
+ QSORT(worktrees + 1, nr - 1, compare_worktree);
+
for (i = 0; worktrees[i]; i++) {
if (porcelain)
show_worktree_porcelain(worktrees[i]);
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH 2/3] get_worktrees() must return main worktree as first item even on error
From: Nguyễn Thái Ngọc Duy @ 2016-11-22 10:00 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, rappazzo, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161122100046.8341-1-pclouds@gmail.com>
This is required by git-worktree.txt, stating that the main worktree is
the first line (especially in --porcelain mode when we can't just change
behavior at will).
There's only one case when get_worktrees() may skip main worktree, when
parse_ref() fails. Update the code so that we keep first item as main
worktree and return something sensible in this case:
- In user-friendly mode, since we're not constraint by anything,
returning "(error)" should do the job (we already show "(detached
HEAD)" which is not machine-friendly). Actually errors should be
printed on stderr by parse_ref() (*)
- In plumbing mode, we do not show neither 'bare', 'detached' or
'branch ...', which is possible by the format description if I read
it right.
Careful readers may realize that when the local variable "head_ref" in
get_main_worktree() is emptied, add_head_info() will do nothing to
wt->head_sha1. But that's ok because head_sha1 is zero-ized in the
previous patch.
(*) Well, it does not. But it's supposed to be a stop gap
implementation until refs we can reuse code, to parse "ref: " stuff
in HEAD, from resolve_refs_unsafe(). Now may be the time since
refs refactoring is mostly done.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/worktree.c | 8 +++++---
worktree.c | 6 ++----
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 5c4854d..b835b91 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -388,7 +388,7 @@ static void show_worktree_porcelain(struct worktree *wt)
printf("HEAD %s\n", sha1_to_hex(wt->head_sha1));
if (wt->is_detached)
printf("detached\n");
- else
+ else if (wt->head_ref)
printf("branch %s\n", wt->head_ref);
}
printf("\n");
@@ -406,10 +406,12 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
else {
strbuf_addf(&sb, "%-*s ", abbrev_len,
find_unique_abbrev(wt->head_sha1, DEFAULT_ABBREV));
- if (!wt->is_detached)
+ if (wt->is_detached)
+ strbuf_addstr(&sb, "(detached HEAD)");
+ else if (wt->head_ref)
strbuf_addf(&sb, "[%s]", shorten_unambiguous_ref(wt->head_ref, 0));
else
- strbuf_addstr(&sb, "(detached HEAD)");
+ strbuf_addstr(&sb, "(error)");
}
printf("%s\n", sb.buf);
diff --git a/worktree.c b/worktree.c
index f7c1b5e..a674efa 100644
--- a/worktree.c
+++ b/worktree.c
@@ -89,7 +89,7 @@ static struct worktree *get_main_worktree(void)
strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
- goto done;
+ strbuf_reset(&head_ref);
worktree = xcalloc(1, sizeof(*worktree));
worktree->path = strbuf_detach(&worktree_path, NULL);
@@ -97,7 +97,6 @@ static struct worktree *get_main_worktree(void)
worktree->is_detached = is_detached;
add_head_info(&head_ref, worktree);
-done:
strbuf_release(&path);
strbuf_release(&worktree_path);
strbuf_release(&head_ref);
@@ -173,8 +172,7 @@ struct worktree **get_worktrees(void)
list = xmalloc(alloc * sizeof(struct worktree *));
- if ((list[counter] = get_main_worktree()))
- counter++;
+ list[counter++] = get_main_worktree();
strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
dir = opendir(path.buf);
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH 1/3] worktree.c: zero new 'struct worktree' on allocation
From: Nguyễn Thái Ngọc Duy @ 2016-11-22 10:00 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, rappazzo, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161122100046.8341-1-pclouds@gmail.com>
This keeps things a bit simpler when we add more fields, knowing that
default values are always zero.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
worktree.c | 14 ++------------
1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/worktree.c b/worktree.c
index f7869f8..f7c1b5e 100644
--- a/worktree.c
+++ b/worktree.c
@@ -91,16 +91,11 @@ static struct worktree *get_main_worktree(void)
if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
goto done;
- worktree = xmalloc(sizeof(struct worktree));
+ worktree = xcalloc(1, sizeof(*worktree));
worktree->path = strbuf_detach(&worktree_path, NULL);
- worktree->id = NULL;
worktree->is_bare = is_bare;
- worktree->head_ref = NULL;
worktree->is_detached = is_detached;
- worktree->is_current = 0;
add_head_info(&head_ref, worktree);
- worktree->lock_reason = NULL;
- worktree->lock_reason_valid = 0;
done:
strbuf_release(&path);
@@ -138,16 +133,11 @@ static struct worktree *get_linked_worktree(const char *id)
if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
goto done;
- worktree = xmalloc(sizeof(struct worktree));
+ worktree = xcalloc(1, sizeof(*worktree));
worktree->path = strbuf_detach(&worktree_path, NULL);
worktree->id = xstrdup(id);
- worktree->is_bare = 0;
- worktree->head_ref = NULL;
worktree->is_detached = is_detached;
- worktree->is_current = 0;
add_head_info(&head_ref, worktree);
- worktree->lock_reason = NULL;
- worktree->lock_reason_valid = 0;
done:
strbuf_release(&path);
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH 0/3] Minor fixes on 'git worktree'
From: Nguyễn Thái Ngọc Duy @ 2016-11-22 10:00 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, rappazzo, Nguyễn Thái Ngọc Duy
This fixes two things:
- make sure the first item is always the main worktree even if we
fail to retrieve some info
- keep 'worktree list' order stable (which in turn fixes the random
failure on my 'worktree-move' series
Nguyễn Thái Ngọc Duy (3):
worktree.c: zero new 'struct worktree' on allocation
get_worktrees() must return main worktree as first item even on error
worktree list: keep the list sorted
builtin/worktree.c | 26 ++++++++++++++++++++++----
worktree.c | 20 ++++----------------
2 files changed, 26 insertions(+), 20 deletions(-)
--
2.8.2.524.g6ff3d78
^ permalink raw reply
* Re: [PATCH 3/3] submodule--helper: add intern-git-dir function
From: Junio C Hamano @ 2016-11-22 7:07 UTC (permalink / raw)
To: Stefan Beller
Cc: Brandon Williams, Jonathan Nieder, git@vger.kernel.org,
Jens Lehmann, Heiko Voigt
In-Reply-To: <CAGZ79kb_4wWs_90AfsT932iPWbCXf6yRq875JUxoRZjUcsBW5A@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> So I guess we should test a bit more extensively, maybe
>
> git status >expect
> git submodule embedgitdirs
> git status >actual
> test_cmp expect actual
> # further testing via
> test -f ..
> test -d ..
Something along that line. "status should succeed" does not tell
the readers what kind of breakage the test is expecting to protect
us from. If we are expecting a breakage in embed-git-dirs would
somehow corrupt an existing submodule, which would lead to "status"
that is run in the superproject report the submodule differently,
then comparing output before and after the operation may be a
reasonable test. Going there to the submodule working tree and
checking the health of the repository (of the submodule) may be
another sensible test.
>> In the
>> extreme, if the failed "git submodule" command did
>>
>> rm -fr .git ?* && git init
>>
>> wouldn't "git status" still succeed?
>
> In that particular case you'd get
> $ git status
> fatal: Not a git repository (or any parent up to mount point ....)
Even with "&& git init"? Or you forgot that part?
^ permalink raw reply
* Re: [PATCH 1/3] submodule: use absolute path for computing relative path connecting
From: Junio C Hamano @ 2016-11-22 7:02 UTC (permalink / raw)
To: Stefan Beller
Cc: Brandon Williams, Jonathan Nieder, git@vger.kernel.org,
Jens Lehmann, Heiko Voigt
In-Reply-To: <CAGZ79ka60_D8xfQyBegkXxkTGW5YDMuagB7kjhiCR6NriLR8+A@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> On Mon, Nov 21, 2016 at 1:03 PM, Stefan Beller <sbeller@google.com> wrote:
>> On Mon, Nov 21, 2016 at 1:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>> Can the effect of this change demonstrated in a new test? There
>>> must be a scenario where the current behaviour is broken and this
>>> change fixes an incorrect computation of relative path, no?
>
> I do not think the current usage exposes this bug in
> connect_work_tree_and_git_dir. It is only used in builtin/mv.c,
> which fills the second parameter `git_dir` via a call to read_gitfile,
> which itself produces an absolute path.
OK. Fixing a potential bug as a preparatory step is good.
> The current caller of connect_work_tree_and_git_dir passes
> an absolute path for the `git_dir` parameter. In the future patch
> we will also pass in relative path for `git_dir`. Extend the functionality
> of connect_work_tree_and_git_dir to take relative paths for parameters.
>
> We could work around this in the future patch by computing the absolute
> path for the git_dir in the calling site, however accepting relative
> paths for either parameter makes the API for this function easier
> to use.
Yup, sounds sensible. Thanks.
^ permalink raw reply
* Re: [PATCHv3 3/3] submodule-config: clarify parsing of null_sha1 element
From: Junio C Hamano @ 2016-11-22 3:42 UTC (permalink / raw)
To: Stefan Beller; +Cc: bmwill, git, jacob.keller
In-Reply-To: <20161122013550.1800-4-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> +Whenever a submodule configuration is parsed in `parse_submodule_config_option`
> +via e.g. `gitmodules_config()`, it will be overwrite the null_sha1 entry.
It will overwrite? It will be overwritten? I guess it is the latter?
> +So in the normal case, when HEAD:.gitmodules is parsed first and then overlayed
> +with the repository configuration, the null_sha1 entry contains the local
> +configuration of a submodule (e.g. consolidated values from local git
> configuration and the .gitmodules file in the worktree).
>
> For an example usage see test-submodule-config.c.
^ permalink raw reply
* Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"
From: Junio C Hamano @ 2016-11-22 3:40 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Jeff King, git, Duy Nguyen
In-Reply-To: <20161122004421.GA12263@google.com>
Jonathan Nieder <jrnieder@gmail.com> writes:
> This trips reproducibly for
>
> git ls-remote https://kernel.googlesource.com/pub/scm/git/git
>
> when run outside of a git repository.
>
> Backtrace:
>
> #0 setup_git_env () at environment.c:172
> #1 get_git_dir () at environment.c:214
> #2 get_helper at transport-helper.c:127
> #3 get_refs_list (for_push=0) at transport-helper.c:1038
> #4 transport_get_remote_refs at transport.c:1076
> #5 cmd_ls_remote at builtin/ls-remote.c:97
>
> transport-helper.c:127 is
>
> argv_array_pushf(&helper->env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
> get_git_dir());
>
> That code is pretty clearly wrong. Should it be made conditional
> on have_git_dir(), like this?
Looks good and I agree with Peff's analysis. Care to wrap it in a
patch with a log message?
Thanks.
>
> Thanks,
> Jonathan
>
> transport-helper.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index 91aed35..e4fd982 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -124,8 +124,9 @@ static struct child_process *get_helper(struct transport *transport)
> helper->git_cmd = 0;
> helper->silent_exec_failure = 1;
>
> - argv_array_pushf(&helper->env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
> - get_git_dir());
> + if (have_git_dir())
> + argv_array_pushf(&helper->env_array, "%s=%s",
> + GIT_DIR_ENVIRONMENT, get_git_dir());
>
> code = start_command(helper);
> if (code < 0 && errno == ENOENT)
^ permalink raw reply
* Re: [PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree
From: Junio C Hamano @ 2016-11-22 3:37 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill, jacob.keller
In-Reply-To: <20161121232709.8906-3-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> It is also possible to pass in a tree hash to lookup a submodule config.
> Make it clear by naming the variables accrodingly. Looking up a submodule
> config by tree hash will come in handy in a later patch.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
Yeah, I noticed that while reading the previous round, too, but...
> -`const struct submodule *submodule_from_name(const unsigned char *commit_sha1, const char *name)`::
> +`const struct submodule *submodule_from_name(const unsigned char *commit_or_tree, const char *name)`::
>
> The same as above but lookup by name.
Doesn't (the) "above", aka submodule_from_path() want the same
treatment?
Also the explanation of "above" has room for improvement. Namely it
says:
Lookup values for one submodule by its commit_sha1 and path.
I do not think the commit-sha1 (or commit-or-tree-object-name) is
"ITS" object name for the submodule. The name belongs to the
containing superproject commit (or tree), no?
Given a tree-ish in the superproject and a path, return the
submodule that is bound at the path in the named tree.
is what I would write for that one. Thinking about it a bit
further, "treeish_name" would be a much better name for the variable
than "commit_or_tree", as "treeish" is an established short and
sweet word that means "commit_or_tree", and having a "name"
somewhere in the variable name makes it clear that we are not
passing the pointer to an in-core object (e.g. "struct commit *").
> +test_expect_success 'using tree sha1 works' '
> + (
> + cd super &&
> + tree=$(git rev-parse HEAD^{tree}) &&
> + commit=$(git rev-parse HEAD^{commit}) &&
> + test-submodule-config $commit b >expect &&
> + test-submodule-config $tree b >actual &&
> + test_cmp expect actual
> + )
> +'
Perhaps also test a tag that points at the commit?
^ permalink raw reply
* Re: [PATCHv2 1/3] submodule config: inline config_from_{name, path}
From: Junio C Hamano @ 2016-11-22 3:27 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill, jacob.keller
In-Reply-To: <20161121232709.8906-2-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> There is no other user of config_from_{name, path}, such that there is no
> reason for the existence of these one liner functions. Just inline these
> to increase readability.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
Makes sense.
^ permalink raw reply
* Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"
From: Jeff King @ 2016-11-22 2:41 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Duy Nguyen
In-Reply-To: <20161122004421.GA12263@google.com>
On Mon, Nov 21, 2016 at 04:44:21PM -0800, Jonathan Nieder wrote:
> > git_dir = getenv(GIT_DIR_ENVIRONMENT);
> > - if (!git_dir)
> > + if (!git_dir) {
> > + if (!startup_info->have_repository)
> > + die("BUG: setup_git_env called without repository");
> > git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
> > + }
>
> This trips reproducibly for
>
> git ls-remote https://kernel.googlesource.com/pub/scm/git/git
>
> when run outside of a git repository.
>
> Backtrace:
>
> #0 setup_git_env () at environment.c:172
> #1 get_git_dir () at environment.c:214
> #2 get_helper at transport-helper.c:127
> #3 get_refs_list (for_push=0) at transport-helper.c:1038
> #4 transport_get_remote_refs at transport.c:1076
> #5 cmd_ls_remote at builtin/ls-remote.c:97
>
> transport-helper.c:127 is
>
> argv_array_pushf(&helper->env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
> get_git_dir());
>
> That code is pretty clearly wrong. Should it be made conditional
> on have_git_dir(), like this?
Yeah, I think making it conditional makes the most sense. Just trying to
think of cases that might not be covered by your patch:
- if we are not in a repository, we shouldn't ever need to override an
existing $GIT_DIR from the environment. Because if $GIT_DIR is
present, then we _would_ be in a repo if it's valid, and die if it
isn't.
- not setting $GIT_DIR may cause a helper to do the usual discovery
walk to find the repository. But we know we're not in one, or we
would have found it ourselves. So worst case it may expend a little
effort to try to find a repository and fail (I think remote-curl
would do this to try to find repo-level config).
Both of those points assume that we've already called
setup_git_directory_gently(), but I think that's a reasonable
assumption. And certainly is true for ls-remote, and should be for any
git command that invokes the transport code.
> diff --git a/transport-helper.c b/transport-helper.c
> index 91aed35..e4fd982 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -124,8 +124,9 @@ static struct child_process *get_helper(struct transport *transport)
> helper->git_cmd = 0;
> helper->silent_exec_failure = 1;
>
> - argv_array_pushf(&helper->env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
> - get_git_dir());
> + if (have_git_dir())
> + argv_array_pushf(&helper->env_array, "%s=%s",
> + GIT_DIR_ENVIRONMENT, get_git_dir());
So yeah, I think this is the extent of the change needed.
Thanks.
-Peff
^ permalink raw reply
* Re: [PATCH 3/3] submodule--helper: add intern-git-dir function
From: Stefan Beller @ 2016-11-22 2:09 UTC (permalink / raw)
To: Junio C Hamano
Cc: Brandon Williams, Jonathan Nieder, git@vger.kernel.org,
Jens Lehmann, Heiko Voigt
In-Reply-To: <xmqqy40ch6wp.fsf@gitster.mtv.corp.google.com>
On Mon, Nov 21, 2016 at 1:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Does this format correctly?
>
> I somehow thought that second and subsequent paragraphs continued
> with "+" want no indentation before them. See for example the
> Values section in config.txt and see how entries for boolean:: and
> color:: use multiple '+' paragraphs.
>
> If we do not have to refrain from indenting the second and
> subsequent paragraphs, that would be great for readability, but I
> take the existing practice as telling me that we cannot do that X-<.
Will fix and test in a resend.
>
>> +test_expect_success 'setup a gitlink with missing .gitmodules entry' '
>> + git init sub2 &&
>> + test_commit -C sub2 first &&
>> + git add sub2 &&
>> + git commit -m superproject
>> +'
>> +
>> +test_expect_success 'intern the git dir fails for incomplete submodules' '
>> + test_must_fail git submodule interngitdirs &&
>> + # check that we did not break the repository:
>> + git status
>> +'
>
> It is not clear what the last "git status" wants to test.
Any errors that I ran into when manually truing to embed a submodules
git dir, were fatal with `git status` already, e.g. missing or wrong
call of connect_work_tree_and_git_dir were my main failure points.
So I guess we should test a bit more extensively, maybe
git status >expect
git submodule embedgitdirs
git status >actual
test_cmp expect actual
# further testing via
test -f ..
test -d ..
> In the
> extreme, if the failed "git submodule" command did
>
> rm -fr .git ?* && git init
>
> wouldn't "git status" still succeed?
In that particular case you'd get
$ git status
fatal: Not a git repository (or any parent up to mount point ....)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
$ echo $?
128
but I get the idea, which is why I propose the double check via status.
That would detect any logical change for the repository, e.g. a
change to the .gitmodules file.
>
> What are the minimum things that we expect from "did not break" to
> see? sub2/.git is still a directory and is a valid repository? The
> contents of the .git/modules/* before and after the "git submodule"
> does not change? Some other things?
I thought about making up a name for such a repo and creating that engry
in .gitmodules. But I refrained from doing so, because it seems too much
for this command.
I dunno, but I would suspect the double status is fine here, too?
^ 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