Git development
 help / color / mirror / Atom feed
* Re: Possibly solved invalid free() in git-remote-http from Git 1.7.2.1
From: Jeff King @ 2011-08-02  3:33 UTC (permalink / raw)
  To: Tay Ray Chuan; +Cc: Ævar Arnfjörð Bjarmason, Git Mailing List
In-Reply-To: <20110802105124.000021c3@unknown>

On Tue, Aug 02, 2011 at 10:51:24AM +0800, Tay Ray Chuan wrote:

> On Mon, 1 Aug 2011 12:00:19 -0600
> Jeff King <peff@peff.net> wrote:
> > Hmm. The memory management in the function seems weird to me.
> 
> How about this?

Yes, that fixes all of the oddities I found (I assume you looked and
agreed with me that they were wrong).

-Peff

^ permalink raw reply

* Re: [PATCH 17/18] revert: Introduce --continue to continue the operation
From: Christian Couder @ 2011-08-02  2:51 UTC (permalink / raw)
  To: Ramkumar Ramachandra
  Cc: Junio C Hamano, Git List, Jonathan Nieder, Daniel Barkalow,
	Jeff King
In-Reply-To: <201108020447.02545.chriscool@tuxfamily.org>

On Tuesday 02 August 2011 04:47:01 Christian Couder wrote:
> We can simplify this loop using strchrnul() like this:
> 
>  	for (i = 1; *p; i++) {
>  		struct commit *commit = parse_insn_line(p, opts);
>  		if (!commit)
>  			return error(_("Could not parse line %d."), i);
>  		next = commit_list_append(commit, next);
>  		p = strchrnul(p, '\n');
>  	}

Ooops, sorry I mean like this:

	for (i = 1; *p; i++) {
		struct commit *commit = parse_insn_line(p, opts);
		if (!commit)
			return error(_("Could not parse line %d."), i);
		next = commit_list_append(commit, next);
		p = strchrnul(p, '\n');
		if (*p)
			p++;
	}

Thanks,
Christian.

^ permalink raw reply

* Re: Possibly solved invalid free() in git-remote-http from Git 1.7.2.1
From: Tay Ray Chuan @ 2011-08-02  2:51 UTC (permalink / raw)
  To: Jeff King; +Cc: Ævar Arnfjörð Bjarmason, Git Mailing List
In-Reply-To: <20110801180018.GA10636@sigill.intra.peff.net>

On Mon, 1 Aug 2011 12:00:19 -0600
Jeff King <peff@peff.net> wrote:
> Hmm. The memory management in the function seems weird to me.

How about this?

