Re: none

Re: none

[Matthieu Moy]

miwilliams@google.com writes:

> From 7201fe08ede76e502211a781250c9a0b702a78b2 Mon Sep 17 00:00:00 2001
> From: Mike Williams <miwilliams@google.com>
> Date: Mon, 11 Apr 2016 14:18:39 -0400
> Subject: [PATCH 1/1] wt-status: Remove '!!' from
> wt_status_collect_changed_cb
>
> The wt_status_collect_changed_cb function uses an extraneous double
> negation (!!)
> when determining whether or not a submodule has new commits.

It's not just a double negation, it's a way to ensure that the value is
0 or 1 (it's a relatively common idiom in C at least in Git's codebase).

new_submodule_commits is a 1-bit bitfield, and you don't want to assign
anything other than 1 or 0 (or you'll get modulo 2^n semantics, with
n==1).

So the old code is correct and your patch would introduce a bug.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH] doc: fix a typo and clarify a sentence

[Mihir Mehta]

I noticed that git-merge-base was unlikely to actually be a git command,
and tried it in my shell. Seeing that it doesn't work, I cleaned up two
places in the docs where it appears.

Signed-off-by: Mihir Mehta <mihir@cs.utexas.edu>
---
 Documentation/git-diff.txt                  | 5 +++--
 Documentation/howto/update-hook-example.txt | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index b180f1fa5..6173f569e 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -72,8 +72,9 @@ two blob objects, or changes between two files on disk.
 	This form is to view the changes on the branch containing
 	and up to the second <commit>, starting at a common ancestor
 	of both <commit>.  "git diff A\...B" is equivalent to
-	"git diff $(git-merge-base A B) B".  You can omit any one
-	of <commit>, which has the same effect as using HEAD instead.
+	"git diff $(git merge-base A B) B".  You can omit any one
+	of the two instances of <commit>, which has the same effect as
+	using HEAD in its place.
 
 Just in case if you are doing something exotic, it should be
 noted that all of the <commit> in the above description, except
diff --git a/Documentation/howto/update-hook-example.txt b/Documentation/howto/update-hook-example.txt
index a5193b1e5..89821ec74 100644
--- a/Documentation/howto/update-hook-example.txt
+++ b/Documentation/howto/update-hook-example.txt
@@ -80,7 +80,7 @@ case "$1" in
       info "The branch '$1' is new..."
     else
       # updating -- make sure it is a fast-forward
-      mb=$(git-merge-base "$2" "$3")
+      mb=$(git merge-base "$2" "$3")
       case "$mb,$2" in
         "$2,$mb") info "Update is fast-forward" ;;
 	*)	  noff=y; info "This is not a fast-forward update.";;
-- 
2.19.0

Re: [PATCH] doc: fix a typo and clarify a sentence

[Junio C Hamano]

Mihir Mehta <mihir@cs.utexas.edu> writes:

> diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
> index b180f1fa5..6173f569e 100644
> --- a/Documentation/git-diff.txt
> +++ b/Documentation/git-diff.txt
> @@ -72,8 +72,9 @@ two blob objects, or changes between two files on disk.
>  	This form is to view the changes on the branch containing
>  	and up to the second <commit>, starting at a common ancestor
>  	of both <commit>.  "git diff A\...B" is equivalent to
> -	"git diff $(git-merge-base A B) B".  You can omit any one
> -	of <commit>, which has the same effect as using HEAD instead.
> +	"git diff $(git merge-base A B) B".  You can omit any one

"git merge-base" is a more modern way to spell "git-merge-base" and
we have been trying to update the mention of the latter in the docs
to the former.  Thanks for doing this.

> +	of the two instances of <commit>, which has the same effect as

The paragraph is about <commit>...<commit> three-dot notation.  I
suspect that you wanted to say <commit>... and ...<commit> are
allowed, implying that a bare ... is not allowed and does not mean
the same thing as what HEAD...HEAD means.  But that is not the case.
Asking "git diff HEAD...HEAD" by omitting both may not give very
interesting output (it always becomes a no-op), but nevertheless it
is a valid thing to ask (iow "git diff $commit1...$commit2" is what
you can safely write without having to worry about one or both going
empty string).  So I'd rather not to see this change in this form.
It is an incomplete attempt to discourage use of <empty>...<empty>
but without giving enough justification.

	Side note.  I am not recommending to do so, but
	"discouragement with enough justification" would look like
	this.

	You can omit <commit> on any side of the three dots, which
	has the same effect as using HEAD instead.  Omitting both
	and leaving only three dots is not an error but that merely
	specifies a set of commits that are and are not reachable
	from HEAD at the same time, which by definition is an empty
	set, hence it is not very useful.

