Git development
 help / color / mirror / Atom feed
* Re: [PATCH 14/14] wt-status.c: Initialise variable to suppress msvc warning
From: Erik Faye-Lund @ 2010-12-10 15:05 UTC (permalink / raw)
  To: Sebastian Schuberth
  Cc: Junio C Hamano, Ramsay Jones, Jonathan Nieder, Johannes Sixt,
	GIT Mailing-list
In-Reply-To: <AANLkTinty08S2vT9ZSVQW03yL5uzrmqW7k_Ozad-q-E2@mail.gmail.com>

On Fri, Dec 10, 2010 at 2:35 PM, Sebastian Schuberth
<sschuberth@gmail.com> wrote:
> On Thu, Dec 9, 2010 at 20:46, Erik Faye-Lund <kusmabite@gmail.com> wrote:
>
>> On Thu, Dec 9, 2010 at 8:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
>>>
>>>> Junio, could you please drop patches 5-14 from the series; the first four patches
>>>> are the important ones and I'd rather they didn't get held up. Thanks!
>>>
>>> Have these four patches been Acked by interested parties?
>>>
>>> I think I saw 1/N and 2/N acked by Erik and 4/N acked by SSchuberth and
>>> J6t, but any words on 3/N?
>>>
>>> Not that I deeply care nor have environment to test changes to [3/N], but
>>> I am wondering if these need conditional definition to futureproof (e.g.
>>> what happens when the header you are using the definition _I64_MIN from,
>>> or some other headers, started defining these constats?).
>>
>> I'm not sure if I follow this entirely. _I64_MIN is defined by
>> limits.h on Windows, and limits.h has a header-guard (or "#pragma
>> once" as Microsoft-code tends to prefer).
>>
>> Oh, right. You mean if someone else starts defining INTMAX_MAX etc? If
>> someone includes an stdint/inttypes-implementation while including
>> git-compat-util.h, we're going to have a boat-load of similar issues
>> anyway. I think guarding them is something that's better left to when
>> we encounter the problem (if ever).
>
> FYI: In contrast to previous versions, Visual Studio 2010 ships with a
> stdint.h header which defines INTMAX_MAX etc. However, that stdint.h
> is not included by limits.h (in fact, not by *any* other shipping
> header file, as it seems), so we should not run into any trouble even
> with VS2010.
>

Very interesting, thanks. Did you try to compile Git on VS2010? This
sounds like a reason for me to install VS2010 on one of my machines...
:)

^ permalink raw reply

* Re: [PATCH 14/14] wt-status.c: Initialise variable to suppress msvc warning
From: Sebastian Schuberth @ 2010-12-10 15:43 UTC (permalink / raw)
  To: kusmabite
  Cc: Junio C Hamano, Ramsay Jones, Jonathan Nieder, Johannes Sixt,
	GIT Mailing-list
In-Reply-To: <AANLkTikE34sk_bsAZSmq-9MaV-RE+GCJgNszm2o2qHGD@mail.gmail.com>

On Fri, Dec 10, 2010 at 16:05, Erik Faye-Lund <kusmabite@gmail.com> wrote:

>> FYI: In contrast to previous versions, Visual Studio 2010 ships with a
>> stdint.h header which defines INTMAX_MAX etc. However, that stdint.h
>> is not included by limits.h (in fact, not by *any* other shipping
>> header file, as it seems), so we should not run into any trouble even
>> with VS2010.
>>
>
> Very interesting, thanks. Did you try to compile Git on VS2010? This
> sounds like a reason for me to install VS2010 on one of my machines...
> :)

I just gave it a quick try by just importing the VS2008 solution, and
there seem to be some CRT related link problems, but these should be
easy to resolve. I don't have the time right now to look any further
into this, sorry.

-- 
Sebastian Schuberth

^ permalink raw reply

* blame moving&coping detection
From: Vitali @ 2010-12-10 16:11 UTC (permalink / raw)
  To: git

Blame with option -C can detect lines moved or copied from other files.
Any files that were modified in the same commit.

How can I tell git to look for moved or copied lines only in file from
which blamed file was created (copied or renamed)? Is it possible?

^ permalink raw reply

* git svn dcommit fails with empty commits
From: Vallon, Justin @ 2010-12-10 16:25 UTC (permalink / raw)
  To: 'git@vger.kernel.org'

I am trying to get git-svn to cooperate with the master branch by doing some merges after the rebase in dcommit.  This ends up creating an empty merge commit (since the trees are identical).  Then, later git commits appear on top of that merge.

However dcommit has a problem with the empty merge (or an empty commit, to be more accurate).  The problem seems to be around the "No changes" case in git-svn.perl/cmd_dcommit.  If there were no changes (apply_diff returns false), then the editor_cb is not called, and $cmt_rev is not updated.  Then $last_rev is set to $cmt_rev, which is now empty.

It seems either:

1) In editor_cb, just assign to $last_rev directly, and remove the $last_rev setting.
2) Use: $last_rev = $cmt_rev if defined $cmt_rev

-- 
-Justin

^ permalink raw reply

* Re: [PATCH v4 1/3] fetch/pull: Add the --recurse-submodules option
From: Junio C Hamano @ 2010-12-10 17:30 UTC (permalink / raw)
  To: Jens Lehmann
  Cc: Jonathan Nieder, Git Mailing List, Kevin Ballard, Jon Seymour,
	Chris Packham, Marc Branchaud
In-Reply-To: <4D016146.9010907@web.de>

Jens Lehmann <Jens.Lehmann@web.de> writes:

