date:20150610

Mike Hommey  writes:

> It can be useful to have grafts or replace refs for specific use-cases while
> keeping the default "view" of the repository pristine (or with a different
> set of grafts/replace refs).
>
> It is possible to use a different graft file with GIT_GRAFT_FILE, but while
> replace refs are more powerful, they don't have an equivalent override.
>
> Add a GIT_REPLACE_NAMESPACE environment variable to control where git is
> going to look for replace refs.
>
> Signed-off-by: Mike Hommey 
> ---
>
> I'm not sure if I need to care about avoiding strlen in log-tree.c, or if I
> should handle the lack of a / at the end of GIT_REPLACE_NAMESPACE.

First, let me agree with you that being able to say "I usually use
refs/replace/ hierarchy as my object replacement database, but for
this particular invocation of Git (or 'all Git invocations from this
subprocess' for that matter), I want to use refs/replace-2/ hierarchy
instead" is a good thing.

I however doubt that it is a good idea to use the word "namespace"
anywhere in the name for that mechanism.  Let me explain, and please
listen with skepticism, as I do not use the ref namespace mechanism
myself---it is entirely possible that my understanding of how the
ref namespace mechanism is meant to be used is faulty, and if that
is the case please correct me.

The ref namespace mechanism is to be used when you want to serve one
or more "virtual repositories" via upload-pack/receive-pack server
side programs from a single repository.  By having two hierarchies,
each of which is similar to the usual HEAD, refs/heads/, refs/tags/,
etc., under refs/namespaces/a and refs/namespaces/b, you can allow
two instances of upload-pack to serve two "virtual repositories".

What is in refs/namespaces/a/{HEAD,refs/heads/*,refs/tags/*,...} is
exposed by one instance of upload-pack to its clients as if they are
the entire world (i.e. HEAD,refs/heads/*,refs/tags/*,...), the other
one does the same to its clients from refs/namespaces/b/*.  And they
do share the same object store (thereby allowing you to implement a
cheap "forks" without having to worry about object borrowing).

The usual server side housekeeping such as "gc" can and must be
arranged to happen without limiting them to either namespace, so
that anything reachable by any ref from either "virtual repository"
will be retained.

Now, does the "For this invocation, please use refs/replace-2/ as
the object replacement database" feature have anything common to the
ref namespace mechanism?  I do not see any commonality, and that is
why I do not think it is a good idea to use that word.  It will
confuse the users and the developers alike.

To me, this mechanism looks very similar to the way you specify a
non-default notes tree is to be used with the --notes=
parameter, core.notesRef configuration and GIT_NOTES_REF environment
variable.  Modeling after that, perhaps using core.replaceRef and
GIT_REPLACE_REF would be more appropriate, I think.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/8] object_id part 2

On Wed, Jun 10, 2015 at 05:21:33PM -0700, Junio C Hamano wrote:
> "brian m. carlson"  writes:
> > [0] https://github.com/bk2204/git.git object-id-part2
> 
> No approach other than just letting reviewers fetch from there and
> taking a look is reasonable, I would think.
> 
> Did you create this manually, or is it a mechanical scripted rewrite
> followed by manual clean-up?  If the latter, it may help people by
> posting the mechanical recipe _and_ a patch that shows the manual
> clean-up.  That is something we can reasonably review and discuss.

It's mostly manual, but some pieces (the is_null_sha1 and sha1_to_hex
conversions) were scripted using the embedded Ruby interpreter in Vim.
The script I used to do that is below.

  def refactor(s)
methods = %w(is_null_sha1 sha1_to_hex).map do |m|
  [m, m.sub('sha1', 'oid')]
end.to_h
s.sub(/\A(.*)(is_null_sha1|sha1_to_hex)\(([^)]+)(\.|->)sha1\)(.*)\z/) do
  [$1, methods[$2], "(&#{$3}#{$4}oid)", $5].join('')
end
  end
  
  def replace_line(method)
func = Object.method(method)
VIM::Buffer.current.line = func.call(VIM::Buffer.current.line)
  end

It was invoked as replace_line(:refactor), so it basically modified each
relevant line by passing it through refactor.

Sorry for choosing a language that's less familiar to the list: if I had
known you'd want to see it, I would have used Perl.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: Digital signature

[PATCH] git-rebase--interactive.sh: add config option for custom instruction format

2015-06-10 Thread Michael Rappazzo

A config option 'rebase.instructionFormat' can override the
default 'oneline' format of the rebase instruction list.

Since the list is parsed using the left, right or boundary mark plus
the sha1, they are prepended to the instruction format.

Signed-off-by: Michael Rappazzo 
---
 Documentation/git-rebase.txt |  7 +++
 git-rebase--interactive.sh   | 34 --
 t/t3415-rebase-autosquash.sh | 33 +
 3 files changed, 68 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 1d01baa..8ddab77 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -213,6 +213,9 @@ rebase.autoSquash::
 rebase.autoStash::
If set to true enable '--autostash' option by default.
 
+rebase.instructionFormat::
+   Custom commit list format to use during an '--interactive' rebase.
+
 OPTIONS
 ---
 --onto ::
@@ -359,6 +362,10 @@ default is `--no-fork-point`, otherwise the default is 
`--fork-point`.
Make a list of the commits which are about to be rebased.  Let the
user edit that list before rebasing.  This mode can also be used to
split commits (see SPLITTING COMMITS below).
++
+The commit list format can be changed by setting the configuration option
+rebase.instructionFormat.  A customized instruction format will automatically
+have the long commit hash prepended to the format.
 
 -p::
 --preserve-merges::
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index dc3133f..6d14315 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -740,10 +740,19 @@ collapse_todo_ids() {
 # "pick sha1 fixup!/squash! msg" appears in it so that the latter
 # comes immediately after the former, and change "pick" to
 # "fixup"/"squash".
+#
+# Note that if the config has specified a custom instruction format
+# each log message will be re-retrieved in order to normalize the 
+# autosquash arrangement
 rearrange_squash () {
# extract fixup!/squash! lines and resolve any referenced sha1's
-   while read -r pick sha1 message
+   while read -r pick sha1 todo_message
do
+   message=${todo_message}
+   if test -n "${format}"
+   then
+   message=$(git log -n 1 --format="%s" ${sha1})
+   fi
case "$message" in
"squash! "*|"fixup! "*)
action="${message%%!*}"
@@ -779,12 +788,17 @@ rearrange_squash () {
test -s "$1.sq" || return
 
used=
-   while read -r pick sha1 message
+   while read -r pick sha1 todo_message
do
case " $used" in
*" $sha1 "*) continue ;;
esac
-   printf '%s\n' "$pick $sha1 $message"
+   message=$todo_message
+   if test -n "${format}"
+   then
+   message=$(git log -n 1 --format="%s" ${sha1})
+   fi
+   printf '%s\n' "$pick $sha1 $todo_message"
used="$used$sha1 "
while read -r squash action msg_prefix msg_content
do
@@ -802,8 +816,13 @@ rearrange_squash () {
case "$message" in "$msg_content"*) emit=1;; 
esac ;;
esac
if test $emit = 1; then
-   real_prefix=$(echo "$msg_prefix" | sed "s/,/! 
/g")
-   printf '%s\n' "$action $squash 
${real_prefix}$msg_content"
+   if test -n "${format}"
+   then
+   msg_content=$(git log -n 1 
--format="${format}" ${squash})
+   else
+   msg_content="$(echo "$msg_prefix" | sed 
"s/,/! /g")$msg_content"
+   fi
+   printf '%s\n' "$action $squash $msg_content"
used="$used$squash "
fi
done <"$1.sq"
@@ -977,7 +996,10 @@ else
revisions=$onto...$orig_head
shortrevisions=$shorthead
 fi
-git rev-list $merges_option --pretty=oneline --reverse --left-right 
--topo-order \
+format=$(git config --get rebase.instructionFormat)
+# the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to 
parse
+git rev-list $merges_option --format="%m%H ${format:-%s}" \
+   --reverse --left-right --topo-order \
$revisions ${restrict_revision+^$restrict_revision} | \
sed -n "s/^>//p" |
 while read -r sha1 rest
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 41370ab..1ef96eb 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -250,4 +250,37 @@ test_expect_success 'squash! fixup!' '
test_auto_fixup_fixup squash fixup
 '
 
+test_expect_success

[PATCH] Allow to control the namespace for replace refs

2015-06-10 Thread Mike Hommey

It can be useful to have grafts or replace refs for specific use-cases while
keeping the default "view" of the repository pristine (or with a different
set of grafts/replace refs).

It is possible to use a different graft file with GIT_GRAFT_FILE, but while
replace refs are more powerful, they don't have an equivalent override.

Add a GIT_REPLACE_NAMESPACE environment variable to control where git is
going to look for replace refs.

Signed-off-by: Mike Hommey 
---

I'm not sure if I need to care about avoiding strlen in log-tree.c, or if I
should handle the lack of a / at the end of GIT_REPLACE_NAMESPACE.

 builtin/replace.c | 6 +++---
 cache.h   | 2 ++
 environment.c | 6 ++
 log-tree.c| 5 +++--
 refs.c| 3 ++-
 5 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index 54bf01a..bb5bc84 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -104,9 +104,9 @@ static int for_each_replace_name(const char **argv, 
each_replace_name_fn fn)
continue;
}
full_hex = sha1_to_hex(sha1);
-   snprintf(ref, sizeof(ref), "refs/replace/%s", full_hex);
+   snprintf(ref, sizeof(ref), "%s%s", git_replace_namespace, 
full_hex);
/* read_ref() may reuse the buffer */
-   full_hex = ref + strlen("refs/replace/");
+   full_hex = ref + strlen(git_replace_namespace);
if (read_ref(ref, sha1)) {
error("replace ref '%s' not found.", full_hex);
had_error = 1;
@@ -134,7 +134,7 @@ static void check_ref_valid(unsigned char object[20],
int force)
 {
if (snprintf(ref, ref_size,
-"refs/replace/%s",
+"%s%s", git_replace_namespace,
 sha1_to_hex(object)) > ref_size - 1)
die("replace ref name too long: %.*s...", 50, ref);
if (check_refname_format(ref, 0))
diff --git a/cache.h b/cache.h
index badf3da..9553edc 100644
--- a/cache.h
+++ b/cache.h
@@ -384,6 +384,7 @@ static inline enum object_type object_type(unsigned int 
mode)
 #define EXEC_PATH_ENVIRONMENT "GIT_EXEC_PATH"
 #define CEILING_DIRECTORIES_ENVIRONMENT "GIT_CEILING_DIRECTORIES"
 #define NO_REPLACE_OBJECTS_ENVIRONMENT "GIT_NO_REPLACE_OBJECTS"
+#define GIT_REPLACE_NAMESPACE_ENVIRONMENT "GIT_REPLACE_NAMESPACE"
 #define GITATTRIBUTES_FILE ".gitattributes"
 #define INFOATTRIBUTES_FILE "info/attributes"
 #define ATTRIBUTE_MACRO_PREFIX "[attr]"
@@ -605,6 +606,7 @@ extern unsigned long pack_size_limit_cfg;
  * been sought but there were none.
  */
 extern int check_replace_refs;
+extern char *git_replace_namespace;
 
 extern int fsync_object_files;
 extern int core_preload_index;
diff --git a/environment.c b/environment.c
index a40044c..c836b61 100644
--- a/environment.c
+++ b/environment.c
@@ -47,6 +47,7 @@ const char *askpass_program;
 const char *excludes_file;
 enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
 int check_replace_refs = 1;
+char *git_replace_namespace;
 enum eol core_eol = EOL_UNSET;
 enum safe_crlf safe_crlf = SAFE_CRLF_WARN;
 unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
@@ -109,6 +110,7 @@ const char * const local_repo_env[] = {
GRAFT_ENVIRONMENT,
INDEX_ENVIRONMENT,
NO_REPLACE_OBJECTS_ENVIRONMENT,
+   GIT_REPLACE_NAMESPACE_ENVIRONMENT,
GIT_PREFIX_ENVIRONMENT,
GIT_SHALLOW_FILE_ENVIRONMENT,
NULL
@@ -145,6 +147,7 @@ static void setup_git_env(void)
 {
const char *gitfile;
const char *shallow_file;
+   const char *replace_namespace;
 
git_dir = getenv(GIT_DIR_ENVIRONMENT);
if (!git_dir)
@@ -156,6 +159,9 @@ static void setup_git_env(void)
git_graft_file = git_path_from_env(GRAFT_ENVIRONMENT, "info/grafts");
if (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))
check_replace_refs = 0;
+   replace_namespace = getenv(GIT_REPLACE_NAMESPACE_ENVIRONMENT);
+   git_replace_namespace = xstrdup(replace_namespace ? replace_namespace
+ : "refs/replace/");
namespace = expand_namespace(getenv(GIT_NAMESPACE_ENVIRONMENT));
namespace_len = strlen(namespace);
shallow_file = getenv(GIT_SHALLOW_FILE_ENVIRONMENT);
diff --git a/log-tree.c b/log-tree.c
index c931615..9e60137 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -96,11 +96,12 @@ static int add_ref_decoration(const char *refname, const 
unsigned char *sha1, in
 
assert(cb_data == NULL);
 
-   if (starts_with(refname, "refs/replace/")) {
+   if (starts_with(refname, git_replace_namespace)) {
unsigned char original_sha1[20];
if (!check_replace_refs)
return 0;
-   if (get_sha1_hex(refname + 13, original_sha1)) {
+   if (get_sha1_hex(refname + strlen(git_replace_namespace),
+

[PATCH v3] git-rebase--interactive.sh: add config option for custom instruction format

2015-06-10 Thread Michael Rappazzo

Difference between v2 and v3 of this patch:

- Fixed autosquash
- Added documentation on the config options
- Added two tests to t3414 (rebase-autosquash)

Michael Rappazzo (1):
  git-rebase--interactive.sh: add config option for custom instruction
format

 Documentation/git-rebase.txt |  7 +++
 git-rebase--interactive.sh   | 34 --
 t/t3415-rebase-autosquash.sh | 33 +
 3 files changed, 68 insertions(+), 6 deletions(-)

-- 
2.4.2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/8] object_id part 2

"brian m. carlson"  writes:

> On Wed, Jun 10, 2015 at 11:51:14PM +, brian m. carlson wrote:
>> On Wed, Jun 10, 2015 at 03:50:32PM -0700, Junio C Hamano wrote:
>> > "brian m. carlson"  writes:
>> > >   Convert struct object to object_id
>> > 
>> > It seems that the last one didn't make it...
>> 
>> It appears the mail was too large for vger.  Unfortunately for
>> bisectability reasons, it is necessarily large.  I'll resubmit the patch
>> with less context.
>
> Unfortunately, the only patch I can generate that falls under to 100 KB
> limit is with -U0, which isn't very useful.  How do you want to proceed?
> The branch is available at [0], or I can send the -U0 patch, or I can
> split it into unbisectable pieces.
>
> [0] https://github.com/bk2204/git.git object-id-part2

No approach other than just letting reviewers fetch from there and
taking a look is reasonable, I would think.

Did you create this manually, or is it a mechanical scripted rewrite
followed by manual clean-up?  If the latter, it may help people by
posting the mechanical recipe _and_ a patch that shows the manual
clean-up.  That is something we can reasonably review and discuss.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/8] object_id part 2

On Wed, Jun 10, 2015 at 11:51:14PM +, brian m. carlson wrote:
> On Wed, Jun 10, 2015 at 03:50:32PM -0700, Junio C Hamano wrote:
> > "brian m. carlson"  writes:
> > >   Convert struct object to object_id
> > 
> > It seems that the last one didn't make it...
> 
> It appears the mail was too large for vger.  Unfortunately for
> bisectability reasons, it is necessarily large.  I'll resubmit the patch
> with less context.

Unfortunately, the only patch I can generate that falls under to 100 KB
limit is with -U0, which isn't very useful.  How do you want to proceed?
The branch is available at [0], or I can send the -U0 patch, or I can
split it into unbisectable pieces.

[0] https://github.com/bk2204/git.git object-id-part2
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: Digital signature

Re: [PATCH 0/8] object_id part 2

On Wed, Jun 10, 2015 at 03:50:32PM -0700, Junio C Hamano wrote:
> "brian m. carlson"  writes:
> >   Convert struct object to object_id
> 
> It seems that the last one didn't make it...

It appears the mail was too large for vger.  Unfortunately for
bisectability reasons, it is necessarily large.  I'll resubmit the patch
with less context.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: Digital signature

Re: [PATCH v4] receive-pack: Create a HEAD ref for ref namespace

2015-06-10 Thread Johannes Löthberg


On 05/06, Johannes Löthberg wrote:

Each ref namespace have their own separate branches, tags, and HEAD, so
when pushing to a namespace we need to make sure that there exists a
HEAD ref for the namespace, otherwise you will not be able to check out
the repo after cloning from a namespace

Signed-off-by: Johannes Löthberg 
---
Changes since v3:
 test:
   * Use a single printf statement
   * Check that the contents of the file and sha of the commits in the
 initial and cloned repositories matches



Any other comments?

--
Sincerely,
 Johannes Löthberg
 PGP Key ID: 0x50FB9B273A9D0BB5
 https://theos.kyriasis.com/~kyrias/


signature.asc
Description: PGP signature

What's cooking in git.git (Jun 2015, #03; Wed, 10)

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.

Somehow we are getting quite a many new topics in the past few
weeks.  All the new contributors we acquired recently, including but
not limited to the GSoC and Matthieu's students, are making good
progress, throwing patches and responding to reviews in a reasonable
way, proving themselves to be real assets to the community.

Which, I am very happy about, even though I am a bit behind catching
up with them.  "Let's skip commute to gain an extra productive hour"
I tried yesterday did not help enough X-<.  I am sure I've either
missed or postponed more than a few topics.  I'll try to get to them
but some of them may fall in the cracks; please help by reviewing on
uncommented patches.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[New Topics]

* af/tcsh-completion-noclobber (2015-06-09) 1 commit
 - git-completion.tcsh: fix redirect with noclobber

 The tcsh completion writes a bash scriptlet but that would have
 failed for users with noclobber set.

 Will merge to 'next'.


* es/utf8-stupid-compiler-workaround (2015-06-05) 1 commit
 - utf8: NO_ICONV: silence uninitialized variable warning

 A compilation workaround.

 Will merge to 'next'.


* fk/doc-format-patch-vn (2015-06-10) 1 commit
 - doc: format-patch: fix typo

 Docfix.

 Will merge to 'next'.


* gp/status-rebase-i-info (2015-06-09) 5 commits
 - SQUASH??? fix misindent
 - status: add new tests for status during rebase -i
 - status: give more information during rebase -i
 - status: differentiate interactive from non-interactive rebases
 - status: factor two rebase-related messages together

 Teach "git status" to show a more detailed information regarding
 the "rebase -i" session in progress.

 Expecting a reroll.


* jc/prompt-document-ps1-state-separator (2015-06-10) 1 commit
 - git-prompt.sh: document GIT_PS1_STATESEPARATOR

 Docfix.

 Will merge to 'next'.


* jk/index-pack-reduce-recheck (2015-06-09) 1 commit
 - index-pack: avoid excessive re-reading of pack directory

 Disable "have we lost a race with competing repack?" check while
 receiving a huge object transfer that runs index-pack.

 Will merge to 'next'.


* js/sleep-without-select (2015-06-05) 4 commits
 - lockfile: wait using sleep_millisec() instead of select()
 - lockfile: convert retry timeout computations to millisecond
 - help.c: wrap wait-only poll() invocation in sleep_millisec()
 - lockfile: replace random() by rand()

 Portability fix.

 Will merge to 'next'.


* ld/p4-changes-block-size (2015-06-10) 4 commits
 - git-p4: fixing --changes-block-size handling
 - git-p4: add tests for non-numeric revision range
 - git-p4: test with limited p4 server results
 - git-p4: additional testing of --changes-block-size

 More Perforce row number limit workaround for "git p4".

 Will merge to 'next'.


* mh/fsck-reflog-entries (2015-06-08) 2 commits
 - fsck: report errors if reflog entries point at invalid objects
 - fsck_handle_reflog_sha1(): new function

 "git fsck" used to ignore missing or invalid objects recorded in reflog.

 Will merge to 'next'.


* mk/utf8-no-iconv-warn (2015-06-08) 1 commit
 - utf8.c: print warning about disabled iconv

 Warn when a reencoding is requested in a build without iconv
 support, as the end user is likely to get an unexpected result.  I
 think the same level of safety should be added to a build with
 iconv support when the specified encoding is not available, but the
 patch does not go there.

 Expecting a reroll.


* mr/rebase-i-customize-insn-sheet (2015-06-08) 1 commit
 - git-rebase--interactive.sh: add config option for custom instruction format

 Breaks tests.

 Expecting a reroll.


* nd/untracked-cache (2015-06-08) 1 commit
 - read-cache: fix untracked cache invalidation when split-index is used

 Hotfix for a topic already in 'master'.

 Will merge to 'next'.


* pt/am-abort-fix (2015-06-08) 6 commits
 - am --abort: keep unrelated commits on unborn branch
 - am --abort: support aborting to unborn branch
 - am --abort: revert changes introduced by failed 3way merge
 - am --skip: support skipping while on unborn branch
 - am -3: support 3way merge on unborn branch
 - am --skip: revert changes introduced by failed 3way merge

 Various fixes around "git am" that applies a patch to a history
 that is not there yet.

 Will merge to 'next'.


* pt/am-foreign (2015-06-08) 6 commits
 - SQUASH??? do not cat a single file into a pipe
 - am: teach mercurial patch parser how to read from stdin
 - am: use gmtime() to parse mercurial patch date
 - t4150: test applying StGit series
 - am: teach StGit patch parser how to read from stdin
 - t4150: test applying StGit patch

 Various enhancements around "git am" reading patches generated by
 for

Re: [PATCH v2 15/19] pull: teach git pull about --rebase

Paul Tan  writes:

> +/**
> + * Returns remote's upstream branch for the current branch. If remote is 
> NULL,
> + * the current branch's configured default remote is used. Returns NULL if
> + * `remote` does not name a valid remote, HEAD does not point to a branch,
> + * remote is not the branch's configured remote or the branch does not have 
> any
> + * configured upstream branch.
> + */
> +static char *get_upstream_branch(const char *remote)
> +{
> + struct remote *rm;
> + struct branch *curr_branch;
> +
> + rm = remote_get(remote);
> + if (!rm)
> + return NULL;
> +
> + curr_branch = branch_get("HEAD");
> + if (!curr_branch)
> + return NULL;
> +
> + if (curr_branch->remote != rm)
> + return NULL;
> +
> + if (!curr_branch->merge_nr)
> + return NULL;
> +
> + return xstrdup(curr_branch->merge[0]->dst);
> +}

This part needs to be rebased, as branch->remote no longer exists in
the recent world order.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/8] object_id part 2

"brian m. carlson"  writes:

> The final piece in this series is the conversion of struct object to use
> struct object_id.  This is a necessarily large patch because of the
> large number of places this code is used.
>
> brian m. carlson (8):
>   refs: convert some internal functions to use object_id
>   sha1_file: introduce has_object_file helper.
>   Convert struct ref to use object_id.
>   Add a utility function to make parsing hex values easier.
>   add_sought_entry_mem: convert to struct object_id
>   parse_fetch: convert to use struct object_id
>   ref_newer: convert to use struct object_id
>   Convert struct object to object_id

It seems that the last one didn't make it...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: format-patch and submodules

2015-06-10 Thread Christopher Dunn

Well, now it gets more complicated. I want git-p4 to ignore submodules
completely. But it fails only *only* the submodules changed. (At
least, my version fails. I'll try to diff against latest.)

But to debug this, I had to add a dry-run mode to git-p4. And I am
using a version of git-p4 which uses 'git-notes' rather than
re-writing history. If you want, you can try my version:

  https://github.com/pb-cdunn/git-p4

On Wed, Jun 10, 2015 at 4:14 PM, Luke Diamand  wrote:
> On 10/06/15 18:04, Christopher Dunn wrote:
>>
>> Sorry. I thought empty patches were made to work in other cases.
>>
>> 'git-p4' needs to skip these. Wrong mailing list then.
>
>
> Possibly the right mailing list - can you explain what you mean here w.r.t
> git-p4 please?
>
> Thanks!
> Luke
>
>
>
>
>>
>> On Tue, Jun 9, 2015 at 1:52 PM, Jens Lehmann  wrote:
>>>
>>> Am 05.06.2015 um 01:20 schrieb Christopher Dunn:


 (Seen in git versions: 2.1.0 and 1.9.3 et al.)

 $ git format-patch --stdout X^..X | git apply check -
 fatal: unrecognized input

 This fails when the commit consists of nothing but a submodule change
 (as in 'git add submodule foo'), but it passes when a file change is
 added to the same commit.

 There used to be a similar problem for empty commits, but that was
 fixed around git-1.8:



 http://stackoverflow.com/questions/20775132/cannot-apply-git-patch-replacing-a-file-with-a-link

 Now, 'git format-patch' outputs nothing for an empty commit. I suppose
 that needs to be the behavior also when only submodules are changed,
 since in that case there is no 'diff' section from 'format-patch'.

 Use-case: git-p4

 Of course, we do not plan to add the submodule into Perforce, but we
 would like this particular command to behave the same whether there
 are other diffs or not.
>>>
>>>
>>>
>>> Hmm, I'm not sure that this is a bug. It looks to me like doing a
>>>
>>> $ git format-patch --stdout X^..X | git apply check -
>>>
>>> when nothing is changed except submodules and expecting it to work
>>> is the cause of the problem.
>>>
>>> I get the same error when I do:
>>>
>>> $git format-patch --stdout master..master | git apply --check -
>>> fatal: unrecognized input
>>>
>>> No submodules involved, just an empty patch.
>>>
>>> I assume you want to ignore all submodule changes, so you should
>>> check if e.g. "git diff --ignore-submodules X^..X" returns anything
>>> before applying that? (From the command you ran I assume you might
>>> be able to drop the --ignore-submodules because you already did set
>>> "diff.ignoreSubmodules" to "all"?)
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe git" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 7/7] bisect: allows any terms set by user

Junio C Hamano  writes:

> I like this change very much; it removes the mysteriously misnamed
> start-bad-good variable (because you do not really _care_ that
> 'start' was what implicitly decided that good/bad pair is the term
> we use in this session; what you care is that the terms are already
> known or not).
>
> That is another reason why I think it would be a better organization
> for the patch series to do without the intermediate "we now add new/old
> as another hardcoded values on top of the traditional bad/good".
>
> That is, I would think a reasonable progression of the series would
> look more like these three steps:
>
>  - preliminary clean-up steps (e.g. "correct 'mistook'");
>
>  - use $name_new and $name_old throughout the code, giving them
>'bad' and 'good' as hardcoded values; finally
>
>  - add 'bisect terms' support.

Just in case I confused readers with a message that apparently
conflicts with what I said in the ancient thread:

  http://thread.gmane.org/gmane.comp.version-control.git/199758/focus=200025

I am admitting that I was wrong.  Or perhaps I was right back then,
but the world has changed ;-).

We have been hearing "bisect bad/good" is hard to use for the last 3
years since that thread discussed this topic, and that made me
realize that addition of single new/old may not be good enough, and
we should bite the bullet to support 'bisect terms' properly, making
bad/good and new/old even less special (contrary to what I said back
then in that thread "we only need these two pairs"), following the
suggestion by Phil Hord in that thread.

And the suggested three-step approach above reflects that new world
order in my mind.  We admit that the machinery should have been
built around a value-agnostic "old vs new" in the first place, but
unfortunately it wasn't.  So we belatedly update the system to use
these two terms internally to express the logic by naming the
variables name_old and name_new after these two value-agnostic
concepts, and then support the traditional "good vs bad" as a mere
default values for the names of old and new states.

One thing I forgot to say in the review of this patch:

> +bisect_terms () {
> + test $# -eq 2 ||
> + die "You need to give me at least two arguments"
> +
> + if ! test -s "$GIT_DIR/BISECT_START"
> + then
> + echo $1 >"$GIT_DIR/BISECT_TERMS" &&
> + echo $2 >>"$GIT_DIR/BISECT_TERMS" &&
> + echo "1" > "$GIT_DIR/TERMS_DEFINED"
> + else
> + die "A bisection has already started, please use "\
> + "'git bisect reset' to restart and change the terms"
> + fi
> +}
> +

I think "git bisect terms" is a good way to help a user to recall
what two names s/he decided to use for the current session.  So
dying 'already started' with suggestion for 'reset' is OK, but at
the same time, helping the user to continue the current bisection by
giving a message along the lines of "You are hunting for a commit
that is at the boundary of the old state (you are calling it
'$NAME_OLD') and the new state ('$NAME_NEW')" would be a good idea.

Thanks.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] stash: require a clean index to apply

2015-06-10 Thread bär

On Wed, Jun 10, 2015 at 4:27 PM, Jeff King  wrote:
> I dunno. With respect to the original patch, I am OK if we just want to
> revert it. This area of stash seems a bit under-designed IMHO, but if
> people were happy enough with it before, I do not think the safety
> benefit from ed178ef is that great (it is not saving you from destroying
> working tree content, only the index state; the individual content blobs
> are still available from git-fsck).

I feel the same way, in fact I'm +1 to revert it until we figure out a
better way to deal with this properly.

-- 
Ber Clausen
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] doc: format-patch: fix typo

Frans Klaver  writes:

> reroll count documentation states that v will be pretended to the
> filename. Judging by the examples that should have been 'prepended'.
>
> Signed-off-by: Frans Klaver 
> ---

Good eyes; thanks.

>  Documentation/git-format-patch.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/git-format-patch.txt 
> b/Documentation/git-format-patch.txt
> index bb3ea93..0dac4e9 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -170,7 +170,7 @@ will want to ensure that threading is disabled for `git 
> send-email`.
>  -v ::
>  --reroll-count=::
>   Mark the series as the -th iteration of the topic. The
> - output filenames have `v` pretended to them, and the
> + output filenames have `v` prepended to them, and the
>   subject prefix ("PATCH" by default, but configurable via the
>   `--subject-prefix` option) has ` v` appended to it.  E.g.
>   `--reroll-count=4` may produce `v4-0001-add-makefile.patch`
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/14] tempfile: a new module for handling temporary files

Michael Haggerty  writes:

> On 06/10/2015 07:36 PM, Junio C Hamano wrote:
>> Michael Haggerty  writes:
>> 
>>> diff --git a/builtin/add.c b/builtin/add.c
>>> index df5135b..aaa9ce4 100644
>>> --- a/builtin/add.c
>>> +++ b/builtin/add.c
>>> @@ -5,6 +5,7 @@
>>>   */
>>>  #include "cache.h"
>>>  #include "builtin.h"
>>> +#include "tempfile.h"
>>>  #include "lockfile.h"
>>>  #include "dir.h"
>>>  #include "pathspec.h"
>> 
>> It is a bit sad that all users of lockfile.h has to include
>> tempfile.h; even when trying to find out something as basic as the
>> name of the file on which the lock is held, they need to look at
>> lk->tempfile.filename and that requires inclusion of tempfile.h
> ...
> Hmmm, currently lockfile.h doesn't include tempfile.h. But I think it is
> a good idea for it to do so. (I would have done it already but I thought
> it was against project policy.)

The project policy is "include what you use, do not rely on others
that happen include what you use" with a minor exception for the
"must be the first" headers git-compat-util.h (which cache.h and
friends include), I think.

If it does not include tempfile.h itself, lockfile.h would be at the
mercy of the *.c file that includes it to be able to see the struct
it uses; if *.c does not include tempfile.h before lockfile.h, it
would break.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] index-pack: avoid excessive re-reading of pack directory

2015-06-10 Thread Shawn Pearce

On Wed, Jun 10, 2015 at 7:00 AM, Jeff King  wrote:
> On Tue, Jun 09, 2015 at 08:46:24PM -0700, Shawn Pearce wrote:
>
>> > This patch introduces a "quick" flag to has_sha1_file which
>> > callers can use when they would prefer high performance at
>> > the cost of false negatives during repacks. There may be
>> > other code paths that can use this, but the index-pack one
>> > is the most obviously critical, so we'll start with
>> > switching that one.
>>
>> Hilarious. We did this in JGit back in ... uhm before 2009. :)
>>
>> But its Java. So of course we had to do optimizations.
>
> This is actually how Git did it up until v1.8.4.2, in 2013. I changed it
> then because the old way was racy (and git could flakily report refs as
> broken and skip them during repacks!).
>
> If you are doing it the "quick" way everywhere in JGit, you may want to
> reexamine the possibility for races. :)

Correct, fortunately we are not this naive.

JGit always does the reprepare_packed_git() + retry search on a miss.

But we have a code path to bypass that inside critical loops like our
equivalent of index-pack pulling off the wire. We snapshot the object
tree at the start of the operation before we read in the pack header,
and then require that the incoming pack be completed with that
snapshot. Since the snapshot was taking after ref
negotiation/advertisement, we should be at least as current as the
refs that were exchanged on the wire.

>> > @@ -3169,6 +3169,8 @@ int has_sha1_file(const unsigned char *sha1)
>> > return 1;
>> > if (has_loose_object(sha1))
>> > return 1;
>> > +   if (flags & HAS_SHA1_QUICK)
>> > +   return 0;
>> > reprepare_packed_git();
>> > return find_pack_entry(sha1, &e);
>>
>> Something else we do is readdir() over the loose objects and store
>> them in a map in memory. That way we avoid stat() calls during that
>> has_loose_object() path. This is apparently a win enough of the time
>> that we always do that when receiving a pack over the wire (client or
>> server).
>
> Yeah, I thought about that while writing this. It would be a win as long
> as you have a small number of loose objects and were going to make a
> large number of requests (otherwise you are traversing even though
> nobody is going to look it up). According to perf, though, loose object
> lookups are not a large expenditure[1].

Interesting. We were getting hurt by this in JGit. For most
repositories it was cheaper to issue 256 readdir() and build a set of
SHA-1s we found. I think we even just hit every directory 00..ff, we
don't even bother with a readdir() on the $GIT_DIR/objects itself.

> I'm also hesitant to go that route because it's basically caching, which
> introduces new opportunities for race conditions when the cache is stale
> (we do the same thing with loose refs, and we have indeed run into races
> there).

Yes. But see above, we do this only after we snapshot the packs, and
only after the ref negotiation, and only for the duration of parsing
the pack off the wire. So we should never have a data race.

Since JGit is multi-threaded, this cache is also effectively a
thread-local. Its never shared across threads.

> [1] As measured mostly by __d_lookup_rcu calls. Of course, my patch
> gives a 5% improvement over the original, and we were not spending
> 5% of the time there originally. I suspect part of the problem is
> that we do the lookup under a lock, so the longer we spend there,
> the more contention we have between threads, and the less
> parallelism. Indeed, I just did a quick repeat of my tests with
> pack.threads=1, and the size of the improvement shrinks.

Interesting. Yea, fine-grained locking can hurt parallel execution... :(
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 7/7] bisect: allows any terms set by user

Antoine Delaite  writes:

> -USAGE='[help|start|bad|good|new|old|skip|next|reset|visualize|replay|log|run]'
> +USAGE='[help|start|bad|good|new|old|terms|skip|next|reset|visualize|replay|log|run]'

I think this patch makes the whole series go in the right direction.

I wonder if you can skip the "we only support new/old if you are not
doing bog-standard bad/good" step and start from this "bisect terms"
one, though.

Then you do not even have to treat new/old any specially, and do not
even have to list them in the above list.

> @@ -79,9 +81,16 @@ bisect_start() {
>   orig_args=$(git rev-parse --sq-quote "$@")
>   bad_seen=0
>   eval=''
> - # start_bad_good is used to detect if we did a 
> - # 'git bisect start bad_rev good_rev'
> - start_bad_good=0
> + # terms_defined is used to detect if we did a
> + # 'git bisect start bad_rev good_rev' or if the user
> + # defined his own terms with git bisect terms
> + terms_defined=0

I like this change very much; it removes the mysteriously misnamed
start-bad-good variable (because you do not really _care_ that
'start' was what implicitly decided that good/bad pair is the term
we use in this session; what you care is that the terms are already
known or not).

That is another reason why I think it would be a better organization
for the patch series to do without the intermediate "we now add new/old
as another hardcoded values on top of the traditional bad/good".

That is, I would think a reasonable progression of the series would
look more like these three steps:

 - preliminary clean-up steps (e.g. "correct 'mistook'");

 - use $name_new and $name_old throughout the code, giving them
   'bad' and 'good' as hardcoded values; finally

 - add 'bisect terms' support.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: format-patch and submodules

2015-06-10 Thread Luke Diamand

On 10/06/15 18:04, Christopher Dunn wrote:

Sorry. I thought empty patches were made to work in other cases.

'git-p4' needs to skip these. Wrong mailing list then.

Possibly the right mailing list - can you explain what you mean here
w.r.t git-p4 please?

Thanks!
Luke

On Tue, Jun 9, 2015 at 1:52 PM, Jens Lehmann wrote:

Am 05.06.2015 um 01:20 schrieb Christopher Dunn:

(Seen in git versions: 2.1.0 and 1.9.3 et al.)

$ git format-patch --stdout X^..X | git apply check -
fatal: unrecognized input

This fails when the commit consists of nothing but a submodule change
(as in 'git add submodule foo'), but it passes when a file change is
added to the same commit.

There used to be a similar problem for empty commits, but that was
fixed around git-1.8:

http://stackoverflow.com/questions/20775132/cannot-apply-git-patch-replacing-a-file-with-a-link

Now, 'git format-patch' outputs nothing for an empty commit. I suppose
that needs to be the behavior also when only submodules are changed,
since in that case there is no 'diff' section from 'format-patch'.

Use-case: git-p4

Of course, we do not plan to add the submodule into Perforce, but we
would like this particular command to behave the same whether there
are other diffs or not.

Hmm, I'm not sure that this is a bug. It looks to me like doing a

$ git format-patch --stdout X^..X | git apply check -

when nothing is changed except submodules and expecting it to work
is the cause of the problem.

I get the same error when I do:

$git format-patch --stdout master..master | git apply --check -
fatal: unrecognized input

No submodules involved, just an empty patch.

I assume you want to ignore all submodule changes, so you should
check if e.g. "git diff --ignore-submodules X^..X" returns anything
before applying that? (From the command you ran I assume you might
be able to drop the --ignore-submodules because you already did set
"diff.ignoreSubmodules" to "all"?)

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/7] bisect: add the terms old/new

Antoine Delaite  writes:

> Related discussions:
>
>   - http://thread.gmane.org/gmane.comp.version-control.git/86063
>   introduced bisect fix unfixed to find fix.
>   - http://thread.gmane.org/gmane.comp.version-control.git/182398
>   discussion around bisect yes/no or old/new.
>   - http://thread.gmane.org/gmane.comp.version-control.git/199758
>   last discussion and reviews

Hmph, recent reviews and discussions we had here do not count?

I do agree with Matthieu's review on the last round, which is still
not quite addressed in this round.  The current code only supports
good/bad and this series merely adds another hardcoded pair old/new,
which is disappointing, given that this is a very good opportunity
to place an infrastructure to allow other pairs like unfixed/fixed
building on top of the most generic old/new.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] doc: format-patch: fix typo

2015-06-10 Thread Frans Klaver

reroll count documentation states that v will be pretended to the
filename. Judging by the examples that should have been 'prepended'.

Signed-off-by: Frans Klaver 
---
 Documentation/git-format-patch.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-format-patch.txt 
b/Documentation/git-format-patch.txt
index bb3ea93..0dac4e9 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -170,7 +170,7 @@ will want to ensure that threading is disabled for `git 
send-email`.
 -v ::
 --reroll-count=::
Mark the series as the -th iteration of the topic. The
-   output filenames have `v` pretended to them, and the
+   output filenames have `v` prepended to them, and the
subject prefix ("PATCH" by default, but configurable via the
`--subject-prefix` option) has ` v` appended to it.  E.g.
`--reroll-count=4` may produce `v4-0001-add-makefile.patch`
-- 
2.4.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/14] tempfile: a new module for handling temporary files