-->8--
diff --git a/http.c b/http.c
index b2ae8de..4da6293 100644
--- a/http.c
+++ b/http.c
@@ -1116,9 +1116,8 @@ struct http_pack_request *new_http_pack_request(
 	struct strbuf buf = STRBUF_INIT;
 	struct http_pack_request *preq;
 
-	preq = xmalloc(sizeof(*preq));
+	preq = xcalloc(1, sizeof(*preq));
 	preq->target = target;
-	preq->range_header = NULL;
 
 	end_url_with_slash(&buf, base_url);
 	strbuf_addf(&buf, "objects/pack/pack-%s.pack",
@@ -1210,7 +1209,7 @@ struct http_object_request *new_http_object_request(const char *base_url,
 	struct curl_slist *range_header = NULL;
 	struct http_object_request *freq;
 
-	freq = xmalloc(sizeof(*freq));
+	freq = xcalloc(1, sizeof(*freq));
 	hashcpy(freq->sha1, sha1);
 	freq->localfile = -1;
 
@@ -1248,8 +1247,6 @@ struct http_object_request *new_http_object_request(const char *base_url,
 		goto abort;
 	}
 
-	memset(&freq->stream, 0, sizeof(freq->stream));
-
 	git_inflate_init(&freq->stream);
 
 	git_SHA1_Init(&freq->c);
@@ -1324,7 +1321,6 @@ struct http_object_request *new_http_object_request(const char *base_url,
 	return freq;
 
 abort:
-	free(filename);
 	free(freq->url);
 	free(freq);
 	return NULL;


--
Cheers,
Ray Chuan

^ permalink raw reply related

* Re: [PATCH 17/18] revert: Introduce --continue to continue the operation
From: Christian Couder @ 2011-08-02  2:47 UTC (permalink / raw)
  To: Ramkumar Ramachandra
  Cc: Junio C Hamano, Git List, Jonathan Nieder, Daniel Barkalow,
	Jeff King
In-Reply-To: <201108020436.42148.chriscool@tuxfamily.org>

On Tuesday 02 August 2011 04:36:41 Christian Couder wrote:
>
> static int parse_insn_buffer(char *buffer,
> 			     struct commit_list **todo_list,
> 			     struct replay_opts *opts)
> {
> 	struct commit_list **next = todo_list;
> 	char *p = buffer;
> 	int i;
> 
> 	for (i = 1; p && *p; i++) {
> 		struct commit *commit = parse_insn_line(p, opts);
> 		if (!commit)
> 			return error(_("Could not parse line %d."), i);
> 		next = commit_list_append(commit, next);
> 		p = strchr(p, '\n');
> 		if (p)
> 			p++;
> 	}

We can simplify this loop using strchrnul() like this:

 	for (i = 1; *p; i++) {
 		struct commit *commit = parse_insn_line(p, opts);
 		if (!commit)
 			return error(_("Could not parse line %d."), i);
 		next = commit_list_append(commit, next);
 		p = strchrnul(p, '\n');
 	}

Thanks,
Christian.

^ permalink raw reply

* Re: [PATCH 17/18] revert: Introduce --continue to continue the operation
From: Christian Couder @ 2011-08-02  2:36 UTC (permalink / raw)
  To: Ramkumar Ramachandra
  Cc: Junio C Hamano, Git List, Jonathan Nieder, Daniel Barkalow,
	Jeff King
In-Reply-To: <1312222025-28453-18-git-send-email-artagnon@gmail.com>

On Monday 01 August 2011 20:07:04 Ramkumar Ramachandra wrote:
>
> +static void read_populate_todo(struct commit_list **todo_list,
> +			struct replay_opts *opts)
> +{
> +	const char *todo_file = git_path(SEQ_TODO_FILE);
> +	struct strbuf buf = STRBUF_INIT;
> +	struct commit_list **next;
> +	struct commit *commit;
> +	char *p;
> +	int fd;
> +
> +	fd = open(todo_file, O_RDONLY);
> +	if (fd < 0)
> +		die_errno(_("Could not open %s."), todo_file);
> +	if (strbuf_read(&buf, fd, 0) < 0) {
> +		close(fd);
> +		strbuf_release(&buf);
> +		die(_("Could not read %s."), todo_file);
> +	}
> +	close(fd);
> +
> +	next = todo_list;
> +	for (p = buf.buf; *p; p = strchr(p, '\n') + 1) {
> +		commit = parse_insn_line(p, opts);
> +		if (!commit)
> +			goto error;
> +		next = commit_list_append(commit, next);
> +	}
> +	if (!*todo_list)
> +		goto error;
> +	strbuf_release(&buf);
> +	return;
> +error:
> +	strbuf_release(&buf);
> +	die(_("Unusable instruction sheet: %s"), todo_file);
> +}

I'd suggest using 2 functions like this:

static int parse_insn_buffer(char *buffer,
			     struct commit_list **todo_list,
			     struct replay_opts *opts)
{
	struct commit_list **next = todo_list;
	char *p = buffer;
	int i;

	for (i = 1; p && *p; i++) {
		struct commit *commit = parse_insn_line(p, opts);
		if (!commit)
			return error(_("Could not parse line %d."), i);
		next = commit_list_append(commit, next);
		p = strchr(p, '\n');
		if (p)
			p++;
	}
	if (!*todo_list)
		return error(_("Could not parse one commit."));
	return 0;
}

static void read_populate_todo(struct commit_list **todo_list,
			       struct replay_opts *opts)
{
	const char *todo_file = git_path(SEQ_TODO_FILE);
	struct strbuf buf = STRBUF_INIT;
	int fd, res;

	fd = open(todo_file, O_RDONLY);
	if (fd < 0)
		die_errno(_("Could not open %s."), todo_file);
	if (strbuf_read(&buf, fd, 0) < 0) {
		close(fd);
		strbuf_release(&buf);
		die(_("Could not read %s."), todo_file);
	}
	close(fd);

	res = parse_insn_buffer(buf.buf, todo_list, opts);
	strbuf_release(&buf);
	if (res)
		die(_("Unusable instruction sheet: %s"), todo_file);
}

Thanks,
Christian.

^ permalink raw reply

* Re: [PATCH 0/5] bisect: Add support for a --no-checkout option.
From: Jon Seymour @ 2011-08-02  1:41 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Christian Couder, git, j6t, jnareb
In-Reply-To: <CAH3AnrrrvX64s3p_=5mrVcwx5FeO0iT8uX47remyCuCwPpOo=A@mail.gmail.com>

On Tue, Aug 2, 2011 at 11:15 AM, Jon Seymour <jon.seymour@gmail.com> wrote:
> On Tue, Aug 2, 2011 at 9:33 AM, Junio C Hamano <gitster@pobox.com> wrote:

>
> How about in the current series, I change the option parsed by
> bisect--helper.c to be:
>
>    --bisect-mode=checkout|update-ref
>
> The porcelain option can still be --no-checkout (or should it be
> --bisect-mode too?). The porcelain will maintain a state variable
> called (rather than BISECT_NO_CHECKOUT).
>
>     BISECT_MODE
>
> Then, the next series can add a --bisect-head option (instead of
> --update-ref) to allow the head to be varied (defaulting to
> BISECT_HEAD) if not specified.

To clarify: --bisect-head would default to HEAD if mode is checkout,
to BISECT_HEAD if the mode is update-ref.

jon.

^ permalink raw reply

* Re: [PATCH 0/5] bisect: Add support for a --no-checkout option.
From: Jon Seymour @ 2011-08-02  1:15 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Christian Couder, git, j6t, jnareb
In-Reply-To: <7vaabshfmb.fsf@alter.siamese.dyndns.org>

On Tue, Aug 2, 2011 at 9:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jon Seymour <jon.seymour@gmail.com> writes:
>
>> On Tue, Aug 2, 2011 at 3:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Jon Seymour <jon.seymour@gmail.com> writes:
>>>
>>>> It might become more important if someone ever writes a tool that does
>>>> a bisection on the user's behalf. In this case, aborting the tool
>>>> might leave the HEAD in, what appears to the user, a confused state.
>> ...
>> In this hypothetical additional series, are you happy for
>> --no-checkout to become a synonym for --update-ref=HEAD in the manner
>> of v8? From a technical perspective, it doesn't seem necessary to
>> duplicate the state variables and parameters.
>
> Well, from a technical perspective, which ref is to be updated is an
> option that is valid _only_ under no-checkout mode, and no-checkout mode
> could be using HEAD, so I think you would need two variables. One for
> "what mode are we running", and the other that is only valid under "we are
> in no-checkout mode" that says "we update BISECT_HEAD".
>
> If no-checkout mode _never_ updates HEAD, because the normal mode _always_
> updates HEAD, you could reduce them into a single variable (i.e. if we are
> updating HEAD then we are in normal mode, otherwise we are in no-checkout
> mode).  I actually have a mild suspicion that the no-checkout mode may
> turn out to be too confusing for people if it by default updates HEAD, and
> we may want to have it update something that is not HEAD by default (or
> forbid --update-ref from specifying HEAD), so if that turns out to be the
> approach we are going to proceed, we _might_ end up needing only one
> variable, but I do not think we know it just yet.  At least I don't.
>

How about in the current series, I change the option parsed by
bisect--helper.c to be:

    --bisect-mode=checkout|update-ref

The porcelain option can still be --no-checkout (or should it be
--bisect-mode too?). The porcelain will maintain a state variable
called (rather than BISECT_NO_CHECKOUT).

     BISECT_MODE

Then, the next series can add a --bisect-head option (instead of
--update-ref) to allow the head to be varied (defaulting to
BISECT_HEAD) if not specified.

Sound ok?

jon.

^ permalink raw reply

* Re: [PATCH v2 2/2] grep: long context options
From: Tait @ 2011-08-02  1:01 UTC (permalink / raw)
  To: Git Mailing List; +Cc: René Scharfe, Sverre Rabbelier, Junio C Hamano
In-Reply-To: <4E36E0EC.1000508@lsrfire.ath.cx>

René Scharfe <rene.scharfe_lsrfire.ath.cx> said (on 2011/08/01):
> Take long option names for -A (--after-context), -B (--before-context)
> and -C (--context) from GNU grep and add a similar long option name
> for -W (--function-context).

Why not just add --context=function? Then when I want --context=indent
to give context based on the indent-level, it is an intuitive extension
of the existing options. (Of course, --context=<number> would still do
exactly what it does now.)

^ permalink raw reply

* Re: [RFC] Questions for "Git User's Survey 2011"
From: Heiko Voigt @ 2011-08-01 23:43 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: git
In-Reply-To: <201107252233.02088.jnareb@gmail.com>

Hi,

On Mon, Jul 25, 2011 at 10:33:01PM +0200, Jakub Narebski wrote:
> === 17. Which of the following features would you like to see implemented in git? ===
> (multiple choice)
> 
>  + better support for big files (large media)
>  + resumable clone/fetch (and other remote operations)
>  + GitTorrent Protocol, or git-mirror
>  + lazy clone / on-demand fetching of object
>  + support for tracking empty directories
>  + environmental variables in config, 
>    and expanding ~ and ~user in paths in config
>  + better undo/abort/continue, and for more commands
>  + '-n' like option for each command, which describes what would happen
>  + side-by-side diffs and/or color-words diff in gitweb
>  + admin and/or write features in gitweb
>  + graphical history view in gitweb
>  + GUI for rebase in git-gui
>  + GUI for creating repository in git-gui
>  + filename encoding (in repository vs in filesystem)
>  + git push --create
>  + wholesame directory rename detection
>  + graphical merge tool integrated with git-gui
>  + union checkouts (some files from one branch, some from other)
>  + advisory locking / "this file is being edited"
>  + "commands issued" (or "command equivalents") in git-gui / gitk
>  + warn before/when rewriting published history
>  + built-in gitjour/bananajour support
>  + syntax highlighting in git-gui
> 
>  + other (describe below)
> 
> NOTES:
> ^^^^^^
> What features should be mentioned besides those above?  What criteria
> should we have for including features in this list?

How about adding:

 + improved submodule support

 ?

Cheers Heiko

^ permalink raw reply

* Re: [PATCH 0/5] bisect: Add support for a --no-checkout option.
From: Junio C Hamano @ 2011-08-01 23:33 UTC (permalink / raw)
  To: Jon Seymour; +Cc: Christian Couder, git, j6t, jnareb
In-Reply-To: <CAH3Anrqp3BVMpTz7DhYBL=9nt1F30_20t=FmcmdZHqMHLEqXqA@mail.gmail.com>

Jon Seymour <jon.seymour@gmail.com> writes:

> On Tue, Aug 2, 2011 at 3:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jon Seymour <jon.seymour@gmail.com> writes:
>>
>>> It might become more important if someone ever writes a tool that does
>>> a bisection on the user's behalf. In this case, aborting the tool
>>> might leave the HEAD in, what appears to the user, a confused state.
> ...
> In this hypothetical additional series, are you happy for
> --no-checkout to become a synonym for --update-ref=HEAD in the manner
> of v8? From a technical perspective, it doesn't seem necessary to
> duplicate the state variables and parameters.

Well, from a technical perspective, which ref is to be updated is an
option that is valid _only_ under no-checkout mode, and no-checkout mode
could be using HEAD, so I think you would need two variables. One for
"what mode are we running", and the other that is only valid under "we are
in no-checkout mode" that says "we update BISECT_HEAD".

If no-checkout mode _never_ updates HEAD, because the normal mode _always_
updates HEAD, you could reduce them into a single variable (i.e. if we are
updating HEAD then we are in normal mode, otherwise we are in no-checkout
mode).  I actually have a mild suspicion that the no-checkout mode may
turn out to be too confusing for people if it by default updates HEAD, and
we may want to have it update something that is not HEAD by default (or
forbid --update-ref from specifying HEAD), so if that turns out to be the
approach we are going to proceed, we _might_ end up needing only one
variable, but I do not think we know it just yet.  At least I don't.

^ permalink raw reply

* Re: [PATCH v2 1/2] grep: add option to show whole function as context
From: Junio C Hamano @ 2011-08-01 23:17 UTC (permalink / raw)
  To: René Scharfe; +Cc: Git Mailing List, Sverre Rabbelier
In-Reply-To: <4E36E075.20603@lsrfire.ath.cx>

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Add a new option, -W, to show the whole surrounding function of a match.

Thanks, will queue both patches.

It feels somewhat dirty to take the range between the previous "funcname"
and the next "funcname" and consider it the whole function, as if there is
nothing outside the function, though. I certainly understand that this is
a natural and unfortunate consequence that "funcname" is a mechanism
designed to mark only the _beginning_, and we didn't have any need for a
mechanism to mark the _end_.

I am not complaining; just making an observation.  I do not offhand have a
suggestion for improving this, and I think the obvious "then let's come up
with another configuration to mark the end" is not an improvement but
making things worse, so...

^ permalink raw reply

* Re: [PATCH] pull: remove extra space from reflog message
From: Junio C Hamano @ 2011-08-01 23:04 UTC (permalink / raw)
  To: Ori Avtalion; +Cc: git
In-Reply-To: <4e3260f8.41d0e30a.741c.51cb@mx.google.com>

Ori Avtalion <ori@avtalion.name> writes:

> When executing "git pull" with no arguments, the reflog message was:
>   "pull : Fast-forward"
>
> Signed-off-by: Ori Avtalion <ori@avtalion.name>
> ---
>
> This is a correction of my earlier patch, based on the suggestion
> by Andreas Schwab
>
>  git-pull.sh |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/git-pull.sh b/git-pull.sh
> index a10b129..eec3a07 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -10,7 +10,7 @@ SUBDIRECTORY_OK=Yes
>  OPTIONS_SPEC=
>  . git-sh-setup
>  . git-sh-i18n
> -set_reflog_action "pull $*"
> +set_reflog_action "pull${1+ $*}"
>  require_work_tree
>  cd_to_toplevel

Thanks, will queue.

I wonder if we have any test in the contents of the reflog messages,
though.

^ permalink raw reply

* Re: [PATCH v2] Documentation/submodule: add command references and update options
From: Junio C Hamano @ 2011-08-01 22:34 UTC (permalink / raw)
  To: Jens Lehmann; +Cc: Git Mailing List, Marc Branchaud, Nikolai Weibull
In-Reply-To: <4E371151.4040407@web.de>

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

> Reference the "git diff" and "git status" commands where they learned
> functionality that in earlier git versions was only available through the
> 'summary' and 'status' subcommands of "git submodule".

Thanks; will queue.

^ permalink raw reply

* Re: [PATCH v2] ls-files: fix pathspec display on error
From: Junio C Hamano @ 2011-08-01 22:30 UTC (permalink / raw)
  To: Clemens Buchacher; +Cc: Michael J Gruber, git, rrt, john
In-Reply-To: <20110801211958.GA23238@toss>

Clemens Buchacher <drizzd@aon.at> writes:

> ... No changes except to address your comments.
> Thanks for reviewing.

Thank *you* for re-rolling.
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 0e98bff..fef5642 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -353,11 +353,14 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)
>  	}
>  }
>  
> -int report_path_error(const char *ps_matched, const char **pathspec, int prefix_len)
> +int report_path_error(const char *ps_matched, const char **pathspec,
> +		const char *prefix, int prefix_len)
>  {
>  	/*
>  	 * Make sure all pathspec matched; otherwise it is an error.
>  	 */
> +	struct strbuf sb = STRBUF_INIT;
> +	const char *name;
>  	int num, errors = 0;
>  	for (num = 0; pathspec[num]; num++) {
>  		int other, found_dup;

> @@ -382,10 +385,12 @@ int report_path_error(const char *ps_matched, const char **pathspec, int prefix_
>  		if (found_dup)
>  			continue;
>  
> +		name = quote_path_relative(pathspec[num], -1, &sb, prefix);
>  		error("pathspec '%s' did not match any file(s) known to git.",
> -		      pathspec[num] + prefix_len);
> +		      name);

Is prefix_len still being used in this function?

> +		ls ../x* >>expect &&
> +		(git ls-files -c --error-unmatch ../[xy]* || true) >actual 2>&1 &&

The "|| true" construct says "ls-files _may_ exit with non-zero, but we do
not care". Shouldn't we be actively expecting it to exit with non-zero,
using test-must-fail here?

I am not sure if passing NULL in checkout.c is the right thing to do. I
know that is not the problem this patch introduces, but it leads to this
inconsistent behaviour:

	$ cd Documentation
        $ git checkout nosuch.txt
        error: pathspec 'Documentation/nosuch.txt' did not match...
        $ git commit nosuch.txt
        error: pathspec 'nosuch.txt' did not match...

I suspect that cmd_checkout() should pass prefix down to checkout_paths()
so that the latter can properly strip it from the pathspec.

Perhaps this squashed in on top of your patch...

 builtin/checkout.c           |    6 +++---
 builtin/commit.c             |    2 +-
 builtin/ls-files.c           |    5 ++---
 cache.h                      |    2 +-
 t/t3005-ls-files-relative.sh |    6 +++---
 5 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index efe5e8e..a5717f1 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -201,7 +201,7 @@ static int checkout_merged(int pos, struct checkout *state)
 }
 
 static int checkout_paths(struct tree *source_tree, const char **pathspec,
-			  struct checkout_opts *opts)
+			  const char *prefix, struct checkout_opts *opts)
 {
 	int pos;
 	struct checkout state;
@@ -231,7 +231,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,
 		match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, ps_matched);
 	}
 
-	if (report_path_error(ps_matched, pathspec, NULL, -1))
+	if (report_path_error(ps_matched, pathspec, prefix))
 		return 1;
 
 	/* "checkout -m path" to recreate conflicted state */
@@ -1060,7 +1060,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		if (1 < !!opts.writeout_stage + !!opts.force + !!opts.merge)
 			die(_("git checkout: --ours/--theirs, --force and --merge are incompatible when\nchecking out of the index."));
 
-		return checkout_paths(source_tree, pathspec, &opts);
+		return checkout_paths(source_tree, pathspec, prefix, &opts);
 	}
 
 	if (patch_mode)
