* Re: [PATCHv6 4/7] worktree: get worktrees from submodules
From: Stefan Beller @ 2016-12-09 23:10 UTC (permalink / raw)
To: Brandon Williams; +Cc: Duy Nguyen, Git Mailing List, Junio C Hamano
In-Reply-To: <20161209230029.GF88637@google.com>
On Fri, Dec 9, 2016 at 3:00 PM, Brandon Williams <bmwill@google.com> wrote:
> On 12/08, Stefan Beller wrote:
>> On Thu, Dec 8, 2016 at 2:09 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> > On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller <sbeller@google.com> wrote:
>> >>
>> >> worktree = xcalloc(1, sizeof(*worktree));
>> >> worktree->path = strbuf_detach(&worktree_path, NULL);
>> >> @@ -101,7 +101,8 @@ static struct worktree *get_main_worktree(void)
>> >
>> > All the good stuff is outside context lines again.. Somewhere between
>> > here we call add_head_info() which calls resolve_ref_unsafe(), which
>> > always uses data from current repo, not the submodule we want it to
>> > look at.
>>
>> Unrelated side question: What would you think of "variable context line
>> configuration" ? e.g. you could configure it to include anything from
>> up that line
>> that is currently shown after the @@ which is the function signature line.
>>
>> As to the add_head_info/resolve_ref_unsafe what impact does that have?
>> It produces a wrong head info but AFAICT it will never die(), such that for the
>> purposes of this series (which only wants to know if a submodule uses the
>> worktree feature) it should be fine.
>>
>> It is highly misleading though for others to build upon this.
>> So maybe I'll only add the functionality internally in worktree.c
>> and document why the values are wrong, and only expose the
>> "int submodule_uses_worktrees(const char *path)" ?
>>
>> >> @@ -209,6 +211,30 @@ struct worktree **get_worktrees(unsigned flags)
>> >> return list;
>> >
>> > Right before this line is mark_current_worktree(), which in turn calls
>> > get_git_dir() and not suitable for peeking into another repository the
>> > way submodule code does. get_worktree_git_dir() called within that
>> > function shares the same problem.
>>
>> It actually works correctly: "No submodule is the current worktree".
>>
>>
>> >
>> >> }
>> >>
>> >> +struct worktree **get_worktrees(unsigned flags)
>> >> +{
>> >> + return get_worktrees_internal(get_git_common_dir(), flags);
>> >> +}
>> >> +
>> >> +struct worktree **get_submodule_worktrees(const char *path, unsigned flags)
>> >> +{
>> >> + char *submodule_gitdir;
>> >> + struct strbuf sb = STRBUF_INIT;
>> >> + struct worktree **ret;
>> >> +
>> >> + submodule_gitdir = git_pathdup_submodule(path, "%s", "");
>> >> + if (!submodule_gitdir)
>> >> + return NULL;
>> >> +
>> >> + /* the env would be set for the superproject */
>> >> + get_common_dir_noenv(&sb, submodule_gitdir);
>> >
>> > Technically we need to read submodule_gitdir/.config and see if we can
>> > understand core.repositoryformatversion, or find any unrecognized
>> > extensions. But the problem is not specific to this code. And fixing
>> > it is no small task. But perhaps we could call a dummy
>> > validate_submodule_gitdir() here? Then when we implement that function
>> > for real, we don't have to search the entire code base to see where to
>> > put it.
>> >
>> > Kinda off-topic though. Feel free to ignore the above comment.
>>
>> ok I'll add a TODO/emptyfunction for that.
>
> So is using resolve_gitdir not ok when trying to see if a submodule has
> a gitdir at a particular path?
>
Well it depends on the question that you are trying to answer:
Assume we introduce a new repository format in Git version 3.0.
By some means the submodule repository is converted to the new format,
but the superproject is not. (e.g. it is auto migrating repositories that have
a lot of large blobs/files)
Now for some reason you use the older git 2.X in the superproject.
(e.g. the code is on a shared network mount, or git3 has a bug, or ...)
Then the code above may tell that the submodule doesn't use worktrees
(as we cannot make any assumptions on the new crazy repository format),
but in fact it is, so in this case we technically we would need to:
1) read the config
2) check the repository format version (if larger than we know of,
assume the worst or just die?)
As the function in this patch is used for safe guarding we may want to be
extra cautious, another function that is using resolve_gitdir may have other
assumptions on what is ok even for newer repository formats.
^ permalink raw reply
* Re: [PATCH] describe: add tests for unusual graphs
From: Junio C Hamano @ 2016-12-09 23:12 UTC (permalink / raw)
To: Quinn Grier; +Cc: git
In-Reply-To: <20161209131141.3096-1-quinn@quinngrier.com>
Quinn Grier <quinn@quinngrier.com> writes:
> git describe may give incorrect results if there are backdated commits
> or multiple roots. This commit adds two test_expect_failure tests that
> demonstrate these problems.
I am not sure if this is a good patch to take. test_expect_failure
is to demonstrate an incorrect behaviour that we wish to correct
later, but I do not think these demonstrate incorrect behaviours to
begin with.
For example, the latter one seems to expect that by asking to
describe D in this picture
> +#
> +# A---B*--D master
> +# /
> +# C* other
> +#
you seem to expect the description is based on B.
It is not at all clear why it is incorrect if the description were
made based on C. If D were described relative to A, ignoring B,
then I understand why that result is incorrect and I would agree
that describing D in terms of B is more correct. But I do not think
that is what the test is trying to demonstrate.
But it is hard to guess only from looking at the test and the
proposed log message, because it does not say what makes you think
the behaviour you saw was incorrect.
^ permalink raw reply
* Re: [PATCH 0/4] doc: fixes to gitcore-tutorial.txt
From: Junio C Hamano @ 2016-12-09 23:13 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <20161209155112.2112-1-kristoffer.haugsbakk@gmail.com>
Kristoffer Haugsbakk <kristoffer.haugsbakk@gmail.com> writes:
> This series of patches attempts to fix some minor mistakes in
> gitcore-tutorial.txt that I found while reading it. They are all
> concerned with grammar and things like accidentally omitted words.
Grammar is not my forte, so even though I'll queue them as-is
because I didn't spot anything wrong in them and thought they are
all improvements, I'd appreciate somebody else to lend an eye or two
over them.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 12/16] pathspec: create parse_long_magic function
From: Junio C Hamano @ 2016-12-09 23:44 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, pclouds
In-Reply-To: <1481223550-65277-13-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> Factor out the logic responsible for parsing long magic into its own
> function. As well as hoist the prefix check logic outside of the inner
> loop as there isn't anything that needs to be done after matching
> "prefix:".
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
These refactoring changes look like they are all going in the good
direction. Stefan's :(attr:<attribute spec>)path" changes however
have severe conflicts (e.g. the topic already does something similar
to this step and calls the factored-out function eat_long_magic()).
My gut feeling is that we probably should ask Stefan's series to be
rebased on top of this series that cleans up pathspec implementation,
once it stabilizes. We could probably go the other way around, but
logically it makes more sense to build "pathspec can also match
using attributes information" on top of a refactored codebase.
Thoughts?
^ permalink raw reply
* Re: [PATCH v2 12/16] pathspec: create parse_long_magic function
From: Stefan Beller @ 2016-12-09 23:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, git@vger.kernel.org, Duy Nguyen
In-Reply-To: <xmqqwpf8pt0g.fsf@gitster.mtv.corp.google.com>
On Fri, Dec 9, 2016 at 3:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Williams <bmwill@google.com> writes:
>
>> Factor out the logic responsible for parsing long magic into its own
>> function. As well as hoist the prefix check logic outside of the inner
>> loop as there isn't anything that needs to be done after matching
>> "prefix:".
>>
>> Signed-off-by: Brandon Williams <bmwill@google.com>
>
> These refactoring changes look like they are all going in the good
> direction. Stefan's :(attr:<attribute spec>)path" changes however
> have severe conflicts (e.g. the topic already does something similar
> to this step and calls the factored-out function eat_long_magic()).
>
> My gut feeling is that we probably should ask Stefan's series to be
> rebased on top of this series that cleans up pathspec implementation,
> once it stabilizes.
Very much so.
Jonathan Nieder mentioned off list that he prefers to see that
series rerolled without mutexes if possible. That is possible by
creating the questions "struct attr_check" before preloading the
index and then using the read only questions in the threaded code,
to obtain answers fast; also no need for a mutex.
I did not look into that yet, though. So I think you could discard that
series (again) until I find time to either redo the series or
resend it with a proper explanation on why the approach above
is not feasible.
> We could probably go the other way around, but
> logically it makes more sense to build "pathspec can also match
> using attributes information" on top of a refactored codebase.
>
> Thoughts?
Please let the refactoring in in favor of the attr series.
^ permalink raw reply
* Re: [RFC PATCH] send-email: allow a custom hook to prevent sending email
From: Junio C Hamano @ 2016-12-09 23:52 UTC (permalink / raw)
To: Stefan Beller; +Cc: Brandon Williams, Jeff King, git@vger.kernel.org
In-Reply-To: <CAGZ79kaqw=XqrNF5+Ta8CwcD7FyA853UQUdMxHmBAaMHPMHrXg@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> So you are suggesting to
> * have the check later in the game (e.g. just after asking
> "Send this email? ([y]es|[n]o|[q]uit|[a]ll): " as then other information
> such as additional @to @cc are available.
Yeah, probably before the loop starts asking that question for each
message. And hook does not necessarily need to cause the program to
die. The question can be reworded to "Your hook says no, but do you
really want to send it?",
^ permalink raw reply
* Re: [RFC PATCH] send-email: allow a custom hook to prevent sending email
From: Stefan Beller @ 2016-12-09 23:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, Jeff King, git@vger.kernel.org
In-Reply-To: <xmqqshpwpsn8.fsf@gitster.mtv.corp.google.com>
On Fri, Dec 9, 2016 at 3:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> So you are suggesting to
>> * have the check later in the game (e.g. just after asking
>> "Send this email? ([y]es|[n]o|[q]uit|[a]ll): " as then other information
>> such as additional @to @cc are available.
>
> Yeah, probably before the loop starts asking that question for each
> message. And hook does not necessarily need to cause the program to
> die. The question can be reworded to "Your hook says no, but do you
> really want to send it?",
You could, but that would be inconsistent with the "*** SUBJECT ***"
treatment, which currently dies. That could also ask "do you really want
to send out an unfinished series" and continue if the user wants.
I assume we want to be consistent with the existing UI and just ask the
user to use force instead?
^ permalink raw reply
* Re: [PATCH 2/3] difftool: chdir as early as possible
From: David Aguilar @ 2016-12-10 0:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git ML
In-Reply-To: <xmqqbmwkr9ji.fsf@gitster.mtv.corp.google.com>
On Fri, Dec 09, 2016 at 03:02:09PM -0800, Junio C Hamano wrote:
> David Aguilar <davvid@gmail.com> writes:
>
> > @@ -182,10 +188,6 @@ EOF
> > }
> > }
> >
> > - # Go to the root of the worktree so that the left index files
> > - # are properly setup -- the index is toplevel-relative.
> > - chdir($workdir);
> > -
> > # Setup temp directories
> > my $tmpdir = tempdir('git-difftool.XXXXX', CLEANUP => 0, TMPDIR => 1);
> > my $ldir = "$tmpdir/left";
>
> What codebase are you basing your work on? I do not see these
> removed four lines in my tree, so it seems that the patch is fixing
> up some other fix I do not yet have.
Sorry about that.
I forgot to mention that this is based on the patches I recently
sent, which were in the "pu" branch. The whats-cooking report
mentioned that they'll be merged to "next", so they might be
there already too.
The patch this was based upon:
difftool: fix dir-diff index creation when in a subdirectory
--
David
^ permalink raw reply
* Re: [PATCH v2 12/16] pathspec: create parse_long_magic function
From: Brandon Williams @ 2016-12-10 0:29 UTC (permalink / raw)
To: Stefan Beller; +Cc: Junio C Hamano, git@vger.kernel.org, Duy Nguyen
In-Reply-To: <CAGZ79ka0P0rKF8QH3V0jC-O19eT0oaE+fJLGifbfmm3jC_SijA@mail.gmail.com>
On 12/09, Stefan Beller wrote:
> On Fri, Dec 9, 2016 at 3:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Brandon Williams <bmwill@google.com> writes:
> >
> >> Factor out the logic responsible for parsing long magic into its own
> >> function. As well as hoist the prefix check logic outside of the inner
> >> loop as there isn't anything that needs to be done after matching
> >> "prefix:".
> >>
> >> Signed-off-by: Brandon Williams <bmwill@google.com>
> >
> > These refactoring changes look like they are all going in the good
> > direction. Stefan's :(attr:<attribute spec>)path" changes however
> > have severe conflicts (e.g. the topic already does something similar
> > to this step and calls the factored-out function eat_long_magic()).
> >
> > My gut feeling is that we probably should ask Stefan's series to be
> > rebased on top of this series that cleans up pathspec implementation,
> > once it stabilizes.
>
> Very much so.
>
> Jonathan Nieder mentioned off list that he prefers to see that
> series rerolled without mutexes if possible. That is possible by
> creating the questions "struct attr_check" before preloading the
> index and then using the read only questions in the threaded code,
> to obtain answers fast; also no need for a mutex.
>
> I did not look into that yet, though. So I think you could discard that
> series (again) until I find time to either redo the series or
> resend it with a proper explanation on why the approach above
> is not feasible.
>
> > We could probably go the other way around, but
> > logically it makes more sense to build "pathspec can also match
> > using attributes information" on top of a refactored codebase.
> >
> > Thoughts?
>
> Please let the refactoring in in favor of the attr series.
Sounds good. I only looked at your series briefly, but I'm hoping that
these refactoring changes I'm proposing make it relatively easy for you
to build on top of in the future.
--
Brandon Williams
^ permalink raw reply
* [PATCH] mergetools: fix xxdiff hotkeys
From: David Aguilar @ 2016-12-10 2:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git ML
xxdiff was using a mix of "Ctrl-<key>" and "Ctrl+<key>" hotkeys.
The dashed "-" form is not accepted by newer xxdiff versions.
Use the plus "+" form only.
Signed-off-by: David Aguilar <davvid@gmail.com>
---
This patch is based on top of da/mergetool-diff-order
mergetools/xxdiff | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/mergetools/xxdiff b/mergetools/xxdiff
index e284811ff2..ce5b8e9f29 100644
--- a/mergetools/xxdiff
+++ b/mergetools/xxdiff
@@ -1,7 +1,7 @@
diff_cmd () {
"$merge_tool_path" \
-R 'Accel.Search: "Ctrl+F"' \
- -R 'Accel.SearchForward: "Ctrl-G"' \
+ -R 'Accel.SearchForward: "Ctrl+G"' \
"$LOCAL" "$REMOTE"
}
@@ -9,15 +9,15 @@ merge_cmd () {
if $base_present
then
"$merge_tool_path" -X --show-merged-pane \
- -R 'Accel.SaveAsMerged: "Ctrl-S"' \
+ -R 'Accel.SaveAsMerged: "Ctrl+S"' \
-R 'Accel.Search: "Ctrl+F"' \
- -R 'Accel.SearchForward: "Ctrl-G"' \
+ -R 'Accel.SearchForward: "Ctrl+G"' \
--merged-file "$MERGED" "$LOCAL" "$BASE" "$REMOTE"
else
"$merge_tool_path" -X $extra \
- -R 'Accel.SaveAsMerged: "Ctrl-S"' \
+ -R 'Accel.SaveAsMerged: "Ctrl+S"' \
-R 'Accel.Search: "Ctrl+F"' \
- -R 'Accel.SearchForward: "Ctrl-G"' \
+ -R 'Accel.SearchForward: "Ctrl+G"' \
--merged-file "$MERGED" "$LOCAL" "$REMOTE"
fi
}
--
2.11.0.rc3.8.gaca8798.dirty
^ permalink raw reply related
* [PATCH 1/2] mergetools/kompare: simplify can_merge() by using "false"
From: David Aguilar @ 2016-12-10 3:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git ML
Signed-off-by: David Aguilar <davvid@gmail.com>
---
This patch builds upon da/mergetool-trust-exit-code
mergetools/kompare | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mergetools/kompare b/mergetools/kompare
index e8c0bfa678..321022500b 100644
--- a/mergetools/kompare
+++ b/mergetools/kompare
@@ -1,5 +1,5 @@
can_merge () {
- return 1
+ false
}
diff_cmd () {
--
2.11.0.27.gdeff8c7
^ permalink raw reply related
* [PATCH 2/2] mergetools/tortoisemerge: simplify can_diff() by using "false"
From: David Aguilar @ 2016-12-10 3:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git ML
In-Reply-To: <20161210032144.25503-1-davvid@gmail.com>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
This patch builds upon da/mergetool-trust-exit-code
mergetools/tortoisemerge | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mergetools/tortoisemerge b/mergetools/tortoisemerge
index d7ab666a59..9067d8a4e5 100644
--- a/mergetools/tortoisemerge
+++ b/mergetools/tortoisemerge
@@ -1,5 +1,5 @@
can_diff () {
- return 1
+ false
}
merge_cmd () {
--
2.11.0.27.gdeff8c7
^ permalink raw reply related
* Re: [PATCH] describe: add tests for unusual graphs
From: Quinn Grier @ 2016-12-10 6:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq60msr92w.fsf@gitster.mtv.corp.google.com>
On 2016-12-09 17:12, Junio C Hamano wrote:
> Quinn Grier <quinn@quinngrier.com> writes:
>
>> git describe may give incorrect results if there are backdated commits
>> or multiple roots. This commit adds two test_expect_failure tests that
>> demonstrate these problems.
>
> I am not sure if this is a good patch to take. test_expect_failure
> is to demonstrate an incorrect behaviour that we wish to correct
> later, but I do not think these demonstrate incorrect behaviours to
> begin with.
>
> For example, the latter one seems to expect that by asking to
> describe D in this picture
>
>> +#
>> +# A---B*--D master
>> +# /
>> +# C* other
>> +#
>
> you seem to expect the description is based on B.
>
> It is not at all clear why it is incorrect if the description were
> made based on C. If D were described relative to A, ignoring B,
> then I understand why that result is incorrect and I would agree
> that describing D in terms of B is more correct. But I do not think
> that is what the test is trying to demonstrate.
>
> But it is hard to guess only from looking at the test and the
> proposed log message, because it does not say what makes you think
> the behaviour you saw was incorrect.
>
I thought the behavior was incorrect because of the following paragraph
from the documentation for git describe:
If multiple tags were found during the walk then the tag
which has the fewest commits different from the input
commit-ish will be selected and output. Here fewest commits
different is defined as the number of commits which would be
shown by git log tag..input will be the smallest number of
commits possible.
^ permalink raw reply
* Re: [PATCH 2/2] mergetools/tortoisemerge: simplify can_diff() by using "false"
From: Johannes Sixt @ 2016-12-10 8:15 UTC (permalink / raw)
To: David Aguilar; +Cc: Junio C Hamano, Git ML
In-Reply-To: <20161210032144.25503-2-davvid@gmail.com>
Am 10.12.2016 um 04:21 schrieb David Aguilar:
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
> This patch builds upon da/mergetool-trust-exit-code
>
> mergetools/tortoisemerge | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mergetools/tortoisemerge b/mergetools/tortoisemerge
> index d7ab666a59..9067d8a4e5 100644
> --- a/mergetools/tortoisemerge
> +++ b/mergetools/tortoisemerge
> @@ -1,5 +1,5 @@
> can_diff () {
> - return 1
> + false
> }
Why is this a simplification?
My concern is that 'false' is not necessarily a shell built-in. Then
this is actually a pessimization.
-- Hannes
^ permalink raw reply
* Re: [BUG] Colon in remote urls
From: Jeff King @ 2016-12-10 8:26 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Klaus Ethgen, git
In-Reply-To: <88bed7c9-4d5d-45d5-5d13-6a8ae834e602@kdbg.org>
On Fri, Dec 09, 2016 at 10:32:48PM +0100, Johannes Sixt wrote:
> > + if (*p == '\\')
> > + literal = 1;
>
> There are too many systems out there that use a backslash in path names. I
> don't think it is wise to use it also as the quoting character.
Yeah, I picked it arbitrarily as the common quoting character, but I
agree it probably makes backwards compatibility (and general usability
when you have to double-backslash each instance) pretty gross on
Windows.
Most of the printable characters are going to suffer from backwards
compatibility issues. Syntactically, we could use any ASCII control code
below 0x20. Certainly our C code would be fine with that, but it seems
pretty nasty.
There's probably some crazy non-printing UTF-8 sequence we could use,
too, but I'm not sure I want to go there.
-Peff
^ permalink raw reply
* Re: Any interest in 'git merge --continue' as a command
From: Chris Packham @ 2016-12-10 8:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, GIT
In-Reply-To: <xmqqshpwrjyz.fsf@gitster.mtv.corp.google.com>
On Sat, Dec 10, 2016 at 8:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>>> They knew about git rebase --continue (and git am and git cherry-pick)
>>> but they were unsure how to "continue" a merge (it didn't help that
>>> the advice saying to use 'git commit' was scrolling off the top of the
>>> terminal). I know that using 'git commit' has been the standard way to
>>> complete a merge but given other commands have a --continue should
>>> merge have it as well?
>>
>> It seems like that would be in line with 35d2fffdb (Provide 'git merge
>> --abort' as a synonym to 'git reset --merge', 2010-11-09), whose stated
>> goal was providing consistency with other multi-command operations.
>>
>> I assume it would _just_ run a vanilla "git commit", and not try to do
>> any trickery with updating the index (which could be disastrous).
>
> If we were to have "merge --continue", I agree that it would be the
> logical implementation.
>
> There is nothing to "continue" in a stopped merge where Git asked
> for help from the user, and because of that, I view the final "git
> commit" as "concluding the merge", not "continuing". "continue"
> makes quite a lot of sense with rebase and cherry-pick A..B that
> stopped; it concludes the current step and let it continue to
> process the remainder. So from that point of view, it somewhat
> feels strange to call it "merge --continue", but it probably is just
> me.
>
Yeah I did think that --continue wasn't quite the right word. git
merge --conclude would probably be the most accurate.
^ permalink raw reply
* Re: [BUG] Colon in remote urls
From: Jeff King @ 2016-12-10 8:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Klaus Ethgen, git
In-Reply-To: <xmqqwpf8rkeq.fsf@gitster.mtv.corp.google.com>
On Fri, Dec 09, 2016 at 11:07:25AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > (One other option is to just declare that the quarantine feature doesn't
> > work with colons in the pathname, but stop turning it on by default. I'm
> > not sure I like that, though).
>
> I think we long time ago in 2005 have declared that a colon in a
> directory name would not work for Git repositories because of things
> like GIT_CEILING_DIRECTORIES, GIT_ALTERNATE_OBJECT_DIRECTORIES; so I
> do not think we terribly mind that direction.
I don't mind declaring the feature incompatible. But I'm not sure
whether I like not having it on by default, if only because it means we
have two distinct code paths to care about. They're sufficiently
different that I'd worry about a bug creeping into one and not the
other.
> > Here's a rough idea of what the quoting solution could look like. It
> > should make your case work, but I'm not sure what we want to do about
> > backwards-compatibility, if anything.
>
> Yes, obviously it robs from those with backslash in their pathnames
> to please those with colons; we've never catered to the latter, so I
> am not sure if the trade-off is worth it.
>
> I can see how adding a new environment variable could work, though.
> A naive way would be to add GIT_ALT_OBJ_DIRS that uses your quoting
> when splitting its elements, give it precedence over the existing
> one (or allow to use both and take union as the set of alternate
> object stores) and make sure that the codepaths we use internally
> uses the new variable.
Yeah, a new variable solves the backwards-compatibility issue, though if
we continue to use backslash as an escape, then people on Windows will
annoying _have_ to backslash-escape "C:\\" (worse, AIUI the conversion
from "/unix/path" to "C:\unix\path" happens transparently via msys
magic, and it would not know that we need to quote).
I think the least-gross alternative would be to make newline the new
delimiter. It's already the delimiter (and not quotable) in the
objects/info/alternates file, and I have a lot less trouble telling
people with newline in their filenames that they're Doing It Wrong.
> But if the quarantine codepath will stay to
> be the only user of this mechanism (and I strongly suspect that will
> be the case), the new environment could just be pointing at a single
> directory, i.e. GIT_OBJECT_QUARANTINE_DIRECTORY, whose value is
> added without splitting to the list of alternate object stores, and
> the quarantine codepath can export that instead.
Yes, I also considered that approach. The big downside is that it does
not help other users of GIT_ALTERNATE_OBJECT_DIRECTORIES. I doubt that
matters much in practice, though.
My other question is whether we care about compatibility between Git
versions here. If receive-pack sets the variable, we can assume that
index-pack will be run from the same version. But we also run hooks with
the quarantine variables set. The nice thing about the existing scheme
is that older versions of Git (or alternate implementations of Git) will
just work, because it builds on a mechanism that has existed for ages.
And that's the one thing that a quoting scheme has going for it: it only
impacts the variable when there is something to be quoted. So if your
repo path has a colon in it (or semi-colon on Windows) _and_ you are
calling something like jgit from your hook, then you might see a
failure. But either of those by itself is not a problem.
Side note: It makes me wonder if all implementations even support
GIT_ALTERNATE_OBJECT_DIRECTORIES. JGit seems to, looks like libgit2 does
not. The most backwards-compatible thing is not enabling quarantine by
default, and then there's no chance of regression, and you are
responsible for making sure you have a compatible setup before turning
the feature on. But like I said, I'd love to avoid that if we can.
So here's the array of options, as I see it:
1. Disable quarantine by default; only turn it on if you don't have
any of the funny corner cases.
2. Introduce an extra single-item environment variable that gets
appended to the list of alternates, and side-steps quoting issues.
3. Introduce a new variable that can hold an alternate list, and
either teach it reasonable quoting semantics, or give it a
less-common separator.
4. Introduce quoting to the existing variable, but make the quoting
character sufficiently obscure that nobody cares about the lack of
backwards compatibility.
I actually think (4) is reasonably elegant, except that the resulting
quoting scheme would be vaguely insane to look at. E.g., if we pick
newline as the quote character (not the separator), then you end up
with:
$ repo=foo:bar.git
$ GIT_ALTERNATE_OBJECT_DIRECTORIES=$(echo $repo | perl -pe 's/:/\n$&/')
$ echo "$GIT_ALTERNATE_OBJECT_DIRECTORIES"
foo
:bar.git
It's pleasant for machines, but not for humans.
So I dunno. Opinions on those 4 options are welcome.
-Peff
^ permalink raw reply
* Re: git add -p with new file
From: Jeff King @ 2016-12-10 8:55 UTC (permalink / raw)
To: Ariel; +Cc: git
In-Reply-To: <alpine.DEB.2.11.1612091331170.13185@cherryberry.dsgml.com>
On Fri, Dec 09, 2016 at 01:43:24PM -0500, Ariel wrote:
> > It's contrary to the rest of git-add for specifying pathspecs to
> > actually make things _more_ inclusive rather than less.
>
> Is it? Because git add without -p is happy to add new files.
I was just speaking there of whether the presence of the file on the
command-line was relevant. In other words, "git add -u untracked-file"
does not countermand the "-u" to say "also add this file".
> > Historically "add -p" has been more like "add -u" in updating tracked
> > files.
>
> But it doesn't have to be that way. You could make add -p identical to add
> without options, except the -p prompts to review diffs first.
The question is whether you would annoy people using "-p" if you started
including untracked files by default. I agree because it's inherently an
interactive process that we can be looser with backwards compatibility.
Perhaps a config option would be the best path forward (even if we were
to switch the default to "true", it leaves an escape hatch for people
who do not like the new behavior).
> > We have "-A" for "update everything _and_ new files". It doesn't
> > seem unreasonable to me to have a variant of "-p" that is similar.
>
> That seems unnecessarily complex because -p asks about each file, so you
> will never find new files added without realizing it.
If you care about adding new files, wouldn't you just always use "-P"
instead of "-p"?
-Peff
^ permalink raw reply
* Re: Any interest in 'git merge --continue' as a command
From: Jeff King @ 2016-12-10 8:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Chris Packham, GIT
In-Reply-To: <xmqqshpwrjyz.fsf@gitster.mtv.corp.google.com>
On Fri, Dec 09, 2016 at 11:16:52AM -0800, Junio C Hamano wrote:
> > It seems like that would be in line with 35d2fffdb (Provide 'git merge
> > --abort' as a synonym to 'git reset --merge', 2010-11-09), whose stated
> > goal was providing consistency with other multi-command operations.
> >
> > I assume it would _just_ run a vanilla "git commit", and not try to do
> > any trickery with updating the index (which could be disastrous).
>
> If we were to have "merge --continue", I agree that it would be the
> logical implementation.
>
> There is nothing to "continue" in a stopped merge where Git asked
> for help from the user, and because of that, I view the final "git
> commit" as "concluding the merge", not "continuing". "continue"
> makes quite a lot of sense with rebase and cherry-pick A..B that
> stopped; it concludes the current step and let it continue to
> process the remainder. So from that point of view, it somewhat
> feels strange to call it "merge --continue", but it probably is just
> me.
No, I think your reasoning makes sense. But I also think we've already
choosen to have "--continue" mean "conclude the current, and continue if
there is anything left" in other contexts (e.g., a single-item
cherry-pick). It's more vague, but I think it keeps the user's mental
model simpler if we provide a standard set of options for multi-step
commands (e.g., always "--continue/--abort/--skip", though there are
some like merge that omit "--skip" if it does not make sense).
-Peff
^ permalink raw reply
* Re: Any interest in 'git merge --continue' as a command
From: Jeff King @ 2016-12-10 9:00 UTC (permalink / raw)
To: Chris Packham; +Cc: Junio C Hamano, GIT
In-Reply-To: <CAFOYHZAsU_gNb=_K=iMFKFdt60SJ4Wm=Ag5=XMXuQgxNxCqWLA@mail.gmail.com>
On Sat, Dec 10, 2016 at 09:49:13PM +1300, Chris Packham wrote:
> > There is nothing to "continue" in a stopped merge where Git asked
> > for help from the user, and because of that, I view the final "git
> > commit" as "concluding the merge", not "continuing". "continue"
> > makes quite a lot of sense with rebase and cherry-pick A..B that
> > stopped; it concludes the current step and let it continue to
> > process the remainder. So from that point of view, it somewhat
> > feels strange to call it "merge --continue", but it probably is just
> > me.
>
> Yeah I did think that --continue wasn't quite the right word. git
> merge --conclude would probably be the most accurate.
I'd be against giving it a subtly-different name. It's just going to
frustrate people who cannot remember when to use "--conclude" and when
it is "--continue". The strength of the proposal, IMHO, is that it
abstracts the idea of "go on to the next thing or finish" across many
commands.
-Peff
^ permalink raw reply
* Re: [RFC PATCH] send-email: allow a custom hook to prevent sending email
From: Jeff King @ 2016-12-10 9:13 UTC (permalink / raw)
To: Stefan Beller; +Cc: bmwill, git
In-Reply-To: <20161209203449.17940-1-sbeller@google.com>
On Fri, Dec 09, 2016 at 12:34:49PM -0800, Stefan Beller wrote:
> My first perl contribution to Git. :)
Yes, I have some style suggestions below. :)
> Marked as RFC to gauge general interest before writing tests and
> documentation.
It's hard to evaluate without seeing an example of what you'd actually
want to put into a hook.
> diff --git a/git-send-email.perl b/git-send-email.perl
> index da81be40cb..d3ebf666c3 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -815,6 +815,15 @@ if (!$force) {
> . "Pass --force if you really want to send.\n";
> }
> }
> + my @hook = ( $ENV{GIT_DIR}.'hooks/send-email', $f )
It's awkward to use a list here, when you just end up accessing
$hook[0]. You can form the list on the fly when you call system(). You
also probably are missing a trailing "/".
I'm not sure that $GIT_DIR is consistently set; you probably want to
look at "git rev-parse --git-dir" here. But I think we make a Git
repository object earlier, and you can just do:
my $hook = join('/', $repo->repo_path(), 'hooks/send-email');
Or you can just do string concatenation:
my $hook = $repo->repo_path() . '/hooks/send-email';
If I were writing repo_path myself, I'd probably allow:
my $hook = $repo->repo_path('hooks/send-email');
like we do with git_path() in the C code. It wouldn't be hard to add.
> + if( -x $hook[0] ) {
Funny whitespace. I think:
if (-x $hook)
would be more usual for us.
> + unless( system( @hook ) == 0 )
> + {
> + die "Refusing to send because the patch\n\t$f\n"
> + . "was refused by the send-email hook."
> + . "Pass --force if you really want to send.\n";
> + }
I like "unless" as a trailing one-liner conditional for edge cases,
like:
do_this($foo) unless $foo < 5;
but I think here it just makes things more Perl-ish than our code base
usually goes for. Probably:
if (system($hook, $f) != 0) {
die ...
}
would be more usual for us (in a more Perl-ish code base I'd probably
write "system(...) and die ...").
-Peff
^ permalink raw reply
* Re: [BUG] Colon in remote urls
From: Klaus Ethgen @ 2016-12-10 9:29 UTC (permalink / raw)
To: git
In-Reply-To: <xmqqwpf8rkeq.fsf@gitster.mtv.corp.google.com>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
Hello,
Am Fr den 9. Dez 2016 um 20:07 schrieb Junio C Hamano:
> Jeff King <peff@peff.net> writes:
> > (One other option is to just declare that the quarantine feature doesn't
> > work with colons in the pathname, but stop turning it on by default. I'm
> > not sure I like that, though).
>
> I think we long time ago in 2005 have declared that a colon in a
> directory name would not work for Git repositories because of things
> like GIT_CEILING_DIRECTORIES, GIT_ALTERNATE_OBJECT_DIRECTORIES; so I
> do not think we terribly mind that direction.
That is the first I hear and I really wonder about.
A colon a perfectly allowed character in POSIX filesystems.
Moreover, it was no problem before and was introduced as a problem just
in that version. Even more, a pull (and so a clone I believe) of such a
path is absolutely ok. Just the push fails.
> > Here's a rough idea of what the quoting solution could look like. It
> > should make your case work, but I'm not sure what we want to do about
> > backwards-compatibility, if anything.
>
> Yes, obviously it robs from those with backslash in their pathnames
> to please those with colons; we've never catered to the latter, so I
> am not sure if the trade-off is worth it.
As I quote above, a colon is perfect common in POSIX filesystems. A
backslash is at least uncommon and always needed to quote as it, most
often, has special meaning to os/shell.
I cannot see why a tool (as git is) should decide what characters are
"bad" and what are "good". If the filesystem beneath supports it...
By the way, I didn't find anywhere in git documentation that there are
"bad" chars around.
> I can see how adding a new environment variable could work, though.
> A naive way would be to add GIT_ALT_OBJ_DIRS that uses your quoting
> when splitting its elements, give it precedence over the existing
> one (or allow to use both and take union as the set of alternate
> object stores) and make sure that the codepaths we use internally
> uses the new variable. But if the quarantine codepath will stay to
> be the only user of this mechanism (and I strongly suspect that will
> be the case), the new environment could just be pointing at a single
> directory, i.e. GIT_OBJECT_QUARANTINE_DIRECTORY, whose value is
> added without splitting to the list of alternate object stores, and
> the quarantine codepath can export that instead.
I didn't get it, why is there a need to split? I mean, it is not
possible to push to two locations at the same time, so why is there
splitting at all?
Regards
Klaus
- --
Klaus Ethgen http://www.ethgen.ch/
pub 4096R/4E20AF1C 2011-05-16 Klaus Ethgen <Klaus@Ethgen.ch>
Fingerprint: 85D4 CA42 952C 949B 1753 62B3 79D0 B06F 4E20 AF1C
-----BEGIN PGP SIGNATURE-----
Comment: Charset: ISO-8859-1
iQGzBAEBCgAdFiEEMWF28vh4/UMJJLQEpnwKsYAZ9qwFAlhLyvIACgkQpnwKsYAZ
9qyBzgv9EzMEWrEgsTd1Z0gjpzpJlhpO8R2I7H4mGvEjlxoTXtUNwjvM+ojAYzaI
F34IBRv9BCha+h7I6ccoaAsfmurz73lA1AKy1IFPrYAoxompYLomC9exY+8+ggdN
G2uVbMTmiL+CxJGo0ItxmiQCcv7himVoyto70Dekc7se+panbzCq/vG4+Rz7pwRn
sWnlKs0tQomi6QXbibo8992v4ECkAXzE2Xc/l5DvosSwNNPsqgdeeiNHEMDTbQq8
jqencfOruCfyMnQ0ejCaTWNbYY5SVUtfWikwta12jB06D1BgHTCRVKZCfhoHJx5+
Ffqj8uuiCJuZGQcopzJWiYU5X+SUHz7Ya+iA3VQOxNEKsGAZgq6QQqDcd0y9fyPt
pzMAYo26GRioDiuQZzZ4CzA5eC0wWozv3oESsKLD5RsbWHV/9ODbpr7lHMW2TmUp
H2vZhre1K/ZX2bODQByJoRNtDUqKvZmI6GsbXrvRAFKF4aCLByFIgcrZprmj++DH
jlb25tjq
=jOGb
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [BUG] Colon in remote urls
From: Klaus Ethgen @ 2016-12-10 9:32 UTC (permalink / raw)
To: git
In-Reply-To: <88bed7c9-4d5d-45d5-5d13-6a8ae834e602@kdbg.org>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
Am Fr den 9. Dez 2016 um 22:32 schrieb Johannes Sixt:
> There are too many systems out there that use a backslash in path names. I
> don't think it is wise to use it also as the quoting character.
Well, the minority, I believe. And only the minority where the command
line git is used anywhere.
But for the majority of OS, where the command line git is in use
(instead of graphically third party git tools) is perfect known for
backslash as escaping character. However, don't forget that a backslash
could also be part of the file name.
Regads
Klaus
- --
Klaus Ethgen http://www.ethgen.ch/
pub 4096R/4E20AF1C 2011-05-16 Klaus Ethgen <Klaus@Ethgen.ch>
Fingerprint: 85D4 CA42 952C 949B 1753 62B3 79D0 B06F 4E20 AF1C
-----BEGIN PGP SIGNATURE-----
Comment: Charset: ISO-8859-1
iQGzBAEBCgAdFiEEMWF28vh4/UMJJLQEpnwKsYAZ9qwFAlhLy64ACgkQpnwKsYAZ
9qzYfQwAmVIR+bVOvOcZ+yu7HddC5mwo7st6w+vPcdLKpFWcHIhsG//cHq6he+mm
/Dmfhnc4Yp+dSy7Z99p9DV67hAj0Zxj2koxBo4eCdwWhnKrphCHSST8j2IxIg/0h
Y1axQEBc5hV9nImTYmOks0pa5c9wKkZS36aTjP63PKgIv46A8RDY/QXAm2uO4kjj
gfBEiDVkQA99QlvpP1qbuRCK3QUmfqxrP9ldiAhmuDBbNv2smiBxhoN3E6NgKTJG
fM6WjaZPUqpDUiky5gO0xfOpf6s2c/GMTEO1I5aWom6VKtrOoYUyMlyeMiqALWj7
KfN7TJfDqp3THo0AkLXuukrNMv9gdfgiAimGqYgSfFM9P60aPjGeC7nBt875PSae
jdVjIGyl0tHZzSIbBoSecQqx8h65eZaHLaS1uNf704m+EwIs1X2daI7Re3DcAyzL
EWJxtZyZbG/GooCdA9deywlEAtGNOdIg+p+YkThsmEGiaOir/fAMyviSP/jONTCq
OC5oHOcK
=x9y4
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [BUG] Colon in remote urls
From: Klaus Ethgen @ 2016-12-10 9:41 UTC (permalink / raw)
To: git
In-Reply-To: <20161210082657.zjp52a2zdtqifmg3@sigill.intra.peff.net>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
Am Sa den 10. Dez 2016 um 9:26 schrieb Jeff King:
> Yeah, I picked it arbitrarily as the common quoting character, but I
> agree it probably makes backwards compatibility (and general usability
> when you have to double-backslash each instance) pretty gross on
> Windows.
Well, I don't know of many people using the original git on windows.
Most of them using some graphical third-party tools.
The main git suite is most often used on linux where a colon is a valid
character For example using /mnt/c: as mount path for windows file
systems or /bla/foo/dated_repository_2016-12-10_12:00.git for dated and
timed repositories.
My btrfs snapshot dir looks like:
~snapshot> l -gGhN
[...]
drwxr-x--x 1 296 2016-07-30T13:55 daily_2016-12-10_00:00:01.270213478
drwxr-x--x 1 296 2016-07-30T13:55 hourly_2016-12-10_05:00:01.372037552
[...]
Compared to the backslash, although it is a perfect legal character in
POSIX file systems, I do not know any use of it.
Regards
Klaus
- --
Klaus Ethgen http://www.ethgen.ch/
pub 4096R/4E20AF1C 2011-05-16 Klaus Ethgen <Klaus@Ethgen.ch>
Fingerprint: 85D4 CA42 952C 949B 1753 62B3 79D0 B06F 4E20 AF1C
-----BEGIN PGP SIGNATURE-----
Comment: Charset: ISO-8859-1
iQGzBAEBCgAdFiEEMWF28vh4/UMJJLQEpnwKsYAZ9qwFAlhLzc0ACgkQpnwKsYAZ
9qy1jQv/Wcafo8nJuy/dNIpxN5tNaLEENrY6a2dkv379F2miEJYROlWO6UzG86hY
0WIZAm5BKK6SpPVztTMcs2GHPF0iCB4V4RyQFdFa73OhaAgHOJRdy50eaGSz6vt6
lDZkJZsG0FoXcT6Fapdl5xZeoNDXjPcYH/7yFQ7VjMD5HTpLDIs8E5Mb8V1jwehV
JKzQd136vksS2qB96jElAYonXFwImvYfTplH3nELJh/kKRJOT8Mzgj/+X7vxnQcC
NISiLysSxqPm5d9yDsfN1eofMNGn2zgJZStOP6jNV2yqldMgN0fJX4Mt449GpBO8
OSYjN828QsDYXCWdTCKxbLCxjfNxfvQgHHR7ugSlf9xPrro3MjQjg2cMhZ/fCzCm
XcC4X+Iyec2F0wHSQiXqlb7wiOXa1Oup6zmTRe/G5HkhlCap/+R2nOCfkqEEwhkB
moYTqfETqqTJUJiiYVM/U8LBFWGnBBCGWgRPzyNdFna+WnvD93s9JPeg7q9qFm6x
8flMJBm8
=M5IW
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines
From: Vasco Almeida @ 2016-12-10 10:08 UTC (permalink / raw)
To: Junio C Hamano, Johannes Schindelin
Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
Jean-Noël AVILA, Jakub Narębski, David Aguilar
In-Reply-To: <xmqqk2b8rbbb.fsf@gitster.mtv.corp.google.com>
A Sex, 09-12-2016 às 14:23 -0800, Junio C Hamano escreveu:
> > This is exactly the same issue I fixed for rebase -i recently.
>
> Yes, but the patch we see here punts "core.commentChar is not a
> single-byte single-letter--panic!" case differently. I think you
> did "just take the first one" in "rebase -i", which I think is more
> in line with the rest of the system, and this addition to Git.pm
> should do the same, I think.
I hope the changes below are in line with the rest of the system. If
so, I will send a new re-roll with them.
I wonder why this is important when Git errors out when
core.commentChar is set to more than 1 characters or 0 characters. Is
it just to be consistent with "rebase -i" changes introduced
by Johannes Schindelin?
I am not sure what does "if (length($comment_line_char) != 1)" check.
Whether it checks single-byte or single-letter or both...
-- >8 --
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 3a6d846..4e0ab5a 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1072,7 +1072,7 @@ sub edit_hunk_manually {
print $fh @$oldtext;
my $is_reverse = $patch_mode_flavour{IS_REVERSE};
my ($remove_plus, $remove_minus) = $is_reverse ? ('-', '+') :
('+', '-');
- my $comment_line_char = Git::config("core.commentchar") ||
'#';
+ my $comment_line_char = Git::get_comment_line_char;
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..3211650 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -1451,6 +1451,23 @@ sub prefix_lines {
return $string;
}
+=item get_comment_line_char ( )
+
+Gets the core.commentchar configuration value.
+The value falls-back to # if core.commentchar is set to 'auto'.
+
+=cut
+
+sub get_comment_line_char {
+ my $comment_line_char = config("core.commentchar") || '#';
+ $comment_line_char = '#' if ($comment_line_char eq 'auto');
+ if (length($comment_line_char) != 1) {
+ # use first character
+ $comment_line_char = substr($comment_line_char, 0, 1);
+ }
+ return $comment_line_char;
+}
+
=item comment_lines ( STRING [, STRING... ])
Comments lines following core.commentchar configuration.
@@ -1458,7 +1475,7 @@ Comments lines following core.commentchar
configuration.
=cut
sub comment_lines {
- my $comment_line_char = config("core.commentchar") || '#';
+ my $comment_line_char = get_comment_line_char;
return prefix_lines("$comment_line_char ", @_);
}
^ permalink raw reply related
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