2015-06-10 Thread Michael Haggerty

On 06/10/2015 07:36 PM, Junio C Hamano wrote:
> Michael Haggerty  writes:
> 
>> diff --git a/builtin/add.c b/builtin/add.c
>> index df5135b..aaa9ce4 100644
>> --- a/builtin/add.c
>> +++ b/builtin/add.c
>> @@ -5,6 +5,7 @@
>>   */
>>  #include "cache.h"
>>  #include "builtin.h"
>> +#include "tempfile.h"
>>  #include "lockfile.h"
>>  #include "dir.h"
>>  #include "pathspec.h"
> 
> It is a bit sad that all users of lockfile.h has to include
> tempfile.h; even when trying to find out something as basic as the
> name of the file on which the lock is held, they need to look at
> lk->tempfile.filename and that requires inclusion of tempfile.h
> 
> It is a good idea to have tempfile as a separate module, as it
> allows new callers to use the same "clean-on-exit" infrastructure on
> things that are not locks, i.e. they can include tempfile.h without
> having to include lockfile.h, but I have to wonder if it is better
> to include tempfile.h from inside lockfile.h (which is alrady done)
> and allow users of lockfile API to assume that inclusion will always
> stay there.  After all, if they are taking locks, they already know
> lk->tempfile is the mechanism through which they need to learn about
> various aspects of the underlying files.