diff --git a/builtin/commit.c b/builtin/commit.c
index a16d00b..9679a99 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -272,7 +272,7 @@ static int list_paths(struct string_list *list, const char *with_tree,
 			item->util = item; /* better a valid pointer than a fake one */
 	}
 
-	return report_path_error(m, pattern, prefix, -1);
+	return report_path_error(m, pattern, prefix);
 }
 
 static void add_remove_files(struct string_list *list)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 72b986f..468bb13 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -388,8 +388,7 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)
 	}
 }
 
-int report_path_error(const char *ps_matched, const char **pathspec,
-		const char *prefix, int prefix_len)
+int report_path_error(const char *ps_matched, const char **pathspec, const char *prefix)
 {
 	/*
 	 * Make sure all pathspec matched; otherwise it is an error.
@@ -616,7 +615,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 
 	if (ps_matched) {
 		int bad;
-		bad = report_path_error(ps_matched, pathspec, prefix, prefix_len);
+		bad = report_path_error(ps_matched, pathspec, prefix);
 		if (bad)
 			fprintf(stderr, "Did you forget to 'git add'?\n");
 
diff --git a/cache.h b/cache.h
index 80c60b4..d55a6bb 100644
--- a/cache.h
+++ b/cache.h
@@ -1175,7 +1175,7 @@ extern int ws_blank_line(const char *line, int len, unsigned ws_rule);
 #define ws_tab_width(rule)     ((rule) & WS_TAB_WIDTH_MASK)
 
 /* ls-files */
-int report_path_error(const char *ps_matched, const char **pathspec, const char *prefix, int prefix_len);
+int report_path_error(const char *ps_matched, const char **pathspec, const char *prefix);
 void overlay_tree_on_cache(const char *tree_name, const char *prefix);
 
 char *alias_lookup(const char *alias);
diff --git a/t/t3005-ls-files-relative.sh b/t/t3005-ls-files-relative.sh
index 3c3ff5e..a2b63e2 100755
--- a/t/t3005-ls-files-relative.sh
+++ b/t/t3005-ls-files-relative.sh
@@ -48,7 +48,7 @@ test_expect_success 'ls-files -c' '
 		done >expect &&
 		echo "Did you forget to ${sq}git add${sq}?" >>expect &&
 		ls ../x* >>expect &&
-		(git ls-files -c --error-unmatch ../[xy]* || true) >actual 2>&1 &&
+		test_must_fail git ls-files -c --error-unmatch ../[xy]* >actual 2>&1 &&
 		test_cmp expect actual
 	)
 '
