* Re: [PATCH 2/2] gitweb: add a feature to show side-by-side diff
From: Junio C Hamano @ 2011-10-17 19:37 UTC (permalink / raw)
To: Kato Kazuyoshi; +Cc: git
In-Reply-To: <CAFo4x0L4BAKnCDa1uEK0Rskd9kTsR-94D4mkYKnLGqVDnuyuBA@mail.gmail.com>
Kato Kazuyoshi <kato.kazuyoshi@gmail.com> writes:
> @@ -2276,7 +2279,7 @@ sub format_diff_line {
> }
> $line = "<span class=\"chunk_info\">@@ $from_text $to_text @@</span>" .
> "<span class=\"section\">" . esc_html($section, -nbsp=>1) .
> "</span>";
Corrupt patch (wrapped long line); please fix.
I suspect you have similar issues in the [PATCH 1/2] message, too.
^ permalink raw reply
* Re: Re: [PATCH 3/3] git-gui: new config to control staging of untracked files
From: Heiko Voigt @ 2011-10-17 19:27 UTC (permalink / raw)
To: Bert Wesarg; +Cc: Pat Thoyts, git
In-Reply-To: <CAKPyHN3pKUSLTs8_5QMo5i+=3w7KXAHJjDOfQ1XYG92ZbQ1SeA@mail.gmail.com>
Hi,
On Mon, Oct 17, 2011 at 08:47:50PM +0200, Bert Wesarg wrote:
> On Mon, Oct 17, 2011 at 20:34, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> > Here I am wondering whether we have a similar mechanism in git gui like
> > in core git that makes yes,true,1 equivalents (and similar with other
> > values) ?
>
> But it is not only yes,true,1 or no,false,0 its a tristate with the
> third state 'ask'. For booleans, there is such functionality in git
> gui. See is_config_true and is_config_false. Reusing these for this
> tristate wouldn't work. The current check here is indeed very strict
> and should be loosen by at least ignoring the case, surrounding
> spaces, and allow also true/false. But also note, that this variable
> can be set via the Options menu, so you can't mistype it.
Well if using git config you can ;-). I just wanted to ask whether we
may already have machinery which supports such tristate.
If we do not I think the current "strict" configuration is fine. In most
cases the user will use the gui itself to configure such behavior so
thats no big deal.
If someone needs that it can be added later on.
Thanks, Heiko
^ permalink raw reply
* Re: [PATCH v2 00/14] Tidying up references code
From: Junio C Hamano @ 2011-10-17 19:12 UTC (permalink / raw)
To: mhagger
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips
In-Reply-To: <1318837163-27112-1-git-send-email-mhagger@alum.mit.edu>
mhagger@alum.mit.edu writes:
> From: Michael Haggerty <mhagger@alum.mit.edu>
>
> Patch series re-rolled against v4 of "Provide API to invalidate refs
> cache"...
Thanks; queued (but not pushed out yet).
> BTW, whenever I add comments to existing code, it is just an attempt
> to record information that I have inferred from reverse-engineering.
Thanks again. I often find me scratching head while reading other people's
code, long after I reviewed (or read other's reviews) and accepted their
patches. It often is not the lack of review that caused undercommented
code to get in my tree. During the review process, the issue the code is
trying to solve is so fresh in everybody's mind, that certain things do
not need to be explained to be understood. But that kind of memory
eventually fades and only the code remains.
It is a rather unfortunate result of the human nature that the next person
who touches that code is in the best position to find out what aspect of
the code is hard to understand and deserves comment.
^ permalink raw reply
* Re: [PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class
From: Junio C Hamano @ 2011-10-17 19:02 UTC (permalink / raw)
To: Kato Kazuyoshi; +Cc: git
In-Reply-To: <CAFo4x0LP4fXgSNAnss_WRLo-TH_qe=esYn7P+=iS6t87tdzcbw@mail.gmail.com>
Kato Kazuyoshi <kato.kazuyoshi@gmail.com> writes:
> The format_diff_line() will return $diff_class and HTML in upcoming changes.
An auxiliary piece of information like this is fine at the end of the
commit log message, but the patch itself wants to be justified
standalone. Perhaps this should be sufficient:
The $diff_class variable to classify the kind of line in the diff
output was prefixed with a SP, only so that the code to synthesize
value for "class" attribute can blindly concatenate it with
another value "diff". This made the code unnecessarily ugly.
Instead, add SP that separates the value of $diff_class from
another class value "diff" where <div class="..."> string is
created and drop the leading SP from the value of $diff_class.
Explained this way, it does not even have to mention that the return value
will be changed in a different patch.
We all know that the hidden motivation of this change is that the caller
of this function, after it starts using the returned value of $diff_class,
does not want to worry about the ugly SP prefix in that variable. Saying
only "This function will return this variable in the future" still does
not fully explain that hidden motivation unless you say "and the caller
shouldn't have to worry about the leading SP", so let's not even mention
it in the log message of this change.
> ---
Sign-off?
> gitweb/gitweb.perl | 24 +++++++++++++-----------
> 1 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 85d64b2..095adda 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2235,28 +2235,30 @@ sub format_diff_line {
> ...
> +
> + my $div_open = '<div class="' . (join ' ', ('diff', $diff_class)) . '">';
I think using a separate helper variable like this is a good change. You
do not have to worry about the issue in three different places.
But doesn't join(" ", ("frotz", "")) still give you "frotz "? It is OK to
punt and say
my $div_open = '<div class="diff $diff_class">';
which would be far easier to read. It may sacrifice a bit of tidiness in
the resulting HTML but the tidiness of the source outweighs it.
Of course, if you have tons of classes, it may be worth doing something
like
join(" ", grep { defined $_ && $_ ne ""} @diff_classes);
but I do not think it is worth it in this particular case.
Thanks.
^ permalink raw reply
* Re: [PATCH] Git-p4: git-p4.changeOnSubmit to do 'change' instead of 'submit'.
From: Luke Diamand @ 2011-10-17 18:53 UTC (permalink / raw)
To: Andrei Warkentin; +Cc: git, gitster, Pete Wyckoff, Andrei Warkentin
In-Reply-To: <83923897.7841.1318868319131.JavaMail.root@zimbra-prod-mbox-2.vmware.com>
On 17/10/11 17:18, Andrei Warkentin wrote:
> Hi,
>
> ----- Original Message -----
> Anyway, the other suggestion I had was to create a new command
> instead of overriding behaviour of an existing one. Of course,
> copy-pasting P4Submit into P4Change is silly, so...
>
> How about something like this?
>
> The commands dict maps command name to class and optional dict passed to cmd.run(). That way 'change'
> can really mean P4Submit with an extra parameter not to submit but to do a changelist instead. The
> reason why I initially made the config flag was because I didn't want to copy-paste P4Submit into P4Change.
>
> commands = {
> "debug" : [ P4Debug, {} ]
> "submit" : [ P4Submit, { "doChange" : 0 } ]
> "commit" : [ P4Submit, { "doChange" : 0 } ]
> "change" : [ P4Submit, { "doChange" : 1 } ]
> "sync" : [ P4Sync, {} ],
> "rebase" : [ P4Rebase, {} ],
> "clone" : [ P4Clone, {} ],
> "rollback" : [ P4RollBack, {} ],
> "branches" : [ P4Branches, {} ]
> }
>
> Thanks for the review,
> A
>
Sounds plausible to me. The alternative would be a command line
parameter, although that could get annoying and error prone, especially
as you can't easily unsubmit a perforce change.
^ permalink raw reply
* Re: [PATCH 3/3] git-gui: new config to control staging of untracked files
From: Bert Wesarg @ 2011-10-17 18:47 UTC (permalink / raw)
To: Heiko Voigt; +Cc: Pat Thoyts, git
In-Reply-To: <20111017183430.GA2540@sandbox-rc>
On Mon, Oct 17, 2011 at 20:34, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> Hi,
>
> what the series tries to achieve looks good to me. Just one comment.
Thanks.
>
> On Fri, Oct 14, 2011 at 09:25:21PM +0200, Bert Wesarg wrote:
> [...]
>> diff --git a/lib/index.tcl b/lib/index.tcl
>> index 014acf9..45094c2 100644
>> --- a/lib/index.tcl
>> +++ b/lib/index.tcl
>> @@ -367,7 +367,19 @@ proc do_add_all {} {
>> }
>> }
>> if {[llength $untracked_paths]} {
>> - set reply [ask_popup [mc "Stage also untracked files?"]]
>> + set reply 0
>> + switch -- [get_config gui.stageuntracked] {
>> + no {
>> + set reply 0
>> + }
> [...]
>
> Here I am wondering whether we have a similar mechanism in git gui like
> in core git that makes yes,true,1 equivalents (and similar with other
> values) ?
But it is not only yes,true,1 or no,false,0 its a tristate with the
third state 'ask'. For booleans, there is such functionality in git
gui. See is_config_true and is_config_false. Reusing these for this
tristate wouldn't work. The current check here is indeed very strict
and should be loosen by at least ignoring the case, surrounding
spaces, and allow also true/false. But also note, that this variable
can be set via the Options menu, so you can't mistype it.
Bert
> If we don't I think the series is fine as it is otherwise it
> probably makes sense to use that mechanism.
>
> Cheers Heiko
>
^ permalink raw reply
* Re: [PATCH v4 0/7] Provide API to invalidate refs cache
From: Heiko Voigt @ 2011-10-17 18:47 UTC (permalink / raw)
To: mhagger
Cc: Junio C Hamano, git, Jeff King, Drew Northup, Jakub Narebski,
Johan Herland, Julian Phillips
In-Reply-To: <1318819091-7066-1-git-send-email-mhagger@alum.mit.edu>
Hi,
On Mon, Oct 17, 2011 at 04:38:04AM +0200, mhagger@alum.mit.edu wrote:
> I won't myself have time to figure out who, outside of refs.c, has to
> *call* invalidate_ref_cache(). The candidates that I know off the top
> of my head are git-clone, git-submodule [1], and git-pack-refs. It
> would be great if experts in those areas would insert calls to
> invalidate_ref_cache() where needed.
[...]
> [1] http://marc.info/?l=git&m=131827641227965&w=2
> In this mailing list thread, Heiko Voigt stated that git-submodule
> does not modify any references, so it should not have to use the
> API.
This is not entirely true. I was saying that my submodule-merge code is
currently the only one using the refs api for submodules and that does
not need to modify submodule refs. I imagine that there will be some
users when submodule support matures (e.g. recursive push).
Cheers Heiko
^ permalink raw reply
* [PATCH] resolve_gitlink_packed_ref(): fix mismerge
From: Junio C Hamano @ 2011-10-17 18:43 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Mark Levedahl, git
In-Reply-To: <7v4nz7o7mg.fsf@alter.siamese.dyndns.org>
2c5c66b (Merge branch 'jp/get-ref-dir-unsorted', 2011-10-10) merged a
topic that forked from the mainline before a new helper function
get_packed_refs() refactored code to read packed-refs file. The merge made
the call to the helper function with an incorrect argument. The parameter
to the function has to be a path to the submodule.
Fix the mismerge.
Helped-by: Mark Levedahl <mlevedahl@gmail.com>
Helped-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Junio C Hamano <gitster@pobox.com> writes:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> ...
>> The problem is that the parameter "name" is not NUL-terminated. The old
>> code turned it into a (NUL-terminated) filename via
>>
>> strcpy(name + pathlen, "packed-refs");
>>
>> but the new code passes it (unterminated) to get_packed_refs()
Let's do this on the 'master' front while mh/refs-api and mh/refs-api-2
(the new 14 patch series) are cooking in the 'next' branch. The added
test does not pass until the second-to-last patch in your new series.
I made trial merges of this with mh/refs-api and mh/refs-api-2 and both
were trivial to resolve (merge with mh/refs-api will keep the fix, and
merge with mh/refs-api-2 can be made by dropping this change), so this is
purely as interim fix plus---the value of the patch lies primarily in the
test for regression avoidance.
refs.c | 12 +++++++++++-
t/t3000-ls-files-others.sh | 19 +++++++++++++++++++
2 files changed, 30 insertions(+), 1 deletions(-)
diff --git a/refs.c b/refs.c
index 9911c97..cab4394 100644
--- a/refs.c
+++ b/refs.c
@@ -393,12 +393,22 @@ static struct ref_array *get_loose_refs(const char *submodule)
#define MAXDEPTH 5
#define MAXREFLEN (1024)
+/*
+ * Called by resolve_gitlink_ref_recursive() after it failed to read
+ * from "name", which is "module/.git/<refname>". Find <refname> in
+ * the packed-refs file for the submodule.
+ */
static int resolve_gitlink_packed_ref(char *name, int pathlen, const char *refname, unsigned char *result)
{
int retval = -1;
struct ref_entry *ref;
- struct ref_array *array = get_packed_refs(name);
+ struct ref_array *array;
+ /* being defensive: resolve_gitlink_ref() did this for us */
+ if (pathlen < 6 || memcmp(name + pathlen - 6, "/.git/", 6))
+ die("Oops");
+ name[pathlen - 6] = '\0'; /* make it path to the submodule */
+ array = get_packed_refs(name);
ref = search_ref_array(array, refname);
if (ref != NULL) {
memcpy(result, ref->sha1, 20);
diff --git a/t/t3000-ls-files-others.sh b/t/t3000-ls-files-others.sh
index 2eec011..e9160df 100755
--- a/t/t3000-ls-files-others.sh
+++ b/t/t3000-ls-files-others.sh
@@ -65,4 +65,23 @@ test_expect_success '--no-empty-directory hides empty directory' '
test_cmp expected3 output
'
+test_expect_success SYMLINKS 'ls-files --others with symlinked submodule' '
+ git init super &&
+ git init sub &&
+ (
+ cd sub &&
+ >a &&
+ git add a &&
+ git commit -m sub &&
+ git pack-refs --all
+ ) &&
+ (
+ cd super &&
+ "$TEST_DIRECTORY/../contrib/workdir/git-new-workdir" ../sub sub
+ git ls-files --others --exclude-standard >../actual
+ ) &&
+ echo sub/ >expect &&
+ test_cmp expect actual
+'
+
test_done
--
1.7.7.370.gefe43
^ permalink raw reply related
* Re: What should "git fetch origin +next" should do?
From: Junio C Hamano @ 2011-10-17 18:34 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20111017171041.GA12837@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I think the exact same confusion exists. I told git to update 'next'
> from origin, but it didn't touch refs/remotes/origin/next.
Except that you didn't tell git to *update* the remote tracking branch for
'next'; you merely told it to fetch 'next' at the remote.
> ... But I suspect that is not how many git users think of it.
I am inclined to agree that it might be the case; see my other message in
this thread.
> We've discussed this before, of course:
>
> http://thread.gmane.org/gmane.comp.version-control.git/127163/focus=127215
Yes, you brought up the "remote state as of the time I told git to record
it is a precious piece of information" issue, and I share the reasoning,
hence I am somewhat torn.
We might be better off biting the bullet and do the "rewrite a command
line colon-less refspec using a matching configured refspec iff exists"
and defer the history of remote tracking branches to its reflog in the
longer term.
^ permalink raw reply
* Re: [PATCH 3/3] git-gui: new config to control staging of untracked files
From: Heiko Voigt @ 2011-10-17 18:34 UTC (permalink / raw)
To: Bert Wesarg; +Cc: Pat Thoyts, git
In-Reply-To: <03727ea04f20c953e7de3f84ab1724a8360ca2c4.1318620267.git.bert.wesarg@googlemail.com>
Hi,
what the series tries to achieve looks good to me. Just one comment.
On Fri, Oct 14, 2011 at 09:25:21PM +0200, Bert Wesarg wrote:
[...]
> diff --git a/lib/index.tcl b/lib/index.tcl
> index 014acf9..45094c2 100644
> --- a/lib/index.tcl
> +++ b/lib/index.tcl
> @@ -367,7 +367,19 @@ proc do_add_all {} {
> }
> }
> if {[llength $untracked_paths]} {
> - set reply [ask_popup [mc "Stage also untracked files?"]]
> + set reply 0
> + switch -- [get_config gui.stageuntracked] {
> + no {
> + set reply 0
> + }
[...]
Here I am wondering whether we have a similar mechanism in git gui like
in core git that makes yes,true,1 equivalents (and similar with other
values) ? If we don't I think the series is fine as it is otherwise it
probably makes sense to use that mechanism.
Cheers Heiko
^ permalink raw reply
* Re: [PATCH v3 2/7] invalidate_ref_cache(): take the submodule as parameter
From: Junio C Hamano @ 2011-10-17 18:00 UTC (permalink / raw)
To: Michael Haggerty
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips
In-Reply-To: <4E960F91.5020103@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> On 10/12/2011 09:19 PM, Junio C Hamano wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> ...
>> Probably that is what all the existing callers want, but I would have
>> expected that an existing feature would be kept, perhaps like this
>> instead:
>>
>> if (!submodule) {
>> struct ref_cache *c;
>> for (c = ref_cache; c; c = c->next)
>> clear_ref_cache(c);
>> } else {
>> clear_ref_cache(get_ref_cache(submodule);
>> }
> ...
> Your specific suggestion would not work because currently
> submodule==NULL signifies the main module. However, it would be easy to
> add the few-line function when/if it is needed.
I think "submodule==NULL" is probably a mistake; "" would make more sense
given that you are storing the string in name[FLEX_ARRAY] field.
^ permalink raw reply
* Re: regression in git-gui since 2c5c66b... Merge branch 'jp/get-ref-dir-unsorted
From: Junio C Hamano @ 2011-10-17 17:22 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Mark Levedahl, git
In-Reply-To: <4E9C2F3C.7080405@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> static int resolve_gitlink_packed_ref(char *name, int pathlen, const
> char *refname, unsigned char *result)
> {
> int retval = -1;
> struct ref_entry *ref;
> struct ref_array *array = get_packed_refs(name);
>
> ref = search_ref_array(array, refname);
> if (ref != NULL) {
> memcpy(result, ref->sha1, 20);
> retval = 0;
> }
> return retval;
> }
>
> The problem is that the parameter "name" is not NUL-terminated. The old
> code turned it into a (NUL-terminated) filename via
>
> strcpy(name + pathlen, "packed-refs");
>
> but the new code passes it (unterminated) to get_packed_refs()
Thanks for digging this through before I got to it. Very much appreciated,
and sorry or the mismerge (incidentally this was why I wanted to merge
early these two topics that tried to improve different things that
happened to touch the same part of the code, as I knew such a merge was
risky and needed plenty time before it hits released versions).
^ permalink raw reply
* Re: What should "git fetch origin +next" should do?
From: Jeff King @ 2011-10-17 17:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v7h45s8rh.fsf@alter.siamese.dyndns.org>
On Sun, Oct 16, 2011 at 12:20:02AM -0700, Junio C Hamano wrote:
> I just tried to update one of them with "git pull --ff-only", and after
> seeing that the fetch phase failed with non-ff on 'next', ran
>
> $ git fetch origin +next
>
> which happily copied the tip of updated next to FETCH_HEAD and nowhere
> else. Of course, a colon-less refspec means do not store it anywhere,
> i.e. "<colon-less-refspec>" === "<colon-less refspec>:", so prefixing it
> with '+' to force would logically be a no-op. But it nevertheless was
> somewhat surprising and irritating.
I don't see that this has anything to do with the "+" at all. If I said:
$ git fetch origin next
I think the exact same confusion exists. I told git to update 'next'
from origin, but it didn't touch refs/remotes/origin/next. You and I
both know what is happening, because we understand that "next" is a
refspec, and that it has no RHS. But I suspect that is not how many git
users think of it.
We've discussed this before, of course:
http://thread.gmane.org/gmane.comp.version-control.git/127163/focus=127215
-Peff
^ permalink raw reply
* Re: What should "git fetch origin +next" should do?
From: Junio C Hamano @ 2011-10-17 17:09 UTC (permalink / raw)
To: Marc Branchaud; +Cc: git
In-Reply-To: <4E9C3D27.3060504@xiplink.com>
Marc Branchaud <marcnarc@xiplink.com> writes:
> On 11-10-16 03:20 AM, Junio C Hamano wrote:
>> As some might know, I use the traditional non-separate-remotes layout in
>> many of my working trees. I am old fashioned.
>
> Being hip and modern :) I use separate remote refspecs. As I read your post,
> I kept thinking that it makes no sense for fetch to ever update local refs
> and that you're a victim of your stodgy old ways.
Imagine a scenario where I run the same "git fetch origin +next" in a
repository with separate remotes layout, expecting that the remote
tracking branch refs/remotes/origin/next to be force updated. The fetched
value will be stored only in FETCH_HEAD, and I would feel exactly the same
minor irritation.
In other words, the issue does not have anything to do with the layout.
My mention of layout variants was only to clarify that the ref to be force
updated on the local side is determined by the suggested behaviours (2)
and (3) based on the fetch refspec (i.e. refs/heads/next in the
traditional layout, refs/remotes/origin/next in the separate remotes
layout).
This is a tangent but we have seen in the past some new people wonder why
their configured remote tracking refs are not updated when they do
$ git fetch origin next
This is a variant without '+', and in such a case, in addition to list the
fetched tip in FETCH_HEAD, it might be more natural for the user to expect
that the usual remote tracking branch update to happen. And I suspect
that the suggested semantics (2) might better match what the users expect
in general.
That is, when fetching from a remote that has configured fetch refspecs,
colon-less refspecs are given from the command line, are first matched
against the configured fetch refspecs for the remote, and used to update
the remote tracking branches. IOW, with the separate remote layout fetch
refspec, the above command line is re-written to
$ git fetch origin refs/heads/next:refs/remotes/origin/next
and leaves the fetched tip of 'next' in FETCH_HEAD and updates the remote
tracking branch; nothing else is fetched.
And if we apply the '+' prefix in this context, as the natural
consequence, my original example would be rewritten to:
$ git fetch origin +refs/heads/next:refs/remotes/origin/next
in a repository with the separate remote layout fetch refspec, and in a
repository with the non-separate remote layout, it would be rewritten to:
$ git fetch origin +refs/heads/next:refs/heads/next
Historically, we never used configured fetch refspecs when refspecs are
given on the command line, so such a change definitely breaks backward
compatibility, but possibly in a good way, and might deserve consideration
for Git 2.0.
^ permalink raw reply
* Re: [PATCH] Git-p4: git-p4.changeOnSubmit to do 'change' instead of 'submit'.
From: Andrei Warkentin @ 2011-10-17 16:18 UTC (permalink / raw)
To: Luke Diamand; +Cc: git, gitster, Pete Wyckoff, Andrei Warkentin
In-Reply-To: <4E99E8D2.6020107@diamand.org>
Hi,
----- Original Message -----
> From: "Luke Diamand" <luke@diamand.org>
> To: "Andrei Warkentin" <andreiw@vmware.com>
> Cc: git@vger.kernel.org, gitster@pobox.com, "Pete Wyckoff" <pw@padd.com>
> Sent: Saturday, October 15, 2011 4:10:58 PM
> Subject: Re: [PATCH] Git-p4: git-p4.changeOnSubmit to do 'change' instead of 'submit'.
>
> On 14/10/11 22:51, Andrei Warkentin wrote:
> > Many users of p4/sd use changelists for review, regression
> > tests and batch builds, thus changes are almost never directly
> > submitted.
> >
> > This new config option lets a 'p4 change -i' run instead of
> > the 'p4 submit -i'.
> >
> > Signed-off-by: Andrei Warkentin<andreiw@vmware.com>
> > ---
> > contrib/fast-import/git-p4 | 16 ++++++++++++----
> > contrib/fast-import/git-p4.txt | 10 ++++++++++
> > 2 files changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/contrib/fast-import/git-p4
> > b/contrib/fast-import/git-p4
> > index 2f7b270..19c295b 100755
> > --- a/contrib/fast-import/git-p4
> > +++ b/contrib/fast-import/git-p4
> > @@ -959,7 +959,10 @@ class P4Submit(Command, P4UserMap):
> > submitTemplate =
> > message[:message.index(separatorLine)]
> > if self.isWindows:
> > submitTemplate =
> > submitTemplate.replace("\r\n", "\n")
> > - p4_write_pipe("submit -i", submitTemplate)
> > + if gitConfig("git-p4.changeOnSubmit"):
> > + p4_write_pipe("change -i", submitTemplate)
> > + else:
> > + p4_write_pipe("subadasdmit -i",
> > submitTemplate)
>
>
> What does "p4 subadasmit" do? That's a new command to me!
>
Ack, that's emabarrasing. How did that get there :-)?
Anyway, the other suggestion I had was to create a new command
instead of overriding behaviour of an existing one. Of course,
copy-pasting P4Submit into P4Change is silly, so...
How about something like this?
The commands dict maps command name to class and optional dict passed to cmd.run(). That way 'change'
can really mean P4Submit with an extra parameter not to submit but to do a changelist instead. The
reason why I initially made the config flag was because I didn't want to copy-paste P4Submit into P4Change.
commands = {
"debug" : [ P4Debug, {} ]
"submit" : [ P4Submit, { "doChange" : 0 } ]
"commit" : [ P4Submit, { "doChange" : 0 } ]
"change" : [ P4Submit, { "doChange" : 1 } ]
"sync" : [ P4Sync, {} ],
"rebase" : [ P4Rebase, {} ],
"clone" : [ P4Clone, {} ],
"rollback" : [ P4RollBack, {} ],
"branches" : [ P4Branches, {} ]
}
Thanks for the review,
A
^ permalink raw reply
* Re: What should "git fetch origin +next" should do?
From: Ramkumar Ramachandra @ 2011-10-17 16:15 UTC (permalink / raw)
To: Marc Branchaud; +Cc: Junio C Hamano, git
In-Reply-To: <4E9C3D27.3060504@xiplink.com>
Hi Marc,
Marc Branchaud writes:
> Being hip and modern :) I use separate remote refspecs. As I read your post,
> I kept thinking that it makes no sense for fetch to ever update local refs
> and that you're a victim of your stodgy old ways.
Hm, I like the symmetry with the `git push` UI. I wouldn't want to
checkout every non-ff upstream branch and merge by hand before
rebasing my work on top of it!
-- Ram
^ permalink raw reply
* Re: What should "git fetch origin +next" should do?
From: Ramkumar Ramachandra @ 2011-10-17 16:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v7h45s8rh.fsf@alter.siamese.dyndns.org>
Hi Junio,
Junio C Hamano writes:
> I just tried to update one of them with "git pull --ff-only", and after
> seeing that the fetch phase failed with non-ff on 'next', ran
>
> $ git fetch origin +next
>
> which happily copied the tip of updated next to FETCH_HEAD and nowhere
> else. Of course, a colon-less refspec means do not store it anywhere,
> i.e. "<colon-less-refspec>" === "<colon-less refspec>:", so prefixing it
> with '+' to force would logically be a no-op. But it nevertheless was
> somewhat surprising and irritating.
Interesting: I wouldn't have expected this behavior either. I'll see
if I can add something useful to this.
> As some might know, I use the traditional non-separate-remotes layout in
> many of my working trees. I am old fashioned.
As an interesting aside, I recently stopped using the word 'origin' to
name a remote since I started using multiple remotes: your remote is
called 'junio', mine's called 'ram', Jonathan's is called 'jrn', and
so on. I personally use a variation of `git fetch junio master:master
next:next +pu:pu`. It "fails" when:
1. Some uncommitted work is left: I'm a bit messy with multiple
worktrees (git-new-workdir).
2. I'm doing some work directly on top of `master` or some other
upstream branch, and haven't forked out yet (I only fork out and name
the branch if the volume of work justifies it).
3. Sometimes `next` is non-ff, and I'm curious about what happened. I
inspect the changes before invariably using a `git reset --hard
junio/next` to throw away the useless commits.
> (2) Do notice '+' and understand that it is a request to force fetch into
> some ref locally, and from the lack of colon and RHS, assume that the
> user wants Git to infer the RHS using the configured refspec (in my
> case, "refs/heads/next:refs/heads/next" is one of the configured
> fetch refspec; "refs/heads/*:refs/remotes/origin/*" would be the one
> that would match in the separate-remotes layout). In other words,
> treat it as if the user typed "+refs/heads/next:refs/heads/next" (or
> "+refs/heads/next:refs/remotes/origin/next" in the separate-remote
> layout) from the command line.
Ugh, no. Such smartness is probably desirable at the `pull` level.
> (3) Do notice '+' is applied to 'next' but otherwise ignore the fact that
> it is a command line pathspec, which would cause us to ignore
> configured refspecs. In other words, fetch normally as if there were
> no refspec on the command line, but when updating refs/heads/next (or
> refs/remotes/origin/next in the separate-remotes layout), allow non
> fast-forward updates.
This is unnecessarily complicated and ugly imho. I think `git fetch`
is trying to be over-smart here: If I don't choose to update my local
refs by hand immediately after the fetch, I'll be surprised later.
> Perhaps we can/want to implement (1)?
Yeah, I think it's the right thing to do. For the implementation,
should we update the condition in fetch.c:451 or try to implement it
at the refspec-parsing level?
Thanks.
-- Ram
^ permalink raw reply
* Re: What should "git fetch origin +next" should do?
From: Marc Branchaud @ 2011-10-17 14:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v7h45s8rh.fsf@alter.siamese.dyndns.org>
On 11-10-16 03:20 AM, Junio C Hamano wrote:
> As some might know, I use the traditional non-separate-remotes layout in
> many of my working trees. I am old fashioned.
Being hip and modern :) I use separate remote refspecs. As I read your post,
I kept thinking that it makes no sense for fetch to ever update local refs
and that you're a victim of your stodgy old ways.
> I just tried to update one of them with "git pull --ff-only", and after
> seeing that the fetch phase failed with non-ff on 'next', ran
>
> $ git fetch origin +next
>
> which happily copied the tip of updated next to FETCH_HEAD and nowhere
> else. Of course, a colon-less refspec means do not store it anywhere,
> i.e. "<colon-less-refspec>" === "<colon-less refspec>:", so prefixing it
> with '+' to force would logically be a no-op. But it nevertheless was
> somewhat surprising and irritating.
>
> This is one of the many things that is so minor that it probably is not
> worth risking backward compatibility issues to change, but something that
> we would design differently if we were starting from scratch. Maybe in Git
> 2.0.
>
> The question however is what should it do. I can see three possibilities:
>
> (1) Forcing to fetch into FETCH_HEAD does not make any sense, so instead
> of silently ignoring the '+' prefix, error it out and do not fetch
> anything. This is easy to explain and logically makes more sense than
> the current behaviour.
I think this makes the most sense.
I'd even go so far as to make fetch error out if there's a colon in the
refspec. Fetch has no business updating local refs. There are other
commands for that, and which command you use depends on how you want your
local ref updated. I think it would be a mistake to start going down a path
where fetch learns different ways to update a local ref. Madness!
M.
^ permalink raw reply
* Re: regression in git-gui since 2c5c66b... Merge branch 'jp/get-ref-dir-unsorted
From: Jeff King @ 2011-10-17 13:55 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Mark Levedahl, Junio C Hamano, git
In-Reply-To: <4E9C2F3C.7080405@alum.mit.edu>
On Mon, Oct 17, 2011 at 03:35:56PM +0200, Michael Haggerty wrote:
> On 10/17/2011 12:07 PM, Mark Levedahl wrote:
> > Your modification of my script does not show the error for me, unless I
> > have *installed* a version of git with the failure: I suspect git-gui
> > invokes installed components, and not what is in the build directory, so
> > having a good version of git installed with the bad version in the build
> > directory does not show the error. And yes, I am quite sure that all of
> > the git commands I am running are from the one version.
>
> Yes, you seem to be right. Even if I set PATH to list my git build
> directory before the directory where it is installed, "git-gui"
> sometimes invokes git-rev-parse from the libexec path of the installed
> version.
If you are testing directly out of the build directory, you need to set
GIT_EXEC_PATH, too. The bin-wrappers/git script will do this for you
(and is what the test scripts use).
But note that there's a catch with git-gui, as its built version doesn't
live in the top-level. So running:
bin-wrappers/git gui
will try to exec the git-gui directory. You can work around it with:
bin-wrappers/git gui/git-gui
-Peff
^ permalink raw reply
* Re: regression in git-gui since 2c5c66b... Merge branch 'jp/get-ref-dir-unsorted
From: Michael Haggerty @ 2011-10-17 13:35 UTC (permalink / raw)
To: Mark Levedahl; +Cc: Junio C Hamano, git
In-Reply-To: <4E9BFE66.5070906@gmail.com>
On 10/17/2011 12:07 PM, Mark Levedahl wrote:
> Your modification of my script does not show the error for me, unless I
> have *installed* a version of git with the failure: I suspect git-gui
> invokes installed components, and not what is in the build directory, so
> having a good version of git installed with the bad version in the build
> directory does not show the error. And yes, I am quite sure that all of
> the git commands I am running are from the one version.
Yes, you seem to be right. Even if I set PATH to list my git build
directory before the directory where it is installed, "git-gui"
sometimes invokes git-rev-parse from the libexec path of the installed
version.
When I install the compiled git, then I see the behavior that you describe.
The invocation that behaves differently is
git ls-files --others -z --exclude-standard
(run in the "super" directory). It doesn't seem to matter which version
of git is used to create the test repository. Under 2c5c66b, it outputs
"sub/a", whereas under either of the merge commit's ancestors, the
command outputs "sub/".
git ls-files --others
I believe that the problem originates in code in
resolve_gitlink_packed_ref() that was invented during the merge 2c5c66b:
static int resolve_gitlink_packed_ref(char *name, int pathlen, const
char *refname, unsigned char *result)
{
int retval = -1;
struct ref_entry *ref;
struct ref_array *array = get_packed_refs(name);
ref = search_ref_array(array, refname);
if (ref != NULL) {
memcpy(result, ref->sha1, 20);
retval = 0;
}
return retval;
}
The problem is that the parameter "name" is not NUL-terminated. The old
code turned it into a (NUL-terminated) filename via
strcpy(name + pathlen, "packed-refs");
but the new code passes it (unterminated) to get_packed_refs()
Coincidentally, the old (correct) behavior is restored by a patch that I
submitted earlier today:
"Pass a (ref_cache *) to the resolve_gitlink_*() helper functions".
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [PATCH] gitweb: provide a way to customize html headers
From: Jakub Narebski @ 2011-10-17 11:56 UTC (permalink / raw)
To: Lénaïc Huard; +Cc: git
In-Reply-To: <201110170928.56075.lenaic@lhuard.fr.eu.org>
On Mon, 17 Oct 2011, Lénaïc Huard wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>
> > Lénaïc Huard <lenaic@lhuard.fr.eu.org> writes:
>
> > > This allows web sites to add some specific html headers to the pages
> > > generated by gitweb.
>
> > What do you need this for?
>
> I want to decorate the gitweb pages with the “Google Analytics” tracking code.
> In order to do so, today, Google is recommending to add a <script> tag just
> before the closing </head> tag.
>
> https://www.google.com/support/analyticshelp/bin/answer.py?answer=1008080&hl=en
Hmmm... the modern recommendation from both Google and Yahoo is to put
script tags at the end of HTML, just before closing </body>, which you
can do nowadays with $site_footer / GITWEB_SITE_FOOTER.
But I guess that analytics script needs to be loaded earlier.
> > > The new variable $site_htmlheader can be set to a filename the content
> > > of which will be inserted at the end of the <head> section of each
> > > page generated by gitweb.
>
> > Hmmm... I wonder if a file with html header fragment (which is quite
> > specific subset of HTML) is a best solution.
>
> That’s true. The piece of code to be inserted in <head> is maybe small enough
> so that we don’t need a file. Maybe $site_htmlheader could contain directly
> the html snippet to be inserted in the pages?
I think it might be a better solution.
> > > ---
> > > gitweb/INSTALL | 3 +++
>
> > Nb. there is patch in flight adding gitweb.conf(5) and gitweb(1)
> > manpages...
>
> Ok. So, I’ll update them once a decision will be taken concerning this
> $site_htmlheader.
You might have to wait a bit till patches containing gitweb.conf(5)
manpage are merged in, and rebase your patch to add information about
new config variable not to gitweb/INSTALL, but to Documentation/gitweb.conf.txt
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH 6/6] revert: Simplify passing command-line arguments around
From: Ramkumar Ramachandra @ 2011-10-17 10:10 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Tay Ray Chuan, Git List, Junio C Hamano, Jeff King,
Daniel Barkalow, Christian Couder
In-Reply-To: <20111009092432.GB29150@elie.hsd1.il.comcast.net>
Hi Jonathan,
Jonathan Nieder writes:
> Thanks, but I shouldn't have had to ask. Care to fix the commit
> message? :)
Ah, yes- my persistent lack of clarity in commit messages :|
Hopefully, the next iteration will have better commit messages.
-- Ram
^ permalink raw reply
* Re: regression in git-gui since 2c5c66b... Merge branch 'jp/get-ref-dir-unsorted
From: Mark Levedahl @ 2011-10-17 10:07 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Junio C Hamano, git
In-Reply-To: <4E9BA39B.709@alum.mit.edu>
On 10/16/2011 11:40 PM, Michael Haggerty wrote:
> Please bear with me because I don't use git-gui so I don't really know
> what to expect.
>
> When I check out 2c5c66b and run the above script (actually, the script
> listed below) what I see in git-gui is:
>
> * In the "Unstaged Changes" window, "sub" is listed (not "sub/a").
>
> * When I click on "sub", then in the "Untracked, not staged" window,
> "Git Repository (subproject)" appears.
>
> I see the exact same thing when I run the same test script on the
> version before merge 2c5c66b.
>
> What do you see?
>
> What do you expect to see?
>
> What versions of git, exactly, are you testing (what version do you
> consider "good"; presumably it is version 2c5c66b that you consider "bad")?
>
> Are you certain that you are using the same git version for all commands
> ("git", "git-gui", and "git-new-workdir")? Please especially note that
> git-new-workdir is not part of a default git install, and therefore it
> would be easy to accidentally use a different version of this script
> than of the other commands.
>
> Michael
>
> #!/bin/bash
>
> SRC=$(cd $(dirname $0); pwd)
> GIT=$SRC/git
> GIT_NEW_WORKDIR=$SRC/contrib/workdir/git-new-workdir
> GITGUI=$SRC/git-gui/git-gui
>
> rm -rf super sub
> mkdir super sub
> cd sub
> $GIT init
> touch a
> $GIT add a
> $GIT commit -m 'file' a
> $GIT pack-refs --all
> cd ../super
> $GIT init
> $GIT_NEW_WORKDIR ../sub sub
> $GITGUI&
Michael,
Thanks for looking....
Your modification of my script does not show the error for me, unless I
have *installed* a version of git with the failure: I suspect git-gui
invokes installed components, and not what is in the build directory, so
having a good version of git installed with the bad version in the build
directory does not show the error. And yes, I am quite sure that all of
the git commands I am running are from the one version.
What I expect to see is what you saw: the "sub" directory listed under
Unstaged Changes. What I get when I have installed version 2c5c66b (or
current master) is the file "sub/a" listed under Unstaged Changes, in
other words git-gui no longer recognizes that sub is a submodule.
Mark
^ permalink raw reply
* Re: [PATCH 4/6] revert: Make commit descriptions in insn sheet optional
From: Ramkumar Ramachandra @ 2011-10-17 10:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git List, Jonathan Nieder, Jeff King, Daniel Barkalow,
Christian Couder
In-Reply-To: <7vwrc97wfe.fsf@alter.siamese.dyndns.org>
Hi Junio,
Junio C Hamano writes:
> Like this, perhaps. I did this on top of the whole series only as a
> demonstration but the change should be squashed into this step when the
> series is rerolled.
>
> builtin/revert.c | 47 +++++++++++++++++++----------------------------
> [...]
Thanks! This is fantastic. I'll re-roll soon with better commit messages :)
-- Ram
^ permalink raw reply
* Re: interrupting "git rebase" (Re: git rebase +)
From: Ramkumar Ramachandra @ 2011-10-17 10:03 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Martin von Zweigbergk, Johannes Sixt, Adam Piatyszek, git
In-Reply-To: <20111015194414.GB12893@elie.hsd1.il.comcast.net>
Hi,
Jonathan Nieder writes:
> Martin von Zweigbergk wrote:
>> On Thu, Oct 13, 2011 at 10:26 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>> Johannes Sixt wrote:
>
>>>> Hitting Ctrl-C during git-rebase results undefined behavior.
> [...]
>>> Wait, really? That's bad, and unlike most git commands.
>>
>> If Ctrl-C is pressed while the state is being written, it could be
>> left with incomplete information, yes. It has been like that forever I
>> think. I'll put it on my todo list, but it will not be my top priority
>> (and I have very little git time now anyways).
> [...]
Ah, yes. I've been bitten by this behavior several times myself :)
>>> By the way, what happened to the "git rebase --abort-softly" synonym
>>> for "rm -fr .git/rebase-apply" discussed a while ago?
>>
>> I think we simply did not agree on a syntax, but here was also some
>> discussion about future plans for the sequencer. I remember seeing
>> some discussions about making "git reset --hard" remove the sequencer
>> state, but I don't remember the conclusion. It is not clear to me what
>> is ok to implement in git-rebase nowadays and what would just be
>> double work if it needs to be re-implemented in the sequencer.
> [...]
> Certainly, a lot of sequencer features were inspired by "git rebase".
> Improvements to "git rebase" are only likely to make future
> improvements to "git sequencer" easier. Part of what helps here is
> that "git rebase" is a shell script, so it is a little easier to
> prototype features there.
I concur. Work on `git rebase` should continue independently of the
sequencer because that's where we pick up ideas from! I don't see it
as double-work: simply a translation of ideas. Apart from the fixup
mini-series, the overall interface of the sequencer is still very
unclear to me (see long discussion with Junio, Jonathan).
Yes, 95eb88 (reset: Make reset remove the sequencer state, 2011-08-04)
is merged. But it's pretty unrelated to the main issue at hand: sure,
"reset --hard" is a great hammer, but that shouldn't prevent us from
developing tools and interfaces that are more sophisticated and
elegant, no? In other words, I think "--abort-softly" is a great
idea: we should pour ideas into our shell scripts!
Thanks.
-- Ram
^ 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