> +	using HEAD in its place.

> +++ b/Documentation/howto/update-hook-example.txt
> @@ -80,7 +80,7 @@ case "$1" in
>        info "The branch '$1' is new..."
>      else
>        # updating -- make sure it is a fast-forward
> -      mb=$(git-merge-base "$2" "$3")
> +      mb=$(git merge-base "$2" "$3")

I strongly suspect that inside update-hook, the original still
should work (iow, $GIT_EXEC_PATH should already have been prepended
to $PATH before a hoook is called).  But the updated form should
also work, and it is the form we humans need to type, so let's take
this change.

Thanks.

>        case "$mb,$2" in
>          "$2,$mb") info "Update is fast-forward" ;;
>  	*)	  noff=y; info "This is not a fast-forward update.";;

(no subject)

[Mihir Mehta]

Thanks, Junio. Instead of removing that part of the patch, I opted to
expand it to make it a little clearer (in my opinion) than it was
before. Let me know if this works.

Mihir.

[PATCH] doc: fix a typo and clarify a sentence

[Mihir Mehta]

I noticed that git-merge-base was unlikely to actually be a git command,
and tried it in my shell. Seeing that it doesn't work, I cleaned up two
places in the docs where it appears.

Signed-off-by: Mihir Mehta <mihir@cs.utexas.edu>
---
 Documentation/git-diff.txt                  | 7 ++++---
 Documentation/howto/update-hook-example.txt | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index b180f1fa5..a122f9ded 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -72,10 +72,11 @@ two blob objects, or changes between two files on disk.
 	This form is to view the changes on the branch containing
 	and up to the second <commit>, starting at a common ancestor
 	of both <commit>.  "git diff A\...B" is equivalent to
-	"git diff $(git-merge-base A B) B".  You can omit any one
-	of <commit>, which has the same effect as using HEAD instead.
+	"git diff $(git merge-base A B) B".  You can omit any one
+	of the two instances of <commit>, which has the same effect as
+	using HEAD in its place; omitting both results in an empty diff.
 
-Just in case if you are doing something exotic, it should be
+Just in case you are doing something exotic, it should be
 noted that all of the <commit> in the above description, except
 in the last two forms that use ".." notations, can be any
 <tree>.
diff --git a/Documentation/howto/update-hook-example.txt b/Documentation/howto/update-hook-example.txt
index a5193b1e5..89821ec74 100644
--- a/Documentation/howto/update-hook-example.txt
+++ b/Documentation/howto/update-hook-example.txt
@@ -80,7 +80,7 @@ case "$1" in
       info "The branch '$1' is new..."
     else
       # updating -- make sure it is a fast-forward
-      mb=$(git-merge-base "$2" "$3")
+      mb=$(git merge-base "$2" "$3")
       case "$mb,$2" in
         "$2,$mb") info "Update is fast-forward" ;;
 	*)	  noff=y; info "This is not a fast-forward update.";;
-- 
2.19.0

Re: none

[Junio C Hamano]

Mihir Mehta <mihir@cs.utexas.edu> writes:

> Thanks, Junio. Instead of removing that part of the patch, I opted to
> expand it to make it a little clearer (in my opinion) than it was
> before. Let me know if this works.

I am mildly negative on that change.  "Omitting both would give an
empty diff" would be understandable to anybody who understands that
an omitted end of dot-dot is substituted with HEAD *and* thinks what
range HEAD..HEAD means, so it is just an additional noise to them,
and to those who do not want to waste time on thinking, it is a
statement that reads as if "it will be an error" without saying why
it is an error.  So overall, it seems, at least to me, that the
additional text adds negative value.

So, I dunno.

[PATCH] doc: fix a typo and clarify a sentence

[Mihir Mehta]

I noticed that git-merge-base was unlikely to actually be a git command,
and tried it in my shell. Seeing that it doesn't work, I cleaned up two
places in the docs where it appears.

Signed-off-by: Mihir Mehta <mihir@cs.utexas.edu>
---
 Documentation/git-diff.txt                  | 4 ++--
 Documentation/howto/update-hook-example.txt | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index b180f1fa5..030f162f3 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -72,10 +72,10 @@ two blob objects, or changes between two files on disk.
 	This form is to view the changes on the branch containing
 	and up to the second <commit>, starting at a common ancestor
 	of both <commit>.  "git diff A\...B" is equivalent to
-	"git diff $(git-merge-base A B) B".  You can omit any one
+	"git diff $(git merge-base A B) B".  You can omit any one
 	of <commit>, which has the same effect as using HEAD instead.
 