@@ -58,11 +58,11 @@ test_expect_success 'ls-files -o' '
 		cd top/sub &&
 		for f in ../x*
 		do
-			echo "error: pathspec ${sq}${f}${sq} did not match any file(s) known to git."
+			echo "error: pathspec $sq$f$sq did not match any file(s) known to git."
 		done >expect &&
 		echo "Did you forget to ${sq}git add${sq}?" >>expect &&
 		ls ../y* >>expect &&
-		(git ls-files -o --error-unmatch ../[xy]* || true) >actual 2>&1 &&
+		test_must_fail git ls-files -o --error-unmatch ../[xy]* >actual 2>&1 &&
 		test_cmp expect actual
 	)
 '

^ permalink raw reply related

* Re: [PATCH 0/5] bisect: Add support for a --no-checkout option.
From: Jon Seymour @ 2011-08-01 22:19 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Christian Couder, git, j6t, jnareb
In-Reply-To: <7vvcuhhw96.fsf@alter.siamese.dyndns.org>

On Tue, Aug 2, 2011 at 3:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jon Seymour <jon.seymour@gmail.com> writes:
>
>> It might become more important if someone ever writes a tool that does
>> a bisection on the user's behalf. In this case, aborting the tool
>> might leave the HEAD in, what appears to the user, a confused state.
>
> Yes, I would prefer a series without --use-this-ref-for-bisect-status and
> then a follow-up series on top of it to add that as a separate feature.
>
> Thanks.
>

Ok, I understand that you are not convinced that such a series is
necessary and so may not integrate it at this point.

However, I will prepare one, just for the record.

In this hypothetical additional series, are you happy for
--no-checkout to become a synonym for --update-ref=HEAD in the manner
of v8? From a technical perspective, it doesn't seem necessary to
duplicate the state variables and parameters.

jon.

^ permalink raw reply

* [PATCH v11 4a/7] bisect: introduce support for --no-checkout option.
From: Jon Seymour @ 2011-08-01 22:14 UTC (permalink / raw)
  To: git; +Cc: chriscool, gitster, j6t, jnareb, Jon Seymour
In-Reply-To: <7vmxfthuf8.fsf@alter.siamese.dyndns.org>

If --no-checkout is specified, then the bisection process uses:

	git update-ref --no-deref HEAD <trial>

at each trial instead of:

	git checkout <trial>

Improved-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 bisect.c                 |   33 ++++++++++++++++++++++-----------
 bisect.h                 |    2 +-
 builtin/bisect--helper.c |    7 +++++--
 3 files changed, 28 insertions(+), 14 deletions(-)

Junio wrote:
> Please. No indentations with just a few SPs.
 
Apologies - it appears my default emacs modes are unhelpful.

This is a replacement for [PATCH v11 4/7] with whitespace fix ups. Let me 
know if you want me to repost the whole series.

diff --git a/bisect.c b/bisect.c
index dd7e8ed..0427117 100644
--- a/bisect.c
+++ b/bisect.c
@@ -24,6 +24,7 @@ struct argv_array {
 
 static const char *argv_checkout[] = {"checkout", "-q", NULL, "--", NULL};
 static const char *argv_show_branch[] = {"show-branch", NULL, NULL};
+static const char *argv_update_ref[] = {"update-ref", "--no-deref", "HEAD", NULL, NULL};
 
 /* bits #0-15 in revision.h */
 
@@ -707,16 +708,23 @@ static void mark_expected_rev(char *bisect_rev_hex)
 		die("closing file %s: %s", filename, strerror(errno));
 }
 
-static int bisect_checkout(char *bisect_rev_hex)
+static int bisect_checkout(char *bisect_rev_hex, int no_checkout)
 {
 	int res;
 
 	mark_expected_rev(bisect_rev_hex);
 
 	argv_checkout[2] = bisect_rev_hex;
-	res = run_command_v_opt(argv_checkout, RUN_GIT_CMD);
-	if (res)
-		exit(res);
+	if (no_checkout) {
+		argv_update_ref[3] = bisect_rev_hex;
+		if (run_command_v_opt(argv_update_ref, RUN_GIT_CMD))
+			die("update-ref --no-deref HEAD failed on %s",
+			    bisect_rev_hex);
+	} else {
+		res = run_command_v_opt(argv_checkout, RUN_GIT_CMD);
+		if (res)
+			exit(res);
+	}
 
 	argv_show_branch[1] = bisect_rev_hex;
 	return run_command_v_opt(argv_show_branch, RUN_GIT_CMD);
@@ -788,7 +796,7 @@ static void handle_skipped_merge_base(const unsigned char *mb)
  * - If one is "skipped", we can't know but we should warn.
  * - If we don't know, we should check it out and ask the user to test.
  */
-static void check_merge_bases(void)
+static void check_merge_bases(int no_checkout)
 {
 	struct commit_list *result;
 	int rev_nr;
@@ -806,7 +814,7 @@ static void check_merge_bases(void)
 			handle_skipped_merge_base(mb);
 		} else {
 			printf("Bisecting: a merge base must be tested\n");
-			exit(bisect_checkout(sha1_to_hex(mb)));
+			exit(bisect_checkout(sha1_to_hex(mb), no_checkout));
 		}
 	}
 
@@ -849,7 +857,7 @@ static int check_ancestors(const char *prefix)
  * If a merge base must be tested by the user, its source code will be
  * checked out to be tested by the user and we will exit.
  */
-static void check_good_are_ancestors_of_bad(const char *prefix)
+static void check_good_are_ancestors_of_bad(const char *prefix, int no_checkout)
 {
 	const char *filename = git_path("BISECT_ANCESTORS_OK");
 	struct stat st;
@@ -868,7 +876,7 @@ static void check_good_are_ancestors_of_bad(const char *prefix)
 
 	/* Check if all good revs are ancestor of the bad rev. */
 	if (check_ancestors(prefix))
-		check_merge_bases();
+		check_merge_bases(no_checkout);
 
 	/* Create file BISECT_ANCESTORS_OK. */
 	fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);
@@ -908,8 +916,11 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
  * We use the convention that exiting with an exit code 10 means that
  * the bisection process finished successfully.
  * In this case the calling shell script should exit 0.
+ *
+ * If no_checkout is non-zero, the bisection process does not
+ * checkout the trial commit but instead simply updates HEAD.
  */
-int bisect_next_all(const char *prefix)
+int bisect_next_all(const char *prefix, int no_checkout)
 {
 	struct rev_info revs;
 	struct commit_list *tried;
@@ -920,7 +931,7 @@ int bisect_next_all(const char *prefix)
 	if (read_bisect_refs())
 		die("reading bisect refs failed");
 
-	check_good_are_ancestors_of_bad(prefix);
+	check_good_are_ancestors_of_bad(prefix, no_checkout);
 
 	bisect_rev_setup(&revs, prefix, "%s", "^%s", 1);
 	revs.limited = 1;
@@ -966,6 +977,6 @@ int bisect_next_all(const char *prefix)
 	       "(roughly %d step%s)\n", nr, (nr == 1 ? "" : "s"),
 	       steps, (steps == 1 ? "" : "s"));
 
-	return bisect_checkout(bisect_rev_hex);
+	return bisect_checkout(bisect_rev_hex, no_checkout);
 }
 
diff --git a/bisect.h b/bisect.h
index 0862ce5..22f2e4d 100644
--- a/bisect.h
+++ b/bisect.h
@@ -27,7 +27,7 @@ struct rev_list_info {
 	const char *header_prefix;
 };
 
-extern int bisect_next_all(const char *prefix);
+extern int bisect_next_all(const char *prefix, int no_checkout);
 
 extern int estimate_bisect_steps(int all);
 
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 5b22639..d96a193 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -4,16 +4,19 @@
 #include "bisect.h"
 
 static const char * const git_bisect_helper_usage[] = {
-	"git bisect--helper --next-all",
+	"git bisect--helper --next-all [--no-checkout]",
 	NULL
 };
 
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	int next_all = 0;
+	int no_checkout = 0;
 	struct option options[] = {
 		OPT_BOOLEAN(0, "next-all", &next_all,
 			    "perform 'git bisect next'"),
+		OPT_BOOLEAN(0, "no-checkout", &no_checkout,
+			    "update HEAD instead of checking out the current commit"),
 		OPT_END()
 	};
 
@@ -24,5 +27,5 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_bisect_helper_usage, options);
 
 	/* next-all */
-	return bisect_next_all(prefix);
+	return bisect_next_all(prefix, no_checkout);
 }
-- 
1.7.6.352.g3b113

^ permalink raw reply related

* Re: tracking submodules out of main directory.
From: Heiko Voigt @ 2011-08-01 22:12 UTC (permalink / raw)
  To: henri GEIST
  Cc: Jens Lehmann, Alexei Sholik, Junio C Hamano, git,
	Sverre Rabbelier
In-Reply-To: <1311932377.3734.182.camel@Naugrim.eriador.com>

Hi,

On Fri, Jul 29, 2011 at 11:39:37AM +0200, henri GEIST wrote:
> Let say a concret exemple
> 
> 3 different teams work on libtiff, libpng, and libjpeg they are totally
> unrelated.
> 
> One more team is working on the "gimp". And they need those 3 libs in
> specific versions not necessarily there heads.
> 
> One other unrelated team is working on "gqview" and need the same libs
> in other specifics versions (Why should they know what te gimp team
> does)
> 
> Neither "gimp" and "gqview" project will contain directory with those
> libs inside. They just depend on them.
> 
> And the last team work on the gnome project which need the "gimp" and
> "gqview". It will be this team witch have to care about having both
> "gimp" and "gqview" sharing the same libs version>
> And has well the gnome project will not contain "gqview" and "gimp" in
> its own tree.
> It will also depend on them.
> 
> It is just the same with aptitude on debian.
> Each package know there dependency by themselves, does not contain there
> dependencies, and do not need a bigger superpackage to tell them what
> are there own dependencies.

As Jens mentioned already in this example you have a

        somemodule A needs a version of lib C higher than X
	somemodule B needs a version of lib C higher than Y

relation. Which in the case of submodules is A points to X and B points
to Y. Lets assume X is contained in Y. Since only the superproject knows
about both A and B its the only instance that can resolve this conflict
of dependence on C and can choose Y. In your example aptitude would be
the superproject containing everything.

This is actually (simplified) the way submodule merge is implemented. So
you see if you want both A and B to use the same version of C you need a
superproject recording this knowledge.

Adding the ability to point to git repositories outside of the worktree
does not solve anything but rather creates more problems. Resolving such
dependencies can not be achieved if only A knows that it needs version X
and only B knows that it needs version Y.

Cheers Heiko

^ permalink raw reply

* Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork
From: René Scharfe @ 2011-08-01 21:52 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Jeff King, Erik Faye-Lund, Junio C Hamano, git
In-Reply-To: <4E3718B4.6090803@kdbg.org>

Am 01.08.2011 23:20, schrieb Johannes Sixt:
> Am 01.08.2011 22:48, schrieb René Scharfe:
>> So git archive gives the right results when writing to a pipe, but
>> always the same wrong result when writing directly to a file.
> 
> This could indeed be a CRLF issue. archive-tar.c runs gzip to let it
> write to the original fd 1 (stdout). gzip is an MSYS program, and MSYS
> is "clever" and sets up the channel in text mode (CRLF conversion) if it
> is a regular file, but in binary mode if it is a pipe.
> 
> Without the gzip filter, git-archive writes to stdout itself. Since we
> have set up all our channels in binary mode, we do not suffer from the
> same problem for plain tar format.
> 
> So, I don't think we can do a lot about it, short of patching MSYS again...

Or we could pipe the output through us, i.e. attach a builtin version of
cat at the output end of the called command.  Only on Windows, of
course.  Better ugly and limping then wrong, right?

René

^ permalink raw reply

* Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork
From: René Scharfe @ 2011-08-01 21:42 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Jeff King, Erik Faye-Lund, Junio C Hamano, git
In-Reply-To: <4E3718B4.6090803@kdbg.org>

Am 01.08.2011 23:20, schrieb Johannes Sixt:
> Am 01.08.2011 22:48, schrieb René Scharfe:
>> So git archive gives the right results when writing to a pipe, but
>> always the same wrong result when writing directly to a file.
> 
> This could indeed be a CRLF issue. archive-tar.c runs gzip to let it
> write to the original fd 1 (stdout). gzip is an MSYS program, and MSYS
> is "clever" and sets up the channel in text mode (CRLF conversion) if it
> is a regular file, but in binary mode if it is a pipe.
> 
> Without the gzip filter, git-archive writes to stdout itself. Since we
> have set up all our channels in binary mode, we do not suffer from the
> same problem for plain tar format.

That seems to be it indeed:

	$ git config tar.t.command 'tar tf -'
	$ git archive --format=t v1.7.6 >a.t
	$ git archive --format=t v1.7.6 | cat >b.t

	$ diff -b a.t b.t | wc -l
	      0

	$ md5sum <a.t
	8026ba48963ec4f849ae863f822bccbc *-

	$ md5sum <b.t
	7bcaa37bfcc47b1ae535369ae0399d90 *-

	$ sed 's/\r//' <a.t | md5sum
	7bcaa37bfcc47b1ae535369ae0399d90 *-

René

^ permalink raw reply

* Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork
From: Johannes Sixt @ 2011-08-01 21:20 UTC (permalink / raw)
  To: René Scharfe; +Cc: Jeff King, Erik Faye-Lund, Junio C Hamano, git
In-Reply-To: <4E371109.7050500@lsrfire.ath.cx>

Am 01.08.2011 22:48, schrieb René Scharfe:
> So git archive gives the right results when writing to a pipe, but
> always the same wrong result when writing directly to a file.

This could indeed be a CRLF issue. archive-tar.c runs gzip to let it
write to the original fd 1 (stdout). gzip is an MSYS program, and MSYS
is "clever" and sets up the channel in text mode (CRLF conversion) if it
is a regular file, but in binary mode if it is a pipe.

Without the gzip filter, git-archive writes to stdout itself. Since we
have set up all our channels in binary mode, we do not suffer from the
same problem for plain tar format.

So, I don't think we can do a lot about it, short of patching MSYS again...

-- Hannes

^ permalink raw reply

* [PATCH v2] ls-files: fix pathspec display on error
From: Clemens Buchacher @ 2011-08-01 21:19 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Michael J Gruber, git, rrt, john
In-Reply-To: <7vhb60j3e1.fsf@alter.siamese.dyndns.org>

The following sequence of commands reveals an issue with error
reporting of relative paths:

 $ mkdir sub
 $ cd sub
 $ git ls-files --error-unmatch ../bbbbb
 error: pathspec 'b' did not match any file(s) known to git.
 $ git commit --error-unmatch ../bbbbb
 error: pathspec 'b' did not match any file(s) known to git.

This bug is visible only if the normalized path (i.e., the relative
path from the repository root) is longer than the prefix.
Otherwise, the code skips over the normalized path and reads from
an unused memory location which still contains a leftover of the
original command line argument.

So instead, use the existing facilities to deal with relative paths
correctly.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
On Mon, Aug 01, 2011 at 01:14:14PM -0700, Junio C Hamano wrote:
> 
> Thanks.  This should be a part of the primary patch, not a
> standalone patch.

Guilty on all counts! No changes except to address your comments.
Thanks for reviewing.

Clemens

 builtin/checkout.c           |    2 +-
 builtin/commit.c             |    2 +-
 builtin/ls-files.c           |   11 +++++--
 cache.h                      |    2 +-
 quote.c                      |    8 +++-
 t/t3005-ls-files-relative.sh |   70 ++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 87 insertions(+), 8 deletions(-)
 create mode 100755 t/t3005-ls-files-relative.sh

diff --git a/builtin/checkout.c b/builtin/checkout.c
index d647a31..a3380d9 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -231,7 +231,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,
 		match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, ps_matched);
 	}
 
-	if (report_path_error(ps_matched, pathspec, 0))
+	if (report_path_error(ps_matched, pathspec, NULL, -1))
 		return 1;
 
 	/* "checkout -m path" to recreate conflicted state */