> Am 09.12.2010 22:16, schrieb Junio C Hamano:
> ...
>>> +int fetch_populated_submodules(int num_options, const char **options,
>>> +			       const char *prefix, int quiet)
>>> +{
>>> +	int i, result = 0, argc = 0;
>>> +	struct child_process cp;
>>> +	const char **argv;
>>> +	struct string_list_item *name_for_path;
>>> +	const char *work_tree = get_git_work_tree();
>>> +	if (!work_tree)
>>> +		return 0;
>>> +
>>> +	if (!the_index.initialized)
>>> +		if (read_cache() < 0)
>>> +			die("index file corrupt");
>>> +
>>> +	argv = xcalloc(num_options + 5, sizeof(const char *));
>> 
>> Where is this '5' coming from?  "fetch" "--submodule-prefix", the prefix,
>> and the terminating NULL?  What did I miss?
>
> No, you didn't miss anything but I have been off by one ... '4' is
> sufficient here.

Ok, thanks for double checking.

-- >8 --
Subject: [PATCH] fetch_populated_submodules(): document dynamic allocation size

... while fixing a miscounting.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 submodule.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/submodule.c b/submodule.c
index 4e62900..6f1c107 100644
--- a/submodule.c
+++ b/submodule.c
@@ -264,7 +264,8 @@ int fetch_populated_submodules(int num_options, const char **options,
 		if (read_cache() < 0)
 			die("index file corrupt");
 
-	argv = xcalloc(num_options + 5, sizeof(const char *));
+	/* 4: "fetch" (options) "--submodule-prefix" prefix NULL */
+	argv = xcalloc(num_options + 4, sizeof(const char *));
 	argv[argc++] = "fetch";
 	for (i = 0; i < num_options; i++)
 		argv[argc++] = options[i];

^ permalink raw reply related

* Re: [PATCH v4 1/3] fetch/pull: Add the --recurse-submodules option
From: Jens Lehmann @ 2010-12-10 18:03 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Jonathan Nieder, Git Mailing List, Kevin Ballard, Jon Seymour,
	Chris Packham, Marc Branchaud
In-Reply-To: <7vlj3xedru.fsf@alter.siamese.dyndns.org>

Am 10.12.2010 18:30, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
>> No, you didn't miss anything but I have been off by one ... '4' is
>> sufficient here.
> 
> Ok, thanks for double checking.
> 
> -- >8 --
> Subject: [PATCH] fetch_populated_submodules(): document dynamic allocation size

Thanks and ack.

(This was a leftover from the time the "--recurse-submodules" option had been
added in this function before I moved that into add_options_to_argv())

^ permalink raw reply

* Re: [PATCH] transport-helper: avoid dependency on thread-utils.h
From: Junio C Hamano @ 2010-12-10 18:21 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: git, Ilari Liusvaara
In-Reply-To: <20101210114839.GA5771@burratino>

Jonathan Nieder <jrnieder@gmail.com> writes:

> Since the transport-helper does not use recursive mutexes or ask the
> number of cores, it does not require any declarations from the
> thread-utils lib.  Removing the unnecessary #include avoids false
> positives from "make CHECK_HEADER_DEPENDENCIES=1".

Sorry, I was slow and took me some time to realize what is going on.

Isn't it more like "Makefile does not say transport-helper.o depends on
thread-utils.h, but transport-helper.c does include it"?  IOW, it not a
false positive; perhaps a real but an unnecessary positive.

> The #include directive in question was added in 7851b1e60
> (remote-fd/ext: finishing touches after code review, 2010-11-17),
> at the same times as the necessary #include for <pthread.h>.

I think the whole four-line block was cut & paste from other places that
use the threading, namely grep.c and pack-objects.c.

I wonder if it is better to do something like this instead.  This way, we
can later add more things that threaded part of the system would need to
thread-utils.h inside "ifndef NO_PTHREADS" and they will automatically get
included by all the threaders.


 builtin/grep.c         |    4 ----
 builtin/pack-objects.c |    4 ----
 thread-utils.h         |    4 ++++
 transport-helper.c     |    4 ----
 4 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index adb5424..fdf7131 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -17,11 +17,7 @@
 #include "grep.h"
 #include "quote.h"
 #include "dir.h"
-
-#ifndef NO_PTHREADS
-#include <pthread.h>
 #include "thread-utils.h"
-#endif
 
 static char const * const grep_usage[] = {
 	"git grep [options] [-e] <pattern> [<rev>...] [[--] <path>...]",
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index f027b3a..b0503b2 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -16,11 +16,7 @@
 #include "list-objects.h"
 #include "progress.h"
 #include "refs.h"
-
-#ifndef NO_PTHREADS
-#include <pthread.h>
 #include "thread-utils.h"
-#endif
 
 static const char pack_usage[] =
   "git pack-objects [ -q | --progress | --all-progress ]\n"
diff --git a/thread-utils.h b/thread-utils.h
index 1727a03..6fb98c3 100644
--- a/thread-utils.h
+++ b/thread-utils.h
@@ -1,7 +1,11 @@
 #ifndef THREAD_COMPAT_H
 #define THREAD_COMPAT_H
 
+#ifndef NO_PTHREADS
+#include <pthread.h>
+
 extern int online_cpus(void);
 extern int init_recursive_mutex(pthread_mutex_t*);
 
+#endif
 #endif /* THREAD_COMPAT_H */
diff --git a/transport-helper.c b/transport-helper.c
index 3a50856..4e4754c 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -8,11 +8,7 @@
 #include "quote.h"
 #include "remote.h"
 #include "string-list.h"
-
-#ifndef NO_PTHREADS
-#include <pthread.h>
 #include "thread-utils.h"
-#endif
 
 static int debug;
 

^ permalink raw reply related

* Oops, I screwed it up
From: Péter András Felvégi @ 2010-12-10 18:31 UTC (permalink / raw)
  To: git
In-Reply-To: <AANLkTinyZ-dqV5=yaucLZTj14hdxS61v=VT-j82SqN7r@mail.gmail.com>

Dear All,

I did some rebasing which ended in an unexpected result.

This is what I did precisely:
- added 3 files to the index, then committed (#1)
- realized that a file was left out (not yet tracked), so added + committed it
- stashed the local changes
- used rebase -i to meld the last commit into the previous one (fixup)
- stash pop
- added more files to the index, committed (#2)
- made some minor changes to a file committed in #1, added, committed
- decided to meld into #1
- there were 3 more locally changed files
- stashed them
- rebase -i, moved the last item one line up, set to fixup, so that it
melds into #1
- stash pop: surprise! the stash was empty

git log --stat showed that the 3 stashed files got melded into commit #1, too.

I didn't expect this. Is this the normal operation?

I was able to undo the effects by rebase -i, edit (as in the git
rebase manpage under SPLITTING COMMITS), committing again w/o the 3
files I didn't need, saving them to a temp dir, revert them via
checkout, rebase --continue, then move the files from temp to have
them locally changed again. This is where I would have been if stash
pop succeeded.

The files in commit #1, #2, and the last 3 locally changed ones where disjunct.

I don't know how those 3 locally changed, stashed files ended up in
the commit. I checked my command line history, I wasn't adding them.

Unfortunately, I wasn't able to reproduce the phenomenon on a test
repo, by 3 files:
- commit a, b, c
- change a, commit
- change b, commit
- change a, commit
- change c
- stash c
- rebase -i to meld the two a's, making b the last commit
- stash pop : it worked

Any ideas?

I'm using git version 1.7.2.3 under Debian Squeeze AMD64.

Please CC me, I'm not on the list. Thanks.

Regards, Peter

^ permalink raw reply

* [PATCH 1/2] Add --force to git-send-email documentation
From: Alejandro R. Sedeño @ 2010-12-10 18:44 UTC (permalink / raw)
  To: git; +Cc: Thomas Rast
In-Reply-To: <1291869878-19645-1-git-send-email-asedeno@mit.edu>

Signed-off-by: Alejandro R. Sedeño <asedeno@mit.edu>
---
 Documentation/git-send-email.txt |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index ebc024a..7ec9dab 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -322,6 +322,9 @@ have been specified, in which case default to 'compose'.
 Default is the value of 'sendemail.validate'; if this is not set,
 default to '--validate'.
 
+--force::
+	Send emails even if safety checks would prevent it.
+
 
 CONFIGURATION
 -------------
-- 
1.7.3.3

^ permalink raw reply related

* git-send-email: add some short options and update documentation
From: Alejandro R. Sedeño @ 2010-12-10 18:44 UTC (permalink / raw)
  To: git; +Cc: Thomas Rast
In-Reply-To: <1291869878-19645-1-git-send-email-asedeno@mit.edu>

Add short options that were mentioned in the thread to git-send-email,
specifically:

-n for --dry-run
-f for --force
-q for --quiet

Since --force didn't have documentation in
Documentation/git-send-email.txt, a separate commit adds some
first.

[PATCH 1/2] Add --force to git-send-email documentation
[PATCH 2/2] git-send-email: Add some short options

Documentation/git-send-email.txt |    6 ++++++
git-send-email.perl              |   12 ++++++------
2 files changed, 12 insertions(+), 6 deletions(-)

^ permalink raw reply

* [PATCH 2/2] git-send-email: Add some short options
From: Alejandro R. Sedeño @ 2010-12-10 18:44 UTC (permalink / raw)
  To: git; +Cc: Thomas Rast
In-Reply-To: <1291869878-19645-1-git-send-email-asedeno@mit.edu>

* Accept -n as a synonym for --dry-run
* Accept -f as a synonym for --force
* Accept -q as a synonym for --quiet

Signed-off-by: Alejandro R. Sedeño <asedeno@mit.edu>
---
 Documentation/git-send-email.txt |    3 +++
 git-send-email.perl              |   12 ++++++------
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 7ec9dab..2d0faf2 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -297,6 +297,7 @@ Default is the value of 'sendemail.confirm' configuration value; if that
 is unspecified, default to 'auto' unless any of the suppress options
 have been specified, in which case default to 'compose'.
 
+-n::
 --dry-run::
 	Do everything except actually send the emails.
 
@@ -306,6 +307,7 @@ have been specified, in which case default to 'compose'.
 	or as a file name ('--no-format-patch'). By default, when such a conflict
 	occurs, git send-email will fail.
 
+-q::
 --quiet::
 	Make git-send-email less verbose.  One line per email should be
 	all that is output.
@@ -322,6 +324,7 @@ have been specified, in which case default to 'compose'.
 Default is the value of 'sendemail.validate'; if this is not set,
 default to '--validate'.
 
+-f::
 --force::
 	Send emails even if safety checks would prevent it.
 
diff --git a/git-send-email.perl b/git-send-email.perl
index 76565de..ede7835 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -84,12 +84,12 @@ git send-email [options] <file | directory | rev-list options >
   Administering:
     --confirm               <str>  * Confirm recipients before sending;
                                      auto, cc, compose, always, or never.
-    --quiet                        * Output one line of info per email.
-    --dry-run                      * Don't actually send the emails.
+    -q, --quiet                    * Output one line of info per email.
+    -n, --dry-run                  * Don't actually send the emails.
     --[no-]validate                * Perform patch sanity checks. Default on.
     --[no-]format-patch            * understand any non optional arguments as
                                      `git format-patch` ones.
-    --force                        * Send even if safety checks would prevent it.
+    -f, --force                    * Send even if safety checks would prevent it.
 
 EOT
 	exit(1);
@@ -298,19 +298,19 @@ my $rc = GetOptions("sender|from=s" => \$sender,
 		    "identity=s" => \$identity,
 		    "annotate" => \$annotate,
 		    "compose" => \$compose,
-		    "quiet" => \$quiet,
+		    "quiet|q" => \$quiet,
 		    "cc-cmd=s" => \$cc_cmd,
 		    "suppress-from!" => \$suppress_from,
 		    "suppress-cc=s" => \@suppress_cc,
 		    "signed-off-cc|signed-off-by-cc!" => \$signed_off_by_cc,
 		    "confirm=s" => \$confirm,
-		    "dry-run" => \$dry_run,
+		    "dry-run|n" => \$dry_run,
 		    "envelope-sender=s" => \$envelope_sender,
 		    "thread!" => \$thread,
 		    "validate!" => \$validate,
 		    "format-patch!" => \$format_patch,
 		    "8bit-encoding=s" => \$auto_8bit_encoding,
-		    "force" => \$force,
+		    "force|f" => \$force,
 	 );
 
 unless ($rc) {
-- 
1.7.3.3

^ permalink raw reply related

* [WIP/RFC PATCH] Teach fetch and pull the on-demand mode for submodules
From: Jens Lehmann @ 2010-12-10 18:45 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Kevin Ballard, Jonathan Nieder, Junio C Hamano, Thomas Rast

To be able to recursively checkout submodules it is sufficient to only
then fetch a submodule recursively when the new commits fetched in the
superproject record new commits for it.

Before a new or changed ref from upstream is updated in update_local_ref()
"git rev-list <new-sha1> --not --branches --remotes" is used to determine
all newly fetched commits. These are then walked and diffed against their
parent(s) to see if a submodule has been changed. If that is the case, its
path is stored to fetch it after the superproject fetch is completed. This
is the new default behavior for fetch and pull.

Using the "--recurse-submodules" or the "--no-recurse-submodules" option
disables the examination of the fetched refs because the result will be
ignored anyway.

Because of the currently lacking infrastructure for storing deleted and
new submodules in the .git directory of the superproject fetch and pull
can currently only fetch submodules that are already checked out.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---


So here is the first usable iteration of on-demand fetching for submodules
for people to test and comment on.

Still missing:

1) A new "on-demand" (other suggestions are welcome) value for the
   recurse[-]submodules command line and config options.

2) Tests. This patch currently breaks 5 tests and needs some of its own.

3) Optimizations:

   a) Check if the submodule commit is already present locally and don't
      add it to the to-be-fetched-list then.

   b) It hurts me to do the same computations again locally that the remote
      side already did to decide what commits to send. But not being very
      familiar with revision walking and transport I couldn't come up with
      a better solution for now. Ideas welcome.

And when we start implementing deletion and creation of submodules where
the .git directories have to live in the superprojects .git this code can
be extended to fetch into those directories when the submodule isn't even
checked out.

This patch applies to next.


 builtin/fetch.c |   11 ++++++++-
 submodule.c     |   66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 submodule.h     |    1 +
 3 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 357f3cd..417fe77 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -284,6 +284,9 @@ static int update_local_ref(struct ref *ref,
 		else {
 			msg = "storing head";
 			what = "[new branch]";
+			if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
+			    (recurse_submodules != RECURSE_SUBMODULES_ON))
+				check_for_new_submodule_commits(ref->new_sha1);
 		}

 		r = s_update_ref(msg, ref, 0);
@@ -299,6 +302,9 @@ static int update_local_ref(struct ref *ref,
 		strcpy(quickref, find_unique_abbrev(current->object.sha1, DEFAULT_ABBREV));
 		strcat(quickref, "..");
 		strcat(quickref, find_unique_abbrev(ref->new_sha1, DEFAULT_ABBREV));
+		if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
+		    (recurse_submodules != RECURSE_SUBMODULES_ON))
+			check_for_new_submodule_commits(ref->new_sha1);
 		r = s_update_ref("fast-forward", ref, 1);
 		sprintf(display, "%c %-*s %-*s -> %s%s", r ? '!' : ' ',
 			TRANSPORT_SUMMARY_WIDTH, quickref, REFCOL_WIDTH, remote,
@@ -310,6 +316,9 @@ static int update_local_ref(struct ref *ref,
 		strcpy(quickref, find_unique_abbrev(current->object.sha1, DEFAULT_ABBREV));
 		strcat(quickref, "...");
 		strcat(quickref, find_unique_abbrev(ref->new_sha1, DEFAULT_ABBREV));
+		if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
+		    (recurse_submodules != RECURSE_SUBMODULES_ON))
+			check_for_new_submodule_commits(ref->new_sha1);
 		r = s_update_ref("forced-update", ref, 1);
 		sprintf(display, "%c %-*s %-*s -> %s  (%s)", r ? '!' : '+',
 			TRANSPORT_SUMMARY_WIDTH, quickref, REFCOL_WIDTH, remote,
@@ -951,7 +960,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 		int num_options = 0;
 		/* Set recursion as default when we already are recursing */
 		if (submodule_prefix[0])
-			set_config_fetch_recurse_submodules(1);
+			set_config_fetch_recurse_submodules(recurse_submodules == RECURSE_SUBMODULES_ON ? 1 : -1);
 		gitmodules_config();
 		git_config(submodule_config, NULL);
 		add_options_to_argv(&num_options, options);
diff --git a/submodule.c b/submodule.c
index 4e62900..54eda96 100644
--- a/submodule.c
+++ b/submodule.c
@@ -12,7 +12,8 @@
 struct string_list config_name_for_path;
 struct string_list config_fetch_recurse_submodules_for_name;
 struct string_list config_ignore_for_name;
-static int config_fetch_recurse_submodules;
+static int config_fetch_recurse_submodules = -1;
+struct string_list changed_submodule_paths;

 static int add_submodule_odb(const char *path)
 {
@@ -248,6 +249,66 @@ void set_config_fetch_recurse_submodules(int value)
 	config_fetch_recurse_submodules = value;
 }

+static void submodule_collect_changed_cb(struct diff_queue_struct *q,
+					 struct diff_options *options,
+					 void *data)
+{
+	int i;
+	for (i = 0; i < q->nr; i++) {
+		struct diff_filepair *p = q->queue[i];
+		if (S_ISGITLINK(p->two->mode)) {
+			if (S_ISGITLINK(p->one->mode)) {
+				/* FIXME: We should be using the name configured in the
+				 * .gitmodules file of the commit we are examining here to
+				 * be able to follow moved submodules. */
+				struct string_list_item *path;
+				path = unsorted_string_list_lookup(&changed_submodule_paths, p->two->path);
+				if (!path)
+					string_list_append(&changed_submodule_paths, xstrdup(p->two->path));
+			} else {
+				/* Submodule is new or was moved here */
+
+				/* FIXME: When the .git directories of submodules live inside
+				 * the superprojects .git directory some day we should fetch
+				 * new submodules into that location too so they can be
+				 * checked out from there when needed. */
+				return;
+			}
+		}
+	}
+}
+
+void check_for_new_submodule_commits(unsigned char new_sha1[20])
+{
+	struct rev_info rev;
+	struct commit *commit;
+	int argc = 5;
+	const char *argv[] = {NULL, NULL, "--not", "--branches", "--remotes", NULL};
+
+	init_revisions(&rev, NULL);
+	argv[1] = xstrdup(sha1_to_hex(new_sha1));
+	setup_revisions(argc, argv, &rev, NULL);
+	if (prepare_revision_walk(&rev))
+		die("revision walk setup failed");
+
+	while ((commit = get_revision(&rev))) {
+		struct commit_list *parent = commit->parents;
+		while (parent) {
+			struct diff_options diff_opts;
+			diff_setup(&diff_opts);
+			diff_opts.output_format |= DIFF_FORMAT_CALLBACK;
+			diff_opts.format_callback = submodule_collect_changed_cb;
+			if (diff_setup_done(&diff_opts) < 0)
+				die("diff-setup");
+			diff_tree_sha1(parent->item->object.sha1, commit->object.sha1, "", &diff_opts);
+			diffcore_std(&diff_opts);
+			diff_flush(&diff_opts);
+			parent = parent->next;
+		}
+	}
+	free((char *)argv[1]);
+}
+
 int fetch_populated_submodules(int num_options, const char **options,
 			       const char *prefix, int ignore_config,
 			       int quiet)
@@ -300,6 +361,9 @@ int fetch_populated_submodules(int num_options, const char **options,
 			} else {
 				if (!config_fetch_recurse_submodules)
 					continue;
+				if ((config_fetch_recurse_submodules < 0) &&
+				    !unsorted_string_list_lookup(&changed_submodule_paths, ce->name))
+					continue;
 			}
 		}

diff --git a/submodule.h b/submodule.h
index 4729023..a574f11 100644
--- a/submodule.h
+++ b/submodule.h
@@ -14,6 +14,7 @@ void show_submodule_summary(FILE *f, const char *path,
 		unsigned dirty_submodule,
 		const char *del, const char *add, const char *reset);
 void set_config_fetch_recurse_submodules(int value);
+void check_for_new_submodule_commits(unsigned char new_sha1[20]);
 int fetch_populated_submodules(int num_options, const char **options,
 			       const char *prefix, int ignore_config,
 			       int quiet);
-- 
1.7.3.3.711.g95750

^ permalink raw reply related

* Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators
From: Jonathan Nieder @ 2010-12-10 19:03 UTC (permalink / raw)
  To: Nguyen Thai Ngoc Duy
  Cc: Junio C Hamano, Jakub Narebski, git, Kevin Ballard, Yann Dirson,
	Jeff King
In-Reply-To: <AANLkTimg4Cy83gjzgC3fwg+t=B57b6gzOE98p-=2AR+K@mail.gmail.com>

Nguyen Thai Ngoc Duy wrote:

> Can we use ! modifier for other ^{} too? What I have in mind is how to
> say ^{commit} that has two parents. Or even better, "search from the
> given tip for a commit that has two parents and the commit message
> matches 'foo'". Hmm.. too complex. Perhaps "^{grep: <grep arguments>}"
> that pulls the whole git-grep functionality in.

My thoughts, in no particular order:

 - '!' can be a pain in the neck to supply on the bash command line.
   Single quotes and backslash quoting work while double quotes do
   not, unless 'set +H' has been run.  But that's not a huge deal and
   arguably it's a bash misfeature.

 - What is the intended use for this family of modifiers?  I sort
   of understand ^{:i/... } for people that forget what case they
   have used, but why the :nth and others?

 - Why do we have to carve out the namespace right away, anyway?  If
   we just ^{/... } for the ordinary "start here" search, that leaves
   room for anything after the { other than "/<pattern>" and a few
   fixed strings like "upstream", "tree", etc, right?  It might be
   easier to anticipate what syntax will be useful when there is
   functionality to go with it.

^ permalink raw reply

* Re: git notes and git-commit
From: Jonathan Nieder @ 2010-12-10 19:11 UTC (permalink / raw)
  To: Johan Herland; +Cc: git, Nguyen Thai Ngoc Duy
In-Reply-To: <201012101513.43890.johan@herland.net>

Johan Herland wrote:

> That said, there might well be good use cases for this (e.g. using notes 
> to store data types - e.g. binary data - that cannot be part of the 
> commit message)

A small clarification: Can't binary data be part of the commit
message, too?  I suppose you mean "e.g. using notes to store data -
e.g. build products - that would be obnoxious to be unconditionally
shown by git log".

^ permalink raw reply

* Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators
From: Jakub Narebski @ 2010-12-10 19:26 UTC (permalink / raw)
  To: Jonathan Nieder
  Cc: Nguyen Thai Ngoc Duy, Junio C Hamano, git, Kevin Ballard,
	Yann Dirson, Jeff King
In-Reply-To: <20101210190332.GA6210@burratino>

On Fri, 10 Dec 2010 20:03, Jonathan Nieder wrote:
> Nguyen Thai Ngoc Duy wrote:
> 
> > Can we use ! modifier for other ^{} too? What I have in mind is how to
> > say ^{commit} that has two parents. Or even better, "search from the
> > given tip for a commit that has two parents and the commit message
> > matches 'foo'". Hmm.. too complex. Perhaps "^{grep: <grep arguments>}"
> > that pulls the whole git-grep functionality in.
> 
> My thoughts, in no particular order:
> 
>  - '!' can be a pain in the neck to supply on the bash command line.
>    Single quotes and backslash quoting work while double quotes do
>    not, unless 'set +H' has been run.  But that's not a huge deal and
>    arguably it's a bash misfeature.
> 
>  - What is the intended use for this family of modifiers?  I sort
>    of understand ^{:i/... } for people that forget what case they
>    have used, but why the :nth and others?

* :nth(N) and :g, to show nth match and all matches, respectively; might
  be not necessary as it impinges a bit on --grep=<regexp> domain
* :N / :notesRef to search also through attached notes

If we was possible, perhaps also

* :b / :basechar to ignore accents and other marks (according to commit
  encoding).


-- 
Jakub Narebski
Poland

^ permalink raw reply

* [PATCH 0/2] Re: transport-helper: avoid dependency on thread-utils.h
From: Jonathan Nieder @ 2010-12-10 19:46 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Ilari Liusvaara
In-Reply-To: <7vhbeleber.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:

> I wonder if it is better to do something like this instead.

Yes, I think so.  How about this?

Jonathan Nieder (1):
  Makefile: transport-helper uses thread-utils.h

Junio C Hamano (1):
  simplify inclusion of pthread.h

 Makefile               |    3 +--
 builtin/grep.c         |    4 ----
 builtin/pack-objects.c |    4 ----
 thread-utils.h         |    4 ++++
 transport-helper.c     |    4 ----
 5 files changed, 5 insertions(+), 14 deletions(-)

-- 
1.7.2.4

^ permalink raw reply

* [PATCH 1/2] simplify inclusion of pthread.h
From: Jonathan Nieder @ 2010-12-10 19:47 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Ilari Liusvaara
In-Reply-To: <20101210194637.GA6801@burratino>

In v1.7.1.1~56^2 (Thread-safe xmalloc and xrealloc needs a recursive
mutex, 2010-04-08), the thread-utils header started using
pthread_mutex_t, so callers, which generally had been using

	#ifndef NO_PTHREADS
	#include "thread-utils.h"
	#include <pthread.h>
	#endif

before, were changed to include <pthread.h> first.  It is simpler to
let thread-utils.h take care of the whole matter.  This way, we can
later add more things that the threaded part of the system would need
to thread-utils.h inside "ifndef NO_PTHREADS" and they will
automatically get included by all headers.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/grep.c         |    4 ----
 builtin/pack-objects.c |    4 ----
 thread-utils.h         |    4 ++++
 transport-helper.c     |    4 ----
 4 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index adb5424..fdf7131 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -17,11 +17,7 @@
 #include "grep.h"
 #include "quote.h"
 #include "dir.h"
-
-#ifndef NO_PTHREADS
-#include <pthread.h>
 #include "thread-utils.h"
-#endif
 
 static char const * const grep_usage[] = {
 	"git grep [options] [-e] <pattern> [<rev>...] [[--] <path>...]",
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index f027b3a..b0503b2 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -16,11 +16,7 @@
 #include "list-objects.h"
 #include "progress.h"
 #include "refs.h"
-
-#ifndef NO_PTHREADS
-#include <pthread.h>
 #include "thread-utils.h"
-#endif
 
 static const char pack_usage[] =
   "git pack-objects [ -q | --progress | --all-progress ]\n"
diff --git a/thread-utils.h b/thread-utils.h
index 1727a03..6fb98c3 100644
--- a/thread-utils.h
+++ b/thread-utils.h
@@ -1,7 +1,11 @@
 #ifndef THREAD_COMPAT_H
 #define THREAD_COMPAT_H
 
+#ifndef NO_PTHREADS
+#include <pthread.h>
+
 extern int online_cpus(void);
 extern int init_recursive_mutex(pthread_mutex_t*);
 
+#endif
 #endif /* THREAD_COMPAT_H */
diff --git a/transport-helper.c b/transport-helper.c
index 3a50856..4e4754c 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -8,11 +8,7 @@
 #include "quote.h"
 #include "remote.h"
 #include "string-list.h"
-
-#ifndef NO_PTHREADS
-#include <pthread.h>
 #include "thread-utils.h"
-#endif
 
 static int debug;
 
-- 
1.7.2.4

^ permalink raw reply related

* [PATCH 2/2] Makefile: transport-helper uses thread-utils.h
From: Jonathan Nieder @ 2010-12-10 19:48 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Ilari Liusvaara
In-Reply-To: <20101210194637.GA6801@burratino>

transport-helper.o gained a dependency on thread-utils.h in
7851b1e (remote-fd/ext: finishing touches after code review,
2010-11-17).

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Makefile |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 1fe1a3b..41ab490 100644
--- a/Makefile
+++ b/Makefile
@@ -1937,13 +1937,12 @@ builtin/branch.o builtin/checkout.o builtin/clone.o builtin/reset.o branch.o tra
 builtin/bundle.o bundle.o transport.o: bundle.h
 builtin/bisect--helper.o builtin/rev-list.o bisect.o: bisect.h
 builtin/clone.o builtin/fetch-pack.o transport.o: fetch-pack.h
-builtin/grep.o: thread-utils.h
+builtin/grep.o builtin/pack-objects.o transport-helper.o: thread-utils.h
 builtin/send-pack.o transport.o: send-pack.h
 builtin/log.o builtin/shortlog.o: shortlog.h
 builtin/prune.o builtin/reflog.o reachable.o: reachable.h
 builtin/commit.o builtin/revert.o wt-status.o: wt-status.h
 builtin/tar-tree.o archive-tar.o: tar.h
-builtin/pack-objects.o: thread-utils.h
 connect.o transport.o http-backend.o: url.h
 http-fetch.o http-walker.o remote-curl.o transport.o walker.o: walker.h
 http.o http-walker.o http-push.o http-fetch.o remote-curl.o: http.h
-- 
1.7.2.4

^ permalink raw reply related

* Re: [PATCH 1/2] simplify inclusion of pthread.h
From: Jonathan Nieder @ 2010-12-10 19:51 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Ilari Liusvaara
In-Reply-To: <20101210194754.GB6801@burratino>

Jonathan Nieder wrote:

> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

This one is of course

	From: Junio C Hamano <gitster@pobox.com>

Often enough I remind myself to include the pseudo-header but when the
rapid cutting-and-pasting time comes it is easy to forget. :/

^ permalink raw reply

* Re: What's cooking in git.git (Dec 2010, #01; Sat, 4)
From: Junio C Hamano @ 2010-12-10 19:55 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, Aleksi Aalto, git
In-Reply-To: <20101209172708.GA1817@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> Subject: [PATCH] default color.status.branch to "same as header"
>
> This gives it the same behavior as we had prior to 1d28232
> (status: show branchname with a configurable color).
>
> To do this we need the concept of a "NIL" color, which is
> provided by color.[ch]. The implementation is very simple;
> in particular, there are no precautions taken against code
> accidentally printing the NIL. This should be fine in
> practice because:
>
>   1. You can't input a NIL color in the config, so it must
>      come from the in-code defaults. Which means it is up
>      the client code to handle the NILs it defines.
>
>   2. If we do ever print a NIL, it will be obvious what the
>      problem is, and the bug can be fixed.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I resisted the urge to make a generic "same as $X" token, which would
> allow users to do something like:
>
>   [color "status"]
>     branch = from:color.status.header
>
> if they really wanted. But that would be a lot more code, and I'm not
> sure it would be all that useful (it would be if people did stuff like
> theming git colors like they do window managers, but I don't think we
> are at quite that level).

Also if you go that route you would need to worry about dependencies,
which would not be worth it.

> This is simple, solves the current regression, and provides an easy
> blueprint for handling the case in the future.

As I said, I don't care deeply, but you obviously cared enough to produce
a patch that is pretty simple and straightforward.  Let's take it.

Thanks.

^ permalink raw reply

* Re: git-cvsimport with cvsps output in commit msg breaks imports
From: Andreas Schwab @ 2010-12-10 19:58 UTC (permalink / raw)
  To: Thomas Adam; +Cc: git, cvsps, smurf
In-Reply-To: <20101209220347.GA3180@shuttle.home>

Thomas Adam <thomas@xteddy.org> writes:

> More specifically, the commit which I think introduced the problem, and all
> subsequent commits thereafter is here:
>
> https://github.com/ThomasAdam/tmux/commit/f0220a10b01a764e0dc52ea1b2407f58600a30eb
>
> Note that from this commit onwards, the commit *message* has a bunch of
> cvsps output in it.  I can only surmise that this somehow causes problems
> for cvsimport.

The cvsps output is ambigous.  If the commit log includes text that
resembles the patchset format generated by cvsps then git-cvsimport can
easily get confused.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

^ permalink raw reply

* Re: [PATCH 17/18] gitweb: Prepare for cached error pages & better error page handling
From: Jakub Narebski @ 2010-12-10 20:33 UTC (permalink / raw)
  To: J.H.; +Cc: git
In-Reply-To: <4D01E5CC.6010301@eaglescrag.net>

On Fri, 10 Dec 2010, J.H. wrote:

> > There is no problem with capturing output of die_error, nor there is a
> > problem with caching error pages (perhaps transiently in memory).
> > 
> > The problem is that subroutines calling die_error assum that it would
> > exit ending subroutine that is responsible for generating current
> > action; see "goto DONE_GITWEB" which should be "goto DONE_REQUEST",
> > and which was "exit 0" some time ago at the end of die_error().
> > 
> > With caching error pages you want die_error to exit $actions{$action}->(),
> > but not exit cache_fetch().  How do you intend to do it?
> 
> Well there's one bug in how that function ends in looking at it again,
> basically the return case shouldn't happen, and that function should
> end, like your suggesting in the first part of your question (with
> respect to DONE_GITWEB)
> 
> In the second part, your not thinking with the fork() going (though in
> thinking sans the fork this might not work right).
> 
> It's the background process that will call die_error in such a way that
> die_error_cache will get invoked.  die_error_cache will write the .err
> file out, and the whole thing should just exit.
> 
> Though now that I say that there's an obvious bug in the case where
> forking didn't work at all, in that case you would get a blank page as
> the connection would just be closed.  If you refreshed (say hitting F5)
> you'd get the error at that point.
> 
> Need to fix that non-forked problem though.

Well, if you, the author, cannot follow code flow of your own code, what
does it matter for being sure that this code is bug free?  What does this
matter for maintability of this code?
 

That rant aside, error / exception handling in gitweb is currently not
fitting well with output caching, at least the locking one.

die_error() functions as a kind of exception handling; we rely that on
the fact that calling die_error() would end request, independent on how
deep in the stack we are.  Originally die_error() ended with 'exit',
which ModPerl::Registry redefined for it to end request and not exit
worker.  Then 'exit' was replaced by 'goto DONE_GITWEB' to jump out of
several levels of calls; I didn't know then about ModPerl::Registry
redefining 'exit'... and actually it should be 'goto DONE_REQUEST', like
in "[PATCH/RFC] gitweb: Go to DONE_REQUEST rather than DONE_GITWEB in
die_error"

  http://permalink.gmane.org/gmane.comp.version-control.git/162156

It is because die_error is exception mechanism, and in current incarnation
always ends request, that is why error pages (generated by die_error) were
not cached: we jump out of capturing and out of caching.  The additional
reasoning is that we don't want to bloat cache with error pages, which 
IMHO usually gets requested only once (assumption: different clients makes
different errors).

Now in most cases the approach taken to modify die_error for caching only
by adding explicit turning off capturing at the beginning of die_error is
enough.

1. Single client, no generating in background (note that if given URL is
   never cached, we would not invoke background generation to refresh shown
   stale data - there wouldn't be stale data).

   In this case if there is an expected error, die_error() gets explicitely
   invoked, turns off capturing, prints error page to client, and ends
   request.

   In the case of uncaught "die", it would be caught by CGI::Carp error
   handler, and passed to handle_errors_html() subroutine (thanks to gitweb
   using set_message(\&handle_errors_html)), which runs die_error() with
   options making it not print HTTP header (which was already printed by
   CGI::Carp error handler), and not exit - the CGI::Carp error handler
   would end request instead.  die_error() turns of capturing, prints
   error page, and CGI::Carp error handler ends request.

2. Two clients arriving at exactly the same error (same link), at the
   same time.  This is quite unlikely.

   In my rewrite there is loop in ->compute method in rewritten caching
   engine, which reads:

      do {
          ...
      } until (<received data to show to user> || <tried to generate data ourself>);

   This means that one client acquires writers lock, die_error prints error
   page and exists, second client notices that it didn't get anything but
   didn't try it itself yet, and dies itself on die_error()

   Dealing with "die"-ing works the same as in the case described above,
   so there is no problem from this area neither.

   Alternate solution would be to treat it as the case described below.

3. Gitweb runs generating cache entry in background.  Note that if error
   pages are not cached, there would be no stale pages to serve while 
   regenerating data in background - so entering background process can
   be done only thanks to "Generating..." page.
   
   We can try _prevent this from happening_, as I did in my rewrite by
   introducing initial/startup delay in "Generating..." (which has also
   other reasons to use), or via 'generating_info_is_safe' protection.

   Otherwise we need to pass error page from background process to
   foreground proces that is showing "Generating..." page; well, to be
   more exact, with current workings of "Generating..." it would be its
   successor (next request, after reload / refresh).

   Note: the fact that it is *next request* that needs an error page
   (otherwise we would show "Generating..." page yet again).

   So what die_error needs to do if it finds itself in the background
   process (perhaps explicit $background boolean variable, perhaps
   comparing $$ with $my_pid, perhaps checking if STDOUT is closed)
   it needs to somehow write cache entry, perhaps in a special way
   marking it as error page.  The problem is to do it in generic way,
   that would not make it impossible to use other caching engine, or
   other capturing engine, in the future.

   Note also that at the end of background process (perhaps at the
   end of die_error) we need to exit process, and not just end request,
   so we should use 'CORE::exit(0);'.

The problem with 3rd case makes me think that it is high time that
die_error use Perl 5 exception throwing and handling mechanism, namely
"die" (for throwing errors, to be used in die_error), and "eval BLOCK"
(to catch errors).

As proposed on #perl channel when asking about this situation, die_error
would use 'die \$DIE_ERROR' to throw reference, or throw an object, to
easy distinguish between handled error from die_error, and unhandled
error from Perl (where we assume that all errors are strings).

run_request() or run() would then use 'eval { ... }', which has the
additional advantage that we can get rid of CGI::Carp::set_message,
which doesn't allow to use custom HTTP status, and supposedly doesn't
work with mod_perl 2.0.  Instead of adding capture_stop() to die_error(),
the capture mechanism should use 'eval { ... }', and just print response
if there was exception (like Capture::Tiny does)... or return captured
error page to be cached in the case of being in background process.


Well, any way we choose to handle it, the code should be very clear,
and handle all cases (other caching engines, perhaps also other capture
engines, non-persistent and persistent environments, redefined 'exit'
like in ModPerl::Registry case, not redefined 'exit' like I think in
FastCGI case, etc., etc.).

>>> This adds two functions:
>>>
>>> die_error_cache() - this gets back called from die_error() so
>>> that the error message generated can be cached.
>> 
>> *How* die_error_cache() gets called back from die_error()?  I don't
>> see any changes to die_error(), or actually any calling sites for
>> die_error_cache() in the patch below.
>>  
>>> cacheDisplayErr() - this is a simplified version of cacheDisplay()
>>> that does an initial check, if the error page exists - display it
>>> and exit.  If not, return.
>> 
>> Errr... isn't it removed in _preceding_ patch?  WTF???
> 
> in breaking up the series it got included in the wrong spot, and
> apparently removed and re-added correctly, should be fixed in v9

[...]
> 
> If you'd rather I can squash 17 & 18 into a single commit.

Yes, please.  Splitting those changes into 17 & 18 didn't make it more
clear (usually smaller commit == easier to review), but rather less
transparent.
 
>>> +sub die_error_cache {
>>> +	my ($output) = @_;
>>> +
>>> +	open(my $cacheFileErr, '>:utf8', "$fullhashpath.err");
>>> +	my $lockStatus = flock($cacheFileErr,LOCK_EX|LOCK_NB);
>> 
>> Why do you need to lock here?  A comment would be nice.
> 
> At any point when a write happens there's the potential for multiple
> simultaneous writes.  Locking becomes obvious, when your trying to
> prevent multiple processes from writing to the same thing at the same
> time...

Or you can use 'write to File::Temp::tempfile, rename file' trick for
atomic update to file.  I use it in early commits in my rewrite of gitweb
caching, see e.g.:

  http://repo.or.cz/w/git/jnareb-git.git/blob/refs/heads/gitweb/cache-kernel-v6:/gitweb/lib/GitwebCache/SimpleFileCache.pm#l362
 
>>> +
>>> +	if (! $lockStatus ){
>>> +		if ( $areForked ){
>> 
>> Grrrr...

Global variables, action at considerable distance.
 
>> But if it is here to stay, a comment if you please.
>> 
>>> +			exit(0);
>>> +		}else{
>>> +			return;
>>> +		}
>>> +	}
> 
> The exit(0) or return have been removed in favor of DONE_GITWEB, as
> we've already errored if we are broken here we should just die.

Note the difference between exit and Core::exit, when running gitweb
from mod_perl using ModPerl::Registry handler.
 

>>> +
>>> +	flock($cacheFileErr,LOCK_UN);
>>> +	close($cacheFileErr);
>> 
>> Closing file will unlock it.
> 
> Doesn't really hurt to be explicit though.

O.K., but please note that I have found LOCK_UN to be unreliable.
 
>>> +
>>> +	if ( $areForked ){
>>> +		exit(0);
>>> +	}else{
>>> +		return;
>> 
>> So die_error_cache would not actually work like "die" here and like
>> die_error(), isn't it?
> 
> that was ejected, it was a bug.  DONE_GITWEB is more correct, though I
> might need to add a hook to display the error message in the case that
> the process didn't fork.

By the way, why do you fork indiscriminately (remember that forking
is not without performance cost), even when background generation is
turned off, or you don't need background generation?
 
Wouldn't fallback on non-background generation if fork() fails, as in
my rewrite of gitweb caching series be a better solution?

>>> +	while( <$cacheFileErr> ){
>>> +		print $_;
>>> +	}
>> 
>> Why not 'print <$cacheFileErr>' (list context), like in insert_file()
>> subroutine?
> 
> I've had buffer problems with 'print <$cacheFileErr>' in some cases.
> This is small enough it shouldn't happen, but I've gotten into the habit
> of doing it this way.  I can change it if you like.

Perhaps

  print while <$cacheFileErr>;

(we use it in already in "print while <$fd>;" in git_blame_common())?

Or, if we use File::Copy, perhaps File::Copy::copy($cacheFileErr, \*STDOUT);
or something like that.
 
-- 
Jakub Narebski
Poland

^ permalink raw reply

* Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators
From: Kevin Ballard @ 2010-12-10 21:21 UTC (permalink / raw)
  To: Jonathan Nieder
  Cc: Nguyen Thai Ngoc Duy, Junio C Hamano, Jakub Narebski, git,
	Yann Dirson, Jeff King
In-Reply-To: <20101210190332.GA6210@burratino>

On Dec 10, 2010, at 11:03 AM, Jonathan Nieder wrote:

> - What is the intended use for this family of modifiers?  I sort
>   of understand ^{:i/... } for people that forget what case they
>   have used, but why the :nth and others?

In my particular case, I was glancing through the logs, and I wanted to grab
the second branch that someone else had made that was merged into pu. I would
have loved to be able to run something like

  git merge origin/pu^{:nth(2)/nd/}

While we're speaking of modifiers, could we use one that says "only search
the first parent hierarchy", e.g. something equivalent to git log's --first-parent
flag?

-Kevin Ballard

^ permalink raw reply

* Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators
From: Jeff King @ 2010-12-10 21:30 UTC (permalink / raw)
  To: Kevin Ballard
  Cc: Jonathan Nieder, Nguyen Thai Ngoc Duy, Junio C Hamano,
	Jakub Narebski, git, Yann Dirson
In-Reply-To: <66D6F30D-4707-4057-BB46-57B2DF01F479@sb.org>

On Fri, Dec 10, 2010 at 01:21:15PM -0800, Kevin Ballard wrote:

> On Dec 10, 2010, at 11:03 AM, Jonathan Nieder wrote:
> 
> > - What is the intended use for this family of modifiers?  I sort
> >   of understand ^{:i/... } for people that forget what case they
> >   have used, but why the :nth and others?
> 
> In my particular case, I was glancing through the logs, and I wanted to grab
> the second branch that someone else had made that was merged into pu. I would
> have loved to be able to run something like
> 
>   git merge origin/pu^{:nth(2)/nd/}
> 
> While we're speaking of modifiers, could we use one that says "only search
> the first parent hierarchy", e.g. something equivalent to git log's --first-parent
> flag?

As neat as this modifier syntax is getting, are we perhaps just
recreating the wheel?

How about:

  git merge `git rev-list -2 --grep=nd/ origin/pu | tail -1`

for the nth one, and:

  git merge `git rev-list --first-parent -1 --grep=nd/ origin/pu`

for a first parent search (I will leave combining them as an exercise to
the reader).

It's not that I'm opposed to a handy ref-specifying syntax. I just
wonder if it is really worth building in all of these obscure scenarios.

-Peff

^ permalink raw reply

* Re: What's cooking in git.git (Dec 2010, #01; Sat, 4)
From: Junio C Hamano @ 2010-12-10 21:59 UTC (permalink / raw)
  To: Miles Bader; +Cc: Yann Dirson, git list
In-Reply-To: <AANLkTinJu0KzXZ2Rjbs2+XH7T=Gq5MOajxo51DHtqoGZ@mail.gmail.com>

Miles Bader <miles@gnu.org> writes:

> Maybe if you renamed every option simultaneously, there would be
> confusion, but seriously, it's only one option.  It's not going to be
> a problem.

Well, let's avoid all of that trouble before it is too late, by putting
this on top of what is in 'next' and ship 1.7.4 with it.

Between "find" and "detect", I do not have much preference either way.  It
may sound more active to "find" them, but if told to "detect" them, git
goes ahead and actively changes its internal behaviour in order to do so,
which amounts to an active "find"ing anyway, so...

-- >8 --
From: Yann Dirson <ydirson@altern.org>
Date: Wed, 10 Nov 2010 21:27:12 +0100
Subject: [PATCH] diff: use "find" instead of "detect" as prefix for long forms of -M and -C

It is more consistent with existing --find-copies-harder; luckily "detect"
variant has not appeared in any officially released version of git.

Signed-off-by: Yann Dirson <ydirson@altern.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/diff-options.txt |    5 ++---
 diff.c                         |   18 +++++++++---------
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 7246e10..c93124b 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -230,7 +230,7 @@ eligible for being picked up as a possible source of a rename to
 another file.
 
 -M[<n>]::
---detect-renames[=<n>]::
+--find-renames[=<n>]::
 ifndef::git-log[]
 	Detect renames.
 endif::git-log[]
@@ -246,12 +246,11 @@ endif::git-log[]
 	hasn't changed.
 
 -C[<n>]::
---detect-copies[=<n>]::
+--find-copies[=<n>]::
 	Detect copies as well as renames.  See also `--find-copies-harder`.
 	If `n` is specified, it has the same meaning as for `-M<n>`.
 
 --find-copies-harder::
---detect-copies-harder::
 	For performance reasons, by default, `-C` option finds copies only
 	if the original file of the copy was modified in the same
 	changeset.  This flag makes the command
diff --git a/diff.c b/diff.c
index dee0bd8..b5ef1ec 100644
--- a/diff.c
+++ b/diff.c
@@ -3150,14 +3150,14 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		if ((options->break_opt = diff_scoreopt_parse(arg)) == -1)
 			return -1;
 	}
-	else if (!prefixcmp(arg, "-M") || !prefixcmp(arg, "--detect-renames=") ||
-		 !strcmp(arg, "--detect-renames")) {
+	else if (!prefixcmp(arg, "-M") || !prefixcmp(arg, "--find-renames=") ||
+		 !strcmp(arg, "--find-renames")) {
 		if ((options->rename_score = diff_scoreopt_parse(arg)) == -1)
 			return -1;
 		options->detect_rename = DIFF_DETECT_RENAME;
 	}
-	else if (!prefixcmp(arg, "-C") || !prefixcmp(arg, "--detect-copies=") ||
-		 !strcmp(arg, "--detect-copies")) {
+	else if (!prefixcmp(arg, "-C") || !prefixcmp(arg, "--find-copies=") ||
+		 !strcmp(arg, "--find-copies")) {
 		if (options->detect_rename == DIFF_DETECT_COPY)
 			DIFF_OPT_SET(options, FIND_COPIES_HARDER);
 		if ((options->rename_score = diff_scoreopt_parse(arg)) == -1)
@@ -3194,7 +3194,7 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		DIFF_OPT_SET(options, TEXT);
 	else if (!strcmp(arg, "-R"))
 		DIFF_OPT_SET(options, REVERSE_DIFF);
-	else if (!strcmp(arg, "--find-copies-harder") || !strcmp(arg, "--detect-copies-harder"))
+	else if (!strcmp(arg, "--find-copies-harder"))
 		DIFF_OPT_SET(options, FIND_COPIES_HARDER);
 	else if (!strcmp(arg, "--follow"))
 		DIFF_OPT_SET(options, FOLLOW_RENAMES);
@@ -3380,12 +3380,12 @@ static int diff_scoreopt_parse(const char *opt)
 			opt += strlen("break-rewrites");
 			if (*opt == 0 || *opt++ == '=')
 				cmd = 'B';
-		} else if (!prefixcmp(opt, "detect-copies")) {
-			opt += strlen("detect-copies");
+		} else if (!prefixcmp(opt, "find-copies")) {
+			opt += strlen("find-copies");
 			if (*opt == 0 || *opt++ == '=')
 				cmd = 'C';
-		} else if (!prefixcmp(opt, "detect-renames")) {
-			opt += strlen("detect-renames");
+		} else if (!prefixcmp(opt, "find-renames")) {
+			opt += strlen("find-renames");
 			if (*opt == 0 || *opt++ == '=')
 				cmd = 'M';
 		}
-- 
1.7.3.3.710.g2d012

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox