* Re: difftool -d symlinks, under what conditions
From: Junio C Hamano @ 2013-03-13 19:28 UTC (permalink / raw)
To: John Keeping
Cc: David Aguilar, Matt McClure, git@vger.kernel.org, Tim Henigan
In-Reply-To: <20130313180106.GL2317@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
> On Wed, Mar 13, 2013 at 10:40:55AM -0700, Junio C Hamano wrote:
>> John Keeping <john@keeping.me.uk> writes:
>>
>> > git-hash-object(1) implies that it will apply the clean filter and EOL
>> > conversion when it's given a path to a file in the working tree (as it
>> > is here). Is that not the case?
>>
>> Applying clean to smudged contents _ought to_ recover clean version,
>> but is that "ought to" something you would want to rely on?
>
> How does git-status figure out that file that has been touch'd does not
> have unstaged changes without relying on this? Surely this case is no
> different from that?
I just checked. ce_modified_check_fs() does ce_compare_data() which
does the same "hash the path and compare the resulting hash". So I
think we are OK.
Thanks.
^ permalink raw reply
* Suggestion for difftool behaviour during a merge conflict
From: Øystein Walle @ 2013-03-13 18:48 UTC (permalink / raw)
To: git
When a merge is ongoing and there are conflicts, 'git difftool' will
output the exact same --cc-style diff output as 'git diff' will without
further explanation. This has lead to some confusion: A couple of weeks
ago a person asked on #git why his difftool wasn't working. After a long
while we realized difftool's behaviour after invocations such as 'git
difftool HEAD~ HEAD' would work as expected but 'git difftool' "simply
wouldn't".
With that in mind I have three suggestions on improving 'git difftool':
- Show a three-way diff between the versions (local, remote, merge)
using the user's configured difftool. Diffing against the version
containing the markers will generate some extra noise in the difftool
and might be confusing, but it's similar in behaviour to how 'git
difftool' normally works.
- Prompt the user if they want to launch 'git mergetool' instead since
a merge is ongoing. Reasonable but it might be awkward if the user is
asked whether the conflict was resolved or not when all they wanted
to do was to visually diff something.
- Add a notice to stderr saying that a --cc-style diff output is shown
instead; or document the behaviour in the man page. Definitely the
simplest option.
I'm willing to try to take a stab at either of these but I wanted to
bring it up first. There is of course an implicit fourth option which is
to do nothing, and that I'm wrong about this :)
Best regards,
Øystein Walle
^ permalink raw reply
* Re: Rename conflicts in the index
From: Junio C Hamano @ 2013-03-13 18:05 UTC (permalink / raw)
To: Edward Thomson; +Cc: git@vger.kernel.org
In-Reply-To: <A54CE3E330039942B33B670D971F857403995D93@TK5EX14MBXC252.redmond.corp.microsoft.com>
Edward Thomson <ethomson@microsoft.com> writes:
> I would propose that we store the data about the file in conflict as it
> occurred through the renames. For example, in a rename 1->2 conflict where
> A was renamed to both B and C, you would have a single conflict entry
> containing the data for A, B and C. This would allow us to provide more
> detailed information to the user - and allow them to (say) choose a single
> name to proceed with.
>
> Is this something that has value to core git as well? Alternately, is
> there something particularly stupid about this proposal?
I do not offhand see anything particularly stupid; a new optional
index extension section CACHE_EXT_RENAME_CONFLICT might be a good
addition.
Is "one side moves A to B while the other side moves it to C" the
only case, or is it just an example? Off the top of my head, "one
side moves A to x while the other side moves B to x/y" would also be
something we would want to know. I am sure there are other cases
that need to be considered.
I do not think we can discuss the design at the concrete level until
the proposal spells out to cover all interesting cases in order for
implementations to agree on the common semantics.
^ permalink raw reply
* Re: difftool -d symlinks, under what conditions
From: John Keeping @ 2013-03-13 18:01 UTC (permalink / raw)
To: Junio C Hamano
Cc: David Aguilar, Matt McClure, git@vger.kernel.org, Tim Henigan
In-Reply-To: <7vppz32o60.fsf@alter.siamese.dyndns.org>
On Wed, Mar 13, 2013 at 10:40:55AM -0700, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
>
> > git-hash-object(1) implies that it will apply the clean filter and EOL
> > conversion when it's given a path to a file in the working tree (as it
> > is here). Is that not the case?
>
> Applying clean to smudged contents _ought to_ recover clean version,
> but is that "ought to" something you would want to rely on?
How does git-status figure out that file that has been touch'd does not
have unstaged changes without relying on this? Surely this case is no
different from that?
John
^ permalink raw reply
* Re: [PATCH v3 3/3] git-merge-one-file: revise merge error reporting
From: Junio C Hamano @ 2013-03-13 17:57 UTC (permalink / raw)
To: Kevin Bracey; +Cc: git, David Aguilar, Ciaran Jessup, Scott Chacon, Alex Riesen
In-Reply-To: <1363137142-18606-3-git-send-email-kevin@bracey.fi>
Kevin Bracey <kevin@bracey.fi> writes:
> Commit 718135e improved the merge error reporting for the resolve
> strategy's merge conflict and permission conflict cases, but led to a
> malformed "ERROR: in myfile.c" message in the case of a file added
> differently.
>
> This commit reverts that change, and uses an alternative approach without
> this flaw.
>
> Signed-off-by: Kevin Bracey <kevin@bracey.fi>
> ---
> git-merge-one-file.sh | 20 +++++++-------------
> 1 file changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
> index 0f164e5..78b07a8 100755
> --- a/git-merge-one-file.sh
> +++ b/git-merge-one-file.sh
> @@ -104,11 +104,13 @@ case "${1:-.}${2:-.}${3:-.}" in
> ;;
> esac
>
> + ret=0
> src1=$(git-unpack-file $2)
> src2=$(git-unpack-file $3)
> case "$1" in
> '')
> - echo "Added $4 in both, but differently."
> + echo "ERROR: Added $4 in both, but differently."
> + ret=1
The problem you identified may be worth fixing, but I do not think
this change is correct.
This message is at the same severity level as the message on the
other arm of this case that says "Auto-merging $4". In that other
case arm, we are attempting a true three-way merge, and in this case
arm, we are attempting a similar three-way merge using your "virtual
base".
Neither has found any error in this case arm yet. The messages are
both "informational", not an error. I do not think you would want
to set ret=1 until you see content conflict.
^ permalink raw reply
* [PATCH] git-p4: support exclusively locked files
From: Danny Thomas @ 2013-03-13 17:51 UTC (permalink / raw)
To: git@vger.kernel.org
By default, newly added binary files are exclusively locked by Perforce:
'add default change (binary+l) *exclusive*'
This results in a 'Could not determine file type' error as the regex
expects
the line to end after the file type matching group. Some repositories are
also configured to always require exclusive locks, so may be a problem for
all revisions in some cases.
Signed-off-by: Danny Thomas <dthomas@blackboard.com>
---
git-p4.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git-p4.py b/git-p4.py
index 0682e61..ffba294 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -326,7 +326,7 @@ def getP4OpenedType(file):
# Returns the perforce file type for the given file.
result = p4_read_pipe(["opened", wildcard_encode(file)])
- match = re.match(".*\((.+)\)\r?$", result)
+ match = re.match(".*\((.+)\)(?:.+)?\r?$", result)
if match:
return match.group(1)
else:
--
1.8.1.5
This email and any attachments may contain confidential and proprietary information of Blackboard that is for the sole use of the intended recipient. If you are not the intended recipient, disclosure, copying, re-distribution or other use of any of this information is strictly prohibited. Please immediately notify the sender and delete this transmission if you received this email in error.
^ permalink raw reply related
* Re: [RFC/PATCH] Documentation/technical/api-fswatch.txt: start with outline
From: Karsten Blees @ 2013-03-13 17:50 UTC (permalink / raw)
To: Duy Nguyen
Cc: Ramkumar Ramachandra, Git List, Junio C Hamano,
Torsten Bögershausen, Robert Zeh, Jeff King, Erik Faye-Lund,
Drew Northup
In-Reply-To: <CACsJy8CBru+Z0+oYVKGdwjiF4DDH3w4vCjunaoCnoDQ6AizwWg@mail.gmail.com>
Am 13.03.2013 02:03, schrieb Duy Nguyen:
> On Wed, Mar 13, 2013 at 6:21 AM, Karsten Blees <karsten.blees@gmail.com> wrote:
>> Hmmm...I don't see how filesystem changes since last invocation can solve the problem, or am I missing something? I think what you mean to say is that the daemon should keep track of the filesystem *state* of the working copy, or alternatively the deltas/changes to some known state (such as .git/index)?
>
> I think git process can keep track of filesystem state (and save it
> down if necessary).
[...]
Ah, saving the state was the missing bits, thanks.
However, AFAIK inotify doesn't work recursively, so the daemon would at least have to track the directory structure to be able to register / unregister inotify handlers as directories come and go.
>> Consider 'git status; make; make clean; git status'...that's a *lot* of changes to process for nothing (potentially slowing down make).
>
> Yeah. In my opinion, the daemon should realize that at some point
> accumulated changes are too much that it's not worth collecting
> anymore, and drop them all. Git will do it the normal/slow way. After
> that the daemon picks up again. We only optimize for the case when
> little changes are made in filesystem.
>
That sounds reasonable...
>> Then there's the issue of stale data in the cache. Modifying porcelain commands that use 'git status --porcelain' to compile their changesets will want 100% exact data. I'm not saying its not doable, but adding another platform specific, caching daemon to the tool chain doesn't exactly simplify things...
>>
>> But perhaps I'm too pessimistic (or just stigmatized by inherently slow and out-of-date TGitCache/TSvnCache on Windows :-)
>
> Thanks. I didn't know about TGitCache. Will dig it up. Maybe we can
> learn something from it (or realize the daemon approach is futile
> after all).
>
TGitCache/TSvnCache are the background processes in TortoiseGit/TortoiseSvn that keep track of filesystem state to display icon overlays in Windows Explorer.
^ permalink raw reply
* Re: git-am and CRLF
From: Erik Faye-Lund @ 2013-03-13 17:46 UTC (permalink / raw)
To: Brandon Casey; +Cc: GIT Mailing-list
In-Reply-To: <CA+sFfMfJp1-REXVjipUhsVrUKDSmhrBkV0yd_=sMwwmiw2aypQ@mail.gmail.com>
On Wed, Mar 13, 2013 at 6:42 PM, Brandon Casey <drafnel@gmail.com> wrote:
> On Wed, Mar 13, 2013 at 9:54 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
>> I recently tried to apply a patch-series to a repo that is
>> unfortunately full of CRLF files, and was a bit surprised that it
>> didn't work at all.
>>
>> So I made a small repro-case, and it seems CRLF new-lines is indeed
>> the problem. Any clue how to fix it? The way I see it, we should
>> simply be able top treat the CR as any other character, and succeed.
>> But that doesn't seem to happen...
>>
>> git init test &&
>> (
>> cd test/ &&
>> git config core.autocrlf false &&
>> printf "%s\r\n%s\r\n" "foo" "bar" > test.txt &&
>> git add test.txt &&
>> git commit -m. &&
>> printf "%s\r\n%s\r\n%s\r\n" "foo" "baz" "bar" > test.txt &&
>> git commit -am. &&
>> git format-patch -1 &&
>> git reset --hard HEAD^ &&
>> git am 0001-.patch
>> )
>
> Does 'git am --keep-cr' help?
>
It does, how silly of me not to try that before posting.
> Unfortunately the original information about line ending is lost once
> a patch is sent via email since RFC2822/822 dictates that the line
> ending in an email must be crlf. So by default, mailsplit strips it.
Hmpf. I didn't transport my patches over e-mail, I simply used
git-format-patch/git-am to transfer the patches from one git-svn clone
to another. But since that's kind of an "abuse" of
git-format-patch/git-am, perhaps just using --keep-cr is the right
thing.
Thanks anyway :)
^ permalink raw reply
* Re: [PATCH 1/2] t2200: check that "add -u" limits itself to subdirectory
From: Junio C Hamano @ 2013-03-13 17:44 UTC (permalink / raw)
To: Jeff King; +Cc: Matthieu Moy, git
In-Reply-To: <20130313041022.GA5378@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> We didn't seem to be testing this transition at all. I think it's sane
> to do so now, and Junio's "now it is 2.0, let's switch" patch should
> update the test.
Yes, but I am not sure if this is testing the right thing.
> +# Note that this is scheduled to change in Git 2.0, when
> +# "git add -u" will become full-tree by default.
> +test_expect_success 'update did not touch files at root' '
> + cat >expect <<-\EOF &&
> + check
> + top
> + EOF
> + git diff-files --name-only >actual &&
> + test_cmp expect actual
> +'
The last "git add -u" we have beforet his block is this test piece:
test_expect_success 'update from a subdirectory' '
(
cd dir1 &&
echo more >sub2 &&
git add -u sub2
)
'
That is not "git add -u" without pathspec, which is the only thing
we are transitioning at Git 2.0 boundary.
^ permalink raw reply
* Re: git-am and CRLF
From: Brandon Casey @ 2013-03-13 17:42 UTC (permalink / raw)
To: kusmabite; +Cc: GIT Mailing-list
In-Reply-To: <CABPQNSb9euNgCY9g617opThHC8OUpZQvJ+peor8Yk39LZ+y+TQ@mail.gmail.com>
On Wed, Mar 13, 2013 at 9:54 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
> I recently tried to apply a patch-series to a repo that is
> unfortunately full of CRLF files, and was a bit surprised that it
> didn't work at all.
>
> So I made a small repro-case, and it seems CRLF new-lines is indeed
> the problem. Any clue how to fix it? The way I see it, we should
> simply be able top treat the CR as any other character, and succeed.
> But that doesn't seem to happen...
>
> git init test &&
> (
> cd test/ &&
> git config core.autocrlf false &&
> printf "%s\r\n%s\r\n" "foo" "bar" > test.txt &&
> git add test.txt &&
> git commit -m. &&
> printf "%s\r\n%s\r\n%s\r\n" "foo" "baz" "bar" > test.txt &&
> git commit -am. &&
> git format-patch -1 &&
> git reset --hard HEAD^ &&
> git am 0001-.patch
> )
Does 'git am --keep-cr' help?
Unfortunately the original information about line ending is lost once
a patch is sent via email since RFC2822/822 dictates that the line
ending in an email must be crlf. So by default, mailsplit strips it.
-Brandon
^ permalink raw reply
* Re: difftool -d symlinks, under what conditions
From: Junio C Hamano @ 2013-03-13 17:40 UTC (permalink / raw)
To: John Keeping
Cc: David Aguilar, Matt McClure, git@vger.kernel.org, Tim Henigan
In-Reply-To: <20130313170821.GK2317@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
> git-hash-object(1) implies that it will apply the clean filter and EOL
> conversion when it's given a path to a file in the working tree (as it
> is here). Is that not the case?
Applying clean to smudged contents _ought to_ recover clean version,
but is that "ought to" something you would want to rely on?
^ permalink raw reply
* Re: Tag peeling peculiarities
From: Junio C Hamano @ 2013-03-13 17:29 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git discussion list
In-Reply-To: <51409439.5090001@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> It is not
> clear to me whether the prohibition of tags outside of refs/tags should
> be made more airtight or whether the peeling of tags outside of
> refs/tags should be fixed.
Retroactively forbidding presense/creation of tags outside the
designated refs/tags hierarchy will not fly. I think we should peel
them when we are reading from "# pack-refs with: peeled" version.
Theoretically, we could introduce "# pack-refs with: fully-peeled"
that records peeled versions for _all_ annotated tags even in
unusual places, but that would be introducing problems to existing
versions of the software to cater to use cases that is not blessed
officially, so I doubt it has much value.
^ permalink raw reply
* Re: difftool -d symlinks, under what conditions
From: John Keeping @ 2013-03-13 17:08 UTC (permalink / raw)
To: Junio C Hamano
Cc: David Aguilar, Matt McClure, git@vger.kernel.org, Tim Henigan
In-Reply-To: <7v1ubj45ac.fsf@alter.siamese.dyndns.org>
On Wed, Mar 13, 2013 at 09:45:47AM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > David Aguilar <davvid@gmail.com> writes:
> >
> >>> The implementation of Junio's suggestion is relatively straightforward
> >>> (this is untested, although t7800 passes, and can probably be improved
> >>> by someone better versed in Perl). Does this work for your original
> >>> scenario?
> >>
> >> This is a nice straightforward approach.
> >>
> >> As Junio mentioned, a good next step would be this patch in
> >> combination with making the truly temporary files created by
> >> dir-diff readonly.
> >
> > Even though I agree that the idea Matt McClure mentioned to run a
> > three-way merge to take the modification back when the path checked
> > out to the temporary tree as a temporary file (because it does not
> > match the working tree version) gets edited by the user might be a
> > better longer-term direction to go, marking the temporaries that the
> > users should not modify clearly as such needs to be done in the
> > shorter term. This thread wouldn't have had to happen if we had
> > such a safety measure in the first place.
>
> One thing I forgot to add. I suspect the patch in the thread will
> not work if the path needs smudge filter and end-of-line conversion,
> as it seems to just hash-object what is in the working tree (which
> should be _after_ these transformations) and compare with the object
> name. The comparison should go the other way around. Try to check
> out the object with these transformations applied, and compare the
> resulting file with what is in the working tree.
git-hash-object(1) implies that it will apply the clean filter and EOL
conversion when it's given a path to a file in the working tree (as it
is here). Is that not the case?
John
^ permalink raw reply
* git-am and CRLF
From: Erik Faye-Lund @ 2013-03-13 16:54 UTC (permalink / raw)
To: GIT Mailing-list
I recently tried to apply a patch-series to a repo that is
unfortunately full of CRLF files, and was a bit surprised that it
didn't work at all.
So I made a small repro-case, and it seems CRLF new-lines is indeed
the problem. Any clue how to fix it? The way I see it, we should
simply be able top treat the CR as any other character, and succeed.
But that doesn't seem to happen...
git init test &&
(
cd test/ &&
git config core.autocrlf false &&
printf "%s\r\n%s\r\n" "foo" "bar" > test.txt &&
git add test.txt &&
git commit -m. &&
printf "%s\r\n%s\r\n%s\r\n" "foo" "baz" "bar" > test.txt &&
git commit -am. &&
git format-patch -1 &&
git reset --hard HEAD^ &&
git am 0001-.patch
)
^ permalink raw reply
* Re: difftool -d symlinks, under what conditions
From: Junio C Hamano @ 2013-03-13 16:45 UTC (permalink / raw)
To: David Aguilar
Cc: John Keeping, Matt McClure, git@vger.kernel.org, Tim Henigan
In-Reply-To: <7vtxof48sg.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> David Aguilar <davvid@gmail.com> writes:
>
>>> The implementation of Junio's suggestion is relatively straightforward
>>> (this is untested, although t7800 passes, and can probably be improved
>>> by someone better versed in Perl). Does this work for your original
>>> scenario?
>>
>> This is a nice straightforward approach.
>>
>> As Junio mentioned, a good next step would be this patch in
>> combination with making the truly temporary files created by
>> dir-diff readonly.
>
> Even though I agree that the idea Matt McClure mentioned to run a
> three-way merge to take the modification back when the path checked
> out to the temporary tree as a temporary file (because it does not
> match the working tree version) gets edited by the user might be a
> better longer-term direction to go, marking the temporaries that the
> users should not modify clearly as such needs to be done in the
> shorter term. This thread wouldn't have had to happen if we had
> such a safety measure in the first place.
One thing I forgot to add. I suspect the patch in the thread will
not work if the path needs smudge filter and end-of-line conversion,
as it seems to just hash-object what is in the working tree (which
should be _after_ these transformations) and compare with the object
name. The comparison should go the other way around. Try to check
out the object with these transformations applied, and compare the
resulting file with what is in the working tree.
Does the temporary checkout correctly apply the smudge filter and
crlf conversion, by the way? If not, regardless of the topic in
this thread, that may want to be fixed as well. I didn't check.
^ permalink raw reply
* Re: [PATCH v3 4/5] status: show more info than "currently not on any branch"
From: Junio C Hamano @ 2013-03-13 16:25 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Matthieu Moy, Jonathan Niedier
In-Reply-To: <1363174973-17597-5-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> When a remote ref or a tag is checked out, HEAD is automatically
> detached. There is no user-friendly way to find out what ref is
> checked out in this case. This patch digs in reflog for this
> information and shows "HEAD detached from origin/master" or "HEAD
> detached at v1.8.0" instead of "currently not on any branch".
>
> When it cannot figure out the original ref, it shows an abbreviated
> SHA-1. "Currently not on any branch" would never display (unless
> reflog is pruned to near empty that the last checkout entry is lost).
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
It does look like using the for_each_in_reverse() does make the
caller less error prone (by the way, I'd love to see people review,
poke holes and plug them in the implementation of reading the reflog
file in reverse; I am not sure if it covered all the corner cases
right).
Thanks.
^ permalink raw reply
* Re: [PATCH v3 1/5] wt-status: move strbuf into read_and_strip_branch()
From: Junio C Hamano @ 2013-03-13 16:20 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Matthieu Moy, Jonathan Niedier
In-Reply-To: <1363174973-17597-2-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
You do not have anything to say above the S-o-b line? The primary
effect of this patch is to change the lifespan and ownership rule of
the piece of memory used for state.branch/state.onto and moving the
strbuf is a mere logical consequence of that change. It is not
explained why this change is a good idea.
> ---
> wt-status.c | 65 ++++++++++++++++++++++++++++---------------------------------
> wt-status.h | 4 ++--
> 2 files changed, 32 insertions(+), 37 deletions(-)
>
> diff --git a/wt-status.c b/wt-status.c
> index ef405d0..6cac27b 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -969,41 +969,41 @@ static void show_bisect_in_progress(struct wt_status *s,
> /*
> * Extract branch information from rebase/bisect
> */
> -static void read_and_strip_branch(struct strbuf *sb,
> - const char **branch,
> - const char *path)
> +static char *read_and_strip_branch(const char *path)
> {
> + struct strbuf sb = STRBUF_INIT;
> unsigned char sha1[20];
>
> - strbuf_reset(sb);
> - if (strbuf_read_file(sb, git_path("%s", path), 0) <= 0)
> - return;
> + if (strbuf_read_file(&sb, git_path("%s", path), 0) <= 0)
> + goto got_nothing;
>
> - while (sb->len && sb->buf[sb->len - 1] == '\n')
> - strbuf_setlen(sb, sb->len - 1);
> - if (!sb->len)
> - return;
> - if (!prefixcmp(sb->buf, "refs/heads/"))
> - *branch = sb->buf + strlen("refs/heads/");
> - else if (!prefixcmp(sb->buf, "refs/"))
> - *branch = sb->buf;
> - else if (!get_sha1_hex(sb->buf, sha1)) {
> + while (&sb.len && sb.buf[sb.len - 1] == '\n')
> + strbuf_setlen(&sb, sb.len - 1);
> + if (!sb.len)
> + goto got_nothing;
> + if (!prefixcmp(sb.buf, "refs/heads/"))
> + strbuf_remove(&sb,0, strlen("refs/heads/"));
> + else if (!prefixcmp(sb.buf, "refs/"))
> + ;
> + else if (!get_sha1_hex(sb.buf, sha1)) {
> const char *abbrev;
> abbrev = find_unique_abbrev(sha1, DEFAULT_ABBREV);
> - strbuf_reset(sb);
> - strbuf_addstr(sb, abbrev);
> - *branch = sb->buf;
> - } else if (!strcmp(sb->buf, "detached HEAD")) /* rebase */
> - ;
> + strbuf_reset(&sb);
> + strbuf_addstr(&sb, abbrev);
> + } else if (!strcmp(sb.buf, "detached HEAD")) /* rebase */
> + goto got_nothing;
> else /* bisect */
> - *branch = sb->buf;
> + ;
> + return strbuf_detach(&sb, NULL);
> +
> +got_nothing:
> + strbuf_release(&sb);
> + return NULL;
> }
>
> static void wt_status_print_state(struct wt_status *s)
> {
> const char *state_color = color(WT_STATUS_HEADER, s);
> - struct strbuf branch = STRBUF_INIT;
> - struct strbuf onto = STRBUF_INIT;
> struct wt_status_state state;
> struct stat st;
>
> @@ -1018,27 +1018,22 @@ static void wt_status_print_state(struct wt_status *s)
> state.am_empty_patch = 1;
> } else {
> state.rebase_in_progress = 1;
> - read_and_strip_branch(&branch, &state.branch,
> - "rebase-apply/head-name");
> - read_and_strip_branch(&onto, &state.onto,
> - "rebase-apply/onto");
> + state.branch = read_and_strip_branch("rebase-apply/head-name");
> + state.onto = read_and_strip_branch("rebase-apply/onto");
> }
> } else if (!stat(git_path("rebase-merge"), &st)) {
> if (!stat(git_path("rebase-merge/interactive"), &st))
> state.rebase_interactive_in_progress = 1;
> else
> state.rebase_in_progress = 1;
> - read_and_strip_branch(&branch, &state.branch,
> - "rebase-merge/head-name");
> - read_and_strip_branch(&onto, &state.onto,
> - "rebase-merge/onto");
> + state.branch = read_and_strip_branch("rebase-merge/head-name");
> + state.onto = read_and_strip_branch("rebase-merge/onto");
> } else if (!stat(git_path("CHERRY_PICK_HEAD"), &st)) {
> state.cherry_pick_in_progress = 1;
> }
> if (!stat(git_path("BISECT_LOG"), &st)) {
> state.bisect_in_progress = 1;
> - read_and_strip_branch(&branch, &state.branch,
> - "BISECT_START");
> + state.branch = read_and_strip_branch("BISECT_START");
> }
>
> if (state.merge_in_progress)
> @@ -1051,8 +1046,8 @@ static void wt_status_print_state(struct wt_status *s)
> show_cherry_pick_in_progress(s, &state, state_color);
> if (state.bisect_in_progress)
> show_bisect_in_progress(s, &state, state_color);
> - strbuf_release(&branch);
> - strbuf_release(&onto);
> + free(state.branch);
> + free(state.onto);
> }
>
> void wt_status_print(struct wt_status *s)
> diff --git a/wt-status.h b/wt-status.h
> index 81e1dcf..b8c3512 100644
> --- a/wt-status.h
> +++ b/wt-status.h
> @@ -79,8 +79,8 @@ struct wt_status_state {
> int rebase_interactive_in_progress;
> int cherry_pick_in_progress;
> int bisect_in_progress;
> - const char *branch;
> - const char *onto;
> + char *branch;
> + char *onto;
> };
>
> void wt_status_prepare(struct wt_status *s);
^ permalink raw reply
* [PATCH 2/2] git-subtree: use ls-remote to check the refspec passed to pull and add
From: Jérémy Rosen @ 2013-03-13 16:17 UTC (permalink / raw)
To: git; +Cc: greened, pcampbell, gitster, Jérémy Rosen
In-Reply-To: <1363191452-28804-1-git-send-email-jeremy.rosen@openwide.fr>
ls-remote is the correct way to check that a parameter is a valid fetchable target
Signed-off-by: Jérémy Rosen <jeremy.rosen@openwide.fr>
---
contrib/subtree/git-subtree.sh | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 8b9d114..61d4eab 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -503,13 +503,8 @@ cmd_add()
"cmd_add_commit" "$@"
elif [ $# -eq 2 ]; then
- # Technically we could accept a refspec here but we're
- # just going to turn around and add FETCH_HEAD under the
- # specified directory. Allowing a refspec might be
- # misleading because we won't do anything with any other
- # branches fetched via the refspec.
- git rev-parse -q --verify "$2^{commit}" >/dev/null ||
- die "'$2' does not refer to a commit"
+ git ls-remote --exit-code "$1" "$2" ||
+ die "'$2' is not a correct reference on '$1'"
"cmd_add_repository" "$@"
else
@@ -700,6 +695,8 @@ cmd_merge()
cmd_pull()
{
ensure_clean
+ git ls-remote --exit-code "$1" "$2" ||
+ die "'$2' is not a correct reference on '$1'"
git fetch "$@" || exit $?
revs=FETCH_HEAD
set -- $revs
--
1.7.10.4
^ permalink raw reply related
* [PATCH 1/2] git-subtree: make sure the SHA saved as ancestor is a commit
From: Jérémy Rosen @ 2013-03-13 16:17 UTC (permalink / raw)
To: git; +Cc: greened, pcampbell, gitster, Jérémy Rosen
In-Reply-To: <1363191452-28804-1-git-send-email-jeremy.rosen@openwide.fr>
When adding or merging the first parameter might not be a commit, it can also be a tag SHA.
This needs to be fixed by using the underlying commit or the ancestor finding code will croak at split time
Signed-off-by: Jérémy Rosen <jeremy.rosen@openwide.fr>
---
contrib/subtree/git-subtree.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 8a23f58..8b9d114 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -531,7 +531,7 @@ cmd_add_repository()
cmd_add_commit()
{
- revs=$(git rev-parse $default --revs-only "$@") || exit $?
+ revs=$(git rev-parse $default --revs-only "$1^{commit}") || exit $?
set -- $revs
rev="$1"
@@ -655,7 +655,7 @@ cmd_split()
cmd_merge()
{
- revs=$(git rev-parse $default --revs-only "$@") || exit $?
+ revs=$(git rev-parse $default --revs-only "$1^{commit}") || exit $?
ensure_clean
set -- $revs
--
1.7.10.4
^ permalink raw reply related
* [PATCH 0/2] git subtree: properly handle remote refs
From: Jérémy Rosen @ 2013-03-13 16:17 UTC (permalink / raw)
To: git; +Cc: greened, pcampbell, gitster, Jérémy Rosen
One common use-case for git subtree is to use it only on taged commits from the remote project. however the current implementation of gi-subtree can not be used in that way. two orthogonal prevent this
the SHA used as the origin revision can be the SHA of a tag which isn't available locally. This works correctly at merge time but will fail at split time. The first patch uses the ^{} notation to make sure a correct commit SHA is used instead of the SHA given as a parameter
the checks for the correctness of the target ref to pull or add checks that the name matches a local ref (which is incorrect). The second patch use ls-remote to validate the ref name given against the ones in the remote repository
Jérémy Rosen (2):
git-subtree: make sure the SHA saved as ancestor is a commit
git-subtree: use ls-remote to check the refspec passed to pull and
add
contrib/subtree/git-subtree.sh | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
--
1.7.10.4
^ permalink raw reply
* Re: [PATCH] status: hint the user about -uno if read_directory takes too long
From: Junio C Hamano @ 2013-03-13 16:16 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, tboegi, artagnon, robert.allan.zeh, finnag
In-Reply-To: <1363179556-4144-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index bbba728..e91d06f 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -178,6 +178,10 @@ advice.*::
> the template shown when writing commit messages in
> linkgit:git-commit[1], and in the help message shown
> by linkgit:git-checkout[1] when switching branch.
> + statusUno::
> + If collecting untracked files in linkgit:git-status[1]
> + takes more than 2 seconds, hint the user that the option
> + `-uno` could be used to stop collecting untracked files.
It looks to me that the way this paragraph conveys information is
vastly different from all the others in the section. The section
begins with "by setting their corresponding variables to false
various advice messages can be squelched; here are the list of
variables and which advice message each of them controls", so the
description should be in "variable:: which advice message" form.
The noise this introduces to the test suite is a bit irritating and
makes us think twice if this really a good change.
> diff --git a/wt-status.c b/wt-status.c
> index ef405d0..6fde08b 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -540,7 +540,16 @@ void wt_status_collect(struct wt_status *s)
> wt_status_collect_changes_initial(s);
> else
> wt_status_collect_changes_index(s);
> - wt_status_collect_untracked(s);
> + if (s->show_untracked_files && advice_status_uno) {
> + struct timeval tv1, tv2;
> + gettimeofday(&tv1, NULL);
> + wt_status_collect_untracked(s);
> + gettimeofday(&tv2, NULL);
> + s->untracked_in_ms =
> + (uint64_t)tv2.tv_sec * 1000 + tv2.tv_usec / 1000 -
> + ((uint64_t)tv1.tv_sec * 1000 + tv1.tv_usec / 1000);
> + } else
> + wt_status_collect_untracked(s);
> }
This is not wrong per-se but it took me two reads to spot that this
is not "if advise is active, do the timer but do not collect;
otherwise do just collect as before". I wonder if we can structure
the code a bit better to make the timing bit less loud.
> static void wt_status_print_unmerged(struct wt_status *s)
> @@ -1097,6 +1106,15 @@ void wt_status_print(struct wt_status *s)
> wt_status_print_other(s, &s->untracked, _("Untracked files"), "add");
> if (s->show_ignored_files)
> wt_status_print_other(s, &s->ignored, _("Ignored files"), "add -f");
> + if (advice_status_uno && s->untracked_in_ms > 2000) {
> + status_printf_ln(s, GIT_COLOR_NORMAL,
> + _("It took %.2f seconds to collect untracked files."),
> + (float)s->untracked_in_ms / 1000);
> + status_printf_ln(s, GIT_COLOR_NORMAL,
> + _("If it happens often, you may want to use option -uno"));
> + status_printf_ln(s, GIT_COLOR_NORMAL,
> + _("to speed up by stopping displaying untracked files"));
> + }
"to speed up by stopping displaying untracked files" does not look
like giving a balanced suggestion. It is increasing the risk of
forgetting about newly created files the user may want to add, but
the risk is not properly warned.
I tend to agree that the new advice would help users if phrased in a
right way. Do we want them in COLOR_NORMAL, or do we want to make
them stand out a bit more (do we have COLOR_BLINK ;-)?
^ permalink raw reply
* Re: [PATCH v8 2/5] blame: introduce $ as "end of file" in -L syntax
From: Junio C Hamano @ 2013-03-13 16:05 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Bo Yang, Zbigniew Jędrzejewski-Szmek, Will Palmer
In-Reply-To: <87wqtbivms.fsf@pctrast.inf.ethz.ch>
Thomas Rast <trast@inf.ethz.ch> writes:
>> Even though I am moving from builtin/blame.c to line-log.c? I would
>> otherwise have to call from a rather lib-ish file into a "front
>
> You haven't sent any reply to this. Does that mean you agree? Would
> you prefer the shared file to be named something like line-range.c?
The latter, or if you do not want a churn, not moving it out of
blame.c at all.
Because "log -L" does not play well with other patch options, from
the end-user's perspective, even though may does not have cmd_logL()
entry point, I do not think line-log.c is any more library-ish than
cmd_log() or cmd_blame().
^ permalink raw reply
* Re: [PATCH] difftool: Make directory diff symlink working tree
From: John Keeping @ 2013-03-13 16:01 UTC (permalink / raw)
To: Matt McClure
Cc: David Aguilar, Junio C Hamano, git@vger.kernel.org, Tim Henigan
In-Reply-To: <CAJELnLEbYrDWUjZH6iWnovEFDh8xFvpJL5wtEcPGOpkhPo+XEA@mail.gmail.com>
On Wed, Mar 13, 2013 at 11:21:40AM -0400, Matt McClure wrote:
> On Wed, Mar 13, 2013 at 4:24 AM, David Aguilar <davvid@gmail.com> wrote:
> > This is a nice straightforward approach.
> >
> > As Junio mentioned, a good next step would be this patch
> > in combination with making the truly temporary files
> > created by dir-diff readonly.
> >
> > Will that need a win32 platform check?
> > Does anyone want to take this and whip it into a proper patch?
>
> An attempt:
>
> From 429ae282ffd7202b6d2fb024a92dea543b8af376 Mon Sep 17 00:00:00 2001
> From: Matt McClure <matthewlmcclure@gmail.com>
> Date: Wed, 13 Mar 2013 11:14:22 -0400
> Subject: [PATCH] difftool: Make directory diff symlink working tree
>
> ...primarily so that a user can edit working tree files in his difftool.
Please don't continue the subject like this, look at some recent commits
in git.git to see how commits are normally formatted.
The subject here is also inaccurate since difftool already symlinks the
working tree in some situations.
> difftool -d formerly knew how to symlink to the working tree when the
> work tree contains uncommitted changes. In practice, prior to this
> change, it would not symlink to the working tree in case there were no
> uncommitted changes, even when the user invoked difftool with the form:
>
> git difftool -d [--options] <commit> [--] [<path>...]
> This form is to view the changes you have in your working tree
> relative to the named <commit>. You can use HEAD to compare it
> with the latest commit, or a branch name to compare with the tip
> of a different branch.
>
> Instead, prior to this change, difftool would use the file's blob SHA1
> to find its content in the index rather than use the working tree
> content. This change teaches `git difftool` to compare the blob SHA1 to
> the file's working tree blob SHA1 and use the working tree file if the
> SHA1s are the same.
You don't need to say things like "formerly..." and "prior to this",
write in the imperative describing how the commit changes things. A
better commit message might be:
difftool -d: symlink working tree files matching RHS
Change the behaviour of git-difftool's directory diff mode so that
instead of symlinking to working tree files only when they have
unstaged changes we now symlink to any files where the working tree
matches the RHS of the diff.
This helps users who like to edit files in their diff tool and
expect to have those changes reflected in the working tree.
> Author: John Keeping <john@keeping.me.uk>
The correct way to credit authorship is via a "From: " header at the top
of the message. In this particular case, I think this change also
requires a documentation update and some test coverage, so it would be
appropriate for whoever adds those to take the credit for the commit and
add a "Based-on-patch-by: John Keeping <john@keeping.me.uk>" footer.
You should also add your own "Signed-off-by:" line.
If no-one else gets there first I'm hoping to have a bit of time to add
the documentation and test case in 4 hours or so.
> Conversation:
> http://thread.gmane.org/gmane.comp.version-control.git/217979/focus=218014
This isn't necessary, just reply to relevant message in that thread and
it will show up correctly in people's mail clients.
> ---
> git-difftool.perl | 21 ++++++++++++++++++---
> 1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/git-difftool.perl b/git-difftool.perl
> index 0a90de4..5f093ae 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -83,6 +83,21 @@ sub exit_cleanup
> exit($status | ($status >> 8));
> }
>
> +sub use_wt_file
> +{
> + my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
> + my $null_sha1 = '0' x 40;
> +
> + if ($sha1 eq $null_sha1) {
> + return 1;
> + } elsif (not $symlinks) {
> + return 0;
> + }
> +
> + my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file");
> + return $sha1 eq $wt_sha1;
> +}
> +
> sub setup_dir_diff
> {
> my ($repo, $workdir, $symlinks) = @_;
> @@ -159,10 +174,10 @@ EOF
> }
>
> if ($rmode ne $null_mode) {
> - if ($rsha1 ne $null_sha1) {
> - $rindex .= "$rmode $rsha1\t$dst_path\0";
> - } else {
> + if (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {
> push(@working_tree, $dst_path);
> + } else {
> + $rindex .= "$rmode $rsha1\t$dst_path\0";
> }
> }
> }
> --
> 1.8.1.5
^ permalink raw reply
* Re: [PATCH 2/2] add: respect add.updateroot config option
From: Junio C Hamano @ 2013-03-13 15:51 UTC (permalink / raw)
To: Jeff King; +Cc: Matthieu Moy, git, David Aguilar
In-Reply-To: <20130313092754.GA12658@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> Your patch doesn't advertise the option in the warning message, which I
>> think is good. You may mention it the commit message that this is a
>> deliberate choice.
>
> Yes, it was deliberate. I can add a note.
>
>> > +add.updateroot::
>>
>> Detail: option names are normally camelCased => updateRoot.
>
> Good point, thanks.
>
>> I think the option name needs a bit more thinking. Without reading the
>> doc,
>>
>> [add]
>> updateRoot = false
>>
>> would be a very tempting thing to try. Perhaps explicitly warning when
>> add.updateroot is set to false would avoid possible confusion.
This is essentially the same as Matthieu's add.use2dot0Behavior but
with that "it is an error to explicitly set it to false" twist, it
alleviates one of the worries. It still has the same "the user saw
it mentioned on stackoverflow and sets it without understanding that
the behaviour is getting changed" effect.
Also it won't give chance for scripts to get fixed, even though I
suspect that instances of "add -u/-A" without pathspec end users
write in their custom scripts almost always would want to be
tree-wide and it is a bug that they do not pass ":/" to them.
The "future.*" (I'd rather call that "migration.*") approach with
the "never set it to false" twist is probably OK from the "complaint
avoidance" perspective. The user cannot later complain "I tried to
squelch the advice but at the same time I ended up adding updated
contents outside my diretory", without getting told "That is the
documented behaviour, isn't it?" But I still think it is much less
nice from "avoid hurting the users" perspective. If the way to
squelch the user learns were to say "git add -u .", where the user
explicitly says "take the updated contents from this directory and
below", there is no room for confusion. We won't hear complaints
either way, but with the "future.*" the reason why we won't hear
complaints is because the users do not have excuse to complain.
With the "require explicit .", it is because the users won't get
hurt in the first place.
> I dunno. I am not all that excited about this patch, due to all of the
> caveats that we need to communicate to the user. The current warning has
> annoyed me a few times, but perhaps it is still too soon, and my fingers
> and brain just need retraining. I think a config option could help some
> people, but maybe it will end up hurting more. I'm inclined at this
> point to table the patch for now and see how I feel in a few weeks.
>
> I do think patch 1/2 makes sense regardless.
These two concluding paragraphs match my current thinking.
Thanks.
^ permalink raw reply
* Re: Tag peeling peculiarities
From: Michael Haggerty @ 2013-03-13 15:34 UTC (permalink / raw)
To: git discussion list; +Cc: Junio C Hamano
In-Reply-To: <51409439.5090001@alum.mit.edu>
On 03/13/2013 03:59 PM, Michael Haggerty wrote:
> I have been working on the pack-refs code [1] and noticed what looks
> like a problem with the handling of peeled refs in the packed-refs file
> and in the reference cache. In particular, the peeled versions of tags
> outside of refs/tags are *not* stored in packed-refs, but after the
> packed-refs file is read it is assumed that such tags cannot be peeled.
>
> It is clear that annotated tags want to live under refs/tags, but there
> are some ways to create them in other places (see below). It is not
> clear to me whether the prohibition of tags outside of refs/tags should
> be made more airtight or whether the peeling of tags outside of
> refs/tags should be fixed.
>
> Example:
> [...]
I should have mentioned that I already understand the programmatic
*cause* of the behavior that I described in my last email:
* in pack-refs.c:handle_one_ref(), tags that are not in refs/tags are
explicitly excluded from being peeled.
* in refs.c:read_packed_refs(), if the packed-refs file starts with
"# pack-refs with: peeled "
then the REF_KNOWS_PEELED bit is set on *every* reference read from
the file into the packed refs cache, whether or not it is under
refs/tags.
* in refs.c:peel_ref(), if a refs cache entry has its REF_KNOWS_PEELED
bit set but its peeled field is empty, then it is assumed that the
reference is unpeelable.
What I am *not* clear about is which of these steps is incorrect, and
also whether this problem will have any significant ill effects.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ 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