diff --git a/builtin/commit.c b/builtin/commit.c
index cb73857..c2db12a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -274,7 +274,7 @@ static int list_paths(struct string_list *list, const char *with_tree,
 			item->util = item; /* better a valid pointer than a fake one */
 	}
 
-	return report_path_error(m, pattern, prefix ? strlen(prefix) : 0);
+	return report_path_error(m, pattern, prefix, -1);
 }
 
 static void add_remove_files(struct string_list *list)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 0e98bff..fef5642 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -353,11 +353,14 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)
 	}
 }
 
-int report_path_error(const char *ps_matched, const char **pathspec, int prefix_len)
+int report_path_error(const char *ps_matched, const char **pathspec,
+		const char *prefix, int prefix_len)
 {
 	/*
 	 * Make sure all pathspec matched; otherwise it is an error.
 	 */
+	struct strbuf sb = STRBUF_INIT;
+	const char *name;
 	int num, errors = 0;
 	for (num = 0; pathspec[num]; num++) {
 		int other, found_dup;
@@ -382,10 +385,12 @@ int report_path_error(const char *ps_matched, const char **pathspec, int prefix_
 		if (found_dup)
 			continue;
 
+		name = quote_path_relative(pathspec[num], -1, &sb, prefix);
 		error("pathspec '%s' did not match any file(s) known to git.",
-		      pathspec[num] + prefix_len);
+		      name);
 		errors++;
 	}
+	strbuf_release(&sb);
 	return errors;
 }
 