Hmmm, currently lockfile.h doesn't include tempfile.h. But I think it is
a good idea for it to do so. (I would have done it already but I thought
it was against project policy.)

I will make this change in v2.

> [...]

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] clone: check if server supports shallow clones

On Wed, Jun 10, 2015 at 04:25:45PM -0400, Michael Edgar wrote:

> > On Wed, Jun 10, 2015 at 3:05 PM, Jeff King  wrote:
> > I see that do_fetch_pack checks server_supports("shallow"). Is that
> > enough to cover all fetch cases? And if it is, why does it not cover the
> > matching clone cases?
> > -Peff
> 
> Great question. I determined that the do_fetch_pack logic doesn't work for
> clones because it also checks is_repository_shallow(), which looks for the
> .git/shallow file (or the alternate file).
> 
> I considered changing clone to create an empty .git/shallow file or alternate,
> but it turns out that an empty shallow file is treated as no shallow file at
> all. Since at this stage in cloning we have nothing to put in the shallow 
> file,
> it seemed like any other fix would require more substantial changes.

Hmm. Can we improve the check in do_fetch_pack? That is, do we have a
way of checking whether the _current_ fetch is going to make us shallow?
I don't see anything obvious (like a "depth" flag) passed into
do_fetch_pack, so I guess it is relying solely on global shallow state
that we've already set up? Or is it the case that "git fetch
--depth=" in a non-shallow repository does not properly check this flag?

If it turns out that "fetch --depth" is fine, and only "clone --depth"
is broken, I can certainly live with the patch you provided. But I think
we need to clarify that a bit in the commit message.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Using clean/smudge scripts from repository

2015-06-10 Thread Bob Bell

On Wed, Jun 10, 2015 at 08:22:18AM -0700, Junio C Hamano wrote:
> Bob Bell  writes:
> > Is this a proper solution, or did I just "luck out"?  Am I perhaps
> > doing something foolish?
> 
> Yes, we happen to run checkout in the index order, but that is not
> something we guarantee, so you can call yourself lucky.  You are
> being doubly lucky that nobody in your project is committing a
> malicious script in the repository.  It may also count as foolish,
> depending on how important the security is for you and how
> trustworthy your collaborators are.

Hrm, that's unfortunate.  So I gather it'll work, consistently, but
there's no guarantee that future versions of git won't break the
ordering assumption?  Is there anything available I can leverage here?
git has to at least assure that .gitattributes is checked out before the
files to which it could refer, right?

This is development in a corporate environment, and the collaborators
are trustworthy.  The alternative is to place the files on some network
share, where the same collaborators could edit it, etc.  But I was
hoping for a more self-contained solution, without such dependencies.

Thanks,
Bob
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] clone: check if server supports shallow clones

2015-06-10 Thread Michael Edgar

> On Wed, Jun 10, 2015 at 3:05 PM, Jeff King  wrote:
> I see that do_fetch_pack checks server_supports("shallow"). Is that
> enough to cover all fetch cases? And if it is, why does it not cover the
> matching clone cases?
> -Peff

Great question. I determined that the do_fetch_pack logic doesn't work for
clones because it also checks is_repository_shallow(), which looks for the
.git/shallow file (or the alternate file).

I considered changing clone to create an empty .git/shallow file or alternate,
but it turns out that an empty shallow file is treated as no shallow file at
all. Since at this stage in cloning we have nothing to put in the shallow file,
it seemed like any other fix would require more substantial changes.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] checkout: don't check worktrees when not necessary

2015-06-10 Thread Spencer Baugh


Duy Nguyen  writes:
> On Sun, May 31, 2015 at 07:16:29PM -0400, Spencer Baugh wrote:
>> --- a/builtin/checkout.c
>> +++ b/builtin/checkout.c
>> @@ -1237,6 +1237,7 @@ static int parse_branchname_arg(int argc, const char 
>> **argv,
>>  char *head_ref = resolve_refdup("HEAD", 0, sha1, &flag);
>>  if (head_ref &&
>>  (!(flag & REF_ISSYMREF) || strcmp(head_ref, new->path)) &&
>> +!(opts->patch_mode || opts->pathspec.nr) &&
>>  !opts->ignore_other_worktrees)
>>  check_linked_checkouts(new);
>>  free(head_ref);
>
> Simple and effective. But if in future we add more options for
> non-switching-branch checkout, we need to update both places, here and
> near the end of cmd_checkout().
>
> Perhaps we can move all this block inside checkout_branch() so we only
> need to test "opts->patch_mode || opts->pathspec.nr" once, at the end
> of cmd_checkout(). Something like below?
>
> I'm not opposed to your change, but if you go with it, you should
> cherry pick my test in the below patch. Or create a similar test.

Sorry for late reply, but I think your change is much better than mine
so I'd suggest just using that instead.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] stash: require a clean index to apply

On Wed, Jun 10, 2015 at 12:16:25PM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > So I am trying to figure out what the use case here is. Clearly the
> > above is a toy case, but why is "stash -k" followed by a quick pop
> > useful in general? Certainly I use "stash" (without "-k") and a quick
> > pop all the time, and I think that is what stash was designed for.
> >
> > The best use case I can think of is Jonathan's original: to see only the
> > staged content in the working tree, and then restore the original state.
> > But stash does not currently work very well for that, as shown above.
> 
> The canonical use case for "stash -k" is to see only the content to
> be committed (for testing), commit it after testing and then pop on
> top of the committed result, which is the same as what you saw in
> the working tree and the index when you did "stash -k".  I do not
> think "stash -k && stash pop" was in the design parameter when "-k"
> was added (as you demonstrated, it would not fundamentally work
> reliably depending on the differences between HEAD-Index-Worktree).

It seems like applying a stash made with "-k" is fundamentally
misdesigned in the current code. We would want to apply to the working
tree the difference between the index and the working tree, but instead
we try to apply the difference between the HEAD and the working tree.
Which is nonsensical for this use case (i.e., to apply the diff between
$stash and $stash^2, not $stash^1).

I don't think there is any way to tell that "-k" was used, though. But
even if the user knew that, I do not think there is any option to tell
"stash apply" to do it this way.

I dunno. With respect to the original patch, I am OK if we just want to
revert it. This area of stash seems a bit under-designed IMHO, but if
people were happy enough with it before, I do not think the safety
benefit from ed178ef is that great (it is not saving you from destroying
working tree content, only the index state; the individual content blobs
are still available from git-fsck).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] stash: require a clean index to apply

Jeff King  writes:

> So I am trying to figure out what the use case here is. Clearly the
> above is a toy case, but why is "stash -k" followed by a quick pop
> useful in general? Certainly I use "stash" (without "-k") and a quick
> pop all the time, and I think that is what stash was designed for.
>
> The best use case I can think of is Jonathan's original: to see only the
> staged content in the working tree, and then restore the original state.
> But stash does not currently work very well for that, as shown above.

The canonical use case for "stash -k" is to see only the content to
be committed (for testing), commit it after testing and then pop on
top of the committed result, which is the same as what you saw in
the working tree and the index when you did "stash -k".  I do not
think "stash -k && stash pop" was in the design parameter when "-k"
was added (as you demonstrated, it would not fundamentally work
reliably depending on the differences between HEAD-Index-Worktree).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] clone: check if server supports shallow clones

On Wed, Jun 10, 2015 at 02:35:20PM -0400, Mike Edgar wrote:

> When the user passes --depth to git-clone the server's capabilities are
> not currently consulted. The client will send shallow requests even if
> the server does not understand them, and the resulting error may be
> unhelpful to the user. This change pre-emptively checks so git-clone can
> exit with a helpful error if necessary.

This sounds like a good thing to do, but...

> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -944,6 +944,9 @@ int cmd_clone(int argc, const char **argv, const char 
> *prefix)
>  
>   refs = transport_get_remote_refs(transport);
>  
> + if (option_depth && !is_local && !server_supports("shallow"))
> + die(_("Server does not support shallow clients"));
> +

It feels a little weird to be checking the option here in cmd_clone.
The transport layer knows we have specified a depth, so it seems like a
more natural place for it (or possibly even lower, in the actual
git-protocol code).

That being said, I think the current capabilities handling is a bit
messy and crosses module boundaries freely. So I would not be surprised
if this is the most reasonable place to put it. But it does make me
wonder whether "git fetch --depth=..." needs the same treatment.

I see that do_fetch_pack checks server_supports("shallow"). Is that
enough to cover all fetch cases? And if it is, why does it not cover the
matching clone cases?

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 6/7] revision: fix rev-list --bisect in old/new mode

From: Louis Stuber 

Calling git rev-list --bisect when an old/new mode bisection was started
shows the help notice. This has been fixed by reading BISECT_TERMS in
revision.c to find the correct bisect refs path (which was always
refs/bisect/bad (or good) before and can be refs/bisect/new (old) now).

Signed-off-by: Louis Stuber 
Signed-off-by: Antoine Delaite 
---
 revision.c |   16 ++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/revision.c b/revision.c
index 7ddbaa0..a332270 100644
--- a/revision.c
+++ b/revision.c
@@ -21,6 +21,9 @@
 
 volatile show_early_output_fn_t show_early_output;
 
