* Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines
From: Vasco Almeida @ 2016-12-09 17:19 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
Jean-Noël AVILA, Jakub Narębski, David Aguilar
In-Reply-To: <xmqqoa17quls.fsf@gitster.mtv.corp.google.com>
A Ter, 22-11-2016 às 09:42 -0800, Junio C Hamano escreveu:
> The incremental update below looks sensible. We'd also want to
> protect this codepath from a misconfigured two-or-more byte sequence
> in core.commentchar, I would suspect, to be consistent.
Are the below changes alright for what you propose? It just checks if
the length of core.commentchar's value is 1, otherwise use '#' as the
comment_line_char.
As a note, when I set core.commentchar with "git config
core.commentChar 'batata'", I get the following error message when I
issue "git add -i":
error: core.commentChar should only be one character
fatal: bad config variable 'core.commentchar' in file '.git/config' at line 6
-- >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..e4da913 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -1451,6 +1451,20 @@ sub prefix_lines {
return $string;
}
+=item get_comment_line_char ( )
+
+Gets the core.commentchar configuration value.
+The value fallbacks 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');
+ $comment_line_char = '#' if (length($comment_line_char) != 1);
+ return $comment_line_char;
+}
+
=item comment_lines ( STRING [, STRING... ])
Comments lines following core.commentchar configuration.
@@ -1458,7 +1472,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
* Re: [PATCH/RFC 0/7] Pie-in-the-sky attempt to fix the early config
From: Johannes Schindelin @ 2016-12-09 17:28 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20161208172626.3ee2nmgsxsh2vovf@sigill.intra.peff.net>
Hi Peff,
On Thu, 8 Dec 2016, Jeff King wrote:
> On Thu, Dec 08, 2016 at 04:35:56PM +0100, Johannes Schindelin wrote:
>
> > The idea here is to discover the .git/ directory gently (i.e. without
> > changing the current working directory), and to use it to read the
> > .git/config file early, before we actually called setup_git_directory()
> > (if we ever do that).
>
> Great. Thanks for taking a stab at this.
Well, I figured that I can go through you to get this integrated into
git.git.
> > Notes:
> >
> > - I find the diff pretty ugly: I wish there was a more elegant way to
> > *disable* discovery of .git/ *just* for `init` and `clone`. I
> > considered a function `about_to_create_git_dir()` that is called in a
> > hard-coded manner *only* for `init` and `clone`, but that would
> > introduce another magic side effect, when all I want is to reduce those.
>
> It looks like a lot of that ugliness comes from passing around the "are
> we OK to peek at git-dir config" flag through the various pager-related
> calls.
Correct.
> I don't think it would be bad to use a global for "we do not want a
> repo".
I would think it would be bad, as the entire reason for this patch series
is that we have global state that gets messed up too early (I am speaking
from the point of view of somebody who patched Git locally so that it does
read config variables *before* launching builtins).
> After all, it's just modifying the _existing_ global for "are we in a
> repo".
No it does not.
The read_early_config() function specifically does not leave any traces in
the global namespace. It calls the git_config_with_options() function
without touching the_config_set.
Of course we could introduce a new config set, just for early use. It
would share all the downsides with the current config set.
I would look more favorably on this idea if we were to teach
the_config_set to record a little bit more about the state from which it
was constructed, and to auto-flush-and-re-read when it detects that, say,
git_dir was changed in the meantime.
> And I do not see that global going away anytime soon.
And that is really, really sad.
> Sometimes it's good to make incremental steps towards an end goal, but
> in this case it seems like we just pay the cost of the step without any
> real plan for reaping the benefit in the long term.
Fine. Let's roll with this for now, then.
I just hope that we do not repeat the "slam the door shut to a better
solution" mistake that led to this situation, as the chdir() way did.
> As an alternative, I also think it would be OK to just always have the
> pager config read from local repo, even for init/clone.
For the purpose of this current discussion, I am utterly uninterested in
the pager config. What I want to use the early config for is substantially
different and more relevant: I need to configure some command to run
before, and after, every single Git command here. And this configuration
needs to be per-repository. And no, I do not want to hardcode anything.
So now the cat is out of the bag, I have ulterior motives.
These motives serve a greater good, though: designing read_early_config()
around a very specific use case will always fall short of a well-designed
read_early_config() that could then be used for any use case that
requires, well, reading the config early.
> In other words, to loosen the idea that git-init can _never_ look in the
> current git-dir, and declare that there is a stage before the command is
> initiated, and during which git may read local-repo config. Aliases
> would fall into this, too, so:
>
> git config --local alias.foo init
> git foo /some/other/dir
>
> would work (as it must, because we cannot know that "foo" is "init"
> until we read the config!).
True.
But is this a good excuse to just shrug our shoulders and let git-init
(which we do know very well) fall into the same trap?
> > - For the moment, I do not handle dashed invocations of `init` and
> > `clone` correctly. The real problem is the continued existence of
> > the dashed git-init and git-clone, of course.
>
> I assume you mean setting the CREATES_GIT_DIR flag here? I think it
> would apply to the dashed invocations, too. They strip off the "git-"
> and end up in handle_builtin() just like "git clone" does. I didn't test
> it, though.
You are correct. I misread the code to expect it to spawn another instance
of git.exe.
> > - There is still duplicated code. For the sake of this RFC, I did not
> > address that yet.
>
> Yeah, I did not read your discover function very carefully. Because I
> think the one thing we really don't want to do here is introduce a
> separate lookup process that is not shared by setup_git_directory(). The
> only sane path, I think, is to refactor setup_git_directory() to build
> on top of discover_git_directory(), which implies that the latter
> handles all of the cases.
This could maybe happen later, but for now I do not concern myself with
building the prefix, as the config machinery does not require that
knowledge.
If we go that route, we should of course cache the cwd, git_dir and prefix
of earlier runs, and avoid the entire discovery if we start from the same
cwd.
> > - The read_early_config() function is called multiple times, re-reading
> > all the config files and re-discovering the .git/ directory multiple
> > times, which is quite wasteful. For the sake of this RFC, I did not
> > address that yet.
>
> We already have a config-caching system. If we went with a global
> "config_discover_refs",
Why "_refs"?
> then I think the sequence for something like git-init would become:
>
> 1. When git.c starts, config_discover_refs is set to "true". Pager and
> alias lookup may look in .git/config if it's available, even if
> they go through the configset cache.
>
> 2. As soon as git-init starts, it disables config_discover_refs, and
> it flushes the config cache. Any configset lookups will now examine
> the reduced config.
>
> 3. When git-init has set up the real repo it is operating on, it can
> reenable config_discover_refs (though it may not even need to; that
> flag probably wouldn't have any effect once we've entered the
> repository and have_git_dir() returns true).
That is a bit fiddly, don't you think? The callers have to have very
intimate knowledge of the config reading to remember to set, and re-set,
that global. And to flush when appropriate.
How much nicer would the code be if the call to git_config() would realize
what needs to be done, don't you agree?
Less chance for bugs to be introduced by contributors with flakey
understanding of the config/git_dir machinery, myself included.
> > - t7006 fails and the error message is a bit cryptic (not to mention the
> > involved function trace, which is mind-boggling for what is supposed
> > to be a simply, shell script-based test suite). I did not have time to
> > look into that yet.
>
> Running t7006 I see a lot of old failures turned into successes, which
> is good (because running from a subdirectory now actually respects local
> pager config). The one failure looks like it is testing the wrong thing.
Yeah, but due to the redirections I was not able to figure out what to
change to "fix" this.
> It is checking that we _don't_ respect a local core.pager in some cases,
> as a proxy for whether or not we are breaking things by doing setup too
> early. But the whole point of your series is to fix that, and respect
> the config without causing the setup breakage. After your patches, the
> proxy behavior and the failure case are disconnected. The test should be
> flipped, and ideally another one added that confirms we didn't actually
> run setup_git_directory(), but I'm not sure how to test that directly.
Some dirty tricks come to mind, but I am not even sure that I want to test
for this. Why exactly do we need to avoid calling setup_git_directory() in
that case?
> > - after discover_git_directory_gently() did its work, the code happily
> > uses its result *only* for the current read_early_config() run, and
> > lets setup_git_dir_gently() do the whole work *again*. For the sake of
> > this RFC, I did not address that yet.
>
> If caching happens at the config layer, then we'd probably only call
> this once anyway (or if we did call it again after a config flush, it
> would be a good sign that we should compute its value again).
I meant the git_dir, not the config... The git_dir is discovered, but a
subsequent setup_git_dir_gently() discovers it *again*, in a different
way.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines
From: Johannes Schindelin @ 2016-12-09 17:32 UTC (permalink / raw)
To: Vasco Almeida
Cc: Junio C Hamano, git, Jiang Xin,
Ævar Arnfjörð Bjarmason, Jean-Noël AVILA,
Jakub Narębski, David Aguilar
In-Reply-To: <1481303956.4934.8.camel@sapo.pt>
[-- Attachment #1: Type: text/plain, Size: 877 bytes --]
Hi Vasco,
On Fri, 9 Dec 2016, Vasco Almeida wrote:
> A Ter, 22-11-2016 às 09:42 -0800, Junio C Hamano escreveu:
> > The incremental update below looks sensible. We'd also want to
> > protect this codepath from a misconfigured two-or-more byte sequence
> > in core.commentchar, I would suspect, to be consistent.
>
> Are the below changes alright for what you propose? It just checks if
> the length of core.commentchar's value is 1, otherwise use '#' as the
> comment_line_char.
> As a note, when I set core.commentchar with "git config
> core.commentChar 'batata'", I get the following error message when I
> issue "git add -i":
>
> error: core.commentChar should only be one character
> fatal: bad config variable 'core.commentchar' in file '.git/config' at line 6
This is exactly the same issue I fixed for rebase -i recently.
Good eyes,
Dscho
^ permalink raw reply
* Re: [PATCH/RFC 0/7] Pie-in-the-sky attempt to fix the early config
From: Jeff King @ 2016-12-09 17:55 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1612091810500.23160@virtualbox>
On Fri, Dec 09, 2016 at 06:28:10PM +0100, Johannes Schindelin wrote:
> > Great. Thanks for taking a stab at this.
>
> Well, I figured that I can go through you to get this integrated into
> git.git.
I am not sure what you mean here, but it _sounds_ like you are
continuing to be negative about the chances of fixes going into git.git
here. I really don't think that negativity is merited, but even if it
is, making snide comments does not help and mostly just makes the
conversation less pleasant.
> > I don't think it would be bad to use a global for "we do not want a
> > repo".
>
> I would think it would be bad, as the entire reason for this patch series
> is that we have global state that gets messed up too early (I am speaking
> from the point of view of somebody who patched Git locally so that it does
> read config variables *before* launching builtins).
I think those are two different things. One is global state that is
munged as a side effect of setup_git_directory(). The other is global
state that the process sets to say "this is an invariant" so it does not
have to deal with passing it through a huge call chain.
> > After all, it's just modifying the _existing_ global for "are we in a
> > repo".
>
> No it does not.
Perhaps I wasn't clear on my "it" here. I mean that we will continue to
have startup_info->have_repository as a global (and if not that, then
certainly the environment variable GIT_DIR and the cwd are process
globals). I'm proposing a global flag to modify how to interpret those
globals. I don't think that really makes anything worse.
> The read_early_config() function specifically does not leave any traces in
> the global namespace. It calls the git_config_with_options() function
> without touching the_config_set.
I'm not talking about read_early_config() modifying the global state.
I'm talking about main() modifying it so that lower-level functions like
read_early_config() can make use of it.
> I would look more favorably on this idea if we were to teach
> the_config_set to record a little bit more about the state from which it
> was constructed, and to auto-flush-and-re-read when it detects that, say,
> git_dir was changed in the meantime.
I considered that when fixing some bugs in git-init a few months ago,
but I think the cure becomes worse than the problem. Automatic cache
invalidation can have some tricky corner cases, and there really
_aren't_ that many places where we need to do it. Just dropping
git_config_clear() in those places is ugly, but practical.
> > And I do not see that global going away anytime soon.
>
> And that is really, really sad.
I think we differ on that.
> > As an alternative, I also think it would be OK to just always have the
> > pager config read from local repo, even for init/clone.
>
> For the purpose of this current discussion, I am utterly uninterested in
> the pager config. What I want to use the early config for is substantially
> different and more relevant: I need to configure some command to run
> before, and after, every single Git command here. And this configuration
> needs to be per-repository. And no, I do not want to hardcode anything.
I do not see how that changes things. If there is the concept of a
"before the command is run" phase during which we would read from the
current-directory config even for git-init, it seems to me that applies
logically to pagers, aliases, _and_ whatever pre-command magic you are
interested in adding.
> > In other words, to loosen the idea that git-init can _never_ look in the
> > current git-dir, and declare that there is a stage before the command is
> > initiated, and during which git may read local-repo config. Aliases
> > would fall into this, too, so:
> >
> > git config --local alias.foo init
> > git foo /some/other/dir
> >
> > would work (as it must, because we cannot know that "foo" is "init"
> > until we read the config!).
>
> True.
>
> But is this a good excuse to just shrug our shoulders and let git-init
> (which we do know very well) fall into the same trap?
I'm not sure I agree it is a trap. There is a consistent and reasonable
mental model where it is the natural thing. I understand that's not the
one you prefer, but it seems like a practical one to me.
> > We already have a config-caching system. If we went with a global
> > "config_discover_refs",
>
> Why "_refs"?
Er, sorry, slip of the tongue. I think I meant config_discover_git, and
for some reason managed to repeat it over and over. Hopefully you
figured out what I meant.
> > then I think the sequence for something like git-init would become:
> >
> > 1. When git.c starts, config_discover_refs is set to "true". Pager and
> > alias lookup may look in .git/config if it's available, even if
> > they go through the configset cache.
> >
> > 2. As soon as git-init starts, it disables config_discover_refs, and
> > it flushes the config cache. Any configset lookups will now examine
> > the reduced config.
> >
> > 3. When git-init has set up the real repo it is operating on, it can
> > reenable config_discover_refs (though it may not even need to; that
> > flag probably wouldn't have any effect once we've entered the
> > repository and have_git_dir() returns true).
>
> That is a bit fiddly, don't you think? The callers have to have very
> intimate knowledge of the config reading to remember to set, and re-set,
> that global. And to flush when appropriate.
Sure, but I think there are literally 2 callers who care about this.
> How much nicer would the code be if the call to git_config() would realize
> what needs to be done, don't you agree?
No, I wouldn't agree until I had seen a very elegant patch that
implements this and handles any corner cases. I looked into making a
patch before and found that it got a bit ugly (sorry, I don't really
remember the details). I am happy to have you show me that patch, but
pardon me if I remain skeptical until I see it. ;)
> > Running t7006 I see a lot of old failures turned into successes, which
> > is good (because running from a subdirectory now actually respects local
> > pager config). The one failure looks like it is testing the wrong thing.
>
> Yeah, but due to the redirections I was not able to figure out what to
> change to "fix" this.
I think test_local_config_ignored would go away, and its callers become
test_core_pager_overrides. More or less a revert of 73e25e7cc (git
--paginate: do not commit pager choice too early, 2010-06-26).
> > proxy behavior and the failure case are disconnected. The test should be
> > flipped, and ideally another one added that confirms we didn't actually
> > run setup_git_directory(), but I'm not sure how to test that directly.
>
> Some dirty tricks come to mind, but I am not even sure that I want to test
> for this. Why exactly do we need to avoid calling setup_git_directory() in
> that case?
I imagine that "git -p init /path/to/new/repo" would start from the
wrong directory, for instance (or any other builtin which does not have
RUN_SETUP would probably get confused).
> > > - after discover_git_directory_gently() did its work, the code happily
> > > uses its result *only* for the current read_early_config() run, and
> > > lets setup_git_dir_gently() do the whole work *again*. For the sake of
> > > this RFC, I did not address that yet.
> >
> > If caching happens at the config layer, then we'd probably only call
> > this once anyway (or if we did call it again after a config flush, it
> > would be a good sign that we should compute its value again).
>
> I meant the git_dir, not the config... The git_dir is discovered, but a
> subsequent setup_git_dir_gently() discovers it *again*, in a different
> way.
I don't think it's such a huge amount of work that it really matters
from an optimization standpoint. The really ugly thing to me is if it
returns a different answer. And I think the best way to deal with that
is to literally use the same code (and then if you want to cache the
result and not do the work again, it's pretty easy).
-Peff
^ permalink raw reply
* Re: Resend: Gitk: memory consumption improvements
From: Stefan Beller @ 2016-12-09 17:57 UTC (permalink / raw)
To: Markus Hitter; +Cc: git@vger.kernel.org, Paul Mackerras
In-Reply-To: <d10d2b12-4ef1-61e9-0b3c-89aa41c9eeff@jump-ing.de>
On Fri, Dec 9, 2016 at 3:51 AM, Markus Hitter <mah@jump-ing.de> wrote:
>
> It's a month now since I sent three patches to this list for reducing memory consumption of Gitk considerably:
>
> https://public-inbox.org/git/de7cd593-0c10-4e93-1681-7e123504f5d5@jump-ing.de/
> https://public-inbox.org/git/e09a5309-351d-d246-d272-f527f50ad444@jump-ing.de/
> https://public-inbox.org/git/8e1c5923-d2a6-bc77-97ab-3f154b41d2ea@jump-ing.de/
> https://public-inbox.org/git/2cb7f76f-0004-a5b6-79f1-9bb4f979cf14@jump-ing.de/
>
> Everybody cheered, but apparently nobody picked these patches up and applied them to either the Git or Gitk repository. They don't appear in the regular "what's cooking" post either.
The "What's cooking" email is done by Junio (the Git maintainer)
describing the development of Git, whereas gitk is maintained by Paul
if I understand correctly.
If you look at the Git history, the changes to gitk-git/ always come in via
a merge/pull from Paul.
>
> Anything missing? I'd like to put a 'done' checkmark behind this.
>
I'd love to see those patches in use here (via a regular upstream update).
So I guess the way to go for you is to keep bugging Paul for an ack?
Thanks,
Stefan
^ permalink raw reply
* Re: BUG: "cherry-pick A..B || git reset --hard OTHER"
From: Junio C Hamano @ 2016-12-09 18:07 UTC (permalink / raw)
To: Duy Nguyen
Cc: Stephan Beyer, Git Mailing List, Christian Couder,
SZEDER Gábor
In-Reply-To: <CACsJy8CX0HO=LxcEK3K+pCecgFY=40R+gpFoy7CGeN5zEJFJVQ@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Thu, Dec 8, 2016 at 3:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stephan Beyer <s-beyer@gmx.net> writes:
>>
>>> [1] By the way: git cherry-pick --quit, git rebase --forget ...
>>> different wording for the same thing makes things unintuitive.
>>
>> It is not too late to STOP "--forget" from getting added to "rebase"
>> and give it a better name.
>
> Having the same operation with different names only increases git
> reputation of bad/inconsistent UI. Either forget is renamed to quit,
> or vice versa. I prefer forget, but the decision is yours and the
> community's. So I'm sending two patches to rename in either direction.
> You can pick one.
I actually was advocating to remove both by making --abort saner.
With an updated --abort that behaves saner, is "rebase --forget"
still necessary?
^ permalink raw reply
* Re: git add -p with new file
From: Ariel @ 2016-12-09 18:26 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List
In-Reply-To: <CACsJy8A_YMyEUgX--1tEfJC4aaYfDbFFL8WEs6CHp4a4=mHRjw@mail.gmail.com>
On Wed, 7 Dec 2016, Duy Nguyen wrote:
> On Wed, Dec 7, 2016 at 8:18 AM, Ariel <asgit@dsgml.com> wrote:
>> If you do git add -p new_file it says:
>> No changes.
>> Which is a rather confusing message. I would expect it to show me the
>> content of the file in patch form, in the normal way that -p works, let me
>> edit it, etc.
> We could improve it a bit, suggesting the user to do git add -N. But
> is there a point of using -p on a new file?
I got into the habit of always using -p as a way of checking my diffs
before committing, so I ran it out of habit on a new file as well and got
that confusing message.
It's even worse if you run it on multiple files, some changed, some added
- the added ones are ignored completely, without any message at all.
> It will be one big chunk, you can't split anything.
That's fine, that's what I would expect.
> Perhaps maybe you want to use 'e' to edit what's added?
I might, but mainly it would show me what it was adding.
-Ariel
^ permalink raw reply
* Re: [PATCH 4/5] Make sequencer abort safer
From: Junio C Hamano @ 2016-12-09 18:33 UTC (permalink / raw)
To: Stephan Beyer
Cc: Johannes Schindelin, git, Christian Couder, SZEDER Gábor
In-Reply-To: <c02708de-8b47-e490-4a1e-77f5727b1156@gmx.net>
Stephan Beyer <s-beyer@gmx.net> writes:
> However:
>
>> -static void update_curr_file()
>> +static void update_current_file(void)
>
> This function name could lead to the impression that there is some
> current file (defined by a global state or whatever) that is updated.
>
> So I'd rather rename the *file* to one of
>
> * sequencer/abort-safety (consistent to am, describes its purpose)
> * sequencer/safety (shorter, still describes the purpose)
> * sequencer/current-head (describes what it contains)
> * sequencer/last (a four-letter word, not totally unambiguous though)
OK, so here is a patch that needs to be squashed further on top of
4/5. I just picked the first one on your list ;-)
Thanks.
sequencer.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 874aaa4cd4..3ac4cb8d3b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -27,7 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
-static GIT_PATH_FUNC(git_path_current_file, "sequencer/current")
+static GIT_PATH_FUNC(git_path_abort_safety_file, "sequencer/abort-safety")
/*
* A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
@@ -306,7 +306,7 @@ static int error_dirty_index(struct replay_opts *opts)
return -1;
}
-static void update_current_file(void)
+static void update_abort_safety_file(void)
{
struct object_id head;
@@ -315,9 +315,9 @@ static void update_current_file(void)
return;
if (!get_oid("HEAD", &head))
- write_file(git_path_current_file(), "%s", oid_to_hex(&head));
+ write_file(git_path_abort_safety_file(), "%s", oid_to_hex(&head));
else
- write_file(git_path_current_file(), "%s", "");
+ write_file(git_path_abort_safety_file(), "%s", "");
}
static int fast_forward_to(const unsigned char *to, const unsigned char *from,
@@ -349,7 +349,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
strbuf_release(&sb);
strbuf_release(&err);
ref_transaction_free(transaction);
- update_current_file();
+ update_abort_safety_file();
return 0;
}
@@ -824,7 +824,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
leave:
free_message(commit, &msg);
- update_current_file();
+ update_abort_safety_file();
return res;
}
@@ -1149,18 +1149,18 @@ static int rollback_is_safe(void)
struct strbuf sb = STRBUF_INIT;
struct object_id expected_head, actual_head;
- if (strbuf_read_file(&sb, git_path_current_file(), 0) >= 0) {
+ if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {
strbuf_trim(&sb);
if (get_oid_hex(sb.buf, &expected_head)) {
strbuf_release(&sb);
- die(_("could not parse %s"), git_path_current_file());
+ die(_("could not parse %s"), git_path_abort_safety_file());
}
strbuf_release(&sb);
}
else if (errno == ENOENT)
oidclr(&expected_head);
else
- die_errno(_("could not read '%s'"), git_path_current_file());
+ die_errno(_("could not read '%s'"), git_path_abort_safety_file());
if (get_oid("HEAD", &actual_head))
oidclr(&actual_head);
@@ -1436,7 +1436,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
return -1;
if (save_opts(opts))
return -1;
- update_current_file();
+ update_abort_safety_file();
res = pick_commits(&todo_list, opts);
todo_list_release(&todo_list);
return res;
^ permalink raw reply related
* Re: git add -p with new file
From: Ariel @ 2016-12-09 18:43 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161209141129.r53b4rbtgd76fn2a@sigill.intra.peff.net>
On Fri, 9 Dec 2016, Jeff King wrote:
> On Tue, Dec 06, 2016 at 08:18:59PM -0500, Ariel wrote:
>> If you do git add -p new_file it says:
>> No changes.
>> Which is a rather confusing message. I would expect it to show me the
>> content of the file in patch form, in the normal way that -p works, let me
>> edit it, etc.
> What should:
[git add directory with two new files]
> do?
It should do the exact same thing that git add without -p does: Add the
directory and both files, but because of the -p also show the diff on the
screen and ask.
> 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.
> 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.
> 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.
> I don't think that variant should just be "add -N everything, and then
> run add -p". As Duy points out, the patch for a new file is just one big
> block. But the decision of "do I want to start tracking this untracked
> file" is potentially an interesting one.
What makes sense to me is a two part question: First ask 'Add new path',
and then if yes, ask to stage the hunk (where the hunk is the entire
file).
This makes -p useful on new files, without hurting prior expectations of
how it works.
-Ariel
^ permalink raw reply
* Re: [PATCHv7 4/6] worktree: have a function to check if worktrees are in use
From: Stefan Beller @ 2016-12-09 18:49 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Brandon Williams, git@vger.kernel.org, Junio C Hamano
In-Reply-To: <20161209120006.GA6609@ash>
On Fri, Dec 9, 2016 at 4:00 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> int submodule_uses_worktrees(const char *path)
> {
> struct strbuf path = STRBUF_INIT;
> DIR *dir;
> struct dirent *d;
> int ret = 0;
>
> strbuf_addf(&path, "%s/worktrees", path);
> dir = opendir(path.buf);
> strbuf_release(&path);
>
> if (!dir)
> return 0;
The submodule may be one of the linked worktrees, which would be
caught if we use the code as I sent it out?
If this is one of the linked worktrees, we'd rather check if a file
"commondir" or "gitdir" exists?
I ask that because I would not know how to relocate such a linked
worktree gitdir?
^ permalink raw reply
* [PATCH v2 1/5] am: Fix filename in safe_to_abort() error message
From: Stephan Beyer @ 2016-12-09 19:01 UTC (permalink / raw)
To: git; +Cc: Stephan Beyer, Christian Couder, Junio C Hamano,
SZEDER Gábor
In-Reply-To: <xmqq4m2drlys.fsf@gitster.mtv.corp.google.com>
Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
builtin/am.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/am.c b/builtin/am.c
index 6981f42ce..7cf40e6f2 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2124,7 +2124,7 @@ static int safe_to_abort(const struct am_state *state)
if (read_state_file(&sb, state, "abort-safety", 1) > 0) {
if (get_oid_hex(sb.buf, &abort_safety))
- die(_("could not parse %s"), am_path(state, "abort_safety"));
+ die(_("could not parse %s"), am_path(state, "abort-safety"));
} else
oidclr(&abort_safety);
--
2.11.0.27.g74d6bea
^ permalink raw reply related
* [PATCH v2 3/5] Add test that cherry-pick --abort does not unsafely change HEAD
From: Stephan Beyer @ 2016-12-09 19:01 UTC (permalink / raw)
To: git; +Cc: Stephan Beyer, Christian Couder, Junio C Hamano,
SZEDER Gábor
In-Reply-To: <xmqq4m2drlys.fsf@gitster.mtv.corp.google.com>
The test expects failure because it is a current breakage
reported by Junio C Hamano.
Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
t/t3510-cherry-pick-sequence.sh | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 7b7a89dbd..efcd4fc48 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -147,6 +147,16 @@ test_expect_success '--abort to cancel single cherry-pick' '
git diff-index --exit-code HEAD
'
+test_expect_failure '--abort does not unsafely change HEAD' '
+ pristine_detach initial &&
+ test_must_fail git cherry-pick picked anotherpick &&
+ git reset --hard base &&
+ test_must_fail git cherry-pick picked anotherpick &&
+ git cherry-pick --abort 2>actual &&
+ test_i18ngrep "You seem to have moved HEAD" actual &&
+ test_cmp_rev base HEAD
+'
+
test_expect_success 'cherry-pick --abort to cancel multiple revert' '
pristine_detach anotherpick &&
test_expect_code 1 git revert base..picked &&
--
2.11.0.27.g74d6bea
^ permalink raw reply related
* [PATCH v2 2/5] am: Change safe_to_abort()'s not rewinding error into a warning
From: Stephan Beyer @ 2016-12-09 19:01 UTC (permalink / raw)
To: git; +Cc: Stephan Beyer, Christian Couder, Junio C Hamano,
SZEDER Gábor
In-Reply-To: <xmqq4m2drlys.fsf@gitster.mtv.corp.google.com>
The error message tells the user that something went terribly wrong
and the --abort could not be performed. But the --abort is performed,
only without rewinding. By simply changing the error into a warning,
we indicate the user that she must not try something like
"git am --abort --force", instead she just has to check the HEAD.
Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
builtin/am.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/am.c b/builtin/am.c
index 7cf40e6f2..826f18ba1 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2134,7 +2134,7 @@ static int safe_to_abort(const struct am_state *state)
if (!oidcmp(&head, &abort_safety))
return 1;
- error(_("You seem to have moved HEAD since the last 'am' failure.\n"
+ warning(_("You seem to have moved HEAD since the last 'am' failure.\n"
"Not rewinding to ORIG_HEAD"));
return 0;
--
2.11.0.27.g74d6bea
^ permalink raw reply related
* [PATCH v2 4/5] Make sequencer abort safer
From: Stephan Beyer @ 2016-12-09 19:01 UTC (permalink / raw)
To: git; +Cc: Stephan Beyer, Christian Couder, Junio C Hamano,
SZEDER Gábor
In-Reply-To: <xmqq4m2drlys.fsf@gitster.mtv.corp.google.com>
In contrast to "git am --abort", a sequencer abort did not check
whether the current HEAD is the one that is expected. This can
lead to loss of work (when not spotted and resolved using reflog
before the garbage collector chimes in).
This behavior is now changed by mimicking "git am --abort":
the abortion is done but HEAD is not changed when the current HEAD
is not the expected HEAD.
A new file "sequencer/current" is added to save the expected HEAD.
The new behavior is only active when --abort is invoked on multiple
picks. The problem does not occur for the single-pick case because
it is handled differently.
Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
sequencer.c | 48 +++++++++++++++++++++++++++++++++++++++++
t/t3510-cherry-pick-sequence.sh | 2 +-
2 files changed, 49 insertions(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index 30b10ba14..35c158471 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -27,6 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
+static GIT_PATH_FUNC(git_path_abort_safety_file, "sequencer/abort-safety")
/*
* A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
@@ -310,6 +311,20 @@ static int error_dirty_index(struct replay_opts *opts)
return -1;
}
+static void update_abort_safety_file(void)
+{
+ struct object_id head;
+
+ /* Do nothing on a single-pick */
+ if (!file_exists(git_path_seq_dir()))
+ return;
+
+ if (!get_oid("HEAD", &head))
+ write_file(git_path_abort_safety_file(), "%s", oid_to_hex(&head));
+ else
+ write_file(git_path_abort_safety_file(), "%s", "");
+}
+
static int fast_forward_to(const unsigned char *to, const unsigned char *from,
int unborn, struct replay_opts *opts)
{
@@ -339,6 +354,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
strbuf_release(&sb);
strbuf_release(&err);
ref_transaction_free(transaction);
+ update_abort_safety_file();
return 0;
}
@@ -813,6 +829,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
leave:
free_message(commit, &msg);
+ update_abort_safety_file();
return res;
}
@@ -1132,6 +1149,30 @@ static int save_head(const char *head)
return 0;
}
+static int rollback_is_safe(void)
+{
+ struct strbuf sb = STRBUF_INIT;
+ struct object_id expected_head, actual_head;
+
+ if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {
+ strbuf_trim(&sb);
+ if (get_oid_hex(sb.buf, &expected_head)) {
+ strbuf_release(&sb);
+ die(_("could not parse %s"), git_path_abort_safety_file());
+ }
+ strbuf_release(&sb);
+ }
+ else if (errno == ENOENT)
+ oidclr(&expected_head);
+ else
+ die_errno(_("could not read '%s'"), git_path_abort_safety_file());
+
+ if (get_oid("HEAD", &actual_head))
+ oidclr(&actual_head);
+
+ return !oidcmp(&actual_head, &expected_head);
+}
+
static int reset_for_rollback(const unsigned char *sha1)
{
const char *argv[4]; /* reset --merge <arg> + NULL */
@@ -1189,6 +1230,12 @@ int sequencer_rollback(struct replay_opts *opts)
error(_("cannot abort from a branch yet to be born"));
goto fail;
}
+
+ if (!rollback_is_safe()) {
+ /* Do not error, just do not rollback */
+ warning(_("You seem to have moved HEAD. "
+ "Not rewinding, check your HEAD!"));
+ } else
if (reset_for_rollback(sha1))
goto fail;
strbuf_release(&buf);
@@ -1393,6 +1440,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
return -1;
if (save_opts(opts))
return -1;
+ update_abort_safety_file();
res = pick_commits(&todo_list, opts);
todo_list_release(&todo_list);
return res;
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index efcd4fc48..372307c21 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -147,7 +147,7 @@ test_expect_success '--abort to cancel single cherry-pick' '
git diff-index --exit-code HEAD
'
-test_expect_failure '--abort does not unsafely change HEAD' '
+test_expect_success '--abort does not unsafely change HEAD' '
pristine_detach initial &&
test_must_fail git cherry-pick picked anotherpick &&
git reset --hard base &&
--
2.11.0.27.g74d6bea
^ permalink raw reply related
* [PATCH v2 5/5] sequencer: Remove useless get_dir() function
From: Stephan Beyer @ 2016-12-09 19:01 UTC (permalink / raw)
To: git; +Cc: Stephan Beyer, Christian Couder, Junio C Hamano,
SZEDER Gábor
In-Reply-To: <xmqq4m2drlys.fsf@gitster.mtv.corp.google.com>
This function is used only once, for the removal of the
directory. It is not used for the creation of the directory
nor anywhere else.
Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
sequencer.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 35c158471..aba096a0a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -47,11 +47,6 @@ static inline int is_rebase_i(const struct replay_opts *opts)
return 0;
}
-static const char *get_dir(const struct replay_opts *opts)
-{
- return git_path_seq_dir();
-}
-
static const char *get_todo_path(const struct replay_opts *opts)
{
return git_path_todo_file();
@@ -160,7 +155,7 @@ int sequencer_remove_state(struct replay_opts *opts)
free(opts->xopts[i]);
free(opts->xopts);
- strbuf_addf(&dir, "%s", get_dir(opts));
+ strbuf_addf(&dir, "%s", git_path_seq_dir());
remove_dir_recursively(&dir, 0);
strbuf_release(&dir);
--
2.11.0.27.g74d6bea
^ permalink raw reply related
* Re: [PATCH v2 00/16] pathspec cleanup
From: Brandon Williams @ 2016-12-09 19:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, sbeller, pclouds
In-Reply-To: <xmqqk2barls5.fsf@gitster.mtv.corp.google.com>
On 12/08, Junio C Hamano wrote:
> Will queue, but with fixes on issues spotted by my pre-acceptance
> mechanical filter squashed in, to fix style issues in the
> destination of code movements.
Is this pre-acceptance filter you use something that I could run
locally?
--
Brandon Williams
^ permalink raw reply
* Re: [BUG] Colon in remote urls
From: Junio C Hamano @ 2016-12-09 19:07 UTC (permalink / raw)
To: Jeff King; +Cc: Klaus Ethgen, git
In-Reply-To: <20161209152219.ehfk475vdg4levop@sigill.intra.peff.net>
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.
> 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. 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.
^ permalink raw reply
* Re: Any interest in 'git merge --continue' as a command
From: Junio C Hamano @ 2016-12-09 19:16 UTC (permalink / raw)
To: Jeff King; +Cc: Chris Packham, GIT
In-Reply-To: <20161209091127.sxxczhfslrqsqs3m@sigill.intra.peff.net>
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.
^ permalink raw reply
* [PATCH 14/16] pathspec: create strip submodule slash helpers
From: Brandon Williams @ 2016-12-09 19:18 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <CACsJy8AX6C8Tux9_8ZynBMNS2EW2pKQOGK8k0hVmbWvbZ8pa=Q@mail.gmail.com>
Factor out the logic responsible for stripping the trailing slash on
pathspecs referencing submodules into its own function.
Change-Id: Icad62647c04b4195309def0e3db416203d14f9e4
Signed-off-by: Brandon Williams <bmwill@google.com>
---
pathspec.c | 68 ++++++++++++++++++++++++++++++++++++++------------------------
1 file changed, 42 insertions(+), 26 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 84a57cf..4d9a6a0 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -258,6 +258,44 @@ static const char *parse_element_magic(unsigned *magic, int *prefix_len,
return parse_short_magic(magic, elem);
}
+static void strip_submodule_slash_cheap(struct pathspec_item *item)
+{
+ if (item->len >= 1 && item->match[item->len - 1] == '/') {
+ int i = cache_name_pos(item->match, item->len - 1);
+
+ if (i >= 0 && S_ISGITLINK(active_cache[i]->ce_mode)) {
+ item->len--;
+ item->match[item->len] = '\0';
+ }
+ }
+}
+
+static void strip_submodule_slash_expensive(struct pathspec_item *item)
+{
+ int i;
+
+ for (i = 0; i < active_nr; i++) {
+ struct cache_entry *ce = active_cache[i];
+ int ce_len = ce_namelen(ce);
+
+ if (!S_ISGITLINK(ce->ce_mode))
+ continue;
+
+ if (item->len <= ce_len || item->match[ce_len] != '/' ||
+ memcmp(ce->name, item->match, ce_len))
+ continue;
+
+ if (item->len == ce_len + 1) {
+ /* strip trailing slash */
+ item->len--;
+ item->match[item->len] = '\0';
+ } else {
+ die(_("Pathspec '%s' is in submodule '%.*s'"),
+ item->original, ce_len, ce->name);
+ }
+ }
+}
+
/*
* Take an element of a pathspec and check for magic signatures.
* Append the result to the prefix. Return the magic bitmap.
@@ -278,7 +316,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
unsigned magic = 0, element_magic = 0;
const char *copyfrom = elt;
char *match;
- int i, pathspec_prefix = -1;
+ int pathspec_prefix = -1;
/* PATHSPEC_LITERAL_PATH ignores magic */
if (flags & PATHSPEC_LITERAL_PATH) {
@@ -329,33 +367,11 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
item->len = strlen(item->match);
item->prefix = prefixlen;
- if ((flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) &&
- (item->len >= 1 && item->match[item->len - 1] == '/') &&
- (i = cache_name_pos(item->match, item->len - 1)) >= 0 &&
- S_ISGITLINK(active_cache[i]->ce_mode)) {
- item->len--;
- match[item->len] = '\0';
- }
+ if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
+ strip_submodule_slash_cheap(item);
if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
- for (i = 0; i < active_nr; i++) {
- struct cache_entry *ce = active_cache[i];
- int ce_len = ce_namelen(ce);
-
- if (!S_ISGITLINK(ce->ce_mode))
- continue;
-
- if (item->len <= ce_len || match[ce_len] != '/' ||
- memcmp(ce->name, match, ce_len))
- continue;
- if (item->len == ce_len + 1) {
- /* strip trailing slash */
- item->len--;
- match[item->len] = '\0';
- } else
- die (_("Pathspec '%s' is in submodule '%.*s'"),
- elt, ce_len, ce->name);
- }
+ strip_submodule_slash_expensive(item);
if (magic & PATHSPEC_LITERAL)
item->nowildcard_len = item->len;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: [PATCH 02/17] dir: convert create_simplify to use the pathspec struct interface
From: Brandon Williams @ 2016-12-09 19:23 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <CACsJy8Cc6hE1Rbqjrc93xDMc0UUm0yMh0A-fyu3dfJ2G1jhENQ@mail.gmail.com>
On 12/09, Duy Nguyen wrote:
> On Fri, Dec 9, 2016 at 1:19 AM, Brandon Williams <bmwill@google.com> wrote:
> > On 12/08, Duy Nguyen wrote:
> >> On Thu, Dec 8, 2016 at 7:03 AM, Brandon Williams <bmwill@google.com> wrote:
> >> > On 12/07, Duy Nguyen wrote:
> >> >> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams <bmwill@google.com> wrote:
> >> >> > Convert 'create_simplify()' to use the pathspec struct interface from
> >> >> > using the '_raw' entry in the pathspec.
> >> >>
> >> >> It would be even better to kill this create_simplify() and let
> >> >> simplify_away() handle struct pathspec directly.
> >> >>
> >> >> There is a bug in this code, that might have been found if we
> >> >> simpify_away() handled pathspec directly: the memcmp() in
> >> >> simplify_away() will not play well with :(icase) magic. My bad. If
> >> >> :(icase) is used, the easiest/safe way is simplify nothing. Later on
> >> >> maybe we can teach simplify_away() to do strncasecmp instead. We could
> >> >> ignore exclude patterns there too (although not excluding is not a
> >> >> bug).
> >> >
> >> > So are you implying that the simplify struct needs to be killed? That
> >> > way the pathspec struct itself is being passed around instead?
> >>
> >> Yes. simplify struct was a thing when pathspec was an array of char *.
> >> At this point I think it can retire (when we have time to retire it)
> >
> > Alright, then for now I can leave this change as is and have a follow up
> > series that kills the simplify struct.
>
> Do let me know if you decide to drop it, so I can put it back in my backlog.
K will do
--
Brandon Williams
^ permalink raw reply
* Re: BUG: "cherry-pick A..B || git reset --hard OTHER"
From: Stephan Beyer @ 2016-12-09 19:24 UTC (permalink / raw)
To: Junio C Hamano, Duy Nguyen
Cc: Git Mailing List, Christian Couder, SZEDER Gábor
In-Reply-To: <xmqq8trprn7f.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On 12/09/2016 07:07 PM, Junio C Hamano wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>> Having the same operation with different names only increases git
>> reputation of bad/inconsistent UI. Either forget is renamed to quit,
>> or vice versa. I prefer forget, but the decision is yours and the
>> community's. So I'm sending two patches to rename in either direction.
>> You can pick one.
>
> I actually was advocating to remove both by making --abort saner.
> With an updated --abort that behaves saner, is "rebase --forget"
> still necessary?
A quick change in t3407 of the "rebase --forget" test to use "rebase
--abort" failed. That's because it checks the use-case of
forgetting/aborting without changing the HEAD. So --abort makes a
rollback, --forget just keeps the current head. I am not sure if that
tested use-case is a real use-case though.
A quick change in the pristine_detach function in t3510 and t3511 from
"cherry-pick --quit" to "cherry-pick --abort" works when one ignores the
return value of "cherry-pick --abort". The "--quit" is used here to
ensure a clean cherry-pick state, and --quit always succeeds, even if no
cherry-pick is in progress. That may be a real use-case somehow that
could also be used for "rebase --forget"
t3510 also shows another use-case for --quit: the title says it all:
"cherry-pick --quit" to "cherry-pick --abort"
With this additional information, I'd vote to keep --quit/--forget and
just make it consistent.
~Stephan
^ permalink raw reply
* Re: BUG: "cherry-pick A..B || git reset --hard OTHER"
From: Stephan Beyer @ 2016-12-09 19:28 UTC (permalink / raw)
To: Junio C Hamano, Duy Nguyen
Cc: Git Mailing List, Christian Couder, SZEDER Gábor
In-Reply-To: <e0780f7c-ccb4-29fe-3d72-80e45a202f65@gmx.net>
On 12/09/2016 08:24 PM, Stephan Beyer wrote:
> t3510 also shows another use-case for --quit: the title says it all:
> "cherry-pick --quit" to "cherry-pick --abort"
I should've read what I actually pasted.
I wanted to paste: '--quit keeps HEAD and conflicted index intact'
Sorry for making no sense ;)
> With this additional information, I'd vote to keep --quit/--forget and
> just make it consistent.
Now!
~Stephan
^ permalink raw reply
* Re: [REGRESSION 2.10.2] problematic "empty auth" changes
From: Junio C Hamano @ 2016-12-09 19:31 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: David Turner, git
In-Reply-To: <alpine.DEB.2.20.1612091048540.23160@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> It would be different, of course, if http.emptyAuth would *not* allow the
> user to type their credentials when accessing something like
> https://github.com/dscho/shhh-secret-repository, *only* trying the login
> credentials. But that is not the case, with http.emptyAuth=true, login
> credentials are attempted first, and when they fail, the user is still
> asked interactively for their credentials.
>
> All I can see is that this would be *an improvement*: corporate users
> trying to access a Git repository that requires their login credentials
> would now not even need to enter empty user name/password.
Yup, my thought process after seeing your first message to David
exactly mirrored the above two paragraphs. It sounds like you two
have a good plan ;-)
Thanks.
> This alone would be already a good reason to change the default, IMHO.
>
> So here is my plan:
>
> - change the default of http.emptyAuth to true in the next Git for Windows
> version
>
> - publish a prerelease for early adopters to test
>
> - contribute this patch here on the Git mailing list, in the hope that it
> will make it into the next major version
>
> Ciao,
> Dscho
^ permalink raw reply
* Re: [PATCH 14/16] pathspec: create strip submodule slash helpers
From: Stefan Beller @ 2016-12-09 19:40 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Duy Nguyen, Junio C Hamano
In-Reply-To: <1481311118-174146-1-git-send-email-bmwill@google.com>
On Fri, Dec 9, 2016 at 11:18 AM, Brandon Williams <bmwill@google.com> wrote:
> Factor out the logic responsible for stripping the trailing slash on
> pathspecs referencing submodules into its own function.
>
> Change-Id: Icad62647c04b4195309def0e3db416203d14f9e4
I think we should come up with a solution to wipe out change ids
before sending emails. ;)
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
> pathspec.c | 68 ++++++++++++++++++++++++++++++++++++++------------------------
> 1 file changed, 42 insertions(+), 26 deletions(-)
>
> diff --git a/pathspec.c b/pathspec.c
> index 84a57cf..4d9a6a0 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -258,6 +258,44 @@ static const char *parse_element_magic(unsigned *magic, int *prefix_len,
> return parse_short_magic(magic, elem);
> }
>
> +static void strip_submodule_slash_cheap(struct pathspec_item *item)
> +{
> + if (item->len >= 1 && item->match[item->len - 1] == '/') {
> + int i = cache_name_pos(item->match, item->len - 1);
> +
> + if (i >= 0 && S_ISGITLINK(active_cache[i]->ce_mode)) {
> + item->len--;
> + item->match[item->len] = '\0';
> + }
> + }
> +}
> +
> +static void strip_submodule_slash_expensive(struct pathspec_item *item)
> +{
> + int i;
> +
> + for (i = 0; i < active_nr; i++) {
> + struct cache_entry *ce = active_cache[i];
> + int ce_len = ce_namelen(ce);
> +
> + if (!S_ISGITLINK(ce->ce_mode))
> + continue;
> +
> + if (item->len <= ce_len || item->match[ce_len] != '/' ||
> + memcmp(ce->name, item->match, ce_len))
> + continue;
> +
> + if (item->len == ce_len + 1) {
> + /* strip trailing slash */
> + item->len--;
> + item->match[item->len] = '\0';
> + } else {
> + die(_("Pathspec '%s' is in submodule '%.*s'"),
> + item->original, ce_len, ce->name);
> + }
> + }
> +}
> +
> /*
> * Take an element of a pathspec and check for magic signatures.
> * Append the result to the prefix. Return the magic bitmap.
> @@ -278,7 +316,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
> unsigned magic = 0, element_magic = 0;
> const char *copyfrom = elt;
> char *match;
> - int i, pathspec_prefix = -1;
> + int pathspec_prefix = -1;
>
> /* PATHSPEC_LITERAL_PATH ignores magic */
> if (flags & PATHSPEC_LITERAL_PATH) {
> @@ -329,33 +367,11 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
> item->len = strlen(item->match);
> item->prefix = prefixlen;
>
> - if ((flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) &&
> - (item->len >= 1 && item->match[item->len - 1] == '/') &&
> - (i = cache_name_pos(item->match, item->len - 1)) >= 0 &&
> - S_ISGITLINK(active_cache[i]->ce_mode)) {
> - item->len--;
> - match[item->len] = '\0';
> - }
> + if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
> + strip_submodule_slash_cheap(item);
>
> if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
> - for (i = 0; i < active_nr; i++) {
> - struct cache_entry *ce = active_cache[i];
> - int ce_len = ce_namelen(ce);
> -
> - if (!S_ISGITLINK(ce->ce_mode))
> - continue;
> -
> - if (item->len <= ce_len || match[ce_len] != '/' ||
> - memcmp(ce->name, match, ce_len))
> - continue;
> - if (item->len == ce_len + 1) {
> - /* strip trailing slash */
> - item->len--;
> - match[item->len] = '\0';
> - } else
> - die (_("Pathspec '%s' is in submodule '%.*s'"),
> - elt, ce_len, ce->name);
> - }
> + strip_submodule_slash_expensive(item);
>
> if (magic & PATHSPEC_LITERAL)
> item->nowildcard_len = item->len;
> --
> 2.8.0.rc3.226.g39d4020
>
^ permalink raw reply
* Re: [PATCH v2 0/4] road to reentrant real_path
From: Brandon Williams @ 2016-12-09 19:42 UTC (permalink / raw)
To: Duy Nguyen
Cc: Git Mailing List, Stefan Beller, Jeff King, Jacob Keller,
Junio C Hamano, Ramsay Jones, Torsten Bögershausen,
Johannes Sixt
In-Reply-To: <CACsJy8A2M_G34MeHh6vGsrf5ePOOduM6u=n17_EZLtu31uDAYg@mail.gmail.com>
On 12/09, Duy Nguyen wrote:
> On Fri, Dec 9, 2016 at 6:58 AM, Brandon Williams <bmwill@google.com> wrote:
> > diff --git a/setup.c b/setup.c
> > index fe572b8..0d9fdd0 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -254,10 +254,12 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
> > if (!is_absolute_path(data.buf))
> > strbuf_addf(&path, "%s/", gitdir);
> > strbuf_addbuf(&path, &data);
> > - strbuf_addstr(sb, real_path(path.buf));
> > + strbuf_realpath(sb, path.buf, 1);
>
> This is not the same because of this hunk in strbuf_realpath()
Then perhaps I shouldn't make this change (and just leave it as is)
since the way real_path_internal/strbuf_realpath is written requires
that the strbuf being used for the resolved path only contains the
resolved path (see the lstat(resolved->buf &st) call). Sidenote it
looks like strbuf_getcwd() also does a reset, though more subtlety,
since it just passes its buffer to getcwd().
>
> > @@ -81,17 +73,18 @@ static const char *real_path_internal(const char *path, int die_on_error)
> > goto error_out;
> > }
> >
> > - strbuf_reset(&resolved);
> > + strbuf_reset(resolved);
> >
> > if (is_absolute_path(path)) {
>
> But if you you remove that, then all (old) callers of
> strbuf_realpath() must do a strbuf_reset() in advance if needed
> (probably just real_path does) which sounds reasonable to me. You're
> probably want to be careful about the strbuf_reset() at the end of the
> function too.
>
> Other than that, I think this diff looks nice.
> --
> Duy
--
Brandon Williams
^ 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