shallow: extract a header file

[...] by making 'commit_graft_pos' non-static so that it can
be called from both 'commit.c' and 'shallow.c'.

Signed-off-by Taylor Blau <[email protected]>

---
 commit.c | 2 +-
 commit.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

commit.h +0 -0

index ab91d21131..eb42e8b6d2 100644

Hi,

Quoted 14 lines · Taylor Blau

[...] by making 'commit_graft_pos' non-static so that it can
be called from both 'commit.c' and 'shallow.c'.

Signed-off-by Taylor Blau <[email protected]>

---
 commit.c | 2 +-
 commit.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

commit.h +0 -0

index ab91d21131..eb42e8b6d2 100644

commit.h struct commit_graft { · line 239

Quoted 4 lines

commit.h +1 -0

@@ -236,6 +236,7 @@ struct commit_graft {
 typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);
 
 struct commit_graft *read_graft_line(struct strbuf *line);
+int commit_graft_pos(struct repository *r, const unsigned char *sha1);

Now that this function isn't file-local, its name becomes more significant. What array does this represent a position in? What does it return if the graft isn't found? From a call site it's not necessarily obvious.

Ideas:

- could include a comment saying that it's an index into r->parsed_objects->grafts

- I'm usually loathe to suggest unnecessary duplication of code, but it might make sense to duplicate the function into shallow.c. Or even to inline it there (in the single call site, that ends up being pretty readable).

Thoughts?

Thanks,
Jonathan

There are many functions in commit.h that are more related to shallow repositories than they are to any sort of generic commit machinery. Likely this began when there were only a few shallow-related functions, and commit.h seemed a reasonable enough place to put them.

But, now there are a good number of shallow-related functions, and placing them all in 'commit.h' doesn't make sense.

This patch extracts a 'shallow.h', which takes all of the headers from

'commit.h' for functions which already exist in 'shallow.c'. We will bring the remaining shallow-related functions defined in 'commit.c' in a subsequent patch.

For now, move only the ones that already are implemented in 'shallow.c', and update the necessary includes.

Signed-off-by Taylor Blau <[email protected]>

First of all, I couldn't apply these patches neither on latest master (86ab15cb15 ("The fourth batch", 2020-04-28)) nor on its 2 immediate ancestors - what is the base of these patches? (Or is there something wrong with my workflow?)

I'll review based on the patches themselves, but what I've seen looks good so far.

builtin.h line 8

Quoted 9 lines

builtin.h +1 -0

index 2b25a80cde..2e701a023c 100644
@@ -5,6 +5,7 @@
 #include "strbuf.h"
 #include "cache.h"
 #include "commit.h"
+#include "shallow.h"

It's a pity that builtin.h has to be modified in this way, but I see that it's necessary - a few files that include builtin.h (at least git.c and builtin/bisect--helper.c) assume that they have shallow function access through this header.

Once I manage to apply these patches myself, I'll verify with --color-moved that the lines moved as I expect, but otherwise patch 1 and 2 look good.

Yes, I should have mentioned in the cover letter that this is based on 'tb/reset-shallow' which is should be part of Junio's next push to master. I didn't mention it because I figured that it would be on master by the time anyone wanted to look at these patches ;).

Anyhow, apologies. It should apply cleanly on any branch that has 'tb/reset-shallow' already.

There are many functions in commit.h that are more related to shallow repositories than they are to any sort of generic commit machinery. Likely this began when there were only a few shallow-related functions, and commit.h seemed a reasonable enough place to put them.

But, now there are a good number of shallow-related functions, and placing them all in 'commit.h' doesn't make sense.

This patch extracts a 'shallow.h', which takes all of the headers from 'commit.h' for functions which already exist in 'shallow.c'. We will bring the remaining shallow-related functions defined in 'commit.c' in a subsequent patch.

For now, move only the ones that already are implemented in 'shallow.c', and update the necessary includes.

In the last commit, we introduced a header for the functions defined in 'shallow.c'. There is one remaining shallow-related function in commit.c which should be moved, too. This patch moves that function.

Signed-off-by Taylor Blau <[email protected]>

---
 commit.c  | 13 -------------
 shallow.c | 13 +++++++++++++
 shallow.h |  1 +
 3 files changed, 14 insertions(+), 13 deletions(-)

Quoted 10 lines · Taylor Blau

In the last commit, we introduced a header for the functions defined in 'shallow.c'. There is one remaining shallow-related function in commit.c which should be moved, too. This patch moves that function.

Signed-off-by Taylor Blau <[email protected]>

---
 commit.c  | 13 -------------
 shallow.c | 13 +++++++++++++
 shallow.h |  1 +
 3 files changed, 14 insertions(+), 13 deletions(-)

Yes, an obviously good thing to do.

Perhaps could be squashed with patch 1 (but also see my review of that one).

Reviewed-by Jonathan Nieder <[email protected]>

Should this take a "struct repository" parameter?

When 'commit_shallow_file()' and 'rollback_shallow_file()' were introduced, they did not have an documenting comment, when they could have benefited from one.

Add a brief note about what these functions do, and make a special note that they reset stat-validity checks.

Signed-off-by Taylor Blau <[email protected]>

---

[...]
This patch introduces that type as a thin wrapper around 'struct lockfile', and updates the two aforementioned functions and their callers to use it.

Signed-off-by Taylor Blau <[email protected]>

---

Quoted 9 lines · Taylor Blau

[...]
This patch introduces that type as a thin wrapper around 'struct lockfile', and updates the two aforementioned functions and their callers to use it.