+static const char *name_bad;
+static const char *name_good;
+
 char *path_name(const struct name_path *path, const char *name)
 {
const struct name_path *p;
@@ -2073,14 +2076,22 @@ void parse_revision_opt(struct rev_info *revs, struct 
parse_opt_ctx_t *ctx,
ctx->argc -= n;
 }
 
+extern void read_bisect_terms(const char **bad, const char **good);
+
 static int for_each_bad_bisect_ref(const char *submodule, each_ref_fn fn, void 
*cb_data)
 {
-   return for_each_ref_in_submodule(submodule, "refs/bisect/bad", fn, 
cb_data);
+   char bisect_refs_path[256];
+   strcpy(bisect_refs_path, "refs/bisect/");
+   strcat(bisect_refs_path, name_bad);
+   return for_each_ref_in_submodule(submodule, bisect_refs_path, fn, 
cb_data);
 }
 
 static int for_each_good_bisect_ref(const char *submodule, each_ref_fn fn, 
void *cb_data)
 {
-   return for_each_ref_in_submodule(submodule, "refs/bisect/good", fn, 
cb_data);
+   char bisect_refs_path[256];
+   strcpy(bisect_refs_path, "refs/bisect/");
+   strcat(bisect_refs_path, name_good);
+   return for_each_ref_in_submodule(submodule, bisect_refs_path, fn, 
cb_data);
 }
 
 static int handle_revision_pseudo_opt(const char *submodule,
@@ -2109,6 +2120,7 @@ static int handle_revision_pseudo_opt(const char 
*submodule,
handle_refs(submodule, revs, *flags, 
for_each_branch_ref_submodule);
clear_ref_exclusion(&revs->ref_excludes);
} else if (!strcmp(arg, "--bisect")) {
+   read_bisect_terms(&name_bad, &name_good);
handle_refs(submodule, revs, *flags, for_each_bad_bisect_ref);
handle_refs(submodule, revs, *flags ^ (UNINTERESTING | BOTTOM), 
for_each_good_bisect_ref);
revs->bisect = 1;
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 7/7] bisect: allows any terms set by user

Introduction of the git bisect terms function.
The user can set its own terms.

List of known commands not available :
`git bisect replay`
`git bisect terms term1 term2
then
git bisect start bad_rev good_rev`

Signed-off-by: Antoine Delaite 
Signed-off-by: Louis Stuber 
---
 Documentation/git-bisect.txt |   19 ++
 git-bisect.sh|   44 ++---
 2 files changed, 55 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index 3c3021a..ef0c03c 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -133,6 +133,25 @@ You must run `git bisect start` without commits as 
argument and run
 `git bisect new `/`git bisect old ...` after to add the
 commits.
 
+Alternative terms: use your own terms
+
+
+If the builtins terms bad/good and new/old do not satisfy you, you can
+set your own terms.
+
+
+git bisect terms term1 term2
+
+
+This command has to be used before a bisection has started.
+The term1 must be associated with the latest revisions and term2 with the
+ancestors of term1.
+
+Only the first bisection following the 'git bisect terms' will use the terms.
+If you mistyped one of the terms you can do again 'git bisect terms term1
+term2'.
+
+
 Bisect visualize
 
 
diff --git a/git-bisect.sh b/git-bisect.sh
index c012f5d..22d65b1 100644
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-USAGE='[help|start|bad|good|new|old|skip|next|reset|visualize|replay|log|run]'
+USAGE='[help|start|bad|good|new|old|terms|skip|next|reset|visualize|replay|log|run]'
 LONG_USAGE='git bisect help
print this long help message.
 git bisect start [--no-checkout] [ [...]] [--] [...]
@@ -11,6 +11,8 @@ git bisect (bad|new) []
 git bisect (good|old) [...]
mark ... known-good revisions/
revisions before change in a given property.
+git bisect terms term1 term2
+   set up term1 and term2 as bisection terms.
 git bisect skip [(|)...]
mark ... untestable revisions.
 git bisect next
@@ -79,9 +81,16 @@ bisect_start() {
orig_args=$(git rev-parse --sq-quote "$@")
bad_seen=0
eval=''
-   # start_bad_good is used to detect if we did a 
-   # 'git bisect start bad_rev good_rev'
-   start_bad_good=0
+   # terms_defined is used to detect if we did a
+   # 'git bisect start bad_rev good_rev' or if the user
+   # defined his own terms with git bisect terms
+   terms_defined=0
+   if test -s "$GIT_DIR/TERMS_DEFINED"
+   then
+   terms_defined=1
+   get_terms
+   rm -rf "$GIT_DIR/TERMS_DEFINED"
+   fi
if test "z$(git rev-parse --is-bare-repository)" != zfalse
then
mode=--no-checkout
@@ -107,7 +116,7 @@ bisect_start() {
break
}
 
-   start_bad_good=1
+   terms_defined=1
 
case $bad_seen in
0) state=$NAME_BAD ; bad_seen=1 ;;
@@ -180,7 +189,7 @@ bisect_start() {
} &&
git rev-parse --sq-quote "$@" >"$GIT_DIR/BISECT_NAMES" &&
eval "$eval true" &&
-   if test $start_bad_good -eq 1 && test ! -s "$GIT_DIR/BISECT_TERMS"
+   if test $terms_defined -eq 1 && test ! -s "$GIT_DIR/BISECT_TERMS"
then
echo "$NAME_BAD" >"$GIT_DIR/BISECT_TERMS" &&
echo "$NAME_GOOD" >>"$GIT_DIR/BISECT_TERMS"
@@ -419,6 +428,7 @@ bisect_clean_state() {
rm -f "$GIT_DIR/BISECT_NAMES" &&
rm -f "$GIT_DIR/BISECT_RUN" &&
rm -f "$GIT_DIR/BISECT_TERMS" &&
+   rm -f "$GIT_DIR/TERMS_DEFINED" &&
# Cleanup head-name if it got left by an old version of git-bisect
rm -f "$GIT_DIR/head-name" &&
git update-ref -d --no-deref BISECT_HEAD &&
@@ -529,7 +539,8 @@ get_terms () {
 check_and_set_terms () {
cmd="$1"
case "$cmd" in
-   bad|good|new|old)
+   skip) ;;
+   *)
if test -s "$GIT_DIR/BISECT_TERMS" && test "$cmd" != 
"$NAME_BAD" && test "$cmd" != "$NAME_GOOD"
then
die "$(eval_gettext "Invalid command: you're currently 
in a \$NAME_BAD/\$NAME_GOOD bisect.")"
@@ -562,6 +573,21 @@ bisect_voc () {
esac
 }
 
+bisect_terms () {
+   test $# -eq 2 ||
+   die "You need to give me at least two arguments"
+
+   if ! test -s "$GIT_DIR/BISECT_START"
+   then
+   echo $1 >"$GIT_DIR/BISECT_TERMS" &&
+   echo $2 >>"$GIT_DIR/BISECT_TERMS" &&
+   echo "1" > "$GIT_DIR/TERMS_DEFINED"
+   else
+   die "A bisection has already started, please use "\
+   "'git bisect reset' to restart and change the terms"
+   fi
+}
+

[PATCH v2 5/7] bisect: change read_bisect_terms parameters

From: Louis Stuber 

The function reads BISECT_TERMS and stores it at the adress given in
parameters (instead of global variables name_bad and name_good).

This allows to use the function outside bisect.c.

Signed-off-by: Antoine Delaite 
Signed-off-by: Louis Stuber 
---
 bisect.c |   15 +++
 1 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/bisect.c b/bisect.c
index eaa85b6..7afd335 100644
--- a/bisect.c
+++ b/bisect.c
@@ -908,12 +908,11 @@ static void show_diff_tree(const char *prefix, struct 
commit *commit)
 }
 
 /*
- * The terms used for this bisect session are stored in
- * BISECT_TERMS: it can be bad/good or new/old.
- * We read them and store them to adapt the messages
- * accordingly. Default is bad/good.
+ * The terms used for this bisect session are stored in BISECT_TERMS.
+ * We read them and store them to adapt the messages accordingly.
+ * Default is bad/good.
  */
-void read_bisect_terms(void)
+void read_bisect_terms(const char **read_bad, const char **read_good)
 {
struct strbuf str = STRBUF_INIT;
const char *filename = git_path("BISECT_TERMS");
@@ -924,9 +923,9 @@ void read_bisect_terms(void)
strerror(errno));
} else {
strbuf_getline(&str, fp, '\n');
-   name_bad = strbuf_detach(&str, NULL);
+   *read_bad = strbuf_detach(&str, NULL);
strbuf_getline(&str, fp, '\n');
-   name_good = strbuf_detach(&str, NULL);
+   *read_good = strbuf_detach(&str, NULL);
}
strbuf_release(&str);
fclose(fp);
@@ -948,7 +947,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
const unsigned char *bisect_rev;
char bisect_rev_hex[GIT_SHA1_HEXSZ + 1];
 
-   read_bisect_terms();
+   read_bisect_terms(&name_bad, &name_good);
if (read_bisect_refs())
die("reading bisect refs failed");
 
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] stash: require a clean index to apply

On Wed, Jun 10, 2015 at 03:19:41PM -0300, bär wrote:

> On Sun, Jun 7, 2015 at 9:40 AM, Jeff King  wrote:
> > Hrm. The new protection in v2.4.2 is meant to prevent you from losing
> > your index state during step 4 when we run into a conflict. But here you
> > know something that git doesn't: that we just created the stash based on
> > this same state, so it should apply cleanly.
> 
> 
> It is strange that `git stash --keep-index && git stash pop` while
> having something in the index, fails with a "Cannot apply stash: Your
> index contains uncommitted changes." error, even if we have a
> `--force` param it find it awkward that one needs to force
> applying/pop'ing even though the stash was created from the same
> snapshot where the stash is being merged with.
> 
> I understand the original issue, but at least it was expected, when
> you stash save/pop/apply, you should know what you are doing anyways.

Yeah, I would expect "stash && pop" to work in general. But I find it
puzzling that it does not always with "-k" (this is with v2.3.x):

  $ git init -q
  $ echo head >file && git add file && git commit -qm head
  $ echo index >file && git add file
  $ echo tree >file
  $ git stash --keep-index && git stash pop
  Saved working directory and index state WIP on master: 74f6d33 head
  HEAD is now at 74f6d33 head
  Auto-merging file
  CONFLICT (content): Merge conflict in file

So I am trying to figure out what the use case here is. Clearly the
above is a toy case, but why is "stash -k" followed by a quick pop
useful in general? Certainly I use "stash" (without "-k") and a quick
pop all the time, and I think that is what stash was designed for.

The best use case I can think of is Jonathan's original: to see only the
staged content in the working tree, and then restore the original state.
But stash does not currently work very well for that, as shown above.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] clone: check if server supports shallow clones

2015-06-10 Thread Mike Edgar

When the user passes --depth to git-clone the server's capabilities are
not currently consulted. The client will send shallow requests even if
the server does not understand them, and the resulting error may be
unhelpful to the user. This change pre-emptively checks so git-clone can
exit with a helpful error if necessary.

Signed-off-by: Mike Edgar 
---
 builtin/clone.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/builtin/clone.c b/builtin/clone.c
index b878252..b4e9846 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -944,6 +944,9 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
 
refs = transport_get_remote_refs(transport);
 
+   if (option_depth && !is_local && !server_supports("shallow"))
+   die(_("Server does not support shallow clients"));
+
if (refs) {
mapped_refs = wanted_peer_refs(refs, refspec);
/*
-- 
2.2.0.rc0.207.ga3a616c

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/14] Introduce a tempfile module

Michael Haggerty  writes:

> These patches are also available from my GitHub repo [2], branch
> "tempfile".

Overall the series made a lot of sense.

On a few points I raised:

 - I still think that exposing the implementation detail of the
   lockfile API that it builds on tempfile API is going backwards.
   I wonder if a few helper functions added to the lockfile
   API to hide it, e.g. instead of letting the caller say
   "lk->tempfile.fd", have them call "lockfile_fd(lk)", would make
   things even more clean.

 - The tempfile API could be built on yet another layer of
   clean_on_exit_file API to unconfuse me on "register_tempfile()",
   but I do not think it is worth it only for two existing callers
   (gc and credential).

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/14] lockfile: remove some redundant functions

2015-06-10 Thread Johannes Sixt


Am 10.06.2015 um 19:40 schrieb Junio C Hamano:

Michael Haggerty  writes:


Remove the following functions and rewrite their callers to use the
equivalent tempfile functions directly:

* fdopen_lock_file() -> fdopen_tempfile()
* reopen_lock_file() -> reopen_tempfile()
* close_lock_file() -> close_tempfile()


Hmph,

My knee-jerk reaction was "I thought lockfile abstraction was
fine---why do we expose the implementation detail of the lockfile,
which is now happen to be implemented on top of the tempfile API, to
the callers?"


Just for the record, I had exactly the same reaction, and I find this 
transition against the spirit of a self-contained lockfile API.


-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git-checkout.txt: Document "git checkout " better

2015-06-10 Thread Torsten Bögershausen

On 2015-06-10 17.05, Junio C Hamano wrote:
> Torsten Bögershausen  writes:
> 
(Need to drop Eric from CC-list( 
>> git checkout  can be used to revert changes in the working tree.
> 
> I somehow thought that concensus in the recent thread was that
> "restore", not "revert", is the more appropriate wording?
> 
> And I think that is indeed sensible because "revert" (or "reset")
> already means something else in Git (and in other systems), while
> "restore" does not have a confusing connotation.  It can only mean
> "overwrite with a pristine copy", which is what the command is
> about.
> 
>> -git-checkout - Checkout a branch or paths to the working tree
>> +git-checkout - Switch branches or reverts changes in the working tree
> 
> Two verbs in different moods; either "switch branches or restore
> changes" or "switches branches or restores changes" would fix that,
> and judging from "git help" output, I think we want to go with the
> former, i.e. "switch branches or restore changes".
OK for me
> 
>>  
>>  SYNOPSIS
>>  
>> @@ -83,7 +83,8 @@ Omitting  detaches HEAD at the tip of the current 
>> branch.
>>  When  or `--patch` are given, 'git checkout' does *not*
>>  switch branches.  It updates the named paths in the working tree
>>  from the index file or from a named  (most often a
>> -commit).  In this case, the `-b` and `--track` options are
>> +commit).  Changes in files are discarded and deleted files are
>> +restored.
> 
[]
> How about this?
> 
>   'git checkout' with  or `--patch` is used to restore
> modified or deleted paths to their original contents from
> the index file or from a named  (most often a
> commit) without switching branches.
OK for me.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] stash: require a clean index to apply

2015-06-10 Thread bär

On Sun, Jun 7, 2015 at 9:40 AM, Jeff King  wrote:
> Hrm. The new protection in v2.4.2 is meant to prevent you from losing
> your index state during step 4 when we run into a conflict. But here you
> know something that git doesn't: that we just created the stash based on
> this same state, so it should apply cleanly.

It is strange that `git stash --keep-index && git stash pop` while
having something in the index, fails with a "Cannot apply stash: Your
index contains uncommitted changes." error, even if we have a
`--force` param it find it awkward that one needs to force
applying/pop'ing even though the stash was created from the same
snapshot where the stash is being merged with.

I understand the original issue, but at least it was expected, when
you stash save/pop/apply, you should know what you are doing anyways.

-- 
Ber Clausen
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] git-prompt.sh: Document GIT_PS1_STATESEPARATOR

2015-06-10 Thread Joe Cridge

The environment variable GIT_PS1_STATESEPARATOR can be used to set the
separator between the branch name and the state symbols in the prompt.

At present the variable is not mentioned in the inline documentation which
makes it difficult for the casual user to identify.

Signed-off-by: Joe Cridge 
---
 contrib/completion/git-prompt.sh | 4 
 1 file changed, 4 insertions(+)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index f18aedc..366f0bc 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -66,6 +66,10 @@
 # git   always compare HEAD to @{upstream}
 # svn   always compare HEAD to your SVN upstream
 #
+# You can change the separator between the branch name and the above
+# state symbols by setting GIT_PS1_STATESEPARATOR. The default separator
+# is SP.
+#
 # By default, __git_ps1 will compare HEAD to your SVN upstream if it can
 # find one, or @{upstream} otherwise.  Once you have set
 # GIT_PS1_SHOWUPSTREAM, you can override it on a per-repository basis by
-- 
2.4.2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 08/14] write_shared_index(): use tempfile module

Michael Haggerty  writes:

> Signed-off-by: Michael Haggerty 
> ---
>  read-cache.c | 37 +
>  1 file changed, 5 insertions(+), 32 deletions(-)

Nicely done.

>
> diff --git a/read-cache.c b/read-cache.c
> index 3e49c49..4f7b70f 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2137,54 +2137,27 @@ static int write_split_index(struct index_state 
> *istate,
>   return ret;
>  }
>  
> -static char *temporary_sharedindex;
> -
> -static void remove_temporary_sharedindex(void)
> -{
> - if (temporary_sharedindex) {
> - unlink_or_warn(temporary_sharedindex);
> - free(temporary_sharedindex);
> - temporary_sharedindex = NULL;
> - }
> -}
> -
> -static void remove_temporary_sharedindex_on_signal(int signo)
> -{
> - remove_temporary_sharedindex();
> - sigchain_pop(signo);
> - raise(signo);
> -}
> +static struct tempfile temporary_sharedindex;
>  
>  static int write_shared_index(struct index_state *istate,
> struct lock_file *lock, unsigned flags)
>  {
>   struct split_index *si = istate->split_index;
> - static int installed_handler;
>   int fd, ret;
>  
> - temporary_sharedindex = git_pathdup("sharedindex_XX");
> - fd = mkstemp(temporary_sharedindex);
> + fd = mks_tempfile(&temporary_sharedindex, 
> git_path("sharedindex_XX"));
>   if (fd < 0) {
> - free(temporary_sharedindex);
> - temporary_sharedindex = NULL;
>   hashclr(si->base_sha1);
>   return do_write_locked_index(istate, lock, flags);
>   }
> - if (!installed_handler) {
> - atexit(remove_temporary_sharedindex);
> - sigchain_push_common(remove_temporary_sharedindex_on_signal);
> - }
>   move_cache_to_base_index(istate);
>   ret = do_write_index(si->base, fd, 1);
> - close(fd);
>   if (ret) {
> - remove_temporary_sharedindex();
> + delete_tempfile(&temporary_sharedindex);
>   return ret;
>   }
> - ret = rename(temporary_sharedindex,
> -  git_path("sharedindex.%s", sha1_to_hex(si->base->sha1)));
> - free(temporary_sharedindex);
> - temporary_sharedindex = NULL;
> + ret = rename_tempfile(&temporary_sharedindex,
> +   git_path("sharedindex.%s", 
> sha1_to_hex(si->base->sha1)));
>   if (!ret)
>   hashcpy(si->base_sha1, si->base->sha1);
>   return ret;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/14] register_tempfile(): new function to handle an existing temporary file

Michael Haggerty  writes:

> Allow an existing file to be registered with the tempfile-handling
> infrastructure; in particular, arrange for it to be deleted on program
> exit.
>
> Signed-off-by: Michael Haggerty 
> ---

Hmph.  Where does such a tempfile that is not on list come from?

Puzzled, but let's read on---this could for example become an
internal implementation detail for create_tempfile().  Also I cannot
tell which one of register_tempfile() and register_tempfile_object()
I should be calling when updating the implementation of this API
from their names.

> diff --git a/tempfile.c b/tempfile.c
> index 890075f..235fc85 100644
> --- a/tempfile.c
> +++ b/tempfile.c
> @@ -82,6 +82,15 @@ int create_tempfile(struct tempfile *tempfile, const char 
> *path)
>   return tempfile->fd;
>  }
>  
> +void register_tempfile(struct tempfile *tempfile, const char *path)
> +{
> + register_tempfile_object(tempfile, path);
> +
> + strbuf_add_absolute_path(&tempfile->filename, path);
> + tempfile->owner = getpid();
> + tempfile->active = 1;
> +}
> +
>  int mks_tempfile_sm(struct tempfile *tempfile,
>   const char *template, int suffixlen, int mode)
>  {
> diff --git a/tempfile.h b/tempfile.h
> index 6276156..18ff963 100644
> --- a/tempfile.h
> +++ b/tempfile.h
> @@ -145,6 +145,14 @@ struct tempfile {
>   */
>  extern int create_tempfile(struct tempfile *tempfile, const char *path);
>  
> +/*
> + * Register an existing file as a tempfile, meaning that it will be
> + * deleted when the program exits. The tempfile is considered closed,
> + * but it can be worked with like any other closed tempfile (for
> + * example, it can be opened using reopen_tempfile()).
> + */
> +extern void register_tempfile(struct tempfile *tempfile, const char *path);
> +
>  
>  /*
>   * mks_tempfile functions
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/14] tempfile: add several functions for creating temporary files

Michael Haggerty  writes:

> Add several functions for creating temporary files with
> automatically-generated names, analogous to mkstemps(), but also
> arranging for the files to be deleted on program exit.
>
> The functions are named according to a pattern depending how they
> operate. They will be used to replace many places in the code where
> temporary files are created and cleaned up ad-hoc.
>
> Signed-off-by: Michael Haggerty 
> ---
>  tempfile.c | 55 ++-
>  tempfile.h | 96 
> ++
>  2 files changed, 150 insertions(+), 1 deletion(-)
>
> diff --git a/tempfile.c b/tempfile.c
> index f76bc07..890075f 100644
> --- a/tempfile.c
> +++ b/tempfile.c
> @@ -48,7 +48,7 @@ static void register_tempfile_object(struct tempfile 
> *tempfile, const char *path
>   tempfile->fp = NULL;
>   tempfile->active = 0;
>   tempfile->owner = 0;
> - strbuf_init(&tempfile->filename, strlen(path));
> + strbuf_init(&tempfile->filename, 0);
>   tempfile->next = tempfile_list;
>   tempfile_list = tempfile;
>   tempfile->on_list = 1;

This probably could have been part of the previous step.  Regardless
of where in the patch series this change is done, I think it makes
sense, as this function does not even know how long the final filename
would be, and strlen(path) is almost always wrong as path is likely to
be relative.

I notice this change makes "path" almost unused in this function,
and the only remaining use is for assert(!tempfile->filename.len).
Perhaps it is not worth passing the "path" parameter?

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/14] lockfile: remove some redundant functions

Michael Haggerty  writes:

> Remove the following functions and rewrite their callers to use the
> equivalent tempfile functions directly:
>
> * fdopen_lock_file() -> fdopen_tempfile()
> * reopen_lock_file() -> reopen_tempfile()
> * close_lock_file() -> close_tempfile()

Hmph, 

My knee-jerk reaction was "I thought lockfile abstraction was
fine---why do we expose the implementation detail of the lockfile,
which is now happen to be implemented on top of the tempfile API, to
the callers?"  I guess that was also where my comments on 02/14 "why
do callers have to include tempfile.h separately?" came from.

I'm undecided but would trust your judgement until I read thru to
the end of the series ;-).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/14] tempfile: a new module for handling temporary files

Michael Haggerty  writes:

> diff --git a/builtin/add.c b/builtin/add.c
> index df5135b..aaa9ce4 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -5,6 +5,7 @@
>   */
>  #include "cache.h"
>  #include "builtin.h"
> +#include "tempfile.h"
>  #include "lockfile.h"
>  #include "dir.h"
>  #include "pathspec.h"

It is a bit sad that all users of lockfile.h has to include
tempfile.h; even when trying to find out something as basic as the
name of the file on which the lock is held, they need to look at
lk->tempfile.filename and that requires inclusion of tempfile.h

It is a good idea to have tempfile as a separate module, as it
allows new callers to use the same "clean-on-exit" infrastructure on
things that are not locks, i.e. they can include tempfile.h without
having to include lockfile.h, but I have to wonder if it is better
to include tempfile.h from inside lockfile.h (which is alrady done)
and allow users of lockfile API to assume that inclusion will always
stay there.  After all, if they are taking locks, they already know
lk->tempfile is the mechanism through which they need to learn about
various aspects of the underlying files.

> @@ -101,60 +72,17 @@ static void resolve_symlink(struct strbuf *path)
>  /* Make sure errno contains a meaningful value on error */
>  static int lock_file(struct lock_file *lk, const char *path, int flags)
>  {
> ...
> + int fd;
> + struct strbuf filename = STRBUF_INIT;
>  
> - if (flags & LOCK_NO_DEREF) {
> - strbuf_add_absolute_path(&lk->filename, path);
> - } else {
> - struct strbuf resolved_path = STRBUF_INIT;
> + strbuf_addstr(&filename, path);
> + if (!(flags & LOCK_NO_DEREF))
> + resolve_symlink(&filename);
>  
> - strbuf_add(&resolved_path, path, pathlen);
> - resolve_symlink(&resolved_path);
> - strbuf_add_absolute_path(&lk->filename, resolved_path.buf);
> - strbuf_release(&resolved_path);
> - }
> ...
> - return lk->fd;
> + strbuf_addstr(&filename, LOCK_SUFFIX);
> + fd = create_tempfile(&lk->tempfile, filename.buf);
> + strbuf_release(&filename);
> + return fd;
>  }

This was the only part of this entire patch that needed more than
cursory reading ;-) and it looks correct.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 11/19] pull: check if in unresolved merge state