-Just in case if you are doing something exotic, it should be
+Just in case you are doing something exotic, it should be
 noted that all of the <commit> in the above description, except
 in the last two forms that use ".." notations, can be any
 <tree>.
diff --git a/Documentation/howto/update-hook-example.txt b/Documentation/howto/update-hook-example.txt
index a5193b1e5..89821ec74 100644
--- a/Documentation/howto/update-hook-example.txt
+++ b/Documentation/howto/update-hook-example.txt
@@ -80,7 +80,7 @@ case "$1" in
       info "The branch '$1' is new..."
     else
       # updating -- make sure it is a fast-forward
-      mb=$(git-merge-base "$2" "$3")
+      mb=$(git merge-base "$2" "$3")
       case "$mb,$2" in
         "$2,$mb") info "Update is fast-forward" ;;
 	*)	  noff=y; info "This is not a fast-forward update.";;
-- 
2.19.0

Re: [PATCH] doc: fix a typo and clarify a sentence

[Junio C Hamano]

Mihir Mehta <mihir@cs.utexas.edu> writes:

> -Just in case if you are doing something exotic, it should be
> +Just in case you are doing something exotic, it should be

Thanks.  Somehow I didn't notice this change earlier, but it looks
good, too.

Will queue.

Re: none

[Junio C Hamano]

Rohit Ashiwal <rohit.ashiwal265@gmail.com> writes:

> Just to be clear of what caused the error:
> 	1. Path not being file, or
> 	2. File not being empty
> I am checking for both.

test -s <path> makes sure <path> is file; if it is not a file, then
it won't yield true.

So why do you need to say test_path_is_file yourself, if you are
asking "test -s"?

Re: none

[Junio C Hamano]

Han-Wen Nienhuys <hanwen@google.com> writes:

> I spent the last few weeks cobbling together an implementation of the
> reftable format in C and in Go. I thought this would be cool to add to
> git-core, but I doubt whether I will have enough time to see such an
> effort through. Maybe some of you would want to try integrating it
> into the Git-core code base?  Example code is here:
>
>   https://github.com/google/reftable/blob/master/c/api.h#L153
>
> cheers!

My initial impression was that the API overuses typedef.  We tend to
avoid doing

	struct _foo { ... };
	typedef struct _foo foo;

and instead write "struct foo" explicitly to make us well aware of
what we are talking about.  That lets us see that we are passing or
returning a structure by value (which we would like authors to think
thrice before doing in C) like so quite easily:

	foo some_function(foo arg1, ...) { ... }

because it would be clear if it were written like so

	struct foo some_function(struct foo arg1, ...) { ... }

without hiding the structure behind a typedef (it also lets us avoid
names with leading underscore, which is frowned upon by some people
for different reasons).

But the set of operations defined in the header file seemed at the
right granularity in order to interface with the refs.h & refs/* API
we have.  It however was unclear to me how transactional ref updates
would work with it.

Thanks.

Re: none

[Han-Wen Nienhuys]

On Tue, Nov 19, 2019 at 8:53 PM Junio C Hamano <gitster@pobox.com> wrote:
> My initial impression was that the API overuses typedef.  We tend to
> avoid doing
>
>         struct _foo { ... };
>         typedef struct _foo foo;
>
> and instead write "struct foo" explicitly to make us well aware of
> what we are talking about.

Thanks, i'll have a look at changing it. I use typedef mainly for
ergonomics, but now that the code is written, I can introduce more
verbosity.

> But the set of operations defined in the header file seemed at the
> right granularity in order to interface with the refs.h & refs/* API
> we have.  It however was unclear to me how transactional ref updates
> would work with it.

Transactions have to interface with the file system. I imagine that
different libraries (libgit vs. cgit) would have different primitives
for dealing with the file system, hence I haven't implemented that
part. Do you have an idea of how I could implement it in a way that is
agnostic of libgit2 vs. cgit?

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Re: none

[Junio C Hamano]

Dorcas Litunya <anonolitunya@gmail.com> writes:

> Bcc: 
> Subject: Re: [PATCH] t/t7601: Modernize test scripts using functions
> Reply-To: 
> In-Reply-To: <xmqq1qdumrto.fsf@gitster.g>

What are these lines doing here?

> So should I replace this in the next version or leave this as is?

Perhaps I was not clear enough, but I found the commit title and
description need to be updated to clearly record the intent of the
change with a handful of points, so I will not be accepting the
patch as-is.

These two sections may be of help.

Documentation/MyFirstContribution.txt::now-what
Documentation/MyFirstContribution.txt::reviewing

Thanks.