Signed-off-by Taylor Blau <[email protected]>

---

shallow.h void set_alternate_shallow_file(struct repository *r, const char *path, int over · line 16

Quoted 7 lines

shallow.h +7 -0

@@ -10,12 +10,22 @@ void set_alternate_shallow_file(struct repository *r, const char *path, int over
+/*
+ * shallow_lock is a thin wrapper around 'struct lock_file' in order to restrict
+ * which locks can be used with '{commit,rollback}_shallow_file()'.
+ */
+struct shallow_lock {
+       struct lock_file lk;
+};

The documentation comment for 'shallow_lock' may help newcomers to C but probably doesn't add much value for seasoned programmers. If this is the sort of idiom we want to introduce (or exists already in this codebase) -- declaring a specific C type to avoid accidental use of an unrelated lock -- then it's probably better documented in CodingGuidelines rather than repeating it at each point in the code which employs the idiom.

shallow.c +1 -1

@@ -428,14 +428,14 @@ void prune_shallow(unsigned options)
 		strbuf_release(&sb);
 		return;
 	}
-	fd = hold_lock_file_for_update(&shallow_lock,
+	fd = hold_lock_file_for_update(&shallow_lock.lk,
 				       git_path_shallow(the_repository),

shallow.h +0 -0

index 08e1bc4fd0..d12096fbc4 100644
@@ -10,12 +10,22 @@ void set_alternate_shallow_file(struct repository *r, const char *path, int over
 int register_shallow(struct repository *r, const struct object_id *oid);

In previous patches, the functions 'commit_shallow_file' and 'rollback_shallow_file' were introduced to reset the shallowness validity checks on a repository after potentially modifying '.git/shallow'.

These functions can be made safer by wrapping the 'struct lockfile *' in a new type, 'shallow_lock', so that they cannot be called with a raw lock (and potentially misused by other code that happens to possess a lockfile, but has nothing to do with shallowness).

This patch introduces that type as a thin wrapper around 'struct lockfile', and updates the two aforementioned functions and their callers to use it.

Suggested-by Junio C Hamano <[email protected]>

Signed-off-by Taylor Blau <[email protected]>

---
 builtin/receive-pack.c |  2 +-
 fetch-pack.c           |  2 +-
 shallow.c              | 22 +++++++++++-----------
 shallow.h              | 16 +++++++++++++---
 4 files changed, 26 insertions(+), 16 deletions(-)

shallow.h +0 -0

@@ -10,12 +10,22 @@ void set_alternate_shallow_file(struct repository *r, const char *path, int over
 int register_shallow(struct repository *r, const struct object_id *oid);
 int unregister_shallow(const struct object_id *oid);

shallow.c +0 -0

shallow.c +1 -1

@@ -366,22 +366,22 @@ const char *setup_temporary_shallow(const struct oid_array *extra)
 	return "";
 }
 
-void setup_alternate_shallow(struct lock_file *shallow_lock,
+void setup_alternate_shallow(struct shallow_lock *shallow_lock,

shallow.c +1 -2

@@ -414,7 +414,7 @@ void advertise_shallow_grafts(int fd)
  */
 void prune_shallow(unsigned options)
 {
-	struct lock_file shallow_lock = LOCK_INIT;
+	struct shallow_lock shallow_lock = SHALLOW_LOCK_INIT;
 	struct strbuf sb = STRBUF_INIT;
 	unsigned flags = SEEN_ONLY;
 	int fd;

@@ -428,14 +428,14 @@ void prune_shallow(unsigned options)
 		strbuf_release(&sb);
 		return;
 	}
-	fd = hold_lock_file_for_update(&shallow_lock,

Quoted 4 lines · Taylor Blau

+/*
+ * shallow_lock is a thin wrapper around 'struct lock_file' in order to restrict
+ * which locks can be used with '{commit,rollback}_shallow_file()'.
+ */

I think I disagree with Eric here: it's useful to have a comment here to describe the purpose of the struct (i.e., the "why" as opposed to the "what").

I wonder if we can go further, though --- when using a shallow_lock, how should I think of it as a caller? In some sense, the use of 'struct lock_file' is an implementation detail, so we could say:

/*
* Lock for updating the $GIT_DIR/shallow file.
*
* Use commit_shallow_file() to commit an update, or
* rollback_shallow_file() to roll it back. In either case,
* any in-memory cached information about which commits are
* shallow will be appropriately invalidated so that future
* operations reflect the new state.
*/

What do you think?

Quoted 9 lines · Jonathan Nieder

Quoted 4 lines · Taylor Blau

+/*
+ * shallow_lock is a thin wrapper around 'struct lock_file' in order to restrict
+ * which locks can be used with '{commit,rollback}_shallow_file()'.
+ */

I think I disagree with Eric here: it's useful to have a comment here to describe the purpose of the struct (i.e., the "why" as opposed to the "what").

I'm not, in general, opposed to the structure being documented; it's just that the comment, as presented, doesn't seem to add value.

Quoted 14 lines

I wonder if we can go further, though --- when using a shallow_lock, how should I think of it as a caller? In some sense, the use of 'struct lock_file' is an implementation detail, so we could say:

/*
* Lock for updating the $GIT_DIR/shallow file.
*
* Use commit_shallow_file() to commit an update, or
* rollback_shallow_file() to roll it back. In either case,
* any in-memory cached information about which commits are
* shallow will be appropriately invalidated so that future
* operations reflect the new state.
*/

Quoted 1 line

What do you think?

This comment makes more sense and wouldn't have led to me questioning its usefulness. Thanks.