Paul Tan  writes:

> On Wed, Jun 10, 2015 at 10:38 PM, Junio C Hamano  wrote:
>>> If you are going to do the git_config() call yourself, it might make
>>> more sense to define git_pull_config() callback and parse the pull.ff
>>> yourself, updating the use of the lazy git_config_get_value() API you
>>> introduced in patch 10/19.
>>>
>>> The above "might" is stronger than my usual "might"; I am undecided
>>> yet before reading the remainder of the series.
>>
>> Let me clarify the above with s/stronger/with much less certainty/;
>
> Uh, I have no idea what a "strong might" or a "less certain might" is. :-/
>
> Parsing all the config values with a git_config() callback function is
> certainly possible, but I was under the impression that we are moving
> towards migrating use of all the git_config() callback loops to the
> git_config_get_X() API.
>
> In this case though, we have to use git_config() to initialize the
> advice.resolveConflict config setting, but I don't see why it must be
> in conflict with the above goal.

I (or at least some part of me) actually view git_config_get_*() as
"if you are only going to peek a few variables, you do not have to
do the looping yourself" convenience, which leads me (or at least
that part of me) to say "if you are doing the looping anyway, you
may be better off picking up the variables you care about yourself
in that loop".

By calling git_config() before calling any git_config_get_*()
functions, you would be priming the cache layer with the entire
contents of the configuration files anyway, so later calls to
git_config_get_*() will read from that cache, so there is no
performance penalty in mixing the two methods to access
configuration data.  That is why I felt less certain that the
suggestion to stick to one method (instead of mixing the two) is a
good thing to do (hence "less certain 'might'").
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RFC: 'git-sym' for large files

2015-06-10 Thread Christopher Dunn

https://github.com/cdunn2001/git-sym

https://github.com/cdunn2001/git-sym-test/wiki/Examples
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: format-patch and submodules

2015-06-10 Thread Christopher Dunn

Sorry. I thought empty patches were made to work in other cases.

'git-p4' needs to skip these. Wrong mailing list then.

On Tue, Jun 9, 2015 at 1:52 PM, Jens Lehmann  wrote:
> Am 05.06.2015 um 01:20 schrieb Christopher Dunn:
>>
>> (Seen in git versions: 2.1.0 and 1.9.3 et al.)
>>
>> $ git format-patch --stdout X^..X | git apply check -
>> fatal: unrecognized input
>>
>> This fails when the commit consists of nothing but a submodule change
>> (as in 'git add submodule foo'), but it passes when a file change is
>> added to the same commit.
>>
>> There used to be a similar problem for empty commits, but that was
>> fixed around git-1.8:
>>
>>
>> http://stackoverflow.com/questions/20775132/cannot-apply-git-patch-replacing-a-file-with-a-link
>>
>> Now, 'git format-patch' outputs nothing for an empty commit. I suppose
>> that needs to be the behavior also when only submodules are changed,
>> since in that case there is no 'diff' section from 'format-patch'.
>>
>> Use-case: git-p4
>>
>> Of course, we do not plan to add the submodule into Perforce, but we
>> would like this particular command to behave the same whether there
>> are other diffs or not.
>
>
> Hmm, I'm not sure that this is a bug. It looks to me like doing a
>
> $ git format-patch --stdout X^..X | git apply check -
>
> when nothing is changed except submodules and expecting it to work
> is the cause of the problem.
>
> I get the same error when I do:
>
> $git format-patch --stdout master..master | git apply --check -
> fatal: unrecognized input
>
> No submodules involved, just an empty patch.
>
> I assume you want to ignore all submodule changes, so you should
> check if e.g. "git diff --ignore-submodules X^..X" returns anything
> before applying that? (From the command you ran I assume you might
> be able to drop the --ignore-submodules because you already did set
> "diff.ignoreSubmodules" to "all"?)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git-checkout.txt: Document "git checkout " better

Ed Avis  writes:

> 'restore' may be more consistent with git's internal terminology.
> But from an outsider's perspective, 'revert' rather than 'restore' is in my
> view much clearer and more consistent with other version control systems:
> for example 'svn revert' is what you use to revert files in the working copy.

The reason why I said "restore" is because it does *not* have any
"internal terminology" connotation.

On the other hand, "revert" that means "create a counter-effect
commit" is not "internal".  "git revert" is a part of end-user
facing command.

The only people that will be helped by using "revert" there will be
the ones who haven't learned "git revert".  And it will make it
harder for them to learn "git revert".  It is unfortunate that other
systems use the word "revert" in a different way, and that is why we
should avoid using that word when describing "checkout".

> The original issue was that I naively expected that 'git checkout PATH' would
> indeed just 'restore' some files, that is, create them when they are missing.
> ...
> If 'revert' is not a suitable verb because of the existing git-revert, then
> I suggest that 'overwrite' or 'replace' might better convey the idea of what
> the command does.

Git is about "contents", not "files".  You modify a file, and
restore its contents to its pristine state.  It is not "restore the
file", as Git is not about "files".

I think "overwrite is better" is primarily coming from not thinking
in terms of "Git tracks contents, not files".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git lock files

Stefan Beller  writes:

> I could imagine a "git lock" command which looks like this:
>
> git config lock.centralServer origin
> git config lock.defaultBranch master
>
> git lock add [branch]  [--] 
> git lock remove [branch] [--] 
> git lock ls []
>
> And the way this is implemented is roughly (unoptimized, just showing
> how you would achieve this with todays command set):
> ...
> git fetch --depth=1 $(git config --get lock.centralServer) 
> refs/locks/$(git config --get lock.defaultBranch)
> git checkout refs/locks/$(git config --get lock.centralServer)/$(git 
> config --get lock.defaultBranch)
> switch(option) {
> case add:
> if exist 
> return -1
> else
> echo $(git config --get user.name) $(date) > 
> git add  && git commit "add new lock"
> fi
> case remove:
> if exist 
> # todo: check if the same user locked it before
> rm  
> else
> return -1
> fi
> case ls:
> ls -R .
> }
> git push $(git config --get lock.centralServer) refs/locks/$(git config 
> --get lock.defaultBranch)
> git 
>
> That said you could just manipulate the git objects directly, no need
> to check out to the working dir.
>
> The server would only need to allow pushes to a refs/locks directory and be 
> done.
> the client side would need to have a plumbing command, so you could easily 
> integrate
> a git locking to your application if you don't want to provide a merge driver.

I do not think that would be very useful nor even be a good starting
point, even though I think it is a good tangent to think about how
to improve the support for the centralized workflow.

You cannot afford to force clients keep polling the central server
for the refs/locks/my-branch, if you want a good "everybody relies
on central-server to coordinate" workflow experience.

And even without "file locking", once you start assuming "everybody
relies on central-server to coordinate" workflow (which is common in
corporate settings), you would be better off introducing a mechanism
to push notification from the server side to the clients to improve
support for other things, like "the client watches these branches,
doing a hanging Get or whatever, waiting for the server to notify"
anyway.  The kind of notification that distributed use of Git can do
without.

And once that kind of mechanism is there, "the client is notified
not to touch these paths on this branch", "the client is notified
that it is now OK to touch these paths on this branch after
updating", etc., would be a natural addition.

I highly doubt that exchanging data via the "git fetch" and "git
push" will be a good vehicle to implement such an async notification
mechanism.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] bisect: simplify the add of new bisect terms

Antoine Delaite  writes:

>>> +get_terms () {
>>> +if test -s "$GIT_DIR/BISECT_TERMS"
>>> +then
>>> +NAME_BAD="$(sed -n 1p "$GIT_DIR/BISECT_TERMS")"
>>> +NAME_GOOD="$(sed -n 2p "$GIT_DIR/BISECT_TERMS")"
>>
>>It is sad that we need to open the file twice.  Can't we do
>>something using "read" perhaps?
>
> The cost of it is quite low and we see directly what we meant. We didn't 
> found a pretty way to read two lines with read.

Should be stg like:

{
read good
read bad
} <"$GIT_DIR/BISECT_TERMS"

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 4/7] bisect: add the terms old/new

When not looking for a regression during a bisect but for a fix or a
change in another given property, it can be confusing to use 'good'
and 'bad'.

This patch introduce `git bisect new` and `git bisect old` as an
alternative to 'bad' and good': the commits which have a certain
property must be marked as `new` and the ones which do not as `old`.

The output will be the first commit after the change in the property.
During a new/old bisect session you cannot use bad/good commands and
vice-versa.

`git bisect replay` works fine for old/new bisect sessions.

Some commands are still not available for old/new:

 * git bisect start [ [...]] is not possible: the
   commits will be treated as bad and good.
 * git rev-list --bisect does not treat the revs/bisect/new and
   revs/bisect/old-SHA1 files.
 * git bisect visualize seem to work partially: the tags are
   displayed correctly but the tree is not limited to the bisect
   section.

Related discussions:

- http://thread.gmane.org/gmane.comp.version-control.git/86063
introduced bisect fix unfixed to find fix.
- http://thread.gmane.org/gmane.comp.version-control.git/182398
discussion around bisect yes/no or old/new.
- http://thread.gmane.org/gmane.comp.version-control.git/199758
last discussion and reviews

Signed-off-by: Antoine Delaite 
Signed-off-by: Louis Stuber 
Signed-off-by: Valentin Duperray 
Signed-off-by: Franck Jonas 
Signed-off-by: Lucien Kong 
Signed-off-by: Thomas Nguy 
Signed-off-by: Huynh Khoi Nguyen Nguyen 

Signed-off-by: Matthieu Moy 
---
 Documentation/git-bisect.txt |   48 -
 bisect.c |   11 +++--
 git-bisect.sh|   30 +
 t/t6030-bisect-porcelain.sh  |   38 +
 4 files changed, 112 insertions(+), 15 deletions(-)

diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index 4cb52a7..3c3021a 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -18,8 +18,8 @@ on the subcommand:
 
  git bisect help
  git bisect start [--no-checkout] [ [...]] [--] [...]
- git bisect bad []
- git bisect good [...]
+ git bisect (bad|new) []
+ git bisect (good|old) [...]
  git bisect skip [(|)...]
  git bisect reset []
  git bisect visualize
@@ -104,6 +104,35 @@ For example, `git bisect reset HEAD` will leave you on the 
current
 bisection commit and avoid switching commits at all, while `git bisect
 reset bisect/bad` will check out the first bad revision.
 
+
+Alternative terms: bisect new and bisect old
+
+
+If you are not at ease with the terms "bad" and "good", perhaps
+because you are looking for the commit that introduced a fix, you can
+alternatively use "new" and "old" instead.
+But note that you cannot mix "bad" and good" with "new" and "old".
+
+
+git bisect new []
+
+
+Marks the commit as new, e.g. "the bug is no longer there", if you are looking
+for a commit that fixed a bug, or "the feature that used to work is now broken
+at this point", if you are looking for a commit that introduced a bug.
+It is the equivalent of "git bisect bad []".
+
+
+git bisect old [...]
+
+
+Marks the commit as old, as the opposite of 'git bisect new'.
+It is the equivalent of "git bisect good [...]".
+
+You must run `git bisect start` without commits as argument and run
+`git bisect new `/`git bisect old ...` after to add the
+commits.
+
 Bisect visualize
 
 
@@ -379,6 +408,21 @@ In this case, when 'git bisect run' finishes, bisect/bad 
will refer to a commit
 has at least one parent whose reachable graph is fully traversable in the sense
 required by 'git pack objects'.
 
+* Look for a fix instead of a regression in the code
++
+
+$ git bisect start
+$ git bisect new HEAD# current commit is marked as new
+$ git bisect old HEAD~10 # the tenth commit from now is marked as old
+
++
+Let's consider the last commit has a given property, and that we are looking
+for the commit which introduced this property. For each commit the bisection
+guide us to, we will test if the property is present. If it is we will mark
+the commit as new with 'git bisect new', otherwise we will mark it as old.
+At the end of the bisect session, the result will be the first new commit (e.g
+the first one with the property).
+
 
 SEE ALSO
 