@@ -577,7 +582,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 
 	if (ps_matched) {
 		int bad;
-		bad = report_path_error(ps_matched, pathspec, prefix_len);
+		bad = report_path_error(ps_matched, pathspec, prefix, prefix_len);
 		if (bad)
 			fprintf(stderr, "Did you forget to 'git add'?\n");
 
diff --git a/cache.h b/cache.h
index 1b5d861..dd3edaa 100644
--- a/cache.h
+++ b/cache.h
@@ -1189,7 +1189,7 @@ extern int ws_blank_line(const char *line, int len, unsigned ws_rule);
 #define ws_tab_width(rule)     ((rule) & WS_TAB_WIDTH_MASK)
 
 /* ls-files */
-int report_path_error(const char *ps_matched, const char **pathspec, int prefix_offset);
+int report_path_error(const char *ps_matched, const char **pathspec, const char *prefix, int prefix_len);
 void overlay_tree_on_cache(const char *tree_name, const char *prefix);
 
 char *alias_lookup(const char *alias);
diff --git a/quote.c b/quote.c
index 63d3b01..532fd3b 100644
--- a/quote.c
+++ b/quote.c
@@ -325,8 +325,12 @@ static const char *path_relative(const char *in, int len,
 
 	if (len < 0)
 		len = strlen(in);
-	if (prefix && prefix_len < 0)
-		prefix_len = strlen(prefix);
+	if (prefix_len < 0) {
+		if (prefix)
+			prefix_len = strlen(prefix);
+		else
+			prefix_len = 0;
+	}
 
 	off = 0;
 	i = 0;
diff --git a/t/t3005-ls-files-relative.sh b/t/t3005-ls-files-relative.sh
new file mode 100755
index 0000000..3c3ff5e
--- /dev/null
+++ b/t/t3005-ls-files-relative.sh
@@ -0,0 +1,70 @@
+#!/bin/sh
+
+test_description='ls-files tests with relative paths
+
+This test runs git ls-files with various relative path arguments.
+'
+
+. ./test-lib.sh
+
+new_line='
+'
+sq=\'
+
+test_expect_success 'prepare' '
+	: >never-mind-me &&
+	git add never-mind-me &&
+	mkdir top &&
+	(
+		cd top &&
+		mkdir sub &&
+		x="x xa xbc xdef xghij xklmno" &&
+		y=$(echo "$x" | tr x y) &&
+		touch $x &&
+		touch $y &&
+		cd sub &&
+		git add ../x*
+	)
+'
+
+test_expect_success 'ls-files with mixed levels' '
+	(
+		cd top/sub &&
+		cat >expect <<-EOF &&
+		../../never-mind-me
+		../x
+		EOF
+		git ls-files $(cat expect) >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'ls-files -c' '
+	(
+		cd top/sub &&
+		for f in ../y*
+		do
+			echo "error: pathspec $sq$f$sq did not match any file(s) known to git."
+		done >expect &&
+		echo "Did you forget to ${sq}git add${sq}?" >>expect &&
+		ls ../x* >>expect &&
+		(git ls-files -c --error-unmatch ../[xy]* || true) >actual 2>&1 &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'ls-files -o' '
+	(
+		cd top/sub &&
+		for f in ../x*
+		do
+			echo "error: pathspec ${sq}${f}${sq} did not match any file(s) known to git."
+		done >expect &&
+		echo "Did you forget to ${sq}git add${sq}?" >>expect &&
+		ls ../y* >>expect &&
+		(git ls-files -o --error-unmatch ../[xy]* || true) >actual 2>&1 &&
+		test_cmp expect actual
+	)
+'
+
+test_done
-- 
1.7.3.1.105.g84915

^ permalink raw reply related

* Re: Storing additional information in commit headers
From: martin f krafft @ 2011-08-01 21:11 UTC (permalink / raw)
  To: Jeff King, git discussion list, Petr Baudis, Clemens Buchacher
In-Reply-To: <20110801201301.GA17111@sigill.intra.peff.net>

[-- Attachment #1: Type: text/plain, Size: 3716 bytes --]

also sprach Jeff King <peff@peff.net> [2011.08.01.2213 +0200]:
> This topic has come up several times in the past few years.

I am sorry that I am bothering the list again. I tried hard to find
whatever I could, but after 2–3 hours of web searching, I came here…

Thank you for taking the time to answer!

> I think some
> of the relevant questions to consider about your new data are:
> 
>   1. Does git actually care about your data? E.g., would it want to use
>      it for reachability analysis in git-fsck?
> 
>   2. Is it an immutable property of a commit, or can it be changed after
>      the fact?

Excellent points, and I have answers to both:

  1. Ideally, I would like to point to another blob containing
     information. Right now, in order to prevent gc from pruning
     it, that would have to be a commit pointed to with a parent
     pointer, which is just not right (it's not a parent) and causes
     the commit to show up in the history (which it should not, as
     it's an implementation detail).

     I'll return to this point further down…

  2. It is immutable. Ideally, I would like to store extra
     information for a ref in ref/heads/*, but there seems to be no
     way of doing this. Hence, I need to store it in commits and
     backtrack for it. Or so I think, at least…

> Otherwise, if (1) is yes, then a commit header makes sense. But
> then, it should also be something that git is taught about, and
> your commit header should not be some topgit-specific thing, but
> a header showing the generalized form.

I agree entirely and would be all too excited to see this happening.
I already had ideas too:

  In addition to the standard tree and parent pointers, there could
  be *-ref and x-*-ref headers, which take a single ref argument,
  presumably to a blob containing more data.

  While I cannot conceive a *-ref example, I think it's obvious that
  x-*-ref should be introduced at the same time to keep the *-ref
  namespace clear for future, "official" Git use.

  In terms of gc and fsck and the like, all *-ref and x-*-ref
  headers would contribute to reachability tests and hence prevent
  pruning of those blobs.

> Otherwise, the usual recommendation is to use a pseudo-header
> within the body of the commit message (i.e., "Topgit-Base: ..." at
> the end of the commit message). The upside is that it's easy to
> create, manipulate, and examine using existing git tools. The
> downside is that it is something that the user is more likely to
> see in "git log" or when editing a rebased commit message.

… to see *and to accidentally mess up*. And while that may even be
unlikely, it does expose information that really ought to be hidden.

> Just about every discussion on this topic ends with the
> pseudo-header recommendation. The only exceptions AFAIK are
> "encoding" (which git itself needs to care about), and
> "generation" (which, as you noted, raises other questions).

I can see how it's arguable too why one would want to give git
commit objects the ability to reference arbitrary blobs containing
additional information. I suppose the answer to this question is
related to the answer to the question of whether Git is
a contained/complete tool as-is, or also serves as
a "framework"/"toolkit" for advanced/creative use.

The availability of the porcelain commands seems to suggest that
extensible/flexible additional features should be welcome! ;)

-- 
martin;              (greetings from the heart of the sun.)
  \____ echo mailto: !#^."<*>"|tr "<*> mailto:" net@madduck
 
http://www.transnationalrepublic.org/
 
spamtraps: madduck.bogus@madduck.net

[-- Attachment #2: Digital signature (see http://martin-krafft.net/gpg/sig-policy/999bbcc4/current) --]
[-- Type: application/pgp-signature, Size: 1124 bytes --]

^ permalink raw reply

* Re: [RFC] Questions for "Git User's Survey 2011"
From: Jakub Narebski @ 2011-08-01 20:57 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: Phil Hord, git
In-Reply-To: <CAMP44s3y0GX6ofa6Am-ioDyf9AEjKJofsHQLU0L9P4nnQ4C+4w@mail.gmail.com>

On Sun, 31 Jul 2011, Felipe Contreras wrote:
> 2011/7/26 Phil Hord <hordp@cisco.com>:
>> On 07/26/2011 06:37 AM, Jakub Narebski wrote:
>>> On Mon, 25 Jul 2011, Phil Hord wrote:

>>>> IDE integration (Eclipse, Netbeans, etc.)
>>>
>>> This isn't strictly _git_ feature, and is in "12. What kind of Git tools
>>> do you use?" anyway.
>>>
>> Yes, it's not a git feature.  But I'm curious how successful any IDE
>> integration is (as opposed to a GUI, for example).  I haven't seen any
>> that use enough of the power of git yet, so I have been disappointed.  I
>> suspect others have also, but I'm hopeful.
> 
> I also think this is important, in my blog I've seen a bunch of people
> mentioning that Git's integration with IDE's is not as good as
> Mercurial. Putting this in the already existing list of tools would
> not cover missing IDE's being integrated, I think this should go into
> "20. In your opinion, which areas in Git need improvement?"

I think this is a good idea, though perhaps not in question about Git
(it is "which areas in _Git_"), but in a separate question about Git
tools.

 21. In your opinion, what Git tools are needed, and which need improvements?

Or something like that.
-- 
Jakub Narebski
Poland

^ permalink raw reply

* Re: Storing additional information in commit headers
From: martin f krafft @ 2011-08-01 20:55 UTC (permalink / raw)
  To: Clemens Buchacher, Sverre Rabbelier, git discussion list,
	Petr Baudis
In-Reply-To: <20110801200149.GA20861@toss>

[-- Attachment #1: Type: text/plain, Size: 1196 bytes --]

also sprach Clemens Buchacher <drizzd@aon.at> [2011.08.01.2201 +0200]:
> Notes are tracked using a 'branch' too. It's just a branch in the
> refs/notes namespace, the notes ref. You could simply tag your
> notes ref or point a ref from the refs/heads namespace to it each
> time you create new notes.

Hi Clemens, thanks for responding!

You suggest integrating refs/notes/foo into refs/heads by means of
a pointer… at which point we are polluting the branch history space
again (think gitk), no?

I appreciate the simplicity of this idea of yours, which I had not
thought of. Indeed, maintaining a head at the top of
refs/notes/topgit-metadata (or whatever) has charm. I do not mean to
discard it at all right now, and will think about this more!

git-notes was designed to be used for such cases, I was pleased to
note the configurability. Maybe it is the ticket.

Still: why not commit headers?

-- 
martin | http://madduck.net/ | http://two.sentenc.es/
 
... with a plastic cup filled with a liquid that was almost,
but not quite, entirely unlike tea.
            -- douglas adams, "the hitchhiker's guide to the galaxy"
 
spamtraps: madduck.bogus@madduck.net

[-- Attachment #2: Digital signature (see http://martin-krafft.net/gpg/sig-policy/999bbcc4/current) --]
[-- Type: application/pgp-signature, Size: 1124 bytes --]

^ permalink raw reply

* Re: Storing additional information in commit headers
From: martin f krafft @ 2011-08-01 20:51 UTC (permalink / raw)
  To: Martin Langhoff, git discussion list, Petr Baudis,
	Clemens Buchacher
In-Reply-To: <CACPiFCLPgsC+9cX7r33oCQ2AnuRXMTqOAE5RZLS7hXdHc6B-9Q@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 1583 bytes --]

also sprach Martin Langhoff <martin.langhoff@gmail.com> [2011.08.01.2133 +0200]:
> What data are you trying to include? Some time ago, I had similar
> ideas to yours for a while... and it ended up being that all I needed
> was to put the additional data /in a file/ and commit that file.

Hi, thanks for taking the time to reply to me!

I am trying to store the top-base of a TopGit branch, which is the
merge of all a branch's dependencies.

TopGit uses refs for that, but a ref can only ever point at one such
merge, and so it's hard-to-impossible to reconstruct a branch
dependency in the past.

TopGit does use files in the worktree too. I would love to get rid
of this as well, since a file like .topmsg (which differs between
all branches, even related ones), requires to always remember to use
the 'ours' merge driver, which requires setup, which makes it harder
to use.

> If you are using a wrapper program,

I am trying to stay as close as possible to plain Git. All of this
could easily be done by a wrapper, but a wrapper always makes too
many assumptions to become a viable standard for Debian packaging.

> it's valid/sane, in the preparations to commit, perhaps ensuring
> that a pre-commit-hook script is in place and executable.

Again, that requires setup, which increases the barrier of entry to
passerby's and new contributors.

-- 
martin | http://madduck.net/ | http://two.sentenc.es/
 
"verbing weirds language."
                                                           -- calvin
 
spamtraps: madduck.bogus@madduck.net

[-- Attachment #2: Digital signature (see http://martin-krafft.net/gpg/sig-policy/999bbcc4/current) --]
[-- Type: application/pgp-signature, Size: 1124 bytes --]

^ permalink raw reply


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