* Re: [PATCHv7] pathspec: give better message for submodule related pathspec error
From: Junio C Hamano @ 2017-01-09 22:53 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill, peff
In-Reply-To: <20170109224330.28405-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> Every once in a while someone complains to the mailing list to have
> run into this weird assertion[1]. The usual response from the mailing
> list is link to old discussions[2], and acknowledging the problem
> stating it is known.
>
> This patch accomplishes two things:
>
> 1. Switch assert() to die("BUG") to give a more readable message.
>
> 2. Take one of the cases where we hit a BUG and turn it into a normal
> "there was something wrong with the input" message.
>
> This assertion triggered for cases where there wasn't a programming
> bug, but just bogus input. In particular, if the user asks for a
> pathspec that is inside a submodule, we shouldn't assert() or
> die("BUG"); we should tell the user their request is bogus.
>
Is it only me who sees funny black rectangles in front of these four
lines instead of blanks, by the way?
> This comes as a single patch again, replacing sb/pathspec-errors.
> It goes directly on top of bw/pathspec-cleanup.
>
> v7:
> do not rely on "test_commit -C" being there, nor the infrastructure
> to request a "good" submodule upstream. Just create a submodule outselves
> to test in.
>
Thanks.
> diff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh
> new file mode 100755
> index 0000000000..d952ae2cae
> --- /dev/null
> +++ b/t/t6134-pathspec-in-submodule.sh
> @@ -0,0 +1,40 @@
> +#!/bin/sh
> +
> +test_description='test case exclude pathspec'
> +
> +TEST_CREATE_SUBMODULE=yes
Did you mean to keep this?
> +. ./test-lib.sh
> +
> +test_expect_success 'setup a submodule' '
> + test_create_repo pretzel &&
> + (
> + cd pretzel &&
> + touch a &&
This is better spelled as
: >a &&
because use of touch, when you do not care about the file timestamp,
is misleading.
Thanks.
^ permalink raw reply
* [PATCHv7] pathspec: give better message for submodule related pathspec error
From: Stefan Beller @ 2017-01-09 22:43 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, peff, Stefan Beller
Every once in a while someone complains to the mailing list to have
run into this weird assertion[1]. The usual response from the mailing
list is link to old discussions[2], and acknowledging the problem
stating it is known.
This patch accomplishes two things:
1. Switch assert() to die("BUG") to give a more readable message.
2. Take one of the cases where we hit a BUG and turn it into a normal
"there was something wrong with the input" message.
This assertion triggered for cases where there wasn't a programming
bug, but just bogus input. In particular, if the user asks for a
pathspec that is inside a submodule, we shouldn't assert() or
die("BUG"); we should tell the user their request is bogus.
The only reason we did not check for it, is the expensive nature
of such a check, so callers avoid setting the flag
PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE. However when we die due
to bogus input, the expense of CPU cycles spent outweighs the user
wondering what went wrong, so run that check unconditionally before
dying with a more generic error message.
Note: There is a case (e.g. "git -C submodule add .") in which we call
strip_submodule_slash_expensive, as git-add requests it via the flag
PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE, but the assert used to
trigger nevertheless, because the flag PATHSPEC_LITERAL was not set,
such that we executed
if (item->nowildcard_len < prefixlen)
item->nowildcard_len = prefixlen;
and prefixlen was not adapted (e.g. it was computed from "submodule/")
So in the die_inside_submodule_path function we also need handle paths,
that were stripped before, i.e. are the exact submodule path. This
is why the conditions in die_inside_submodule_path are slightly
different than in strip_submodule_slash_expensive.
[1] https://www.google.com/search?q=item-%3Enowildcard_len
[2] http://git.661346.n2.nabble.com/assert-failed-in-submodule-edge-case-td7628687.html
https://www.spinics.net/lists/git/msg249473.html
Helped-by: Jeff King <peff@peff.net>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
This comes as a single patch again, replacing sb/pathspec-errors.
It goes directly on top of bw/pathspec-cleanup.
v7:
do not rely on "test_commit -C" being there, nor the infrastructure
to request a "good" submodule upstream. Just create a submodule outselves
to test in.
Thanks,
Stefan
pathspec.c | 35 +++++++++++++++++++++++++++++++++--
t/t6134-pathspec-in-submodule.sh | 40 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 73 insertions(+), 2 deletions(-)
create mode 100755 t/t6134-pathspec-in-submodule.sh
diff --git a/pathspec.c b/pathspec.c
index ff2509ddd1..7ababb3159 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -296,6 +296,27 @@ static void strip_submodule_slash_expensive(struct pathspec_item *item)
}
}
+static void die_inside_submodule_path(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] == '/' || item->match[ce_len] == '\0') ||
+ memcmp(ce->name, item->match, ce_len))
+ continue;
+
+ die(_("Pathspec '%s' is in submodule '%.*s'"),
+ item->original, ce_len, ce->name);
+ }
+}
+
/*
* Perform the initialization of a pathspec_item based on a pathspec element.
*/
@@ -391,8 +412,18 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
}
/* sanity checks, pathspec matchers assume these are sane */
- assert(item->nowildcard_len <= item->len &&
- item->prefix <= item->len);
+ if (item->nowildcard_len > item->len ||
+ item->prefix > item->len) {
+ /*
+ * This case can be triggered by the user pointing us to a
+ * pathspec inside a submodule, which is an input error.
+ * Detect that here and complain, but fallback in the
+ * non-submodule case to a BUG, as we have no idea what
+ * would trigger that.
+ */
+ die_inside_submodule_path(item);
+ die ("BUG: item->nowildcard_len > item->len || item->prefix > item->len)");
+ }
}
static int pathspec_item_cmp(const void *a_, const void *b_)
diff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh
new file mode 100755
index 0000000000..d952ae2cae
--- /dev/null
+++ b/t/t6134-pathspec-in-submodule.sh
@@ -0,0 +1,40 @@
+#!/bin/sh
+
+test_description='test case exclude pathspec'
+
+TEST_CREATE_SUBMODULE=yes
+. ./test-lib.sh
+
+test_expect_success 'setup a submodule' '
+ test_create_repo pretzel &&
+ (
+ cd pretzel &&
+ touch a &&
+ git add a &&
+ git commit -m "add a file" -- a
+ ) &&
+ git submodule add ./pretzel sub &&
+ git commit -a -m "add submodule" &&
+ git submodule deinit --all
+'
+
+cat <<EOF >expect
+fatal: Pathspec 'sub/a' is in submodule 'sub'
+EOF
+
+test_expect_success 'error message for path inside submodule' '
+ echo a >sub/a &&
+ test_must_fail git add sub/a 2>actual &&
+ test_cmp expect actual
+'
+
+cat <<EOF >expect
+fatal: Pathspec '.' is in submodule 'sub'
+EOF
+
+test_expect_success 'error message for path inside submodule from within submodule' '
+ test_must_fail git -C sub add . 2>actual &&
+ test_cmp expect actual
+'
+
+test_done
--
2.11.0.rc2.30.g7c4be45.dirty
^ permalink raw reply related
* Re: [PATCH v3 00/38] Teach the sequencer to act as rebase -i's backend
From: Junio C Hamano @ 2017-01-09 22:26 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Kevin Daudt, Dennis Kaarsemaker, Stephan Beyer, Jeff King
In-Reply-To: <cover.1483370556.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> Changes since v2:
>
> - fixed a TRANSLATORS: comment
> ...
> - replaced a spawned `diff-tree` command by code using the diff functions
> directly
I just finished skimming the interdiff (the difference between the
result of merging the v2 into 'master' and the result of applying
this series on 'master'). I didn't cross reference it with all the
review comments that I saw on the list for the previous round, I
wouldn't have noticed even if there are issues that still haven't
been addressed. But what I saw in the interdiff were mostly good
changes.
I however think that the renaming of read_author_script() is totally
backwards from maintainability's point of view.
Both "am" and "rebase" use an external file whose name is the same
"author-script" (because they are meant to serve the same purpose),
and introduction of new code that is different from what "am" uses
to read it in v2 was bad enough. If the rename of the function in
this round really means "author-script of 'am' can hold only author
information, but that of 'rebase' can hold anything the end user
desires", it makes things even worse. If 'rebase' side extends the
file to hold other environment variables (in its own code), users
would wonder why the same extension will not be available in 'am'
side (and vice versa).
I ran out of time for the day and haven't looked at individual
patches yet. I may find other issues, but from the interdiff, that
was the only thing that I found even worse than the previous round,
and everything else was either the same or has improved.
Thanks.
^ permalink raw reply
* Re: [PATCH 00/13] gitk: tweak rendering of remote-tracking references
From: Marc Branchaud @ 2017-01-09 22:03 UTC (permalink / raw)
To: Michael Haggerty, Paul Mackerras; +Cc: git
In-Reply-To: <24a89c72-af53-0c87-d148-708e014ebe75@alum.mit.edu>
On 2016-12-22 03:15 AM, Michael Haggerty wrote:
> On 12/21/2016 08:07 PM, Marc Branchaud wrote:
>> On 2016-12-20 07:05 PM, Michael Haggerty wrote:
>>> On 12/20/2016 04:01 PM, Marc Branchaud wrote:
>>>> [...]
>>>> Please don't change the remotebgcolor default.
>>>>
>>>> Also, perhaps the default remoterefbgcolor should be
>>>> set remoterefbgcolor $headbgcolor
>>>> ?
>>>>
>>>> I say this because when I applied the series, without the last patch, I
>>>> was miffed that the remote/ref colour had changed.
>>>
>>> This is a one-time inconvenience that gitk developers will experience. I
>>> doubt that users jump backwards and forwards in gitk versions very often.
>>
>> In what way do you mean it's restricted to gitk developers?
>
> Maybe I misunderstood your objection.
>
> While developing this, I realized that the very first time your run gitk
> (i.e., when you don't already have a configuration file), it writes the
> then-current default colors into your configuration file. If you later
> switch gitk versions to a version with different default colors, the
> colors from the first-run version are preserved (unless the names of the
> variables used to hold the colors are changed).
>
> So if you would run the tip version of my branch first, then the parent
> of that version, you would continue to see the colors as modified by the
> final commit. If you then switch to the master version, the remote
> branch names go back to green (because it goes back to using
> `headbgcolor` again), but the remote prefix stays light brown. I thought
> you were unhappy about some form of this unexpected persistence problem.
> But this problem will mostly be experienced by gitk developers who jump
> back and forth between versions.
>
> A normal user probably mostly moves forward through version numbers, and
> only occasionally. Such a user, jumping from master to the tip of my
> branch (assuming they haven't customized anything), would see
>
> * local branches stay lime
> * remote branch prefixes stay pale orange (they don't change to light
> brown because the pale orange color from master is stored in their
> configuration file)
> * remote branch names change from lime to brown (because the
> `remoterefbgcolor` configuration setting didn't exist before)
>
>> Patch 12 introduces remoterefbgcolor, with a specific default value.
>> Prior to that, the "ref part" of remote refs was rendered with
>> headbgcolor. Users who changed their headbgcolor are used to seeing the
>> "ref part" of remote refs in the same color as their local heads.
>> Applying patch 12 changes the "ref part" color of remote refs, for such
>> users.
>>
>> All I'm saying is that make the remoterefbgcolor default be $headbgcolor
>> avoids this.
>
> For somebody who thinks that most people will want local and
> remote-tracking branch names to be rendered in the same color, your
> suggestion would be an improvement.
>
> But for somebody like me who thinks it is a good idea to render
> remote-tracking branch names in a different color than local branch
> names, this is a feature, not a bug. Even a user who explicitly
> configured `headbgcolor` to, say, purple, wasn't necessarily expressing
> a wish to have remote-tracking branch names rendered in purple. Until
> now they had no choice to set these colors separately!
>
> So even for somebody who configured this setting before, I think that
> having the remote-tracking branch names change color when the user
> upgrades to this version is a good thing, because it lets the user know
> that these two things can now be colored independently. If they don't
> like the new default brown color, such a user knows where to change it
> (even to make it agree with `headbgcolor` if they want that).
>
> But I understand that this is a matter of personal preference. I have
> but one "vote" :-)
I think we understand each other, and that we disagree. I don't feel
strongly about it though, so if you want to go this way that's fine by me.
Your case would be helped if the various ref colours were exposed in the
preferences dialog. Someone who upgrades to your gitk and doesn't like
the default remoterefbgcolor has to track down the right setting, close
all running gitk instances, and hand-edit ~/.gitk.
But I think improving gitk's colour-preferences dialog is a bit beyond
the scope of your topic...
M.
^ permalink raw reply
* Re: [PATCH 3/3] blame: output porcelain "previous" header for each file
From: Junio C Hamano @ 2017-01-09 21:57 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20170106042051.nwjiuyyp7ljhs4sr@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> /*
> + * Write out any suspect information which depends on the path. This must be
> + * handled separately from emit_one_suspect_detail(), because a given commit
> + * may have changes in multiple paths. So this needs to appear each time
> + * we mention a new group.
> + *
> * To allow LF and other nonportable characters in pathnames,
> * they are c-style quoted as needed.
> */
> -static void write_filename_info(const char *path)
> +static void write_filename_info(struct origin *suspect)
> {
> + if (suspect->previous) {
> + struct origin *prev = suspect->previous;
> + printf("previous %s ", oid_to_hex(&prev->commit->object.oid));
> + write_name_quoted(prev->path, stdout, '\n');
> + }
> printf("filename ");
> - write_name_quoted(path, stdout, '\n');
> + write_name_quoted(suspect->path, stdout, '\n');
> }
Yes, "previous" is not per commit but per "origin", and "origin" is
(commit, path) pair, so allowing this helper to examine the entire
suspect instead of just suspect->path makes sense.
Thanks for a fix.
^ permalink raw reply
* Re: Test failures when Git is built with libpcre and grep is built without it
From: Jeff King @ 2017-01-09 21:33 UTC (permalink / raw)
To: Andreas Schwab; +Cc: A. Wilcox, git
In-Reply-To: <871swcjsd3.fsf@linux-m68k.org>
On Mon, Jan 09, 2017 at 02:05:44PM +0100, Andreas Schwab wrote:
> On Jan 09 2017, "A. Wilcox" <awilfox@adelielinux.org> wrote:
>
> > Interestingly enough, you seem to be right. The failure is very
> > bizarre and has nothing to do with system /bin/grep:
> >
> > test_must_fail: command succeeded: git grep -G -F -P -E a\x{2b}b\x{2a}c ab
> > not ok 142 - grep -G -F -P -E pattern
> > #
> > # >empty &&
> > # test_must_fail git grep -G -F -P -E "a\x{2b}b\x{2a}c"
> > ab >actual &&
> > # test_cmp empty actual
> > #
> >
> > However:
> >
> > elaine trash directory.t7810-grep # git grep -G -F -P -E
> > a\x{2b}b\x{2a}c ab >actual
>
> You need to quote the regexp argument, see the line starting with
> "test_must_fail" above.
Oh, duh. I checked that the line in the test was quoted, and didn't
notice that only the unquoted version he ran later complained. Thanks
for noticing, as that explains the mystery, I think.
The problem is that we are expecting the regex "\x{2b}" to complain in
regcomp() (as an ERE), but it doesn't. And that probably _is_ related to
musl, which is providing the libc regex (I know this looks like a pcre
test, but it's checking that "-P -E" overrides the pcre option with
"-E").
I'm not sure if musl is wrong for failing to complain about a bogus
regex. Generally making something that would break into something that
works is an OK way to extend the standard. So our test is at fault for
assuming that the regex will fail. I guess we'd need to find some more
exotic syntax that pcre supports, but that ERE doesn't. Maybe "(?:)" or
something.
-Peff
^ permalink raw reply
* Re: Rebasing a branch with merges
From: Philip Oakley @ 2017-01-09 21:03 UTC (permalink / raw)
To: Robert Dailey; +Cc: Git, Johannes Schindelin
In-Reply-To: <CAHd499CuZBXF0AddP7AfqtzvA8rBArwqtktRmNdD-kwGUCruLg@mail.gmail.com>
From: "Robert Dailey" <rcdailey.lists@gmail.com>
> On Fri, Jan 6, 2017 at 3:28 PM, Philip Oakley <philipoakley@iee.org>
> wrote:
>> From: "Robert Dailey" <rcdailey.lists@gmail.com>
>>>
>>> Here's the scenario:
>>>
>>> I create a topic branch so one other developer and myself can work on
>>> a feature that takes 2 weeks to complete. During that 2 week period,
>>> changes are occurring on master that I need in my topic branch. Since
>>> I have a collaborator on the branch, I opt for merges instead of
>>> rebase.
>>>
>>> Each day I merge from master to the topic branch, which changes code
>>> I'm actively working in and requires semantic changes (functions
>>> renamed, moved, etc).
>>>
>>> Once I'm ready to merge the topic branch back into master, I have two
>>> options (bearing in mind the goal is to keep history as clean as
>>> possible. Furthermore this implies that the constant merging into
>>> topic from master has made the topic branch look unwieldy and
>>> difficult to audit):
>>
>>
>> a broader question zero;
>> 0. Is the merge always clean? Do you always do a preparatory fixup! to
>> ensure that the merge will be clean?
>>
>> Ensuring that the merge will be clean should greatly simplify your
>> decision
>> about process.
>
> I don't understand what you're asking. How would I do a fixup with
> merges? Can you explain a bit? Normally the only time I use fixup! or
> squash! is for local changes prior to pushing.
>
I was using 'fixup!' figuratively, rather than literally. If I understand
the general setup correctly, what you want is that if one does a trial
merge, then it must merge cleanly, compile and pass all tests, or if it
doesn't, you would need to add that extra commit to ensure that you get that
clean merge.
The alternative is that there is extra work to be done after the 'merge'
(which may be a management choice) to then get your nice code ready for
wider release - it may even be someone elses job to do 'integration'.
Either way, at least if you rationalise the clean end point it becomes
easier to discuss the start point. If both ends are 'dirty' it is an open
argument...
>>> 1. Do a squash merge, which keeps history clean but we lose context
>>> for the important bits (the commits representing units of work that
>>> contribute to the topic itself).
>>>
>>> 2. Do a final rebase prior to merging.
>>>
>>> #2 doesn't seem to be possible due to patch ordering. For example, if
>>> I have real commits after merge commits that depend on those changes
>>> from master being present as a base at that point in time, the rebase
>>> will cause the patch before it to no longer include those changes from
>>> master.
>>
>>
>> How much of the historic fixups to cover changes on master do you want to
>> keep visible? i.e. how many fork-points are truly needed (a. by you, b.
>> by
>> the project - personal knowledge vs corporate knowledge).?
>
>
> Again, I do not understand. Maybe the first question you asked needs
> to be understood before I can answer this one. Sorry for the trouble.
>
In some scenarios it is important to keep the record of the exact start
point (date and place) to formally justify the work performed (time eleapsed
etc). Now given the other part of your scenario of a moving api / function
naming etc, you now have two reference points - the original start point,
and the release point of the new api/names.
So at that point you probably want to merge in those changes (one way or
another), which gives you a choice of a true merge (option 1), or adding a
patch to cherry pick the bulk changes on master (option2), or just the api
changes (if you can isolate them) (option3). No option is inherently
better - it will depend on local needs.
From a clean code perspective, it's nicest if you can simply rebase your
code, but that isn't alway possible. The hard bit is to be clear about the
local issues, and which ones are immutable. These will decide your choice.
Philip
^ permalink raw reply
* Re: RFC: Enable delayed responses to Git clean/smudge filter requests
From: Stefan Beller @ 2017-01-09 20:44 UTC (permalink / raw)
To: Lars Schneider; +Cc: Git Mailing List
In-Reply-To: <D10F7C47-14E8-465B-8B7A-A09A1B28A39F@gmail.com>
On Mon, Nov 14, 2016 at 1:09 PM, Lars Schneider
<larsxschneider@gmail.com> wrote:
> Hi,
>
> Git always performs a clean/smudge filter on files in sequential order.
> Sometimes a filter operation can take a noticeable amount of time.
> This blocks the entire Git process.
>
> I would like to give a filter process the possibility to answer Git with
> "I got your request, I am processing it, ask me for the result later!".
>
> I see the following way to realize this:
>
> In unpack-trees.c:check_updates() [1] we loop through the cache
> entries and "ask me later" could be an acceptable return value of the
> checkout_entry() call. The loop could run until all entries returned
> success or error.
Late to this thread, but here is an answer nevertheless.
I am currently working on getting submodules working
for working tree modifying commands (prominently checkout, but
also read-tree -u and any other caller that uses the code in
unpack-trees.)
Once the submodules are supported and used, I anticipate that
putting the files in the working tree on disk will become a bottle neck,
i.e. the checkout taking way too long for an oversized project.
So in the future we have to do something to make checkout fast
again, which IMHO is threading. My current vision is to have checkout
automatically choose a number of threads based on expected workload,
c.f. preload-index.c, line 18-25.
> The filter machinery is triggered in various other places in Git and
> all places that want to support "ask me later" would need to be patched
> accordingly.
I think this makes sense, even in a threaded git-checkout.
I assume this idea is implemented before threading hits checkout,
so a question on the design:
Who determines the workload that is acceptable?
From reading this email, it seems to be solely the filter that uses
as many threads/processes as it thinks is ok.
Would it be possible to enhance the protocol further to have
Git also mingle with the workload, i.e. tell the filter it is
allowed to use up (N-M) threads, as it itself already uses
M out of N configured threads?
(I do not want to discuss the details here, but only if such a thing
is viable with this approach as well)
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCHv6 1/2] submodule tests: don't use itself as a submodule
From: Stefan Beller @ 2017-01-09 20:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, Jeff King, git@vger.kernel.org
In-Reply-To: <xmqqh959ynb4.fsf@gitster.mtv.corp.google.com>
On Sun, Jan 8, 2017 at 6:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> This provides an easier way to have submodules in tests, by just setting
>> TEST_CREATE_SUBMODULE to a non empty string, similar to
>> TEST_NO_CREATE_REPO.
>
> Yuck.
>
> I find it doubtful that it is a good idea to create two submodule
> repositories by merely dot-including the test-lib.sh; I find it
> doubly doubtful that it is a good idea to make test_create_repo
> pay attention to the special variable to implement that.
>
> I am OK with a solution where callers that set TEST_CREATE_SUBMODULE
> variable in this patch to instead have an explicit call
>
> test_create_repo --submodule pretzel
>
> That would be a lot more obvious.
agreed.
>
> The primary reason why I hate the implementation in this patch is
> that it is very easy for a test that says TEST_CREATE_SUBMODULE
> upfront, only to get the initial test repository (which everybody
> else gets) with two test submodules, to later gain a test that wants
> to use a separate repository and call "test_create_repo". It will
> always get the pretzel submodules, which may or may not match what
> the test writer who adds a new test needs.
I agree. At the time of writing the patch series, I was anticipating writing
way more submodule tests, but now I have these future tests integrated into
lib-submodule-update.sh.
>
>> Make use of it in those tests that add a submodule from ./. except for
>> the occurrence in create_lib_submodule_repo as there it seems we craft
>> a repository deliberately for both inside as well as outside use.
>
> But isn't the point of this change that use of ./. cannot be
> mimicking any real-world use, hence pointless for the purpose of
> really testing the components of the system? If "we craft
> deliberately for both inside and outside use" indeed _IS_ a good
> thing, then perhaps use of ./. has practical real-world use---if
> not, wouldn't we want to fix the scripts that include the
> lib-submodule-repo helper not to test such an unrealistic layout?
>
Makes sense; I tried to fix it up to avoid the ./. clone in
create_lib_submodule_repo, but the issue is there are too many
implicit assumption of these two repos, such that a faithful conversion
would just duplicate code for the submodule, (e.g. create the same
amount of commits, containing the same diffs, etc.), which then
can be argued to just slow down the test suite as the clone from ./.
is actually reducing the needed work by a factor of 2.
^ permalink raw reply
* Re: [PATCHv6 2/2] pathspec: give better message for submodule related pathspec error
From: Stefan Beller @ 2017-01-09 20:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, Jeff King, git@vger.kernel.org
In-Reply-To: <xmqqbmvhyn1m.fsf@gitster.mtv.corp.google.com>
On Sun, Jan 8, 2017 at 6:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
> For future reference, do not bury a useful fix behind unproven new
> things. The main purpose of this two-patch series is this change,
> and it does not have to wait for the change to allow test_commit to
> notice "-C" you have in another series.
noted.
The bug fixed here doesn't need to be fast-tracked IMO, because
of its history. (It's well documented on the web already)
I think this series can wait until the
"test_commit -C" is available.
In a reroll I'll drop the dependency then.
^ permalink raw reply
* Re: [PATCH] Makefile: POSIX windres
From: Johannes Sixt @ 2017-01-09 19:54 UTC (permalink / raw)
To: Johannes Schindelin, Junio C Hamano; +Cc: Steven Penny, git
In-Reply-To: <alpine.DEB.2.20.1701090903500.3469@virtualbox>
Am 09.01.2017 um 09:09 schrieb Johannes Schindelin:
> Hi Junio,
>
> On Sun, 8 Jan 2017, Junio C Hamano wrote:
>
>> Steven Penny <svnpenn@gmail.com> writes:
>>
>>> When environment variable POSIXLY_CORRECT is set, the "input -o
>>> output" syntax is not supported.
>>>
>>> http://cygwin.com/ml/cygwin/2017-01/msg00036.html
>>>
>>> Signed-off-by: Steven Penny <svnpenn@gmail.com>
>>> ---
>>
>> Who other than cygwin build uses this target? Git for Windows?
>
> Yes, Git for Windows uses this target, as did msysGit (and I suspect
> Hannes' setup).
The patch works for me, too.
-- Hannes
^ permalink raw reply
* Re: [PATCH v3 13/13] mergetool: fix running in subdir when rerere enabled
From: Johannes Sixt @ 2017-01-09 19:53 UTC (permalink / raw)
To: Junio C Hamano, Richard Hansen; +Cc: git, davvid, sbeller, simon
In-Reply-To: <xmqqvatot5ob.fsf@gitster.mtv.corp.google.com>
Am 09.01.2017 um 20:05 schrieb Junio C Hamano:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> I wonder if it makes more sense to always move to toplevel upfront
>> and consistently use path from the toplevel, perhaps like the patch
>
> s/the patch/the attached patch/ I meant.
>
>> does. The first hunk is what you wrote but only inside MERGE_RR
>> block, and the second hunk deals with converting end-user supplied
>> paths that are relative to the original relative to the top-level.
>>
>> The tweaking of $orderfile you have in the first hunk may have to be
>> tightened mimicking the way how "eval ... --sq ... ; shift" is used
>> in the second hunk to avoid confusion in case orderfile specified by
>> the end user happens to be the same as a valid revname
>> (e.g. "master").
>
> And here is a squash-able patch to illustrate what I mean.
>
> I removed both of the comment blocks as the code always works with
> the worktree-relative pathname after this patch while adjusting
> end-user supplied paths from relative to original cwd. As that is
> how the core parts of the system (including the parts written in C)
> work, even though an explanation you did in the log message is
> needed to explain why the change was needed and what the change
> intended to do to readers of "git log", it is not necessary to
> explain it to the readers of the latest code, which is what the
> in-code comment is about.
>
> The single-liner addition to the test creates a branch whose name is
> the same as the specified orderfile to deliberately create a
> confusing situation. I haven't tried, but I am fairly sure that the
> test will demonstrate how broken the orderfile=$(...) in the
> original is, if you apply the test part of the attached patch,
> without the changes to git-mergetool.sh, to your version.
>
>
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index 22f56c25a2..21f82d5b58 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -454,53 +454,34 @@ main () {
> merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
> merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"
>
> - if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR"
> + prefix=$(git rev-parse --show-prefix) || exit 1
> + cd_to_toplevel
> +
> + if test -n "$orderfile"
> then
> - # The pathnames output by the 'git rerere remaining'
> - # command below are relative to the top-level
> - # directory but the 'git diff --name-only' command
> - # further below expects the pathnames to be relative
> - # to the current working directory. Thus, we cd to
> - # the top-level directory before running 'git diff
> - # --name-only'. We change directories even earlier
> - # (before running 'git rerere remaining') in case 'git
> - # rerere remaining' is ever changed to output
> - # pathnames relative to the current working directory.
> - #
> - # Changing directories breaks a relative $orderfile
> - # pathname argument, so fix it up to be relative to
> - # the top-level directory.
> -
> - prefix=$(git rev-parse --show-prefix) || exit 1
> - cd_to_toplevel
> - if test -n "$orderfile"
> - then
> - orderfile=$(git rev-parse --prefix "$prefix" "$orderfile") || exit 1
> - fi
> + orderfile=$(
> + git rev-parse --prefix "$prefix" -- "$orderfile" |
> + sed -e 1d
> + )
> + fi
>
> + if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR"
> + then
> set -- $(git rerere remaining)
> if test $# -eq 0
> then
> print_noop_and_exit
> fi
> + elif test $# -ge 0
> + then
> + eval "set -- $(git rev-parse --sq --prefix "$prefix" -- "$@")"
> + shift
> fi
>
> - # Note: The pathnames output by 'git diff --name-only' are
> - # relative to the top-level directory, but it expects input
> - # pathnames to be relative to the current working directory.
> - # Thus:
> - # * Either cd_to_toplevel must not be run before this or all
> - # relative input pathnames must be converted to be
> - # relative to the top-level directory (or absolute).
> - # * Either cd_to_toplevel must be run after this or all
> - # relative output pathnames must be converted to be
> - # relative to the current working directory (or absolute).
> files=$(git -c core.quotePath=false \
> diff --name-only --diff-filter=U \
> ${orderfile:+"-O$orderfile"} -- "$@")
>
> - cd_to_toplevel
> -
> if test -z "$files"
> then
> print_noop_and_exit
The control flow after this patch looks much more like what I had in
mind. Thanks.
-- Hannes
^ permalink raw reply
* [PATCHv2 2/5] unpack-trees: remove unneeded continue
From: Stefan Beller @ 2017-01-09 19:46 UTC (permalink / raw)
To: gitster, peff, l.s.r; +Cc: git, Stefan Beller
In-Reply-To: <20170109194621.17013-1-sbeller@google.com>
The continue is the last statement in the loop, so not needed.
This situation arose in 700e66d66 (2010-07-30, unpack-trees: let
read-tree -u remove index entries outside sparse area) when statements
after the continue were removed.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
unpack-trees.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index 8e6768f283..d443decb23 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -253,7 +253,6 @@ static int check_updates(struct unpack_trees_options *o)
display_progress(progress, ++cnt);
if (o->update && !o->dry_run)
unlink_entry(ce);
- continue;
}
}
remove_marked_cache_entries(index);
--
2.11.0.rc2.30.g7c4be45.dirty
^ permalink raw reply related
* [PATCHv2 1/5] unpack-trees: move checkout state into check_updates
From: Stefan Beller @ 2017-01-09 19:46 UTC (permalink / raw)
To: gitster, peff, l.s.r; +Cc: git, Stefan Beller
In-Reply-To: <20170109194621.17013-1-sbeller@google.com>
The checkout state was introduced via 16da134b1f9
(read-trees: refactor the unpack_trees() part, 2006-07-30). An attempt to
refactor the checkout state was done in b56aa5b268e (unpack-trees: pass
checkout state explicitly to check_updates(), 2016-09-13), but we can
go even further.
The `struct checkout state` is not used in unpack_trees apart from
initializing it, so move it into the function that makes use of it,
which is `check_updates`.
Signed-off-by: Stefan Beller <sbeller@google.com>
Reviewed-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
unpack-trees.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index ea6bdd20e0..8e6768f283 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -218,14 +218,19 @@ static void unlink_entry(const struct cache_entry *ce)
schedule_dir_for_removal(ce->name, ce_namelen(ce));
}
-static int check_updates(struct unpack_trees_options *o,
- const struct checkout *state)
+static int check_updates(struct unpack_trees_options *o)
{
unsigned cnt = 0, total = 0;
+ int i, errs = 0;
+
struct progress *progress = NULL;
struct index_state *index = &o->result;
- int i;
- int errs = 0;
+ struct checkout state = CHECKOUT_INIT;
+
+ state.force = 1;
+ state.quiet = 1;
+ state.refresh_cache = 1;
+ state.istate = index;
if (o->update && o->verbose_update) {
for (total = cnt = 0; cnt < index->cache_nr; cnt++) {
@@ -240,7 +245,7 @@ static int check_updates(struct unpack_trees_options *o,
}
if (o->update)
- git_attr_set_direction(GIT_ATTR_CHECKOUT, &o->result);
+ git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
for (i = 0; i < index->cache_nr; i++) {
const struct cache_entry *ce = index->cache[i];
@@ -251,7 +256,7 @@ static int check_updates(struct unpack_trees_options *o,
continue;
}
}
- remove_marked_cache_entries(&o->result);
+ remove_marked_cache_entries(index);
remove_scheduled_dirs();
for (i = 0; i < index->cache_nr; i++) {
@@ -264,7 +269,7 @@ static int check_updates(struct unpack_trees_options *o,
display_progress(progress, ++cnt);
ce->ce_flags &= ~CE_UPDATE;
if (o->update && !o->dry_run) {
- errs |= checkout_entry(ce, state, NULL);
+ errs |= checkout_entry(ce, &state, NULL);
}
}
}
@@ -1094,14 +1099,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
int i, ret;
static struct cache_entry *dfc;
struct exclude_list el;
- struct checkout state = CHECKOUT_INIT;
if (len > MAX_UNPACK_TREES)
die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
- state.force = 1;
- state.quiet = 1;
- state.refresh_cache = 1;
- state.istate = &o->result;
memset(&el, 0, sizeof(el));
if (!core_apply_sparse_checkout || !o->update)
@@ -1238,7 +1238,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
}
o->src_index = NULL;
- ret = check_updates(o, &state) ? (-2) : 0;
+ ret = check_updates(o) ? (-2) : 0;
if (o->dst_index) {
if (!ret) {
if (!o->result.cache_tree)
--
2.11.0.rc2.30.g7c4be45.dirty
^ permalink raw reply related
* [PATCHv2 3/5] unpack-trees: factor progress setup out of check_updates
From: Stefan Beller @ 2017-01-09 19:46 UTC (permalink / raw)
To: gitster, peff, l.s.r; +Cc: git, Stefan Beller
In-Reply-To: <20170109194621.17013-1-sbeller@google.com>
This makes check_updates shorter and easier to understand.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
unpack-trees.c | 32 ++++++++++++++++++++------------
1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index d443decb23..b564024472 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -218,9 +218,27 @@ static void unlink_entry(const struct cache_entry *ce)
schedule_dir_for_removal(ce->name, ce_namelen(ce));
}
-static int check_updates(struct unpack_trees_options *o)
+static struct progress *get_progress(struct unpack_trees_options *o)
{
unsigned cnt = 0, total = 0;
+ struct index_state *index = &o->result;
+
+ if (!o->update || !o->verbose_update)
+ return NULL;
+
+ for (; cnt < index->cache_nr; cnt++) {
+ const struct cache_entry *ce = index->cache[cnt];
+ if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))
+ total++;
+ }
+
+ return start_progress_delay(_("Checking out files"),
+ total, 50, 1);
+}
+
+static int check_updates(struct unpack_trees_options *o)
+{
+ unsigned cnt = 0;
int i, errs = 0;
struct progress *progress = NULL;
@@ -232,17 +250,7 @@ static int check_updates(struct unpack_trees_options *o)
state.refresh_cache = 1;
state.istate = index;
- if (o->update && o->verbose_update) {
- for (total = cnt = 0; cnt < index->cache_nr; cnt++) {
- const struct cache_entry *ce = index->cache[cnt];
- if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))
- total++;
- }
-
- progress = start_progress_delay(_("Checking out files"),
- total, 50, 1);
- cnt = 0;
- }
+ progress = get_progress(o);
if (o->update)
git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
--
2.11.0.rc2.30.g7c4be45.dirty
^ permalink raw reply related
* [PATCHv2 4/5] unpack-trees: factor file removal out of check_updates
From: Stefan Beller @ 2017-01-09 19:46 UTC (permalink / raw)
To: gitster, peff, l.s.r; +Cc: git, Stefan Beller
In-Reply-To: <20170109194621.17013-1-sbeller@google.com>
This makes check_updates shorter and easier to understand.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
unpack-trees.c | 29 +++++++++++++++++++++--------
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index b564024472..ac59510251 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -218,6 +218,26 @@ static void unlink_entry(const struct cache_entry *ce)
schedule_dir_for_removal(ce->name, ce_namelen(ce));
}
+static unsigned remove_workingtree_files(struct unpack_trees_options *o,
+ struct progress *progress)
+{
+ int i;
+ unsigned cnt = 0;
+ struct index_state *index = &o->result;
+
+ for (i = 0; i < index->cache_nr; i++) {
+ const struct cache_entry *ce = index->cache[i];
+
+ if (ce->ce_flags & CE_WT_REMOVE) {
+ display_progress(progress, ++cnt);
+ if (o->update && !o->dry_run)
+ unlink_entry(ce);
+ }
+ }
+
+ return cnt;
+}
+
static struct progress *get_progress(struct unpack_trees_options *o)
{
unsigned cnt = 0, total = 0;
@@ -254,15 +274,8 @@ static int check_updates(struct unpack_trees_options *o)
if (o->update)
git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
- for (i = 0; i < index->cache_nr; i++) {
- const struct cache_entry *ce = index->cache[i];
- if (ce->ce_flags & CE_WT_REMOVE) {
- display_progress(progress, ++cnt);
- if (o->update && !o->dry_run)
- unlink_entry(ce);
- }
- }
+ cnt = remove_workingtree_files(o, progress);
remove_marked_cache_entries(index);
remove_scheduled_dirs();
--
2.11.0.rc2.30.g7c4be45.dirty
^ permalink raw reply related
* [PATCHv2 0/5] refactor unpack-trees
From: Stefan Beller @ 2017-01-09 19:46 UTC (permalink / raw)
To: gitster, peff, l.s.r; +Cc: git, Stefan Beller
v2:
* Fixed the return type to be unsigned in patch 4.
This replaces origin/rs/unpack-trees-reduce-file-scope-global,
the first patch has the following diff to that branch
diff --git a/unpack-trees.c b/unpack-trees.c
index edf9fa2f6c..8e6768f283 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -221,10 +221,11 @@ static void unlink_entry(const struct cache_entry *ce)
static int check_updates(struct unpack_trees_options *o)
{
unsigned cnt = 0, total = 0;
+ int i, errs = 0;
+
struct progress *progress = NULL;
struct index_state *index = &o->result;
struct checkout state = CHECKOUT_INIT;
- int i, errs = 0;
state.force = 1;
state.quiet = 1;
Thanks,
Stefan
v1:
unpack-trees is a central file needed for the understanding
of working tree manipulation. To help with the understanding
refactor the code to be more readable.
The first patch was a standalone patch 8 days ago;
now incorporated into this series as a v3,
reducing the scope of the checkout state.
The second patch removes a single continue statement;
it needed some digging to explain, but looks trivial.
The last 3 patches shorten the check_updates function by adding more
functions. If we ever want to parallelize file IO then these smaller
functions would be the scope to do it, keeping the check_updates as
a high level function guiding through the steps what is happening during
a working tree update.
Thanks,
Stefan
Stefan Beller (5):
unpack-trees: move checkout state into check_updates
unpack-trees: remove unneeded continue
unpack-trees: factor progress setup out of check_updates
unpack-trees: factor file removal out of check_updates
unpack-trees: factor working tree update out of check_updates
unpack-trees.c | 96 ++++++++++++++++++++++++++++++++++++++--------------------
1 file changed, 64 insertions(+), 32 deletions(-)
--
2.11.0.31.g919a8d0.dirty
^ permalink raw reply related
* [PATCHv2 5/5] unpack-trees: factor working tree update out of check_updates
From: Stefan Beller @ 2017-01-09 19:46 UTC (permalink / raw)
To: gitster, peff, l.s.r; +Cc: git, Stefan Beller
In-Reply-To: <20170109194621.17013-1-sbeller@google.com>
This makes check_updates shorter and easier to understand.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
unpack-trees.c | 40 ++++++++++++++++++++++++++--------------
1 file changed, 26 insertions(+), 14 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index ac59510251..ddb3e3dcff 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -256,12 +256,13 @@ static struct progress *get_progress(struct unpack_trees_options *o)
total, 50, 1);
}
-static int check_updates(struct unpack_trees_options *o)
+static int update_working_tree_files(struct unpack_trees_options *o,
+ struct progress *progress,
+ unsigned start_cnt)
{
- unsigned cnt = 0;
+ unsigned cnt = start_cnt;
int i, errs = 0;
- struct progress *progress = NULL;
struct index_state *index = &o->result;
struct checkout state = CHECKOUT_INIT;
@@ -270,15 +271,6 @@ static int check_updates(struct unpack_trees_options *o)
state.refresh_cache = 1;
state.istate = index;
- progress = get_progress(o);
-
- if (o->update)
- git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
-
- cnt = remove_workingtree_files(o, progress);
- remove_marked_cache_entries(index);
- remove_scheduled_dirs();
-
for (i = 0; i < index->cache_nr; i++) {
struct cache_entry *ce = index->cache[i];
@@ -288,11 +280,31 @@ static int check_updates(struct unpack_trees_options *o)
ce->name);
display_progress(progress, ++cnt);
ce->ce_flags &= ~CE_UPDATE;
- if (o->update && !o->dry_run) {
+ if (o->update && !o->dry_run)
errs |= checkout_entry(ce, &state, NULL);
- }
}
}
+
+ return errs;
+}
+
+static int check_updates(struct unpack_trees_options *o)
+{
+ struct progress *progress = NULL;
+ struct index_state *index = &o->result;
+ int errs;
+ unsigned total_removed;
+
+ progress = get_progress(o);
+
+ if (o->update)
+ git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
+
+ total_removed = remove_workingtree_files(o, progress);
+ remove_marked_cache_entries(index);
+ remove_scheduled_dirs();
+ errs = update_working_tree_files(o, progress, total_removed);
+
stop_progress(&progress);
if (o->update)
git_attr_set_direction(GIT_ATTR_CHECKIN, NULL);
--
2.11.0.rc2.30.g7c4be45.dirty
^ permalink raw reply related
* Re: [PATCH 4/5] unpack-trees: factor file removal out of check_updates
From: Stefan Beller @ 2017-01-09 19:30 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, René Scharfe, git@vger.kernel.org
In-Reply-To: <20170107013602.act7aouuzgzpwb54@sigill.intra.peff.net>
On Fri, Jan 6, 2017 at 5:36 PM, Jeff King <peff@peff.net> wrote:
>> +static int remove_workingtree_files(struct unpack_trees_options *o,
>> + unsigned cnt = 0;
>
> "cnt" is unsigned here, as it is in the caller. Should the return value
> match?
Yes, obviously. :/
Thanks for catching,
Stefan
^ permalink raw reply
* Re: [PATCH v5 0/5] road to reentrant real_path
From: Junio C Hamano @ 2017-01-09 19:26 UTC (permalink / raw)
To: Brandon Williams
Cc: git, sbeller, peff, jacob.keller, ramsay, tboegi, j6t, pclouds,
larsxschneider
In-Reply-To: <20170109182433.GC62878@google.com>
Brandon Williams <bmwill@google.com> writes:
> On 01/09, Junio C Hamano wrote:
>> Brandon Williams <bmwill@google.com> writes:
>>
>> >> How does this relate to the 5-patch real_path: series that has been
>> >> on 'next' since last year?
>> >
>> > The only difference should be in the first patch of the series which
>> > handles the #define a bit differently due to the discussion that
>> > happened last week.
>> >
>> > Here is the interdiff between v5 and 'origin/bw/realpath-wo-chdir':
>>
>> Then can you make that an incremental patch (or two) with its own
>> log message instead? It (or they) would look and smell like a
>> bugfix patch that follows up a change that has already landed. As
>> you know, we won't eject and replace patches that are already in
>> 'next' during a development cycle.
>>
>> Thanks.
>
> Yes I'll get right on that.
Thanks. Will queue.
^ permalink raw reply
* Re: [PATCH v4 1/4] Avoid Coverity warning about unfree()d git_exec_path()
From: Stefan Beller @ 2017-01-09 19:21 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, git@vger.kernel.org, David Aguilar,
Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <xmqqy3ylyqhf.fsf@gitster.mtv.corp.google.com>
On Sun, Jan 8, 2017 at 5:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
> So with the above, are you saying "Dscho said 'hopefully', and I
> confirm that this change does squelch misdiagnosis by Coverity"?
I could not find the coverity issue any more.
(It really misses easy access to "recently fixed" problems)
> commit c9bb5d101ca657fa466afa8c4368c43ea7b7aca8
> Author: Johannes Schindelin <johannes.schindelin@gmx.de>
> Date: Mon Jan 2 17:22:33 2017 +0100
>
> git_exec_path: avoid Coverity warning about unfree()d result
>
> Technically, it is correct that git_exec_path() returns a possibly
> malloc()ed string returned from system_path(), and it is sometimes
> not allocated. Cache the result in a static variable and make sure
> that we call system_path() only once, which plugs a potential leak.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Sounds good to me,
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v3 13/13] mergetool: fix running in subdir when rerere enabled
From: Junio C Hamano @ 2017-01-09 19:05 UTC (permalink / raw)
To: Richard Hansen; +Cc: git, davvid, j6t, sbeller, simon
In-Reply-To: <xmqq4m18ump1.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> I wonder if it makes more sense to always move to toplevel upfront
> and consistently use path from the toplevel, perhaps like the patch
s/the patch/the attached patch/ I meant.
> does. The first hunk is what you wrote but only inside MERGE_RR
> block, and the second hunk deals with converting end-user supplied
> paths that are relative to the original relative to the top-level.
>
> The tweaking of $orderfile you have in the first hunk may have to be
> tightened mimicking the way how "eval ... --sq ... ; shift" is used
> in the second hunk to avoid confusion in case orderfile specified by
> the end user happens to be the same as a valid revname
> (e.g. "master").
And here is a squash-able patch to illustrate what I mean.
I removed both of the comment blocks as the code always works with
the worktree-relative pathname after this patch while adjusting
end-user supplied paths from relative to original cwd. As that is
how the core parts of the system (including the parts written in C)
work, even though an explanation you did in the log message is
needed to explain why the change was needed and what the change
intended to do to readers of "git log", it is not necessary to
explain it to the readers of the latest code, which is what the
in-code comment is about.
The single-liner addition to the test creates a branch whose name is
the same as the specified orderfile to deliberately create a
confusing situation. I haven't tried, but I am fairly sure that the
test will demonstrate how broken the orderfile=$(...) in the
original is, if you apply the test part of the attached patch,
without the changes to git-mergetool.sh, to your version.
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 22f56c25a2..21f82d5b58 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -454,53 +454,34 @@ main () {
merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"
- if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR"
+ prefix=$(git rev-parse --show-prefix) || exit 1
+ cd_to_toplevel
+
+ if test -n "$orderfile"
then
- # The pathnames output by the 'git rerere remaining'
- # command below are relative to the top-level
- # directory but the 'git diff --name-only' command
- # further below expects the pathnames to be relative
- # to the current working directory. Thus, we cd to
- # the top-level directory before running 'git diff
- # --name-only'. We change directories even earlier
- # (before running 'git rerere remaining') in case 'git
- # rerere remaining' is ever changed to output
- # pathnames relative to the current working directory.
- #
- # Changing directories breaks a relative $orderfile
- # pathname argument, so fix it up to be relative to
- # the top-level directory.
-
- prefix=$(git rev-parse --show-prefix) || exit 1
- cd_to_toplevel
- if test -n "$orderfile"
- then
- orderfile=$(git rev-parse --prefix "$prefix" "$orderfile") || exit 1
- fi
+ orderfile=$(
+ git rev-parse --prefix "$prefix" -- "$orderfile" |
+ sed -e 1d
+ )
+ fi
+ if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR"
+ then
set -- $(git rerere remaining)
if test $# -eq 0
then
print_noop_and_exit
fi
+ elif test $# -ge 0
+ then
+ eval "set -- $(git rev-parse --sq --prefix "$prefix" -- "$@")"
+ shift
fi
- # Note: The pathnames output by 'git diff --name-only' are
- # relative to the top-level directory, but it expects input
- # pathnames to be relative to the current working directory.
- # Thus:
- # * Either cd_to_toplevel must not be run before this or all
- # relative input pathnames must be converted to be
- # relative to the top-level directory (or absolute).
- # * Either cd_to_toplevel must be run after this or all
- # relative output pathnames must be converted to be
- # relative to the current working directory (or absolute).
files=$(git -c core.quotePath=false \
diff --name-only --diff-filter=U \
${orderfile:+"-O$orderfile"} -- "$@")
- cd_to_toplevel
-
if test -z "$files"
then
print_noop_and_exit
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index dfd641d34b..180dd7057a 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -678,6 +678,11 @@ test_expect_success 'diff.orderFile configuration is honored' '
b
a
EOF
+
+ # make sure "order-file" that is ambiguous between
+ # rev and path is understood correctly.
+ git branch order-file HEAD &&
+
git mergetool --no-prompt --tool myecho >output &&
git grep --no-index -h -A2 Merging: output >actual &&
test_cmp expect actual
^ permalink raw reply related
* Re: Rebasing a branch with merges
From: Robert Dailey @ 2017-01-09 18:52 UTC (permalink / raw)
To: Philip Oakley; +Cc: Git, Johannes Schindelin
In-Reply-To: <49C0981FE7D04AE2AC0BBB011FD74B90@PhilipOakley>
On Fri, Jan 6, 2017 at 3:28 PM, Philip Oakley <philipoakley@iee.org> wrote:
> From: "Robert Dailey" <rcdailey.lists@gmail.com>
>>
>> Here's the scenario:
>>
>> I create a topic branch so one other developer and myself can work on
>> a feature that takes 2 weeks to complete. During that 2 week period,
>> changes are occurring on master that I need in my topic branch. Since
>> I have a collaborator on the branch, I opt for merges instead of
>> rebase.
>>
>> Each day I merge from master to the topic branch, which changes code
>> I'm actively working in and requires semantic changes (functions
>> renamed, moved, etc).
>>
>> Once I'm ready to merge the topic branch back into master, I have two
>> options (bearing in mind the goal is to keep history as clean as
>> possible. Furthermore this implies that the constant merging into
>> topic from master has made the topic branch look unwieldy and
>> difficult to audit):
>
>
> a broader question zero;
> 0. Is the merge always clean? Do you always do a preparatory fixup! to
> ensure that the merge will be clean?
>
> Ensuring that the merge will be clean should greatly simplify your decision
> about process.
I don't understand what you're asking. How would I do a fixup with
merges? Can you explain a bit? Normally the only time I use fixup! or
squash! is for local changes prior to pushing.
>> 1. Do a squash merge, which keeps history clean but we lose context
>> for the important bits (the commits representing units of work that
>> contribute to the topic itself).
>>
>> 2. Do a final rebase prior to merging.
>>
>> #2 doesn't seem to be possible due to patch ordering. For example, if
>> I have real commits after merge commits that depend on those changes
>> from master being present as a base at that point in time, the rebase
>> will cause the patch before it to no longer include those changes from
>> master.
>
>
> How much of the historic fixups to cover changes on master do you want to
> keep visible? i.e. how many fork-points are truly needed (a. by you, b. by
> the project - personal knowledge vs corporate knowledge).?
Again, I do not understand. Maybe the first question you asked needs
to be understood before I can answer this one. Sorry for the trouble.
^ permalink raw reply
* [PATCH 2/2] real_path: set errno when max number of symlinks is exceeded
From: Brandon Williams @ 2017-01-09 18:50 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, peff, gitster, larsxschneider
In-Reply-To: <20170109185024.73006-1-bmwill@google.com>
Set errno to ELOOP when the maximum number of symlinks is exceeded, as
would be done by other symlink-resolving functions.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
abspath.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/abspath.c b/abspath.c
index 0393213e5..fce40fddc 100644
--- a/abspath.c
+++ b/abspath.c
@@ -141,6 +141,8 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
strbuf_reset(&symlink);
if (num_symlinks++ > MAXSYMLINKS) {
+ errno = ELOOP;
+
if (die_on_error)
die("More than %d nested symlinks "
"on path '%s'", MAXSYMLINKS, path);
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* [PATCH 1/2] real_path: prevent redefinition of MAXSYMLINKS
From: Brandon Williams @ 2017-01-09 18:50 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, peff, gitster, larsxschneider
In-Reply-To: <xmqqzij0t7uh.fsf@gitster.mtv.corp.google.com>
The macro 'MAXSYMLINKS' is already defined on macOS and Linux in
<sys/param.h>. If 'MAXSYMLINKS' has already been defined, use the value
defined by the OS otherwise default to a value of 32 which is more
inline with what is allowed by many systems.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
abspath.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/abspath.c b/abspath.c
index 1d56f5ed9..0393213e5 100644
--- a/abspath.c
+++ b/abspath.c
@@ -62,7 +62,9 @@ static void get_root_part(struct strbuf *resolved, struct strbuf *remaining)
}
/* We allow "recursive" symbolic links. Only within reason, though. */
-#define MAXSYMLINKS 5
+#ifndef MAXSYMLINKS
+#define MAXSYMLINKS 32
+#endif
/*
* Return the real path (i.e., absolute path, with symlinks resolved
--
2.11.0.390.gc69c2f50cf-goog
^ 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