diff --git a/bisect.c b/bisect.c
index 827d2f3..eaa85b6 100644
--- a/bisect.c
+++ b/bisect.c
@@ -744,6 +744,11 @@ static void handle_bad_merge_base(void)
"This means the bug has been fixed "
"between %s and [%s].\n",
b

Re: [PATCH v3 7/7] send-email: suppress leading and trailing whitespaces before alias expansion

Junio C Hamano  writes:

> Matthieu Moy  writes:
>
>> Junio C Hamano  writes:
>>
>>> Hmph, if I have "A, B, C" and call a function that gives an array of
>>> addresses, treating the input as comma-separated addresses, I would
>>> expect ("A", "B", "C") to be returned from that function, instead of
>>> having to later trim the whitespace around what is returned.
>>
>> It is actually doing this. But if you have " A,B,C  ", then you'll get
>> " A", "B", "C  ". But once you're trimming around commas, trimming
>> leading and trailing spaces fits well with split itself.
>
> I guess we are saying the same thing, then?  That is, trim-list as a
> separate step does not make sense an it is part of the job for the
> helper to turn a single list with multiple addresses into an array?

Yes. I was clarifying what was done and what wasn't, not disagreeing.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/7] bisect: replace hardcoded "bad|good" by variables

To add new tags like old/new and have keywords less confusing, the
first step is to avoid hardcoding the keywords.

The default mode is still bad/good.

Signed-off-by: Antoine Delaite 
Signed-off-by: Louis Stuber 
Signed-off-by: Valentin Duperray 
Signed-off-by: Franck Jonas 
Signed-off-by: Lucien Kong 
Signed-off-by: Thomas Nguy 
Signed-off-by: Huynh Khoi Nguyen Nguyen 

Signed-off-by: Matthieu Moy 
---
 bisect.c  |   49 -
 git-bisect.sh |   57 +++--
 2 files changed, 63 insertions(+), 43 deletions(-)
 mode change 100755 => 100644 git-bisect.sh

diff --git a/bisect.c b/bisect.c
index de92953..eb2f555 100644
--- a/bisect.c
+++ b/bisect.c
@@ -21,6 +21,9 @@ 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", 
"BISECT_HEAD", NULL, NULL};
 
+static const char *name_bad;
+static const char *name_good;
+
 /* Remember to update object flag allocation in object.h */
 #define COUNTED(1u<<16)
 
@@ -403,10 +406,14 @@ struct commit_list *find_bisection(struct commit_list 
*list,
 static int register_ref(const char *refname, const unsigned char *sha1,
int flags, void *cb_data)
 {
-   if (!strcmp(refname, "bad")) {
+   char good_prefix[256];
+   strcpy(good_prefix, name_good);
+   strcat(good_prefix, "-");
+
+   if (!strcmp(refname, name_bad)) {
current_bad_oid = xmalloc(sizeof(*current_bad_oid));
hashcpy(current_bad_oid->hash, sha1);
-   } else if (starts_with(refname, "good-")) {
+   } else if (starts_with(refname, good_prefix)) {
sha1_array_append(&good_revs, sha1);
} else if (starts_with(refname, "skip-")) {
sha1_array_append(&skipped_revs, sha1);
@@ -634,7 +641,7 @@ static void exit_if_skipped_commits(struct commit_list 
*tried,
return;
 
printf("There are only 'skip'ped commits left to test.\n"
-  "The first bad commit could be any of:\n");
+  "The first %s commit could be any of:\n", name_bad);
print_commit_list(tried, "%s\n", "%s\n");
if (bad)
printf("%s\n", oid_to_hex(bad));
@@ -732,18 +739,21 @@ static void handle_bad_merge_base(void)
if (is_expected_rev(current_bad_oid)) {
char *bad_hex = oid_to_hex(current_bad_oid);
char *good_hex = join_sha1_array_hex(&good_revs, ' ');
-
-   fprintf(stderr, "The merge base %s is bad.\n"
-   "This means the bug has been fixed "
-   "between %s and [%s].\n",
-   bad_hex, bad_hex, good_hex);
-
+   if (!strcmp(name_bad, "bad")) {
+   fprintf(stderr, "The merge base %s is bad.\n"
+   "This means the bug has been fixed "
+   "between %s and [%s].\n",
+   bad_hex, bad_hex, good_hex);
+   } else {
+   die("BUG: terms %s/%s not managed", name_bad, 
name_good);
+   }
exit(3);
}
 
-   fprintf(stderr, "Some good revs are not ancestor of the bad rev.\n"
+   fprintf(stderr, "Some %s revs are not ancestor of the %s rev.\n"
"git bisect cannot work properly in this case.\n"
-   "Maybe you mistook good and bad revs?\n");
+   "Maybe you mistook %s and %s revs?\n",
+   name_good, name_bad, name_good, name_bad);
exit(1);
 }
 
@@ -755,10 +765,10 @@ static void handle_skipped_merge_base(const unsigned char 
*mb)
 
warning("the merge base between %s and [%s] "
"must be skipped.\n"
-   "So we cannot be sure the first bad commit is "
+   "So we cannot be sure the first %s commit is "
"between %s and %s.\n"
"We continue anyway.",
-   bad_hex, good_hex, mb_hex, bad_hex);
+   bad_hex, good_hex, name_bad, mb_hex, bad_hex);
free(good_hex);
 }
 
@@ -839,7 +849,7 @@ static void check_good_are_ancestors_of_bad(const char 
*prefix, int no_checkout)
int fd;
 
if (!current_bad_oid)
-   die("a bad revision is needed");
+   die("a %s revision is needed", name_bad);
 
/* Check if file BISECT_ANCESTORS_OK exists. */
if (!stat(filename, &st) && S_ISREG(st.st_mode))
@@ -905,6 +915,8 @@ int bisect_next_all(const char *prefix, int no_checkout)
const unsigned char *bisect_rev;
char bisect_rev_hex[GIT_SHA1_HEXSZ + 1];
 
+   name_bad="bad";
+   name_good="good";
if (read_bisect_refs())
die("reading bisect refs failed");
 
@@ -926,8 +938,10 @@ int bisect_next_all(const char *

[PATCH v2 3/7] bisect: simplify the addition of new bisect terms

We create a file BISECT_TERMS in the repository .git to be read during a
bisection. The fonctions to be changed if we add new terms are quite
few.
In git-bisect.sh :
check_and_set_terms
bisect_voc
In bisect.c :
handle_bad_merge_base

Signed-off-by: Antoine Delaite 
Signed-off-by: Louis Stuber 
Signed-off-by: Valentin Duperray 
Signed-off-by: Franck Jonas 
Signed-off-by: Lucien Kong 
Signed-off-by: Thomas Nguy 
Signed-off-by: Huynh Khoi Nguyen Nguyen 

Signed-off-by: Matthieu Moy 
---
 bisect.c  |   33 +--
 git-bisect.sh |   66 +++-
 2 files changed, 90 insertions(+), 9 deletions(-)

diff --git a/bisect.c b/bisect.c
index eb2f555..827d2f3 100644
--- a/bisect.c
+++ b/bisect.c
@@ -745,7 +745,10 @@ static void handle_bad_merge_base(void)
"between %s and [%s].\n",
bad_hex, bad_hex, good_hex);
} else {
-   die("BUG: terms %s/%s not managed", name_bad, 
name_good);
+   fprintf(stderr, "The merge base %s is %s.\n"
+   "This means the first commit marked %s is "
+   "between %s and [%s].\n",
+   bad_hex, name_bad, name_bad, bad_hex, good_hex);
}
exit(3);
}
@@ -900,6 +903,31 @@ static void show_diff_tree(const char *prefix, struct 
commit *commit)
 }
 
 /*
+ * The terms used for this bisect session are stored in
+ * BISECT_TERMS: it can be bad/good or new/old.
+ * We read them and store them to adapt the messages
+ * accordingly. Default is bad/good.
+ */
+void read_bisect_terms(void)
+{
+   struct strbuf str = STRBUF_INIT;
+   const char *filename = git_path("BISECT_TERMS");
+   FILE *fp = fopen(filename, "r");
+
+   if (!fp) {
+   die("could not read file '%s': %s", filename,
+   strerror(errno));
+   } else {
+   strbuf_getline(&str, fp, '\n');
+   name_bad = strbuf_detach(&str, NULL);
+   strbuf_getline(&str, fp, '\n');
+   name_good = strbuf_detach(&str, NULL);
+   }
+   strbuf_release(&str);
+   fclose(fp);
+}
+
+/*
  * 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.
@@ -915,8 +943,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
const unsigned char *bisect_rev;
char bisect_rev_hex[GIT_SHA1_HEXSZ + 1];
 
-   name_bad="bad";
-   name_good="good";
+   read_bisect_terms();
if (read_bisect_refs())
die("reading bisect refs failed");
 
diff --git a/git-bisect.sh b/git-bisect.sh
index ce6412f..d63b4b0 100644
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -77,6 +77,9 @@ bisect_start() {
orig_args=$(git rev-parse --sq-quote "$@")
bad_seen=0
eval=''
+   # start_bad_good is used to detect if we did a 
+   # 'git bisect start bad_rev good_rev'
+   start_bad_good=0
if test "z$(git rev-parse --is-bare-repository)" != zfalse
then
mode=--no-checkout
@@ -101,6 +104,9 @@ bisect_start() {
die "$(eval_gettext "'\$arg' does not appear to 
be a valid revision")"
break
}
+
+   start_bad_good=1
+
case $bad_seen in
0) state=$NAME_BAD ; bad_seen=1 ;;
*) state=$NAME_GOOD ;;
@@ -172,6 +178,11 @@ bisect_start() {
} &&
git rev-parse --sq-quote "$@" >"$GIT_DIR/BISECT_NAMES" &&
eval "$eval true" &&
+   if test $start_bad_good -eq 1 && test ! -s "$GIT_DIR/BISECT_TERMS"
+   then
+   echo "$NAME_BAD" >"$GIT_DIR/BISECT_TERMS" &&
+   echo "$NAME_GOOD" >>"$GIT_DIR/BISECT_TERMS"
+   fi &&
echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
#
# Check if we can proceed to the next bisect state.
@@ -232,6 +243,7 @@ bisect_skip() {
 bisect_state() {
bisect_autostart
state=$1
+   check_and_set_terms $state
case "$#,$state" in
0,*)
die "$(gettext "Please call 'bisect_state' with at least one 
argument.")" ;;
@@ -291,15 +303,17 @@ bisect_next_check() {
: bisect without $NAME_GOOD...
;;
*)
-
+   bad_syn=$(bisect_voc bad)
+   good_syn=$(bisect_voc good)
if test -s "$GIT_DIR/BISECT_START"
then
-   gettextln "You need to give me at least one good and 
one bad revision.
-(You can use \"git bisect bad\" and \"git bisect good\" for that.)" >&2
+
+   eval_gettextln "You need to give me at least one 
\$bad_

[PATCH v2 1/7] bisect : correction of typo


Signed-off-by: Antoine Delaite 
---
 bisect.c|2 +-
 t/t6030-bisect-porcelain.sh |2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/bisect.c b/bisect.c
index 10f5e57..de92953 100644
--- a/bisect.c
+++ b/bisect.c
@@ -743,7 +743,7 @@ static void handle_bad_merge_base(void)
 
fprintf(stderr, "Some good revs are not ancestor of the bad rev.\n"
"git bisect cannot work properly in this case.\n"
-   "Maybe you mistake good and bad revs?\n");
+   "Maybe you mistook good and bad revs?\n");
exit(1);
 }
 
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 06b4868..9e2c203 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -362,7 +362,7 @@ test_expect_success 'bisect starting with a detached HEAD' '
 test_expect_success 'bisect errors out if bad and good are mistaken' '
git bisect reset &&
test_must_fail git bisect start $HASH2 $HASH4 2> rev_list_error &&
-   grep "mistake good and bad" rev_list_error &&
+   grep "mistook good and bad" rev_list_error &&
git bisect reset
 '
 
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] bisect: simplify the add of new bisect terms

Matthieu Moy  writes:

> Junio C Hamano  writes:
>
>> But I do not think it is a good idea to penalize the normal case by
>> writing the terms file and reading them back from it when the user
>> is bisecting with good/bad in the first place, so
>
> No strong opinion on that, but creating one file doesn't cost much, and
> one advantage of writing it unconditionally is that it unifies bad/good
> and old/new more in the code. Just the creation of BISECT_TERMS becomes
> a special-case.

You have to handle that case anyway when I start bisection, which is
going to take very long and I had to leave for the day to come back
the next day to continue, and during the night the sysadmin updates
the installed version of Git.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 7/7] send-email: suppress leading and trailing whitespaces before alias expansion

Matthieu Moy  writes:

> Junio C Hamano  writes:
>
>> Remi Lespinet  writes:
>>
>>> I agree, I'd like to put it right after split_at_commas in a separate
>>> function "trim_list". Is it a good idea even if the function is one
>>> line long ?
>>
>> Hmph, if I have "A, B, C" and call a function that gives an array of
>> addresses, treating the input as comma-separated addresses, I would
>> expect ("A", "B", "C") to be returned from that function, instead of
>> having to later trim the whitespace around what is returned.
>
> It is actually doing this. But if you have " A,B,C  ", then you'll get
> " A", "B", "C  ". But once you're trimming around commas, trimming
> leading and trailing spaces fits well with split itself.

I guess we are saying the same thing, then?  That is, trim-list as a
separate step does not make sense an it is part of the job for the
helper to turn a single list with multiple addresses into an array?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 7/7] send-email: suppress leading and trailing whitespaces before alias expansion

2015-06-10 Thread Remi Lespinet

Matthieu Moy  writes:

> Junio C Hamano  writes:
> 
> > Remi Lespinet  writes:
> >
> >> I agree, I'd like to put it right after split_at_commas in a separate
> >> function "trim_list". Is it a good idea even if the function is one
> >> line long ?
> >
> > Hmph, if I have "A, B, C" and call a function that gives an array of
> > addresses, treating the input as comma-separated addresses, I would
> > expect ("A", "B", "C") to be returned from that function, instead of
> > having to later trim the whitespace around what is returned.
> 
> It is actually doing this. But if you have " A,B,C  ", then you'll get
> " A", "B", "C  ". But once you're trimming around commas, trimming
> leading and trailing spaces fits well with split itself.

Yes and if we have a single address with leading or/and trailing
whitespaces, such as " A ", I think that we don't expect
split_in_commas to suppress these whitespaces as there's no commas in
this address. As Junio said, I think I should rename the function.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 15/19] pull: teach git pull about --rebase

Paul Tan  writes:

> so it's more or less a direct translation of the shell script, and we
> can be sure it will have the same behavior. I'm definitely in favor of
> switching this to use remote_find_tracking(), the question is whether
> we want to do it in this patch or in a future patch on top.

Ah, OK.

I then agree that the topic should be a straight-forward literal
translation of the current scripted Porcelain.  Switching
implementation detail to share more code can and should come on top
with more careful consideration, as that step can change the
semantics by mistake.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFCv5 3/3] git rebase -i: add static check for commands and SHA-1

Remi Galan Alfonso  writes:

> It is mainly because here the SHA-1 is a long one (40 chars)

OK, but then the minimum would be to add a comment saying that.

Now, this makes me wonder why you are doing the check after the sha1
expansion and not before. Also, when running `git bisect --edit-todo`, I
do get the short sha1. So, there's a piece of code doing what you want
somewhere already. You may want to use it.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] bisect: add the terms old/new

Christian Couder  writes:

> On Wed, Jun 10, 2015 at 5:24 PM, Junio C Hamano  wrote:
>> Matthieu Moy  writes:
>>>
>>> Moving from "one hardcoded pair of terms" to "two hardcoded pairs of
>>> terms" is a nice feature, but hardly a step in the right direction wrt
>>> maintainability.
>>
>> Nicely put.  From that point of view, the variable names and the
>> underlying machinery in general should call these two "new" vs
>> "old".  I.e. name_new=bad name_old=good would be the default, not
>> name_bad=bad name_good=good.
>
> I don't think this would improve maintainability, at least not for me.
> The doc currently uses "good/bad" everywhere.

You are conflating the internal implementation and the end-user
facing interface, I think.

The topic under discussion is about updating the internal
implementation more generic and make it capable of handling both the
traditional and the default 'find transition from good to bad' and
any other kinds that can be expressed by 'find transition from $old
to $new' where the values of $old and $new can be specified by the
end user.  And then we keep old=good new=bad as the default.

And the best time to update the implementation to express its
operation in terms of 'find transition from $old to $new' is when
such a feature is introduced, in other words, inside this topic.

If you still are thinking in terms of 'find transition from $good to
$bad', then I can understand that you would disagree with the above,
but I think you are on the same page as Matthieu and me that we are
updating the system to use "'find transition from $old to $new' where
the values of $old and $new can be specified by the end user" as the
new paradigm.

And it is perfectly fine for the documentation to talk about the
feature using the default pair of words.

Hopefully this clarifies.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFCv5 2/3] git rebase -i: warn about removed commits

2015-06-10 Thread Remi Galan Alfonso

Matthieu Moy  writes:
> Remi Galan Alfonso  writes:
> 
> > Matthieu Moy  writes:
> >> > +warn_file "$todo".miss
> >>
> >> I would find it more elegant with less intermediate files, like
> >>
> >> git rev-list $opt <"$todo".miss | while read -r line
> >> do
> >> warn " - $line"
> >> done
> >
> > I am not really sure since I also use warn_file to display the bad
> > commands and SHA-1 in 3/3.
> 
> I noticed this later indeed. But had the function been eg. warn_pipe,
> you could have written
> 
> git rev-list $opt <"$todo".miss | warn_pipe

Interesting, I'll try to do something similar to this.

Rémi
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFCv5 2/3] git rebase -i: warn about removed commits

Remi Galan Alfonso  writes:

> Matthieu Moy  writes:
>> > +warn_file "$todo".miss
>> 
>> I would find it more elegant with less intermediate files, like
>> 
>> git rev-list $opt <"$todo".miss | while read -r line
>> do
>> warn " - $line"
>> done
>
> I am not really sure since I also use warn_file to display the bad
> commands and SHA-1 in 3/3.

I noticed this later indeed. But had the function been eg. warn_pipe,
you could have written

git rev-list $opt <"$todo".miss | warn_pipe

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFCv5 3/3] git rebase -i: add static check for commands and SHA-1

2015-06-10 Thread Remi Galan Alfonso

> > +git stripspace --strip-comments |
> > +while read -r command sha1 rest
> > +do
> > +case $command in
> > +''|noop|x|"exec")
> > +;;
> > +pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
> > +if test -z $sha1
> > +then
> > +echo "$command $rest" >>"$todo".badsha
> > +else
> > +sha1_verif="$(git rev-parse --verify 
> > --quiet $sha1^{commit})"
> > +if test -z $sha1_verif
> > +then
> > +echo "$command $sha1 $rest" \
> > +>>"$todo".badsha
> 
> When you reach the right end of your screen because of indentation,
> cutting lines with \ is rarely the best option. Having 5 levels of
> indentation is a sign that you should make more functions.

Yeah, I wasn't overly happy with that either, I will try to add some
functions (your example seems like a good way of refactoring).

> > +commit="$(git rev-list --oneline -1 
> > --ignore-missing $sha1 2>/dev/null)"
> > +if test -z "$commit"
> > +then
> > +echo "$command $sha1 $rest" \
> > +>>"$todo".badcmd
> > +else
> > +echo "$command $commit" 
> > >>"$todo".badcmd
> > +fi
> > +fi
> 
> What are you trying to do here? It seems that you are trying to recover
> the line, but the line is your input, you shouldn't have to recompute
> it.
> 
> Why isn't printf '%s %s %s' "$command" "$sha1" "$rest" sufficient in all
> cases?

It is mainly because here the SHA-1 is a long one (40 chars), however
I agree that computing short_sha1 and then printf '%s %s %s'
"$command" "$short_sha1" "$rest" should be good in this case.

> Maybe it would be better to read line by line (to avoid loosing
> whitespace information for example), like
> 
> while read -r line
> do
> printf '%s' "$line" | read -r cmd sha1 rest
> case $sha1 in
> ...
> 
> or maybe it's overkill.

Could be a good idea, though I am not completely convinced about it
yet.

Noted for the other points.

Thanks,
Rémi
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFCv5 2/3] git rebase -i: warn about removed commits

2015-06-10 Thread Remi Galan Alfonso

Matthieu Moy  writes:
> > +warn_file "$todo".miss
> 
> I would find it more elegant with less intermediate files, like
> 
> git rev-list $opt <"$todo".miss | while read -r line
> do
> warn " - $line"
> done

I am not really sure since I also use warn_file to display the bad
commands and SHA-1 in 3/3.

Noted for the other points.

Thanks,
Rémi
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] bisect: add the terms old/new

2015-06-10 Thread Christian Couder

On Wed, Jun 10, 2015 at 5:24 PM, Junio C Hamano  wrote:
> Matthieu Moy  writes:
>
>> "Somebody else did it like that" is not a good justification. Especially
>> when the previous code was not merged: the code wasn't finished.
>>
>> But I actually disagree with the fact that it was not the idea. The
>> point of having the terms in BISECT_TERMS was precisely to be generic
>> enough. Had the goal been just to distinguish good/bad and old/new, we
>> would have needed only one bit of information, and encoding it with the
>> existance/non-existance of a file would have been sufficient (as you
>> tried to do in addition to BISECT_TERMS).
>>
>>> For now we just rebased, corrected and finishing to implement
>>> functionalities.
>>
>> functionalities is one thing, but the code should be maintainable to be
>> merged in git.git. Git would not be where it is if Junio was merging
>> patches based on "it works, we'll see if the code is good enough later"
>> kinds of judgments ;-).
>>
>> Moving from "one hardcoded pair of terms" to "two hardcoded pairs of
>> terms" is a nice feature, but hardly a step in the right direction wrt
>> maintainability.
>
> Nicely put.  From that point of view, the variable names and the
> underlying machinery in general should call these two "new" vs
> "old".  I.e. name_new=bad name_old=good would be the default, not
> name_bad=bad name_good=good.

I don't think this would improve maintainability, at least not for me.
The doc currently uses "good/bad" everywhere.
For example the description is:

   This command uses git rev-list --bisect to help drive the binary search
   process to find which change introduced a bug, given an old "good" commit
   object name and a later "bad" commit object name.

And this is logical if the default is "good/bad". If we use name_new
and name_old in the code, we make it harder for us to see if the doc
matches the code.

And unless we rewrite a lot of them, the tests too will mostly be
still using "good/bad" so it will make it harder to maintain them too.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 7/7] send-email: suppress leading and trailing whitespaces before alias expansion

Junio C Hamano  writes:

> Remi Lespinet  writes:
>
>> I agree, I'd like to put it right after split_at_commas in a separate
>> function "trim_list". Is it a good idea even if the function is one
>> line long ?
>
> Hmph, if I have "A, B, C" and call a function that gives an array of
> addresses, treating the input as comma-separated addresses, I would
> expect ("A", "B", "C") to be returned from that function, instead of
> having to later trim the whitespace around what is returned.

It is actually doing this. But if you have " A,B,C  ", then you'll get
" A", "B", "C  ". But once you're trimming around commas, trimming
leading and trailing spaces fits well with split itself.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 15/19] pull: teach git pull about --rebase

2015-06-10 Thread Paul Tan

On Wed, Jun 10, 2015 at 10:44 PM, Junio C Hamano  wrote:
> Paul Tan  writes:
>
>>> Hmph, it is somewhat surprising that we do not have such a helper
>>> already. Wouldn't we need this logic to implement $branch@{upstream}
>>> syntax?
>>
>> Right, the @{upstream} syntax is implemented by branch_get_upstream()
>> in remote.c. It, however, does not check to see if the branch's remote
>> matches what is provided on the command-line, so we still have to
>> implement this check ourselves, which means this helper function is
>> still required.
>>
>> I guess we could still use branch_get_upstream() in this function though.
>
> It is entirely expected that existing function may not do exactly
> what the new caller you introduce might want to do, or may do more
> than what it wants.  That is where refactoring of existing code
> comes in.
>
> It somewhat feels strange that you have to write more than "shim"
> code to glue existing helpers and API functions together to
> re-implement what a scripted Porcelain is already doing, though.
> It can't be that git-pull.sh implements this logic as shell script,
> and it must be asking existing code in Git to do what the callers
> you added for this function would want to do, right?

Not git-pull.sh, but get_remote_merge_branch() git-parse-remote.sh.
The shell code that get_upstream_branch() in this patch implements is:

0|1)
origin="$1"
default=$(get_default_remote)
test -z "$origin" && origin=$default
curr_branch=$(git symbolic-ref -q HEAD) &&
[ "$origin" = "$default" ] &&

^ This here is where it checks to see if the branch's configured
remote matches the remote provided on the command line.

echo $(git for-each-ref --format='%(upstream)' $curr_branch)
;;

^ While here it calls git to get the upstream branch, which is
implemented by branch_get_upstream() on the C side.

So yes, we can use branch_get_upstream(), but we still need to
implement some code on top.

Just to add on, the shell code that get_tracking_branch() in this
patch implements is:

*)
repo=$1
shift
ref=$1
# FIXME: It should return the tracking branch
#Currently only works with the default mapping
case "$ref" in
+*)
ref=$(expr "z$ref" : 'z+\(.*\)')
;;
esac
expr "z$ref" : 'z.*:' >/dev/null || ref="${ref}:"
remote=$(expr "z$ref" : 'z\([^:]*\):')
case "$remote" in
'' | HEAD ) remote=HEAD ;;
heads/*) remote=${remote#heads/} ;;
refs/heads/*) remote=${remote#refs/heads/} ;;
refs/* | tags/* | remotes/* ) remote=
esac
[ -n "$remote" ] && case "$repo" in
.)
echo "refs/heads/$remote"
;;
*)
echo "refs/remotes/$repo/$remote"
;;
esac

so it's more or less a direct translation of the shell script, and we
can be sure it will have the same behavior. I'm definitely in favor of
switching this to use remote_find_tracking(), the question is whether
we want to do it in this patch or in a future patch on top.

Thanks,
Paul
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] bisect: simplify the add of new bisect terms

Louis-Alexandre Stuber  writes:

> But ENOENT is not a normal case at all. Should we not treat it the same
> way as other fopen() errors ? (either going on with default case or
> returning an error)
>
> Would :
>
>>  if (!fp) {
>>  die("could not read file '%s': %s",
>>  filename, strerror(errno));
>>  } else {
>
> be ok ?

That would be much better than what we had in the patch, which did not
look like an error at all:

+   FILE *fp = fopen(filename, "r");
+
+   if (!fp) {
+   name_bad = "bad";
+   name_good = "good";

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] bisect: simplify the add of new bisect terms

Junio C Hamano  writes:

> Matthieu Moy  writes:
>
>> Louis-Alexandre Stuber  writes:
>>
 That is very different from ENOENT, which is an expected error when
 you are not using a customized terms.
>>>
>>> But in the current state, we are going to create bisect_terms even if
>>> the bisection is in bad/good mode.
>>
>> Which means that in normal cases, you'll either succeed to open it, or
>> get ENOENT. We're talking about unexcepted cases (you don't have
>> permission to read it because it's not your file, because you messed up
>> with a chmod, or whatever reason).
>
> I think both I and you misunderstood what they wanted to do, which
> is to write out good and bad into terms file even though these are
> not customized, and then always read from terms file to learn what
> words are used for good and bad.

Yes, indeed.

> But I do not think it is a good idea to penalize the normal case by
> writing the terms file and reading them back from it when the user
> is bisecting with good/bad in the first place, so

No strong opinion on that, but creating one file doesn't cost much, and
one advantage of writing it unconditionally is that it unifies bad/good
and old/new more in the code. Just the creation of BISECT_TERMS becomes
a special-case.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] bisect: add the terms old/new

Matthieu Moy  writes:

> "Somebody else did it like that" is not a good justification. Especially
> when the previous code was not merged: the code wasn't finished.
>
> But I actually disagree with the fact that it was not the idea. The
> point of having the terms in BISECT_TERMS was precisely to be generic
> enough. Had the goal been just to distinguish good/bad and old/new, we
> would have needed only one bit of information, and encoding it with the
> existance/non-existance of a file would have been sufficient (as you
> tried to do in addition to BISECT_TERMS).
>
>> For now we just rebased, corrected and finishing to implement
>> functionalities.
>
> functionalities is one thing, but the code should be maintainable to be
> merged in git.git. Git would not be where it is if Junio was merging
> patches based on "it works, we'll see if the code is good enough later"
> kinds of judgments ;-).
>
> Moving from "one hardcoded pair of terms" to "two hardcoded pairs of
> terms" is a nice feature, but hardly a step in the right direction wrt
> maintainability.

Nicely put.  From that point of view, the variable names and the
underlying machinery in general should call these two "new" vs
"old".  I.e. name_new=bad name_old=good would be the default, not
name_bad=bad name_good=good.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Using clean/smudge scripts from repository

Bob Bell  writes:

> Is this a proper solution, or did I just "luck out"?  Am I perhaps
> doing something foolish?

Yes, we happen to run checkout in the index order, but that is not
something we guarantee, so you can call yourself lucky.  You are
being doubly lucky that nobody in your project is committing a
malicious script in the repository.  It may also count as foolish,
depending on how important the security is for you and how
trustworthy your collaborators are.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git lock files (Was: GIT for Microsoft Access projects)

2015-06-10 Thread Stefan Beller

On Wed, Jun 10, 2015 at 12:47 AM, Fredrik Gustafsson  wrote:
> On Tue, Jun 09, 2015 at 10:19:43AM -0700, Stefan Beller wrote:
>> Just because Git allows distributed workflows, doesn't mean we
>> should only focus on being distributed IMHO.
>>
>> The question for content not being mergable easily pops up all
>> the time. (Game/Graphics designers, documents, all this binary
>> stuff, where there is no good merge driver).
>>
>> I could imagine a "git lock" command which looks like this:
>
> You do know that gitolite has locking functionality?
>

Yes, and it's cooking its own thing, it's not upstream (in git core I mean)

Locking is useful not just when using gitolite, but any hosting
solution I believe.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFCv5 3/3] git rebase -i: add static check for commands and SHA-1

Galan Rémi  writes:

> +# from the todolist in stdin
> +check_bad_cmd_and_sha () {
> + git stripspace --strip-comments |
> + while read -r command sha1 rest
> + do
> + case $command in
> + ''|noop|x|"exec")
> + ;;
> + pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
> + if test -z $sha1
> + then
> + echo "$command $rest" >>"$todo".badsha
> + else
> + sha1_verif="$(git rev-parse --verify --quiet 
> $sha1^{commit})"
> + if test -z $sha1_verif
> + then
> + echo "$command $sha1 $rest" \
> + >>"$todo".badsha

When you reach the right end of your screen because of indentation,
cutting lines with \ is rarely the best option. Having 5 levels of
indentation is a sign that you should make more functions.

How about:

check_bad_cmd_and_sha () {
git stripspace --strip-comments |
while read -r command sha1 rest
do
case $command in
''|noop|x|"exec")
;;
pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
check_commit_id $sha1
;;
*)
record_bad_command $sha1
;;
esac
}

?

> + *)
> + if test -z $sha1
> + then
> + echo "$command" >>"$todo".badcmd

Avoid echo on user-supplied data.

> + else
> + commit="$(git rev-list --oneline -1 
> --ignore-missing $sha1 2>/dev/null)"
> + if test -z "$commit"
> + then
> + echo "$command $sha1 $rest" \
> + >>"$todo".badcmd
> + else
> + echo "$command $commit" >>"$todo".badcmd
> + fi
> + fi

What are you trying to do here? It seems that you are trying to recover
the line, but the line is your input, you shouldn't have to recompute
it.

Why isn't printf '%s %s %s' "$command" "$sha1" "$rest" sufficient in all
cases?

Maybe it would be better to read line by line (to avoid loosing
whitespace information for example), like

while read -r line
do
printf '%s' "$line" | read -r cmd sha1 rest
case $sha1 in
...

or maybe it's overkill.

> + check_bad_cmd_and_sha <"$todo"
> +
> + if test -s "$todo".badsha
> + then
> + raiseError=t

We usually don't use camelCase in shell-scripts. raise_error would be
the usual way to spell in in Git's codebase.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 5/7] send-email: allow multiple emails using --cc, --to and --bcc

Remi Lespinet  writes:

> From: Jorge Juan Garcia Garcia 
>
> Accept a list of emails separated by commas in flags --cc, --to and
> --bcc.  Multiple addresses can already be given by using these options
> multiple times, but it is more convenient to allow cutting-and-pasting
> a list of addresses from the header of an existing e-mail message,
> which already lists them as comma-separated list, as a value to a
> single parameter.
>
> The following format can now be used:
>
> $ git send-email --to='Jane , m...@example.com'
>
> However format using commas in names doesn't work:
>
> $ git send-email --to='"Jane, Doe" '

That should be "Doe, Jane ", shouldn't it?  When
writing "firstname lastname", people do not put comma in between.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 7/7] send-email: suppress leading and trailing whitespaces before alias expansion

Remi Lespinet  writes:

> I agree, I'd like to put it right after split_at_commas in a separate
> function "trim_list". Is it a good idea even if the function is one
> line long ?

Hmph, if I have "A, B, C" and call a function that gives an array of
addresses, treating the input as comma-separated addresses, I would
expect ("A", "B", "C") to be returned from that function, instead of
having to later trim the whitespace around what is returned.

It suggests that split-at-commas _is_ a wrong abstraction, doesn't
it?  In other words, I think whitespace trimming is part of what the
split-a-single-string-into-array-of-addresses helper function should
be doing.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git-checkout.txt: Document "git checkout " better

2015-06-10 Thread Ed Avis

'restore' may be more consistent with git's internal terminology.
But from an outsider's perspective, 'revert' rather than 'restore' is in my
view much clearer and more consistent with other version control systems:
for example 'svn revert' is what you use to revert files in the working copy.

The original issue was that I naively expected that 'git checkout PATH' would
indeed just 'restore' some files, that is, create them when they are missing.
Its action is rather more drastic than that.

If 'revert' is not a suitable verb because of the existing git-revert, then
I suggest that 'overwrite' or 'replace' might better convey the idea of what
the command does.

-- 
Ed Avis 

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 11/19] pull: check if in unresolved merge state

2015-06-10 Thread Paul Tan

On Wed, Jun 10, 2015 at 10:38 PM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> Paul Tan  writes:
>>
>>> @@ -422,6 +423,14 @@ int cmd_pull(int argc, const char **argv, const char 
>>> *prefix)
>>>
>>>  parse_repo_refspecs(argc, argv, &repo, &refspecs);
>>>
>>> +git_config(git_default_config, NULL);
>>> +
>>> +if (read_cache_unmerged())
>>> +die_resolve_conflict("Pull");
>>> +
>>> +if (file_exists(git_path("MERGE_HEAD")))
>>> +die_conclude_merge();
>>> +
>>>  if (!opt_ff.len)
>>>  config_get_ff(&opt_ff);
>>
>> Hmph.
>>
>> If you are going to do the git_config() call yourself, it might make
>> more sense to define git_pull_config() callback and parse the pull.ff
>> yourself, updating the use of the lazy git_config_get_value() API you
>> introduced in patch 10/19.
>>
>> The above "might" is stronger than my usual "might"; I am undecided
>> yet before reading the remainder of the series.
>
> Let me clarify the above with s/stronger/with much less certainty/;

Uh, I have no idea what a "strong might" or a "less certain might" is. :-/

Parsing all the config values with a git_config() callback function is
certainly possible, but I was under the impression that we are moving
towards migrating use of all the git_config() callback loops to the
git_config_get_X() API.

In this case though, we have to use git_config() to initialize the
advice.resolveConflict config setting, but I don't see why it must be
in conflict with the above goal.

Thanks,
Paul
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] bisect: simplify the add of new bisect terms

Matthieu Moy  writes:

> Louis-Alexandre Stuber  writes:
>
>>> That is very different from ENOENT, which is an expected error when
>>> you are not using a customized terms.
>>
>> But in the current state, we are going to create bisect_terms even if
>> the bisection is in bad/good mode.
>
> Which means that in normal cases, you'll either succeed to open it, or
> get ENOENT. We're talking about unexcepted cases (you don't have
> permission to read it because it's not your file, because you messed up
> with a chmod, or whatever reason).

I think both I and you misunderstood what they wanted to do, which
is to write out good and bad into terms file even though these are
not customized, and then always read from terms file to learn what
words are used for good and bad.

So from _that_ point of view, ENOENT is an error just like others.

But I do not think it is a good idea to penalize the normal case by
writing the terms file and reading them back from it when the user
is bisecting with good/bad in the first place, so

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git-checkout.txt: Document "git checkout " better

Torsten Bögershausen  writes:

> git checkout  can be used to revert changes in the working tree.

I somehow thought that concensus in the recent thread was that
"restore", not "revert", is the more appropriate wording?

And I think that is indeed sensible because "revert" (or "reset")
already means something else in Git (and in other systems), while
"restore" does not have a confusing connotation.  It can only mean
"overwrite with a pristine copy", which is what the command is
about.

> -git-checkout - Checkout a branch or paths to the working tree
> +git-checkout - Switch branches or reverts changes in the working tree

Two verbs in different moods; either "switch branches or restore
changes" or "switches branches or restores changes" would fix that,
and judging from "git help" output, I think we want to go with the
former, i.e. "switch branches or restore changes".

>  
>  SYNOPSIS
>  
> @@ -83,7 +83,8 @@ Omitting  detaches HEAD at the tip of the current 
> branch.
>   When  or `--patch` are given, 'git checkout' does *not*
>   switch branches.  It updates the named paths in the working tree
>   from the index file or from a named  (most often a
> - commit).  In this case, the `-b` and `--track` options are
> + commit).  Changes in files are discarded and deleted files are
> + restored.

I see we are suffering from the common disease of giving one
explanation and then realizing that first explanation can be
misread, clarifying it by more explanation, after reading the
updated text three times.  Let's instead try to clarify the first
explanation to make it harder to misread.

In this case, "updates X from Y" is what causes misunderstanding, as
"updates" does not necessarily mean "restores with the original".

How about this?

'git checkout' with  or `--patch` is used to restore
modified or deleted paths to their original contents from
the index file or from a named  (most often a
commit) without switching branches.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Minor issue: bad Spanish translation

2015-06-10 Thread Johannes Schindelin

Hi Gabriel,

On 2015-06-10 16:51, Gabriel wrote:
> Where it says:
> 
> Su rama está delante de < 
> it should say:
> 
> Su rama está delante de < 
> Notice "para" --> "por".

Good catch.

You could earn eternal fame by cloning Git itself (e.g. via `git clone 
https://github.com/git/git), finding the respective files with `git grep`, 
patching them, making a commit and following the guide lines in 
Documentation/SubmittingPatches to contribute the fix via this mailing 
list[*1*]. That way, you would enter the illustrious group of core Git 
developers.

Ciao,
Johannes

Footnote *1*: If you are more comfortable with GitHub's Pull Requests than with 
sending patches via email, you could use https://submitgit.herokuapp.com/ to 
turn a Pull Request into an appropriately formatted mail to the mailing list.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFCv5 2/3] git rebase -i: warn about removed commits

Galan Rémi  writes:

> Check if commits were removed (i.e. a line was deleted) and print
> warnings or stop git rebase depending on the value of the
> configuration variable rebase.missingCommitsCheck.
>
> This patch gives the user the possibility to avoid silent loss of
> information (losing a commit through deleting the line in this case)
> if he wants.
>
> Add the configuration variable rebase.missingCommitsCheck.
> - When unset or set to "ignore", no checking is done.
> - When set to "warn", the commits are checked, warnings are
>   displayed but git rebase still proceeds.
> - When set to "error", the commits are checked, warnings are
>   displayed and the rebase is stopped.
>   (The user can then use 'git rebase --edit-todo' and
>   'git rebase --continue', or 'git rebase --abort')
>
> rebase.missingCommitsCheck defaults to "ignore".
>
> Signed-off-by: Galan Rémi 
> ---
>  In git-rebase--interactive, in the error case of check_todo_list, I
>  added 'git checkout $onto' so that using 'die' for the error allows
>  to use 'git rebase --edit-todo' (otherwise HEAD would not have been
>  changed and it still would be placed after the commits of the
>  rebase).
>  Since now it doesn't abort the rebase, the documentation and the
>  messages in the case error have changed.
>  I moved the error case away from the initial test case for missing
>  commits as to prepare for 3/3 part of the patch. It is something that
>  was advised by Eric Sunshine when I checked both missing and
>  duplicated commits, but that I removed it when removing the checking
>  for duplicated commits since there was only one test. However I add
>  it again since 3/3 will add more checking.
>  I use the variable raiseError that I affect if the error must be
>  raised instead of testing directly because I think it makes more
>  sense with 3/3 and if we add other check in the future since it adds
>  more possible errors (the test for the error case if not something
>  like 'if (test checkLevel = error && test -s todo.miss) || test cond2
>  || test cond3 || ...').
>  I am wondering if a check_todo_list call should be added in the
>  '--continue' part of the code: with this patch, the checking is only
>  done once, if the user doesn't edit correctly with 'git rebase
>  --edit-todo', it won't be picked by this.
>  In the tests in t3404 I now also test that the warning/error messages
>  are correct.
>  I tried to not change this patch too much since it was already
>  heavily reviewed, but there are some changes (mostly the ones
>  mentionned above).
>
>  Documentation/config.txt  | 11 +
>  Documentation/git-rebase.txt  |  6 +++
>  git-rebase--interactive.sh| 96 
> +++
>  t/t3404-rebase-interactive.sh | 66 +
>  4 files changed, 179 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 4d21ce1..25b2a04 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2160,6 +2160,17 @@ rebase.autoStash::
>   successful rebase might result in non-trivial conflicts.
>   Defaults to false.
>  
> +rebase.missingCommitsCheck::
> + If set to "warn", git rebase -i will print a warning if some
> + commits are removed (e.g. a line was deleted), however the
> + rebase will still proceed. If set to "error", it will print
> + the previous warning and stop the rebase, 'git rebase
> + --edit-todo' can then be used to correct the error. If set to
> + "ignore", no checking is done.
> + To drop a commit without warning or error, use the `drop`
> + command in the todo-list.
> + Defaults to "ignore".
> +
>  receive.advertiseAtomic::
>   By default, git-receive-pack will advertise the atomic push
>   capability to its clients. If you don't want to this capability
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 34bd070..2ca3b8d 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -213,6 +213,12 @@ rebase.autoSquash::
>  rebase.autoStash::
>   If set to true enable '--autostash' option by default.
>  
> +rebase.missingCommitsCheck::
> + If set to "warn", print warnings about removed commits in
> + interactive mode. If set to "error", print the warnings and
> + stop the rebase. If set to "ignore", no checking is
> + done. "ignore" by default.
> +
>  OPTIONS
>  ---
>  --onto ::
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 72abf90..68a71d0 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -834,6 +834,100 @@ add_exec_commands () {
>   mv "$1.new" "$1"
>  }
>  
> +# Print the list of the SHA-1 of the commits
> +# from stdin to stdout
> +todo_list_to_sha_list () {
> + git stripspace --strip-comments |
> + while read -r command sha1 rest
> + do
> + case $command in
> + "$comm

Minor issue: bad Spanish translation

2015-06-10 Thread Gabriel


Where it says:

Su rama está delante de < "por".

Cheers,
Gabriel

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 15/19] pull: teach git pull about --rebase

Paul Tan  writes:

>> Hmph, it is somewhat surprising that we do not have such a helper
>> already. Wouldn't we need this logic to implement $branch@{upstream}
>> syntax?
>
> Right, the @{upstream} syntax is implemented by branch_get_upstream()
> in remote.c. It, however, does not check to see if the branch's remote
> matches what is provided on the command-line, so we still have to
> implement this check ourselves, which means this helper function is
> still required.
>
> I guess we could still use branch_get_upstream() in this function though.

It is entirely expected that existing function may not do exactly
what the new caller you introduce might want to do, or may do more
than what it wants.  That is where refactoring of existing code
comes in.

It somewhat feels strange that you have to write more than "shim"
code to glue existing helpers and API functions together to
re-implement what a scripted Porcelain is already doing, though.
It can't be that git-pull.sh implements this logic as shell script,
and it must be asking existing code in Git to do what the callers
you added for this function would want to do, right?  That suggests
that we must have enough logic already in C.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 11/19] pull: check if in unresolved merge state

Junio C Hamano  writes:

> Paul Tan  writes:
>
>> @@ -422,6 +423,14 @@ int cmd_pull(int argc, const char **argv, const char 
>> *prefix)
>>  
>>  parse_repo_refspecs(argc, argv, &repo, &refspecs);
>>  
>> +git_config(git_default_config, NULL);
>> +
>> +if (read_cache_unmerged())
>> +die_resolve_conflict("Pull");
>> +
>> +if (file_exists(git_path("MERGE_HEAD")))
>> +die_conclude_merge();
>> +
>>  if (!opt_ff.len)
>>  config_get_ff(&opt_ff);
>
> Hmph.
>
> If you are going to do the git_config() call yourself, it might make
> more sense to define git_pull_config() callback and parse the pull.ff
> yourself, updating the use of the lazy git_config_get_value() API you
> introduced in patch 10/19.
>
> The above "might" is stronger than my usual "might"; I am undecided
> yet before reading the remainder of the series.

Let me clarify the above with s/stronger/with much less certainty/;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] index-pack: avoid excessive re-reading of pack directory

2015-06-10 Thread Duy Nguyen

On Wed, Jun 10, 2015 at 9:00 PM, Jeff King  wrote:
> On Tue, Jun 09, 2015 at 08:46:24PM -0700, Shawn Pearce wrote:
>
>> > This patch introduces a "quick" flag to has_sha1_file which
>> > callers can use when they would prefer high performance at
>> > the cost of false negatives during repacks. There may be
>> > other code paths that can use this, but the index-pack one
>> > is the most obviously critical, so we'll start with
>> > switching that one.
>>
>> Hilarious. We did this in JGit back in ... uhm before 2009. :)
>>
>> But its Java. So of course we had to do optimizations.
>
> This is actually how Git did it up until v1.8.4.2, in 2013. I changed it
> then because the old way was racy (and git could flakily report refs as
> broken and skip them during repacks!).
>
> If you are doing it the "quick" way everywhere in JGit, you may want to
> reexamine the possibility for races. :)

I was expecting this :D

>> > @@ -3169,6 +3169,8 @@ int has_sha1_file(const unsigned char *sha1)
>> > return 1;
>> > if (has_loose_object(sha1))
>> > return 1;
>> > +   if (flags & HAS_SHA1_QUICK)
>> > +   return 0;
>> > reprepare_packed_git();
>> > return find_pack_entry(sha1, &e);
>>
>> Something else we do is readdir() over the loose objects and store
>> them in a map in memory. That way we avoid stat() calls during that
>> has_loose_object() path. This is apparently a win enough of the time
>> that we always do that when receiving a pack over the wire (client or
>> server).
>
> Yeah, I thought about that while writing this. It would be a win as long
> as you have a small number of loose objects and were going to make a
> large number of requests (otherwise you are traversing even though
> nobody is going to look it up). According to perf, though, loose object
> lookups are not a large expenditure[1].
>
> I'm also hesitant to go that route because it's basically caching, which
> introduces new opportunities for race conditions when the cache is stale
> (we do the same thing with loose refs, and we have indeed run into races
> there).

Watchman may help avoid races _with_ caching. But we need to support
both ways in that case, falling back to normal file poking when
watchman gives up, or when we're on Windows. Extra work needs big
enough performance gain to justify. I haven't seen that gain yet.

> [1] As measured mostly by __d_lookup_rcu calls. Of course, my patch
> gives a 5% improvement over the original, and we were not spending
> 5% of the time there originally. I suspect part of the problem is
> that we do the lookup under a lock, so the longer we spend there,
> the more contention we have between threads, and the less
> parallelism. Indeed, I just did a quick repeat of my tests with
> pack.threads=1, and the size of the improvement shrinks.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] index-pack: avoid excessive re-reading of pack directory

On Tue, Jun 09, 2015 at 08:46:24PM -0700, Shawn Pearce wrote:

> > This patch introduces a "quick" flag to has_sha1_file which
> > callers can use when they would prefer high performance at
> > the cost of false negatives during repacks. There may be
> > other code paths that can use this, but the index-pack one
> > is the most obviously critical, so we'll start with
> > switching that one.
> 
> Hilarious. We did this in JGit back in ... uhm before 2009. :)
> 
> But its Java. So of course we had to do optimizations.

This is actually how Git did it up until v1.8.4.2, in 2013. I changed it
then because the old way was racy (and git could flakily report refs as
broken and skip them during repacks!).

If you are doing it the "quick" way everywhere in JGit, you may want to
reexamine the possibility for races. :)

> > @@ -3169,6 +3169,8 @@ int has_sha1_file(const unsigned char *sha1)
> > return 1;
> > if (has_loose_object(sha1))
> > return 1;
> > +   if (flags & HAS_SHA1_QUICK)
> > +   return 0;
> > reprepare_packed_git();
> > return find_pack_entry(sha1, &e);
> 
> Something else we do is readdir() over the loose objects and store
> them in a map in memory. That way we avoid stat() calls during that
> has_loose_object() path. This is apparently a win enough of the time
> that we always do that when receiving a pack over the wire (client or
> server).

Yeah, I thought about that while writing this. It would be a win as long
as you have a small number of loose objects and were going to make a
large number of requests (otherwise you are traversing even though
nobody is going to look it up). According to perf, though, loose object
lookups are not a large expenditure[1].

I'm also hesitant to go that route because it's basically caching, which
introduces new opportunities for race conditions when the cache is stale
(we do the same thing with loose refs, and we have indeed run into races
there).

-Peff

[1] As measured mostly by __d_lookup_rcu calls. Of course, my patch
gives a 5% improvement over the original, and we were not spending
5% of the time there originally. I suspect part of the problem is
that we do the lookup under a lock, so the longer we spend there,
the more contention we have between threads, and the less
parallelism. Indeed, I just did a quick repeat of my tests with
pack.threads=1, and the size of the improvement shrinks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/8] sha1_file: introduce has_object_file helper.

On Wed, Jun 10, 2015 at 04:59:58PM +0700, Duy Nguyen wrote:
> On Tue, Jun 9, 2015 at 11:28 PM, brian m. carlson
>  wrote:
> > diff --git a/sha1_file.c b/sha1_file.c
> > index 7e38148..09f7f03 100644
> > --- a/sha1_file.c
> > +++ b/sha1_file.c
> > @@ -3173,6 +3173,11 @@ int has_sha1_file(const unsigned char *sha1)
> > return find_pack_entry(sha1, &e);
> >  }
> >
> > +int has_object_file(const struct object_id *oid)
> > +{
> > +   return has_sha1_file(oid->hash);
> > +}
> > +
> 
> This version could be "static inline" and placed in cache.h. Though it
> may be premature optimization. On top of my head I can't recall any
> place where has_sha1_file() is used so many times for this extra call
> to become significant overhead.

I planned on merging the two into has_object_file when has_sha1_file has
no more callers, so it's more of an incidental artifact that one calls
the other rather than a long-term goal.  In the branch I'm working on
now, I'm down to 27 callers, so it may be rather soon that it goes away.

Of course, if the consensus is that the performance increase is worth it
in the mean time, I can certainly just move it back when they merge.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: Digital signature

"git difftool" is not working as expected during MERGING

2015-06-10 Thread Bossert, Andre

Hello,

i've tested "git difftool" with -t --ext-cmd and other options to see
my diff with external tools, but it always show internal text-diff in
console. The same tests with "git mergetool" working as expected. I've
compared (nanually reviewed) git-mergetool.sh with git-difftool.pl and
see that difftool is mixed / shared implementation with normal "git
diff" command. So during MERGING state there is no possibility to call
external difftool with this command. Any ideas why this was
implemented this way?

-- 
Regards
Andre (anb0s)
eMail: an...@anbos.de
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: On undoing a forced push